[PATCH v1 0/4] Minor optimizations for Intel pinctrl

[PATCH v1 0/4] Minor optimizations for Intel pinctrl

[Raag Jadav]

This series implements minor optimizations for Intel pinctrl driver.

The numbers are as tested with gcc 7.5.0 and may vary with newer versions.

Raag Jadav (4):
  pinctrl: intel: optimize set_mux hook
  pinctrl: intel: optimize irq_set_type hook
  pinctrl: intel: simplify exit path of set_mux hook
  pinctrl: intel: simplify exit path of gpio_request_enable hook

 drivers/pinctrl/intel/pinctrl-intel.c | 63 +++++++++++++++------------
 1 file changed, 35 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH v1 1/4] pinctrl: intel: optimize set_mux hook

[Raag Jadav]

Utilize a temporary variable for common shift operation
inside ->set_mux() hook and save a few bytes.

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-3 (-3)
Function                                     old     new   delta
intel_pinmux_set_mux                         245     242      -3
Total: Before=10472, After=10469, chg -0.03%

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index c7a71c49df0a..e8adf2580321 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -411,18 +411,19 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	/* Now enable the mux setting for each pin in the group */
 	for (i = 0; i < grp->grp.npins; i++) {
 		void __iomem *padcfg0;
-		u32 value;
+		u32 value, pmode;
 
 		padcfg0 = intel_get_padcfg(pctrl, grp->grp.pins[i], PADCFG0);
-		value = readl(padcfg0);
 
+		value = readl(padcfg0);
 		value &= ~PADCFG0_PMODE_MASK;
 
 		if (grp->modes)
-			value |= grp->modes[i] << PADCFG0_PMODE_SHIFT;
+			pmode = grp->modes[i];
 		else
-			value |= grp->mode << PADCFG0_PMODE_SHIFT;
+			pmode = grp->mode;
 
+		value |= pmode << PADCFG0_PMODE_SHIFT;
 		writel(value, padcfg0);
 	}
 
-- 
2.17.1

Re: [PATCH v1 1/4] pinctrl: intel: optimize set_mux hook

[Mika Westerberg]

On Thu, Jun 08, 2023 at 12:30:14PM +0530, Raag Jadav wrote:
> Utilize a temporary variable for common shift operation
> inside ->set_mux() hook and save a few bytes.
> 
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-3 (-3)
> Function                                     old     new   delta
> intel_pinmux_set_mux                         245     242      -3
> Total: Before=10472, After=10469, chg -0.03%

Shouldn't the compiler be able to optimize this if you ask with the -Ox
options?

I don't really see much benefit for "optimizations" like this. That said
using temporary variable here improves readability so this one is
acceptable by me. As long as you change the commit message accordingly.

> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index c7a71c49df0a..e8adf2580321 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -411,18 +411,19 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  	/* Now enable the mux setting for each pin in the group */
>  	for (i = 0; i < grp->grp.npins; i++) {
>  		void __iomem *padcfg0;
> -		u32 value;
> +		u32 value, pmode;
>  
>  		padcfg0 = intel_get_padcfg(pctrl, grp->grp.pins[i], PADCFG0);
> -		value = readl(padcfg0);
>  
> +		value = readl(padcfg0);
>  		value &= ~PADCFG0_PMODE_MASK;
>  
>  		if (grp->modes)
> -			value |= grp->modes[i] << PADCFG0_PMODE_SHIFT;
> +			pmode = grp->modes[i];
>  		else
> -			value |= grp->mode << PADCFG0_PMODE_SHIFT;
> +			pmode = grp->mode;
>  
> +		value |= pmode << PADCFG0_PMODE_SHIFT;
>  		writel(value, padcfg0);
>  	}
>  
> -- 
> 2.17.1

RE: [PATCH v1 1/4] pinctrl: intel: optimize set_mux hook

[Jadav, Raag]

> On Thu, Jun 08, 2023 at 12:30:14PM +0530, Raag Jadav wrote:
> > Utilize a temporary variable for common shift operation inside
> > ->set_mux() hook and save a few bytes.
> >
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-3 (-3)
> > Function                                     old     new   delta
> > intel_pinmux_set_mux                         245     242      -3
> > Total: Before=10472, After=10469, chg -0.03%
> 
> Shouldn't the compiler be able to optimize this if you ask with the -Ox
> options?

Forgot to add. This is with default -O2.
Is it a good idea to mention it?

> I don't really see much benefit for "optimizations" like this. That said using
> temporary variable here improves readability so this one is acceptable by
> me. As long as you change the commit message accordingly.

Re: [PATCH v1 1/4] pinctrl: intel: optimize set_mux hook

[Mika Westerberg]

On Thu, Jun 08, 2023 at 10:20:58AM +0000, Jadav, Raag wrote:
> > On Thu, Jun 08, 2023 at 12:30:14PM +0530, Raag Jadav wrote:
> > > Utilize a temporary variable for common shift operation inside
> > > ->set_mux() hook and save a few bytes.
> > >
> > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-3 (-3)
> > > Function                                     old     new   delta
> > > intel_pinmux_set_mux                         245     242      -3
> > > Total: Before=10472, After=10469, chg -0.03%
> > 
> > Shouldn't the compiler be able to optimize this if you ask with the -Ox
> > options?
> 
> Forgot to add. This is with default -O2.
> Is it a good idea to mention it?

Yes, I think it is.

[PATCH v1 2/4] pinctrl: intel: optimize irq_set_type hook

[Raag Jadav]

Utilize a temporary variable for common shift operation
inside ->irq_set_type() hook and save a few bytes.
While at it, simplify if-else-if chain.

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
Function                                     old     new   delta
intel_gpio_irq_type                          317     301     -16
Total: Before=10469, After=10453, chg -0.15%

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index e8adf2580321..3f78066b1837 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1128,8 +1128,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
 	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned int pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
 	unsigned long flags;
+	u32 value, rxevcfg;
 	void __iomem *reg;
-	u32 value;
 
 	reg = intel_get_padcfg(pctrl, pin, PADCFG0);
 	if (!reg)
@@ -1150,23 +1150,24 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
 	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)
+		value |= PADCFG0_RXINV;
+
+	value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
 	writel(value, reg);
 
 	if (type & IRQ_TYPE_EDGE_BOTH)
-- 
2.17.1

[PATCH v1 3/4] pinctrl: intel: simplify exit path of set_mux hook

[Raag Jadav]

Simplify exit path of ->set_mux() hook and save a few bytes.

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-22 (-22)
Function                                     old     new   delta
intel_pinmux_set_mux                         242     220     -22
Total: Before=10453, After=10431, chg -0.21%

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3f78066b1837..1b5745202058 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -393,7 +393,10 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	const struct intel_pingroup *grp = &pctrl->soc->groups[group];
 	unsigned long flags;
-	int i;
+	int i, ret;
+
+	/* In case we never really enter any of the below loops */
+	ret = 0;
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
@@ -403,8 +406,8 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	 */
 	for (i = 0; i < grp->grp.npins; i++) {
 		if (!intel_pad_usable(pctrl, grp->grp.pins[i])) {
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-			return -EBUSY;
+			ret = -EBUSY;
+			goto out_unlock;
 		}
 	}
 
@@ -427,9 +430,10 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 		writel(value, padcfg0);
 	}
 
+out_unlock:
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
-	return 0;
+	return ret;
 }
 
 static void __intel_gpio_set_direction(void __iomem *padcfg0, bool input)
-- 
2.17.1

Re: [PATCH v1 3/4] pinctrl: intel: simplify exit path of set_mux hook

[Mika Westerberg]

On Thu, Jun 08, 2023 at 12:30:16PM +0530, Raag Jadav wrote:
> Simplify exit path of ->set_mux() hook and save a few bytes.
> 
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-22 (-22)
> Function                                     old     new   delta
> intel_pinmux_set_mux                         242     220     -22
> Total: Before=10453, After=10431, chg -0.21%
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 3f78066b1837..1b5745202058 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -393,7 +393,10 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>  	const struct intel_pingroup *grp = &pctrl->soc->groups[group];
>  	unsigned long flags;
> -	int i;
> +	int i, ret;
> +
> +	/* In case we never really enter any of the below loops */
> +	ret = 0;

How can that happen?

[PATCH v1 4/4] pinctrl: intel: simplify exit path of gpio_request_enable hook

[Raag Jadav]

Simplify exit path of ->gpio_request_enable() hook
and save a few bytes.

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-36 (-36)
Function                                     old     new   delta
intel_gpio_request_enable                    186     150     -36
Total: Before=10431, After=10395, chg -0.35%

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 1b5745202058..947797d87c93 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -489,20 +489,22 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	void __iomem *padcfg0;
 	unsigned long flags;
+	int ret;
 
 	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
 
+	/* For positive cases */
+	ret = 0;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	if (!intel_pad_owned_by_host(pctrl, pin)) {
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-		return -EBUSY;
+		ret = -EBUSY;
+		goto out_unlock;
 	}
 
-	if (!intel_pad_is_unlocked(pctrl, pin)) {
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-		return 0;
-	}
+	if (!intel_pad_is_unlocked(pctrl, pin))
+		goto out_unlock;
 
 	/*
 	 * If pin is already configured in GPIO mode, we assume that
@@ -510,16 +512,15 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	 * potential glitches on the pin. Otherwise, for the pin in
 	 * alternative mode, consumer has to supply respective flags.
 	 */
-	if (intel_gpio_get_gpio_mode(padcfg0) == PADCFG0_PMODE_GPIO) {
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-		return 0;
-	}
+	if (intel_gpio_get_gpio_mode(padcfg0) == PADCFG0_PMODE_GPIO)
+		goto out_unlock;
 
 	intel_gpio_set_gpio_mode(padcfg0);
 
+out_unlock:
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
-	return 0;
+	return ret;
 }
 
 static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
-- 
2.17.1

Re: [PATCH v1 4/4] pinctrl: intel: simplify exit path of gpio_request_enable hook

[Mika Westerberg]

On Thu, Jun 08, 2023 at 12:30:17PM +0530, Raag Jadav wrote:
> Simplify exit path of ->gpio_request_enable() hook
> and save a few bytes.
> 
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-36 (-36)
> Function                                     old     new   delta
> intel_gpio_request_enable                    186     150     -36
> Total: Before=10431, After=10395, chg -0.35%
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 1b5745202058..947797d87c93 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -489,20 +489,22 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
>  	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>  	void __iomem *padcfg0;
>  	unsigned long flags;
> +	int ret;
>  
>  	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
>  
> +	/* For positive cases */
> +	ret = 0;

You can set it

	int ret = 0;

and drop the useless comment.

> +
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
>  	if (!intel_pad_owned_by_host(pctrl, pin)) {
> -		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> -		return -EBUSY;
> +		ret = -EBUSY;
> +		goto out_unlock;
>  	}
>  
> -	if (!intel_pad_is_unlocked(pctrl, pin)) {
> -		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> -		return 0;
> -	}
> +	if (!intel_pad_is_unlocked(pctrl, pin))
> +		goto out_unlock;
>  
>  	/*
>  	 * If pin is already configured in GPIO mode, we assume that
> @@ -510,16 +512,15 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
>  	 * potential glitches on the pin. Otherwise, for the pin in
>  	 * alternative mode, consumer has to supply respective flags.
>  	 */
> -	if (intel_gpio_get_gpio_mode(padcfg0) == PADCFG0_PMODE_GPIO) {
> -		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> -		return 0;
> -	}
> +	if (intel_gpio_get_gpio_mode(padcfg0) == PADCFG0_PMODE_GPIO)
> +		goto out_unlock;
>  
>  	intel_gpio_set_gpio_mode(padcfg0);
>  
> +out_unlock:
>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
> -- 
> 2.17.1

Re: [PATCH v1 0/4] Minor optimizations for Intel pinctrl

[Andy Shevchenko]

On Thu, Jun 08, 2023 at 12:30:13PM +0530, Raag Jadav wrote:
> This series implements minor optimizations for Intel pinctrl driver.
> 
> The numbers are as tested with gcc 7.5.0 and may vary with newer versions.

Please, follow what Mika commented on on both series and issue v2 of each
of them.

-- 
With Best Regards,
Andy Shevchenko