diff mbox series

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

Message ID 20230608070017.28072-2-raag.jadav@intel.com
State New
Headers
Series Minor optimizations for Intel pinctrl |

Commit Message

Raag Jadav June 8, 2023, 7 a.m. UTC 
  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(-)

Comments

Mika Westerberg June 8, 2023, 8:08 a.m. UTC | #1 
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
Raag Jadav June 8, 2023, 10:20 a.m. UTC | #2 
> 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.
Mika Westerberg June 8, 2023, 10:56 a.m. UTC | #3 
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.
diff mbox series

Patch

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);
 	}