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;
From:   "Jadav, Raag" <raag.jadav@intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC:     "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "mika.westerberg@linux.intel.com" <mika.westerberg@linux.intel.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Sangannavar, Mallikarjunappa" 
        <mallikarjunappa.sangannavar@intel.com>,
        "N, Pandith" <pandith.n@intel.com>
Subject: RE: [PATCH v3 2/3] pinctrl: intel: refine ->irq_set_type() hook
Thread-Topic: [PATCH v3 2/3] pinctrl: intel: refine ->irq_set_type() hook
Thread-Index: AQHZndRGEYk7XkvCzE+g3a/tygkcuq+KfTeAgAEjDpA=
Date:   Thu, 15 Jun 2023 09:48:12 +0000
Message-ID: <DM6PR11MB2779F9C28712D7C25F9FB3768C5BA@DM6PR11MB2779.namprd11.prod.outlook.com>
References: <20230613085054.10976-1-raag.jadav@intel.com>
 <20230613085054.10976-3-raag.jadav@intel.com>
 <ZInpT0dUUVUcKdqv@smile.fi.intel.com>
In-Reply-To: <ZInpT0dUUVUcKdqv@smile.fi.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?xqBnOMEnaexh4Qv8cdYbQZxDOIKIXYGMwTvMWqyD/cQzHZAqrl+emn7p4nqd?=
 =?us-ascii?Q?6YMmdvEtEdUSnIWsHrPbCcK5tTgtpFfbPUb/BCH2GYAtdw4AbUs/lJFwXtzu?=
 =?us-ascii?Q?GUMkfO52BQttmPRENloQA8GLmKyWxqXLCIttQbEK5P2Dxc0u8ygO2fYTuf5h?=
 =?us-ascii?Q?iJFHySpPp6+DsBnvOH5w8ihz+fuMpxkukm8HgO6x3Ih1OvytWpSphFtRDulF?=
 =?us-ascii?Q?9LGFmPM9MIhQLEw6NA6iz3aMDD/oQgB3+Jfy6uFFrqDwtiD7RChCkXMDqdLw?=
 =?us-ascii?Q?eJMqWhTMcuDYKdjYKtykDjspNzfzt632sNuO8fxE0PkbP7FPUMAiKKO8gKv3?=
 =?us-ascii?Q?Eo4UhZA+1cXC+qwsBHjR9sVTpE5lGdHH1E7BNMTUuSMNWNH0vS5XJfW1ScAT?=
 =?us-ascii?Q?9QmX8Cs+2QmkIOGye9fe931+02NhlhjdAgZQzODWxTis90J1R8f6L7aJaNXb?=
 =?us-ascii?Q?1Vbcv62eUVXNJDAuIacNaMMOAVLfFIpZczOKcANU9mGYIE3N8QApha8FWHHR?=
 =?us-ascii?Q?lbduC3mEc+HrJtKyKqCz/5xpTLm4ojt/4F8U63WcL8sQtrrVeYgCaZ/vZnfW?=
 =?us-ascii?Q?p5ZXdmkEZF0iOwfP3sBkD53A0aRNx+MJoYaoi2et5XPvM0pbpTy60zXUyB4m?=
 =?us-ascii?Q?33bM0W4IIx53vGDsKGRSYH4zEPNt26+3o/oFNZxBj3ybe365dE2tjnXkqhw4?=
 =?us-ascii?Q?uwkwayJIw6LocqPq+3/0fn0xbPaOjTSJtebD8CssR7aJ6Pw3KqldiQx42H8R?=
 =?us-ascii?Q?yXRQD4FuaJxg+uhXAQtdTJUgft27qYlr3HAY/fg1giq2Gh19XsHugaxCURsn?=
 =?us-ascii?Q?4XG/a+dP5UhNjX9nam6RrnwWQlxbQdRpfks7oVB1tI+L921/NHT2PTPntfE6?=
 =?us-ascii?Q?31lS8VQ7I55VVZtR4VPKgOHsZGegNOFD+q2tWYh/nEVShJ+huLROp5OxD7uR?=
 =?us-ascii?Q?hYK6ygmHl4P1RNI3mArmRuB4q/R3WinjLCLHqNXkdtGU3AhIAKWnRmRE0hk6?=
 =?us-ascii?Q?zzbfYfY10C2c8QfoLyAxwAPPrRzaRfnfLheQ/pKZwnkr8W+Lw4T0BJBvhWRw?=
 =?us-ascii?Q?DmoucsSsH/tBVahRnH0Q0mB/G3uLRcBEEzYc52hNCLdE8PKdEBmiGZJm3LJX?=
 =?us-ascii?Q?TARric/nTuV09Ry0rlr42gDcf9vz2lTzFu1bnKxyDIzLlW7pt9Xzz8axh6vm?=
 =?us-ascii?Q?7AUVVPTtGn+qMCXg7YPJLXHgzAZ33UwLVQVbqGX/HXNCtAdrCYOgD+4hp6Nn?=
 =?us-ascii?Q?XIIEVc3e5e8/sfSIEg8+8au+ue6Gp5zRdGudTeQqN5Z+EV7vtNSxeWQZKq6/?=
 =?us-ascii?Q?sneXBl6doF/YkAYzF50YtNo8ZXGdrK3BH02sBAbbaL6ZxtW9ByEHlblf9HES?=
 =?us-ascii?Q?9psBa1U+oSI7Frmb8UiBhWiQayjlORsvTxwkL7GtqHIb0aOjZf84HBJ9U0VU?=
 =?us-ascii?Q?JY8GT8K6ZUNol4DfUGHjK/fEEQ+3AOP6hy5Dfc/peOspxpTROYO0hFeA+12t?=
 =?us-ascii?Q?/ofZKXgcqH1IScz+jguX9VAP6zuiraPkejOAPLacnCHSsBR5uvqAQG0zFGIa?=
 =?us-ascii?Q?H4fP2qpreaSkT7ffD40=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB2779.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 39164cd2-194f-4cc1-c9af-08db6d85a29c
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Jun 2023 09:48:12.8533
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 5of06UQcqmlVTeNl5tYrBctxbzwA0RzKE4DjLyPn3ExQEGGrw5IhAFBa3duSeqvUF9CTHpG0BzgwOIOww69bFw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7409
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
> > Utilize a temporary variable for common shift operation in
> > ->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=3D10469, After=3D10453, chg -0.15%
>=20
> ...
>=20
> >  	value =3D readl(reg);
> > -
> >  	value &=3D ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> >
> >  	if ((type & IRQ_TYPE_EDGE_BOTH) =3D=3D IRQ_TYPE_EDGE_BOTH) {
> > -		value |=3D PADCFG0_RXEVCFG_EDGE_BOTH <<
> PADCFG0_RXEVCFG_SHIFT;
> > +		rxevcfg =3D PADCFG0_RXEVCFG_EDGE_BOTH;
> >  	} else if (type & IRQ_TYPE_EDGE_FALLING) {
> > -		value |=3D PADCFG0_RXEVCFG_EDGE <<
> PADCFG0_RXEVCFG_SHIFT;
> > -		value |=3D PADCFG0_RXINV;
> > +		rxevcfg =3D PADCFG0_RXEVCFG_EDGE;
> >  	} else if (type & IRQ_TYPE_EDGE_RISING) {
> > -		value |=3D PADCFG0_RXEVCFG_EDGE <<
> PADCFG0_RXEVCFG_SHIFT;
> > +		rxevcfg =3D PADCFG0_RXEVCFG_EDGE;
> >  	} else if (type & IRQ_TYPE_LEVEL_MASK) {
> > -		if (type & IRQ_TYPE_LEVEL_LOW)
> > -			value |=3D PADCFG0_RXINV;
> > +		rxevcfg =3D PADCFG0_RXEVCFG_LEVEL;
> >  	} else {
> > -		value |=3D PADCFG0_RXEVCFG_DISABLED <<
> PADCFG0_RXEVCFG_SHIFT;
> > +		rxevcfg =3D PADCFG0_RXEVCFG_DISABLED;
> >  	}
> >
> > +	if (type =3D=3D IRQ_TYPE_EDGE_FALLING || type =3D=3D
> IRQ_TYPE_LEVEL_LOW)
> > +		value |=3D PADCFG0_RXINV;
> > +
> > +	value |=3D rxevcfg << PADCFG0_RXEVCFG_SHIFT;
> >  	writel(value, reg);
>=20
> Looking at this I realized that entire temporary variable assignments can=
 be
> done outside of spin lock. You probably would need another one for keepin=
g
> rxinv value.

Something like this?

        u32 value, rxevcfg;
        u32 rxinv =3D 0;

        if ((type & IRQ_TYPE_EDGE_BOTH) =3D=3D IRQ_TYPE_EDGE_BOTH) {
                rxevcfg =3D PADCFG0_RXEVCFG_EDGE_BOTH;
        } else if (type & IRQ_TYPE_EDGE_FALLING) {
                rxevcfg =3D PADCFG0_RXEVCFG_EDGE;
        } else if (type & IRQ_TYPE_EDGE_RISING) {
                rxevcfg =3D PADCFG0_RXEVCFG_EDGE;
        } else if (type & IRQ_TYPE_LEVEL_MASK) {
                rxevcfg =3D PADCFG0_RXEVCFG_LEVEL;
        } else {
                rxevcfg =3D PADCFG0_RXEVCFG_DISABLED;
        }

        if (type =3D=3D IRQ_TYPE_EDGE_FALLING || type =3D=3D IRQ_TYPE_LEVEL=
_LOW)
                rxinv =3D PADCFG0_RXINV;

        raw_spin_lock_irqsave(&pctrl->lock, flags);

        intel_gpio_set_gpio_mode(reg);

        value =3D readl(reg);

        value &=3D ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
        value |=3D rxinv;
        value |=3D rxevcfg << PADCFG0_RXEVCFG_SHIFT;

        writel(value, reg);

> Will it give us any memory reduction in comparison to the current code?

add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
Function                                     old     new   delta
intel_gpio_irq_type                          317     321      +4
Total: Before=3D10469, After=3D10473, chg +0.04%

Unfortunately gcc doesn't seem to consider this as best of the sequence,
and I'm not entirely sure why.