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

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

On Fri, Feb 07, 2025 at 03:41:25PM +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 const struct int3472_gpio_map int3472_gpio_map[] = {
> +	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, false, "enable" },
> +};
> +
> +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++) {
> +		/*
> +		 * Map the firmware-provided GPIO to whatever a driver expects
> +		 * (as in DT bindings). First check if the requested GPIO name

What name?

> +		 * matches the GPIO map, then see that the device _HID matches.
> +		 */
> +		if (*type != int3472_gpio_map[i].type_from)
> +			continue;
> +
> +		if (!acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> +			continue;

I still think this is unusual and confusing order of checks.

At the end, it is up to the PDx86 maintainers.

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