Re: [PATCH v5 2/3] platform/x86: int3472: Call "reset" GPIO "enable" for INT347E

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

[-- Attachment #1: Type: text/plain, Size: 4623 bytes --]

On Mon, 3 Feb 2025, Sakari Ailus wrote:

> Hi Ilpo,
> 
> On Mon, Feb 03, 2025 at 10:03:14AM +0200, Ilpo Järvinen wrote:
> > On Fri, 31 Jan 2025, 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.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/platform/x86/intel/int3472/discrete.c | 47 +++++++++++++++++--
> > >  1 file changed, 43 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> > > index 3f7624714869..529ea2d08a21 100644
> > > --- a/drivers/platform/x86/intel/int3472/discrete.c
> > > +++ b/drivers/platform/x86/intel/int3472/discrete.c
> > > @@ -2,6 +2,7 @@
> > >  /* Author: Dan Scally <djrscally@gmail.com> */
> > >  
> > >  #include <linux/acpi.h>
> > > +#include <linux/array_size.h>
> > >  #include <linux/bitfield.h>
> > >  #include <linux/device.h>
> > >  #include <linux/gpio/consumer.h>
> > > @@ -122,10 +123,48 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
> > >  	return desc;
> > >  }
> > >  
> > > -static void int3472_get_func_and_polarity(u8 type, const char **func,
> > > -					  unsigned long *gpio_flags)
> > > +/**
> > > + * 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. INT347E
> > > + * @type_from: The GPIO type from ACPI ?SDT
> > > + * @type_to: The assigned GPIO type, typically same as @type_from
> > > + * @func: The function, e.g. "enable"
> > > + * @polarity_low: GPIO_ACTIVE_LOW true if the @polarity_low is true,
> > > + * GPIO_ACTIVE_HIGH otherwise
> > > + */
> > > +struct int3472_gpio_map {
> > > +	const char *hid;
> > > +	u8 type_from;
> > > +	u8 type_to;
> > > +	bool polarity_low;
> > > +	const char *func;
> > > +};
> > > +
> > > +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++) {
> > > +		if (*type != int3472_gpio_map[i].type_from)
> > > +			continue;
> > > +
> > > +		if (!acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> > > +			continue;
> > > +
> > > +		*type = int3472_gpio_map[i].type_to;
> > > +		*gpio_flags = int3472_gpio_map[i].polarity_low ?
> > > +			GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
> > 
> > Don't start this continuation line left of = sign unless you really 
> > really have to do that, and it's not such a case here!
> 
> Why? The documentation says the subsequent lines should be aligned
> "substantially" (I believe a tab stop qualifies), except in cases of
> arguments in parentheses just right of the opening parenthesis but that's
> not the case here.

Because I say so, it's not substancial to me when it's left of =. This is 
not negotiable.

-- 
 i.


> I can submit v6 with that if others agree.
> 
> > 
> > Either put GPIO_ACTIVE_LOW on the first line and align the defines, or 
> > align the second line as it is with int3472_gpio_map[...].
> > 
> > > +		*func = int3472_gpio_map[i].func;
> > > +		return;
> > > +	}
> > > +
> > > +	switch (*type) {
> > >  	case INT3472_GPIO_TYPE_RESET:
> > >  		*func = "reset";
> > >  		*gpio_flags = GPIO_ACTIVE_LOW;
> > > @@ -218,7 +257,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
> > >  
> > >  	type = FIELD_GET(INT3472_GPIO_DSM_TYPE, obj->integer.value);
> > >  
> > > -	int3472_get_func_and_polarity(type, &func, &gpio_flags);
> > > +	int3472_get_func_and_polarity(int3472->sensor, &type, &func, &gpio_flags);
> > >  
> > >  	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
> > >  	if (pin != agpio->pin_table[0])
> > > 
> 
>