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

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

On Fri, Jan 31, 2025 at 02:01:51PM +0200, Sakari Ailus wrote:
> The DT bindings for ov7251 specify "enable" GPIO (xshutdown in
> documentation) but the int3472 indiscriminately provides this as a "reset"
> GPIO to sensor drivers. Take this into account by assigning it as "enable"
> with active high polarity for INT347E devices, i.e. ov7251. "reset" with
> active low polarity remains the default GPIO name for other devices.

...

> +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
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