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

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

[Sakari Ailus]

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>
---
since v2:

- Implement a more generic GPIO mangling capability, as suggested by Hans
  de Goede.

- Include linux/array_size.h for ARRAY_SIZE().

 drivers/platform/x86/intel/int3472/discrete.c | 43 +++++++++++++++++--
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index d881b2cfcdfc..181675e57c39 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,9 +123,44 @@ 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, u32 *polarity)
+/**
+ * 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
+ * @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: GPIO_ACTIVE_{HIGH,LOW}
+ */
+struct int3472_gpio_map {
+	const char *hid;
+	u8 type_from;
+	u8 type_to;
+	const char *func;
+	unsigned int polarity;
+};
+
+static const struct int3472_gpio_map int3472_gpio_map[] = {
+	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, "enable", GPIO_ACTIVE_HIGH },
+};
+
+static void int3472_get_func_and_polarity(struct acpi_device *adev, u8 *type,
+					  const char **func, u32 *polarity)
 {
-	switch (type) {
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(int3472_gpio_map); i++) {
+		if (*type != int3472_gpio_map[i].type_from ||
+		    !acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
+			continue;
+
+		*type = int3472_gpio_map[i].type_to;
+		*func = int3472_gpio_map[i].func;
+		*polarity = int3472_gpio_map[i].polarity;
+		return;
+	}
+
+	switch (*type) {
 	case INT3472_GPIO_TYPE_RESET:
 		*func = "reset";
 		*polarity = GPIO_ACTIVE_LOW;
@@ -217,7 +253,8 @@ 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, &polarity);
+	int3472_get_func_and_polarity(int3472->sensor, &type, &func,
+				      &polarity);
 
 	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
 	if (pin != agpio->pin_table[0])
-- 
2.39.5

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

[Hans de Goede]

Hi Sakari,

On 22-Jan-25 11:43 AM, 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>
> ---
> since v2:
> 
> - Implement a more generic GPIO mangling capability, as suggested by Hans
>   de Goede.
> 
> - Include linux/array_size.h for ARRAY_SIZE().

Thank you for the new version.

This looks good to me now:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans





> 
>  drivers/platform/x86/intel/int3472/discrete.c | 43 +++++++++++++++++--
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index d881b2cfcdfc..181675e57c39 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,9 +123,44 @@ 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, u32 *polarity)
> +/**
> + * 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
> + * @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: GPIO_ACTIVE_{HIGH,LOW}
> + */
> +struct int3472_gpio_map {
> +	const char *hid;
> +	u8 type_from;
> +	u8 type_to;
> +	const char *func;
> +	unsigned int polarity;
> +};
> +
> +static const struct int3472_gpio_map int3472_gpio_map[] = {
> +	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, "enable", GPIO_ACTIVE_HIGH },
> +};
> +
> +static void int3472_get_func_and_polarity(struct acpi_device *adev, u8 *type,
> +					  const char **func, u32 *polarity)
>  {
> -	switch (type) {
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(int3472_gpio_map); i++) {
> +		if (*type != int3472_gpio_map[i].type_from ||
> +		    !acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> +			continue;
> +
> +		*type = int3472_gpio_map[i].type_to;
> +		*func = int3472_gpio_map[i].func;
> +		*polarity = int3472_gpio_map[i].polarity;
> +		return;
> +	}
> +
> +	switch (*type) {
>  	case INT3472_GPIO_TYPE_RESET:
>  		*func = "reset";
>  		*polarity = GPIO_ACTIVE_LOW;
> @@ -217,7 +253,8 @@ 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, &polarity);
> +	int3472_get_func_and_polarity(int3472->sensor, &type, &func,
> +				      &polarity);
>  
>  	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
>  	if (pin != agpio->pin_table[0])

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

[Andy Shevchenko]

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

> + * @func: The function, e.g. "enable"

Should we speak in terms of GPIOLIB, like connection ID ?

> + * @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

> + */

> +	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?

> +};

...

> -	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.

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

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

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

[Hans de Goede]

Hi Sakari,

On 23-Jan-25 10:14 AM, 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.

Yes renaming func to con_id makes sense, but that should indeed be
a separate change in a follow-up patch.

Regards,

Hans

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

[Ilpo Järvinen]

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.

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

[Andy Shevchenko]

On Thu, Jan 23, 2025 at 12:47:34PM +0200, Ilpo Järvinen wrote:
> On Thu, 23 Jan 2025, Sakari Ailus wrote:

...

> I'm like 'What?' ...I don't know why you guys are arguing about breaking the
> 80 chars limit. :-D

Sakari is too pedantic about this, and I'm the opposite.
It's our main disagreement on the kernel code during the reviews.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

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

[Hans de Goede]

Hi Sakari,

On 23-Jan-25 10:14 AM, 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:

....

>>> -	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.

I known that under drivers/media the consensus is to try and stick to
the 80 chars length limit. But under drivers/platform/x86 we have
embraced the new 100 chars limit and we prefer to use the full 100 chars
over unnecessarily splitting statements over 2 lines.

We are of the opinion that a single slightly longer line is easier
to read then a statement split over 2 lines, so please change this to:

	int3472_get_func_and_polarity(int3472->sensor, &type, &func, &polarity);

Regards,

Hans