[PATCH v2 0/4] Minor improvements for Intel pinctrl

[PATCH v2 0/4] Minor improvements for Intel pinctrl

[Raag Jadav]

This series implements minor improvements for Intel pinctrl driver.

The optimizations are as tested with gcc 7.5.0 with default -O2.

Raag Jadav (4):
  pinctrl: intel: refine set_mux hook
  pinctrl: intel: refine 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 | 57 ++++++++++++++-------------
 1 file changed, 29 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] pinctrl: intel: refine set_mux hook

[Raag Jadav]

Utilize a temporary variable for common shift operation
inside ->set_mux() hook and improve readability.

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 v2 1/4] pinctrl: intel: refine set_mux hook

[Andy Shevchenko]

On Fri, Jun 09, 2023 at 01:55:36PM +0530, Raag Jadav wrote:
> Utilize a temporary variable for common shift operation
> inside ->set_mux() hook and improve readability.

Seems the bloat-o-meter statistics is missing here.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 1/4] pinctrl: intel: refine set_mux hook

[Jadav, Raag]

> On Fri, Jun 09, 2023 at 01:55:36PM +0530, Raag Jadav wrote:
> > Utilize a temporary variable for common shift operation inside
> > ->set_mux() hook and improve readability.
> 
> Seems the bloat-o-meter statistics is missing here.

I changed the commit message as pointed out by Mika in v1.
Do I change it back?

Re: [PATCH v2 1/4] pinctrl: intel: refine set_mux hook

[Andy Shevchenko]

On Mon, Jun 12, 2023 at 07:15:59AM +0000, Jadav, Raag wrote:
> > On Fri, Jun 09, 2023 at 01:55:36PM +0530, Raag Jadav wrote:
> > > Utilize a temporary variable for common shift operation inside
> > > ->set_mux() hook and improve readability.
> > 
> > Seems the bloat-o-meter statistics is missing here.
> 
> I changed the commit message as pointed out by Mika in v1.
> Do I change it back?

No back, only forward!

I.e. you need to update Subject to follow the pattern, ->set_mux()
in this case) and add a note that the change makes code smaller.
It will be two selling points.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/4] pinctrl: intel: refine irq_set_type hook

[Raag Jadav]

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

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 v2 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 | 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 3f78066b1837..2338e84615cc 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -393,7 +393,7 @@ 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 = 0;
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
@@ -403,8 +403,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 +427,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 v2 3/4] pinctrl: intel: simplify exit path of set_mux hook

[Mika Westerberg]

On Fri, Jun 09, 2023 at 01:55:38PM +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 | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

This adds one more line so it is not simplifying ;-) I think the
original code looks better.

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

[Jadav, Raag]

> On Fri, Jun 09, 2023 at 01:55:38PM +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 | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> This adds one more line so it is not simplifying ;-)

Would "optimize" sound any better?

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

[Mika Westerberg]

On Fri, Jun 09, 2023 at 09:10:42AM +0000, Jadav, Raag wrote:
> > On Fri, Jun 09, 2023 at 01:55:38PM +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 | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > This adds one more line so it is not simplifying ;-)
> 
> Would "optimize" sound any better?

No, I think this patch is not useful at all.

[PATCH v2 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 | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 2338e84615cc..12b994fa2aad 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -486,20 +486,19 @@ 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 = 0;
 
 	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
 
 	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
@@ -507,16 +506,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