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>,
        "mika.westerberg@linux.intel.com" <mika.westerberg@linux.intel.com>
CC:     "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "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+KfTeAgAEjDpCAAAMMgIAAD0uAgAADFKA=
Date:   Thu, 15 Jun 2023 11:08:38 +0000
Message-ID: <DM6PR11MB2779664C590484A1601A93298C5BA@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>
 <DM6PR11MB2779F9C28712D7C25F9FB3768C5BA@DM6PR11MB2779.namprd11.prod.outlook.com>
 <20230615095517.GV45886@black.fi.intel.com>
 <ZIrs2YSEUbPyvZWE@smile.fi.intel.com>
In-Reply-To: <ZIrs2YSEUbPyvZWE@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?7aipP4aCpn+X9dmWpKg6+Gwyq80gQMHn8Ts3R69Gu+4lZrUGKL4DVuKOLc63?=
 =?us-ascii?Q?lIbha2YfSp+P4xn0STroKU80hH8xNY2ifUg/ZcKoYrGBh171kLqkXTqY7Z/Z?=
 =?us-ascii?Q?+2r8j3rurBW5AtA4AElZDQ+Y8yyrwLmf1cZuvU5uRHSMDaBxSGmTvp8Vl5Hq?=
 =?us-ascii?Q?qv6L226BuL5oxSysrAtkw7yxOm90+aG6WtUtq5ivTkZt9FmFwip+1tbhSS3P?=
 =?us-ascii?Q?o285qcw8v1iKo3+s4KNzsYwTKCc7Ylg+4rgm+JmxJ+v3Ch6rabv2wXpWJlO8?=
 =?us-ascii?Q?sa8UFn1INjI31klUIb5Z9K9TvCzlkaF4612K4LIR9iZ4AML5otf6D4uICEMF?=
 =?us-ascii?Q?HP2HfhlXGvOxLgZS2IGxtBWp5m6fhgdEpApmg3seqOnJcarajpf0ilxgWcRf?=
 =?us-ascii?Q?OVkXaO+T8z4g00YZH0LNm/WN3F+rQ9r8eikQHZFC6lZLi27H5DgSgz7D4qSq?=
 =?us-ascii?Q?5oVaXzBeR37CM1v8UKIt0V/2/WbAuUDSTM899LmcYZPP3X12WEQvVkM16EOr?=
 =?us-ascii?Q?RS8gfZpbZT+isvvFUr53laRZbm6rAvAa49BmyHIZMo0VhV4NL53+oM7/17ZR?=
 =?us-ascii?Q?ksKAvDe9c/tXrknaoWs8ELbAOut7REbFtQss/s9fMV0e69z0BZiHlOoJSj+e?=
 =?us-ascii?Q?xRUTiE2aeSt0/HOvqdUqzsDKb1P/r8j90Sg33+B885v49TdGCC+GrCMws5f9?=
 =?us-ascii?Q?bbrh0Npna9E0+wp7xKamjRTcAeQ2AHk63rcjPrSWOD/faD6Uw4nVNNcojskJ?=
 =?us-ascii?Q?NGG98Bgn8S4r0DJp9vJg7ZbCE29yZBAaAO7vqooPSmvwhe97CKDESxc+PckK?=
 =?us-ascii?Q?HWfhLUFZjVjCTd8vEFpNliLIK77F85EjGyZycIcxxGXgtoymPf1Fslp9gLkV?=
 =?us-ascii?Q?yOSr9/KUeilyv8mRD2QN3wMqLrd/gy1SZ+iXCaQBkZYE5uMC0XbI9wd6Hw91?=
 =?us-ascii?Q?YcZ17GX853K2Gqo16STUZtaM1/ggHnu9IjPQk+cZWRi36TxqLapbzMkUAnyS?=
 =?us-ascii?Q?tUptAwvTDxcpQPcniFtdyO5pxlSBr4o1/jq7a3mktHwBZ4p5iux/RAasA0+g?=
 =?us-ascii?Q?E5Vd8RzVISC4i/tvgUioT3lhu+U4O1OLtAjbwXnZPG3Dqy8Mk+jJ4F1QdivM?=
 =?us-ascii?Q?ROVLNEd80wKSxAtn7iUGk5soXYM1xcCSOHOLyB/QNuKrjJw9mYzk6no1SP5F?=
 =?us-ascii?Q?mv1Xyn2GH9a3BzTqSsRjlXkjgcFQoyf1E4PQllWEzPfnx3YD5bsGEA4/RbT3?=
 =?us-ascii?Q?59EbncQVceuqpyByedDER7x6KRXlj4skPv+fs3amGqcxjptizI7dIqvCXWEX?=
 =?us-ascii?Q?dKvA8dDvkwgaSPNUGsRDG0km55t7NiDUhQAAq0orR6uTTFDvl4IDCfvH46l5?=
 =?us-ascii?Q?qO94WORRgcvDSxD0126/rFr4XVjIPtYO1xZrMDtXHHpnd4jSEgl7Iiy0p7nB?=
 =?us-ascii?Q?9kudPUHOyivKONI0eVrSkf788Tt9D1hmQ1cAgLzbewuKldwvg4WDLdz3le+Q?=
 =?us-ascii?Q?RespNPU1D6lYUV0eHi89wxHizfOV7TFnKDX+YuhtycqdNBwHoAClHwMuCY2N?=
 =?us-ascii?Q?I8LMkpOa+z/qwPnMEdc=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: 135d6236-8aab-404a-38a3-08db6d90df2d
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Jun 2023 11:08:39.0015
 (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: P+gm0+MJOYG7KcLqREy6PN3od1a2UaQOtsJ6p5Qj0AVfFYod00D+lOEwgsDhmz4VfkbyTvY9bNfD11xBqTQfMw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6317
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 Thu, Jun 15, 2023 at 12:55:17PM +0300,
> mika.westerberg@linux.intel.com wrote:
> > On Thu, Jun 15, 2023 at 09:48:12AM +0000, Jadav, Raag wrote:
> > > > On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
>=20
> ...
>=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 keeping rxinv value.
> > >
> > > Something like this?
>=20
> Almost, see below.
>=20
> > >         u32 value, rxevcfg;
> > >         u32 rxinv =3D 0;
>=20
> No assignment here.
>=20
>          u32 rxinv, rxevcfg;
>          u32 value;
>=20
> > >         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;
> > >         }
>=20
> Now, if it's fully included in the diff (even with --patience parameter),=
 then
> you may drop {}.
>=20
> > >         if (type =3D=3D IRQ_TYPE_EDGE_FALLING || type =3D=3D
> IRQ_TYPE_LEVEL_LOW)
> > >                 rxinv =3D PADCFG0_RXINV;
>=20
> 		else
> 			rxinv =3D 0;
>=20
> > >         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;
>=20
> And I would rewrite these to the standard patterns:
>=20
>          value =3D (value & ~PADCFG0_RXINV) | rxinv;
>          value =3D (value & ~PADCFG0_RXEVCFG_MASK) | (rxevcfg <<
> PADCFG0_RXEVCFG_SHIFT);
>=20
> And looking at this, perhaps do shift also outside the lock:
>=20
>          } else {
>                  rxevcfg =3D PADCFG0_RXEVCFG_DISABLED;
>          }
>          rxevcfg <<=3D PADCFG0_RXEVCFG_SHIFT;
>=20
> But, taking into account scope of the _RXEVCFG_*, I would add shift direc=
tly
> to the definitions and kill that SHIFT entirely:
>=20
> #define PADCFG0_RXEVCFG_LEVEL           (0 << 25)
> #define PADCFG0_RXEVCFG_EDGE            (1 << 25)
> #define PADCFG0_RXEVCFG_DISABLED        (2 << 25)
> #define PADCFG0_RXEVCFG_EDGE_BOTH       (3 << 25)
>=20
> 	 ...
>=20
>          value =3D (value & ~PADCFG0_RXINV) | rxinv;
>          value =3D (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;
>=20
> Try that one and look if it looks better. It might even save bytes after =
all.

Should I add all of this in original patch or send this as a separate patch
on top this series?