On Thu, 23 Jan 2025, Sakari Ailus wrote:
> 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.
Do you mean that on a single line it would not exceed 80 characters (or
that you just did not count at all)? :-)
I'm like 'What?' ...I don't know why you guys are arguing about breaking
the 80 chars limit. :-D
--
i.
