Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 11:08:21 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Raag Jadav <raag.jadav@intel.com>
Cc:     linus.walleij@linaro.org, andriy.shevchenko@linux.intel.com,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        mallikarjunappa.sangannavar@intel.com, pandith.n@intel.com
Subject: Re: [PATCH v1 1/4] pinctrl: intel: optimize set_mux hook
Message-ID: <20230608080821.GF45886@black.fi.intel.com>
References: <20230608070017.28072-1-raag.jadav@intel.com>
 <20230608070017.28072-2-raag.jadav@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230608070017.28072-2-raag.jadav@intel.com>
Precedence: bulk

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