diff mbox series

[v4] pinctrl: intel: refine ->irq_set_type() hook

Message ID 20230615125022.27421-1-raag.jadav@intel.com
State New
Headers
Series [v4] pinctrl: intel: refine ->irq_set_type() hook |

Commit Message

Raag Jadav June 15, 2023, 12:50 p.m. UTC 
  Refine ->irq_set_type() hook and improve its readability by:

- Reducing scope of spinlock by moving unneeded operations out of it.
- Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly
  into PADCFG0_RXEVCFG_* definitions.
- Utilizing temporary variables for common operations.
- Simplifying if-else-if chain.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 45 ++++++++++++++-------------
 1 file changed, 24 insertions(+), 21 deletions(-)


base-commit: e95433c367e681dc6d4613706bd74f483a25acd8

Comments

Andy Shevchenko June 15, 2023, 1:14 p.m. UTC | #1 
On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote:
> Refine ->irq_set_type() hook and improve its readability by:
> 
> - Reducing scope of spinlock by moving unneeded operations out of it.
> - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly
>   into PADCFG0_RXEVCFG_* definitions.
> - Utilizing temporary variables for common operations.
> - Simplifying if-else-if chain.

Two questions out of curiosity.
Do we gain or lose bytes with this?

> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 45 ++++++++++++++-------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index e8adf2580321..036eae74c479 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -55,12 +55,11 @@
>  
>  /* Offset from pad_regs */
>  #define PADCFG0				0x000
> -#define PADCFG0_RXEVCFG_SHIFT		25
>  #define PADCFG0_RXEVCFG_MASK		GENMASK(26, 25)
> -#define PADCFG0_RXEVCFG_LEVEL		0
> -#define PADCFG0_RXEVCFG_EDGE		1
> -#define PADCFG0_RXEVCFG_DISABLED	2
> -#define PADCFG0_RXEVCFG_EDGE_BOTH	3
> +#define PADCFG0_RXEVCFG_LEVEL		(0 << 25)
> +#define PADCFG0_RXEVCFG_EDGE		(1 << 25)
> +#define PADCFG0_RXEVCFG_DISABLED	(2 << 25)
> +#define PADCFG0_RXEVCFG_EDGE_BOTH	(3 << 25)
>  #define PADCFG0_PREGFRXSEL		BIT(24)
>  #define PADCFG0_RXINV			BIT(23)
>  #define PADCFG0_GPIROUTIOXAPIC		BIT(20)
> @@ -1127,9 +1126,9 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
>  	unsigned int pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
> +	u32 rxevcfg, rxinv, value;
>  	unsigned long flags;
>  	void __iomem *reg;
> -	u32 value;
>  
>  	reg = intel_get_padcfg(pctrl, pin, PADCFG0);
>  	if (!reg)
> @@ -1145,28 +1144,32 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
>  		return -EPERM;
>  	}
>  
> -	raw_spin_lock_irqsave(&pctrl->lock, flags);
> -
> -	intel_gpio_set_gpio_mode(reg);
> -
> -	value = readl(reg);
> -
> -	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> -
>  	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> -		value |= PADCFG0_RXEVCFG_EDGE_BOTH << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
>  	} else if (type & IRQ_TYPE_EDGE_FALLING) {
> -		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
> -		value |= PADCFG0_RXINV;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
>  	} else if (type & IRQ_TYPE_EDGE_RISING) {
> -		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
>  	} else if (type & IRQ_TYPE_LEVEL_MASK) {
> -		if (type & IRQ_TYPE_LEVEL_LOW)
> -			value |= PADCFG0_RXINV;
> +		rxevcfg = PADCFG0_RXEVCFG_LEVEL;
>  	} else {
> -		value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_DISABLED;
>  	}
>  
> +	if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
> +		rxinv = PADCFG0_RXINV;
> +	else
> +		rxinv = 0;
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	intel_gpio_set_gpio_mode(reg);
> +
> +	value = readl(reg);

> +	value = (value & ~PADCFG0_RXINV) | rxinv;
> +	value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;

Same question if we change this to be similar to the current code, i.e.

	value = readl(reg);

	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
	value |= rxevcfg;
	value |= rxinv;

And I would keep blank lines after readl() and before writel() since we have
more than a single line in between.

>  	writel(value, reg);
>  
>  	if (type & IRQ_TYPE_EDGE_BOTH)
> 
> base-commit: e95433c367e681dc6d4613706bd74f483a25acd8
> -- 
> 2.17.1
>
Raag Jadav June 15, 2023, 1:35 p.m. UTC | #2 
> On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote:
> > Refine ->irq_set_type() hook and improve its readability by:
> >
> > - Reducing scope of spinlock by moving unneeded operations out of it.
> > - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly
> >   into PADCFG0_RXEVCFG_* definitions.
> > - Utilizing temporary variables for common operations.
> > - Simplifying if-else-if chain.
> 
> Two questions out of curiosity.
> Do we gain or lose bytes with this?

add/remove: 0/0 grow/shrink: 1/0 up/down: 33/0 (33)
Function                                     old     new   delta
intel_gpio_irq_type                          317     350     +33
Total: Before=10469, After=10502, chg +0.32%

> > +	value = readl(reg);
> 
> > +	value = (value & ~PADCFG0_RXINV) | rxinv;
> > +	value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;
> 
> Same question if we change this to be similar to the current code, i.e.
> 
> 	value = readl(reg);
> 
> 	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> 	value |= rxevcfg;
> 	value |= rxinv;
> 
> And I would keep blank lines after readl() and before writel() since we have
> more than a single line in between.

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9 (-9)
Function                                     old     new   delta
intel_gpio_irq_type                          317     308      -9
Total: Before=10469, After=10460, chg -0.09%
Andy Shevchenko June 15, 2023, 2:09 p.m. UTC | #3 
On Thu, Jun 15, 2023 at 01:35:19PM +0000, Jadav, Raag wrote:
> > On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote:
> > > Refine ->irq_set_type() hook and improve its readability by:
> > >
> > > - Reducing scope of spinlock by moving unneeded operations out of it.
> > > - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly
> > >   into PADCFG0_RXEVCFG_* definitions.
> > > - Utilizing temporary variables for common operations.
> > > - Simplifying if-else-if chain.
> > 
> > Two questions out of curiosity.
> > Do we gain or lose bytes with this?
> 
> add/remove: 0/0 grow/shrink: 1/0 up/down: 33/0 (33)
> Function                                     old     new   delta
> intel_gpio_irq_type                          317     350     +33
> Total: Before=10469, After=10502, chg +0.32%
> 
> > > +	value = readl(reg);
> > 
> > > +	value = (value & ~PADCFG0_RXINV) | rxinv;
> > > +	value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;
> > 
> > Same question if we change this to be similar to the current code, i.e.
> > 
> > 	value = readl(reg);
> > 
> > 	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> > 	value |= rxevcfg;
> > 	value |= rxinv;
> > 
> > And I would keep blank lines after readl() and before writel() since we have
> > more than a single line in between.
> 
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9 (-9)
> Function                                     old     new   delta
> intel_gpio_irq_type                          317     308      -9
> Total: Before=10469, After=10460, chg -0.09%

Do I understand correctly that this is your patch + suggested above?
Raag Jadav June 15, 2023, 3:05 p.m. UTC | #4 
> On Thu, Jun 15, 2023 at 01:35:19PM +0000, Jadav, Raag wrote:
> > > On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote:
> > > > Refine ->irq_set_type() hook and improve its readability by:
> > > >
> > > > - Reducing scope of spinlock by moving unneeded operations out of it.
> > > > - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it
> directly
> > > >   into PADCFG0_RXEVCFG_* definitions.
> > > > - Utilizing temporary variables for common operations.
> > > > - Simplifying if-else-if chain.
> > >
> > > Two questions out of curiosity.
> > > Do we gain or lose bytes with this?
> >
> > add/remove: 0/0 grow/shrink: 1/0 up/down: 33/0 (33)
> > Function                                     old     new   delta
> > intel_gpio_irq_type                          317     350     +33
> > Total: Before=10469, After=10502, chg +0.32%
> >
> > > > +	value = readl(reg);
> > >
> > > > +	value = (value & ~PADCFG0_RXINV) | rxinv;
> > > > +	value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;
> > >
> > > Same question if we change this to be similar to the current code, i.e.
> > >
> > > 	value = readl(reg);
> > >
> > > 	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> > > 	value |= rxevcfg;
> > > 	value |= rxinv;
> > >
> > > And I would keep blank lines after readl() and before writel() since
> > > we have more than a single line in between.
> >
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9 (-9)
> > Function                                     old     new   delta
> > intel_gpio_irq_type                          317     308      -9
> > Total: Before=10469, After=10460, chg -0.09%
> 
> Do I understand correctly that this is your patch + suggested above?

Yes, this is tested with gcc 7.5.0 with default -O2.
I see some reordering in disassembly even with this simple change,
and I'm not entirely sure what kind of weird tricks gcc is pulling here.
Andy Shevchenko June 15, 2023, 3:16 p.m. UTC | #5 
On Thu, Jun 15, 2023 at 03:05:42PM +0000, Jadav, Raag wrote:
> > On Thu, Jun 15, 2023 at 01:35:19PM +0000, Jadav, Raag wrote:

...

> > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9 (-9)
> > > Function                                     old     new   delta
> > > intel_gpio_irq_type                          317     308      -9
> > > Total: Before=10469, After=10460, chg -0.09%
> > 
> > Do I understand correctly that this is your patch + suggested above?
> 
> Yes, this is tested with gcc 7.5.0 with default -O2.
> I see some reordering in disassembly even with this simple change,
> and I'm not entirely sure what kind of weird tricks gcc is pulling here.

For me gcc (Debian 12.2.0-14) 12.2.0 gives the same, +4 bytes.
I am going to apply v4 with suggested tweaks.
Andy Shevchenko June 15, 2023, 3:28 p.m. UTC | #6 
On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote:
> Refine ->irq_set_type() hook and improve its readability by:
> 
> - Reducing scope of spinlock by moving unneeded operations out of it.
> - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly
>   into PADCFG0_RXEVCFG_* definitions.
> - Utilizing temporary variables for common operations.
> - Simplifying if-else-if chain.

Pushed to my review and testing queue, thanks!
diff mbox series

Patch

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index e8adf2580321..036eae74c479 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -55,12 +55,11 @@ 
 
 /* Offset from pad_regs */
 #define PADCFG0				0x000
-#define PADCFG0_RXEVCFG_SHIFT		25
 #define PADCFG0_RXEVCFG_MASK		GENMASK(26, 25)
-#define PADCFG0_RXEVCFG_LEVEL		0
-#define PADCFG0_RXEVCFG_EDGE		1
-#define PADCFG0_RXEVCFG_DISABLED	2
-#define PADCFG0_RXEVCFG_EDGE_BOTH	3
+#define PADCFG0_RXEVCFG_LEVEL		(0 << 25)
+#define PADCFG0_RXEVCFG_EDGE		(1 << 25)
+#define PADCFG0_RXEVCFG_DISABLED	(2 << 25)
+#define PADCFG0_RXEVCFG_EDGE_BOTH	(3 << 25)
 #define PADCFG0_PREGFRXSEL		BIT(24)
 #define PADCFG0_RXINV			BIT(23)
 #define PADCFG0_GPIROUTIOXAPIC		BIT(20)
@@ -1127,9 +1126,9 @@  static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned int pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
+	u32 rxevcfg, rxinv, value;
 	unsigned long flags;
 	void __iomem *reg;
-	u32 value;
 
 	reg = intel_get_padcfg(pctrl, pin, PADCFG0);
 	if (!reg)
@@ -1145,28 +1144,32 @@  static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
 		return -EPERM;
 	}
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
-
-	intel_gpio_set_gpio_mode(reg);
-
-	value = readl(reg);
-
-	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
-
 	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
-		value |= PADCFG0_RXEVCFG_EDGE_BOTH << PADCFG0_RXEVCFG_SHIFT;
+		rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
 	} else if (type & IRQ_TYPE_EDGE_FALLING) {
-		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
-		value |= PADCFG0_RXINV;
+		rxevcfg = PADCFG0_RXEVCFG_EDGE;
 	} else if (type & IRQ_TYPE_EDGE_RISING) {
-		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
+		rxevcfg = PADCFG0_RXEVCFG_EDGE;
 	} else if (type & IRQ_TYPE_LEVEL_MASK) {
-		if (type & IRQ_TYPE_LEVEL_LOW)
-			value |= PADCFG0_RXINV;
+		rxevcfg = PADCFG0_RXEVCFG_LEVEL;
 	} else {
-		value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
+		rxevcfg = PADCFG0_RXEVCFG_DISABLED;
 	}
 
+	if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
+		rxinv = PADCFG0_RXINV;
+	else
+		rxinv = 0;
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+
+	intel_gpio_set_gpio_mode(reg);
+
+	value = readl(reg);
+
+	value = (value & ~PADCFG0_RXINV) | rxinv;
+	value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;
+
 	writel(value, reg);
 
 	if (type & IRQ_TYPE_EDGE_BOTH)