Re: [PATCH v3 1/1] platform/x86: int3472: Call "reset" GPIO "enable" for INT347E

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.