Hi Andy,
On Wed, Jan 22, 2025 at 06:51:06PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 22, 2025 at 12:43:44PM +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.
>
> ...
>
> > +/**
> > + * struct int3472_gpio_map - Map GPIOs to whatever is expected by the
> > + * sensor driver (as in DT bindings)
> > + * @hid: The ACPI HID of the device without the instance number e.g. i2c-INT347E
>
> W/o "i2c-" part.
>
> > + * @type_from: The GPIO type from ACPI ?SDT
> > + * @type_to: The assigned GPIO type, typically same as type_from
>
> @type_from
Agreed.
>
> > + * @func: The function, e.g. "enable"
>
> Should we speak in terms of GPIOLIB, like connection ID ?
That rename should be done in the entire driver probably then. I can post a
patch if Hans agrees, after this one.
>
> > + * @polarity: GPIO_ACTIVE_{HIGH,LOW}
>
> Please, avoid using patterns with the defined constants. It's better to have
> this written as
>
> * @polarity: One of %GPIO_ACTIVE_HIGH, %GPIO_ACTIVE_LOW
Sounds good.
>
> > + */
>
> > + const char *hid;
> > + u8 type_from;
> > + u8 type_to;
> > + const char *func;
> > + unsigned int polarity;
>
> Hmm... In other cases we usually use
>
> bool active_low;
>
> Can we do the same here?
This goes to the flags field of struct gpiod_lookup. Bool is a poor choice
for that (but u32 isn't correct either). We can put polarity here but pass
GPIO_ACTIVE_{HIGH,LOW} to GPIO_LOOKUP().
Putting polarity before function would same some bytes, too. Hans, any
thoughts?
>
> > +};
>
> ...
>
> > - int3472_get_func_and_polarity(type, &func, &polarity);
> > + int3472_get_func_and_polarity(int3472->sensor, &type, &func,
> > + &polarity);
>
> AFAIK, we don't have hard attachment to the 80-[character limit rule, please
> use more room on the previous line.
There's no reason for the line to be above 80 characters.
--
Regards,
Sakari Ailus
