On Thu, Jan 23, 2025 at 09:14:56AM +0000, Sakari Ailus wrote:
> 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:
...
> > > + * @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.
Ah, this makes sense. I didn't pay attention that this is already in use
in the driver.
...
> > > + 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().
Maybe then name it as gpio_flags or so to match that structure member?
...
> Putting polarity before function would same some bytes, too. Hans, any
> thoughts?
I'm fine with that. I would also save bytes in the cases when it doesn't
affect code generation (and here seems the case). Also, if we talk about
readability of the each quirk entry the current implementation calls for
a macro to make it neater. In such a case, we save double type and may
put macro's arguments in a better order than structure, which may be optimised
for size.
...
> > > - 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.
It's the opposite. There is no reason to make it two lines.
It's not a v4l2 subsystem, we are not cargo-cult here, I believe.
I.o.w. a common sense should be the first one to be considered.
Now, I even tried myself, and...
int3472_get_func_and_polarity(int3472->sensor, &type, &func, &polarity);
...ha-ha, it's exactly 80 characters! What's wrong with your editor settings?
--
With Best Regards,
Andy Shevchenko
