Hi Andy,
On Fri, Feb 07, 2025 at 05:30:40PM +0200, Andy Shevchenko wrote:
> 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?
Right, I was accidentally thinking of a driver here. How about:
Map the firmware-provided GPIO type to whatever a driver expects (as in DT
bindings). First check if the type matches with the GPIO map, then further
check that the device _HID matches.
>
> > + * 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;
> > + }
>
--
Regards,
Sakari Ailus
