Re: [PATCH v5 2/3] platform/x86: int3472: Call "reset" GPIO "enable" for INT347E

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Feb 03, 2025 at 07:42:06AM +0000, Sakari Ailus wrote:
> On Fri, Jan 31, 2025 at 07:18:54PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 31, 2025 at 02:01:51PM +0200, Sakari Ailus wrote:

...

> > > +static void int3472_get_func_and_polarity(struct acpi_device *adev, u8 *type,
> > > +					  const char **func, unsigned long *gpio_flags)
> > >  {
> > > -	switch (type) {
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(int3472_gpio_map); i++) {
> > > +		if (*type != int3472_gpio_map[i].type_from)
> > > +			continue;
> > 
> > > +		if (!acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> > > +			continue;
> > 
> > Hmm... But why? It's more natural to test if the device even present before
> > continue to check the details of the quirk. This order looks suspicious
> 
> From the result point of view there's no difference. It makes sense to test
> an integer before a rather more elaborate acpi_dev_hid_uid_match(). I don't
> think that needs a comment.

If it doesn't need a comment, it doesn't need to be like this.
Semantically and logically it's better to read:

1. Check the device first, if not, skip the quirk.
2. For the checked device, check if the type is what we are expecting, if not, continue.
3. Perform other checks.
4. Apply a quirk.

> > and unusual. At bare minimum it needs a comment. I.o.w. the Q here is "Why is
> > the type_from check superior to the device?"
> > 
> > > +		*type = int3472_gpio_map[i].type_to;
> > > +		*gpio_flags = int3472_gpio_map[i].polarity_low ?
> > > +			GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
> > > +		*func = int3472_gpio_map[i].func;
> > > +		return;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko