[PATCH v5 0/3] int3472: Support GPIO con_id based on _HID

[PATCH v5 0/3] int3472: Support GPIO con_id based on _HID

[Sakari Ailus]

Hi folks,

One patch turned into a set, the second patch being the original one.

since v4:

- Split the newly added if () clause into two for better readability in
  int3472_get_func_and_polarity().

Sakari Ailus (3):
  platform/x86: int3472: Use correct type for "polarity", call it
    gpio_flags
  platform/x86: int3472: Call "reset" GPIO "enable" for INT347E
  platform/x86: int3472: Call "func" "con_id" instead

 drivers/platform/x86/intel/int3472/discrete.c | 100 ++++++++++++------
 1 file changed, 70 insertions(+), 30 deletions(-)

-- 
2.39.5

[PATCH v5 1/3] platform/x86: int3472: Use correct type for "polarity", call it gpio_flags

[Sakari Ailus]

Struct gpiod_lookup flags field's type is unsigned long. Thus use unsigned
long for values to be assigned to that field. Similarly, also call the
field gpio_flags which it really is.

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 | 39 ++++++++++---------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index d881b2cfcdfc..3f7624714869 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -55,7 +55,7 @@ static void skl_int3472_log_sensor_module_name(struct int3472_discrete_device *i
 
 static int skl_int3472_fill_gpiod_lookup(struct gpiod_lookup *table_entry,
 					 struct acpi_resource_gpio *agpio,
-					 const char *func, u32 polarity)
+					 const char *func, unsigned long gpio_flags)
 {
 	char *path = agpio->resource_source.string_ptr;
 	struct acpi_device *adev;
@@ -70,14 +70,14 @@ static int skl_int3472_fill_gpiod_lookup(struct gpiod_lookup *table_entry,
 	if (!adev)
 		return -ENODEV;
 
-	*table_entry = GPIO_LOOKUP(acpi_dev_name(adev), agpio->pin_table[0], func, polarity);
+	*table_entry = GPIO_LOOKUP(acpi_dev_name(adev), agpio->pin_table[0], func, gpio_flags);
 
 	return 0;
 }
 
 static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int3472,
 					  struct acpi_resource_gpio *agpio,
-					  const char *func, u32 polarity)
+					  const char *func, unsigned long gpio_flags)
 {
 	int ret;
 
@@ -87,7 +87,7 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 	}
 
 	ret = skl_int3472_fill_gpiod_lookup(&int3472->gpios.table[int3472->n_sensor_gpios],
-					    agpio, func, polarity);
+					    agpio, func, gpio_flags);
 	if (ret)
 		return ret;
 
@@ -100,7 +100,7 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 static struct gpio_desc *
 skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
 				       struct acpi_resource_gpio *agpio,
-				       const char *func, u32 polarity)
+				       const char *func, unsigned long gpio_flags)
 {
 	struct gpio_desc *desc;
 	int ret;
@@ -111,7 +111,7 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
 		return ERR_PTR(-ENOMEM);
 
 	lookup->dev_id = dev_name(int3472->dev);
-	ret = skl_int3472_fill_gpiod_lookup(&lookup->table[0], agpio, func, polarity);
+	ret = skl_int3472_fill_gpiod_lookup(&lookup->table[0], agpio, func, gpio_flags);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -122,32 +122,33 @@ 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)
+static void int3472_get_func_and_polarity(u8 type, const char **func,
+					  unsigned long *gpio_flags)
 {
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 		*func = "reset";
-		*polarity = GPIO_ACTIVE_LOW;
+		*gpio_flags = GPIO_ACTIVE_LOW;
 		break;
 	case INT3472_GPIO_TYPE_POWERDOWN:
 		*func = "powerdown";
-		*polarity = GPIO_ACTIVE_LOW;
+		*gpio_flags = GPIO_ACTIVE_LOW;
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
 		*func = "clk-enable";
-		*polarity = GPIO_ACTIVE_HIGH;
+		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	case INT3472_GPIO_TYPE_PRIVACY_LED:
 		*func = "privacy-led";
-		*polarity = GPIO_ACTIVE_HIGH;
+		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
 		*func = "power-enable";
-		*polarity = GPIO_ACTIVE_HIGH;
+		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	default:
 		*func = "unknown";
-		*polarity = GPIO_ACTIVE_HIGH;
+		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	}
 }
@@ -194,7 +195,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	struct gpio_desc *gpio;
 	const char *err_msg;
 	const char *func;
-	u32 polarity;
+	unsigned long gpio_flags;
 	int ret;
 
 	if (!acpi_gpio_get_io_resource(ares, &agpio))
@@ -217,7 +218,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, &polarity);
+	int3472_get_func_and_polarity(type, &func, &gpio_flags);
 
 	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
 	if (pin != agpio->pin_table[0])
@@ -227,16 +228,16 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	active_value = FIELD_GET(INT3472_GPIO_DSM_SENSOR_ON_VAL, obj->integer.value);
 	if (!active_value)
-		polarity ^= GPIO_ACTIVE_LOW;
+		gpio_flags ^= GPIO_ACTIVE_LOW;
 
 	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func,
 		agpio->resource_source.string_ptr, agpio->pin_table[0],
-		str_high_low(polarity == GPIO_ACTIVE_HIGH));
+		str_high_low(gpio_flags == GPIO_ACTIVE_HIGH));
 
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
-		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
+		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, gpio_flags);
 		if (ret)
 			err_msg = "Failed to map GPIO pin to sensor\n";
 
@@ -244,7 +245,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
 	case INT3472_GPIO_TYPE_PRIVACY_LED:
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
-		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, func, polarity);
+		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, func, gpio_flags);
 		if (IS_ERR(gpio)) {
 			ret = PTR_ERR(gpio);
 			err_msg = "Failed to get GPIO\n";
-- 
2.39.5

[PATCH v5 2/3] 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>
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;
+		*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])
-- 
2.39.5

[PATCH v5 3/3] platform/x86: int3472: Call "func" "con_id" instead

[Sakari Ailus]

"con_id" is an established variable name for the GPIO naming for drivers.
Use it instead of "func" in the int3472 driver, too.

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 | 48 +++++++++----------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 529ea2d08a21..0820a66c72fe 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -56,7 +56,7 @@ static void skl_int3472_log_sensor_module_name(struct int3472_discrete_device *i
 
 static int skl_int3472_fill_gpiod_lookup(struct gpiod_lookup *table_entry,
 					 struct acpi_resource_gpio *agpio,
-					 const char *func, unsigned long gpio_flags)
+					 const char *con_id, unsigned long gpio_flags)
 {
 	char *path = agpio->resource_source.string_ptr;
 	struct acpi_device *adev;
@@ -71,14 +71,14 @@ static int skl_int3472_fill_gpiod_lookup(struct gpiod_lookup *table_entry,
 	if (!adev)
 		return -ENODEV;
 
-	*table_entry = GPIO_LOOKUP(acpi_dev_name(adev), agpio->pin_table[0], func, gpio_flags);
+	*table_entry = GPIO_LOOKUP(acpi_dev_name(adev), agpio->pin_table[0], con_id, gpio_flags);
 
 	return 0;
 }
 
 static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int3472,
 					  struct acpi_resource_gpio *agpio,
-					  const char *func, unsigned long gpio_flags)
+					  const char *con_id, unsigned long gpio_flags)
 {
 	int ret;
 
@@ -88,7 +88,7 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 	}
 
 	ret = skl_int3472_fill_gpiod_lookup(&int3472->gpios.table[int3472->n_sensor_gpios],
-					    agpio, func, gpio_flags);
+					    agpio, con_id, gpio_flags);
 	if (ret)
 		return ret;
 
@@ -101,7 +101,7 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 static struct gpio_desc *
 skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
 				       struct acpi_resource_gpio *agpio,
-				       const char *func, unsigned long gpio_flags)
+				       const char *con_id, unsigned long gpio_flags)
 {
 	struct gpio_desc *desc;
 	int ret;
@@ -112,12 +112,12 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
 		return ERR_PTR(-ENOMEM);
 
 	lookup->dev_id = dev_name(int3472->dev);
-	ret = skl_int3472_fill_gpiod_lookup(&lookup->table[0], agpio, func, gpio_flags);
+	ret = skl_int3472_fill_gpiod_lookup(&lookup->table[0], agpio, con_id, gpio_flags);
 	if (ret)
 		return ERR_PTR(ret);
 
 	gpiod_add_lookup_table(lookup);
-	desc = devm_gpiod_get(int3472->dev, func, GPIOD_OUT_LOW);
+	desc = devm_gpiod_get(int3472->dev, con_id, GPIOD_OUT_LOW);
 	gpiod_remove_lookup_table(lookup);
 
 	return desc;
@@ -129,7 +129,7 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
  * @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"
+ * @con_id: The name of the GPIO for the device
  * @polarity_low: GPIO_ACTIVE_LOW true if the @polarity_low is true,
  * GPIO_ACTIVE_HIGH otherwise
  */
@@ -138,15 +138,15 @@ struct int3472_gpio_map {
 	u8 type_from;
 	u8 type_to;
 	bool polarity_low;
-	const char *func;
+	const char *con_id;
 };
 
 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)
+static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type,
+					    const char **con_id, unsigned long *gpio_flags)
 {
 	unsigned int i;
 
@@ -160,33 +160,33 @@ static void int3472_get_func_and_polarity(struct acpi_device *adev, u8 *type,
 		*type = int3472_gpio_map[i].type_to;
 		*gpio_flags = int3472_gpio_map[i].polarity_low ?
 			GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
-		*func = int3472_gpio_map[i].func;
+		*con_id = int3472_gpio_map[i].con_id;
 		return;
 	}
 
 	switch (*type) {
 	case INT3472_GPIO_TYPE_RESET:
-		*func = "reset";
+		*con_id = "reset";
 		*gpio_flags = GPIO_ACTIVE_LOW;
 		break;
 	case INT3472_GPIO_TYPE_POWERDOWN:
-		*func = "powerdown";
+		*con_id = "powerdown";
 		*gpio_flags = GPIO_ACTIVE_LOW;
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		*func = "clk-enable";
+		*con_id = "clk-enable";
 		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	case INT3472_GPIO_TYPE_PRIVACY_LED:
-		*func = "privacy-led";
+		*con_id = "privacy-led";
 		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
-		*func = "power-enable";
+		*con_id = "power-enable";
 		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	default:
-		*func = "unknown";
+		*con_id = "unknown";
 		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	}
@@ -233,7 +233,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	union acpi_object *obj;
 	struct gpio_desc *gpio;
 	const char *err_msg;
-	const char *func;
+	const char *con_id;
 	unsigned long gpio_flags;
 	int ret;
 
@@ -257,26 +257,26 @@ 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(int3472->sensor, &type, &func, &gpio_flags);
+	int3472_get_con_id_and_polarity(int3472->sensor, &type, &con_id, &gpio_flags);
 
 	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
 	if (pin != agpio->pin_table[0])
 		dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n",
-			 func, agpio->resource_source.string_ptr, pin,
+			 con_id, agpio->resource_source.string_ptr, pin,
 			 agpio->pin_table[0]);
 
 	active_value = FIELD_GET(INT3472_GPIO_DSM_SENSOR_ON_VAL, obj->integer.value);
 	if (!active_value)
 		gpio_flags ^= GPIO_ACTIVE_LOW;
 
-	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func,
+	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", con_id,
 		agpio->resource_source.string_ptr, agpio->pin_table[0],
 		str_high_low(gpio_flags == GPIO_ACTIVE_HIGH));
 
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
-		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, gpio_flags);
+		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags);
 		if (ret)
 			err_msg = "Failed to map GPIO pin to sensor\n";
 
@@ -284,7 +284,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
 	case INT3472_GPIO_TYPE_PRIVACY_LED:
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
-		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, func, gpio_flags);
+		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
 		if (IS_ERR(gpio)) {
 			ret = PTR_ERR(gpio);
 			err_msg = "Failed to get GPIO\n";
-- 
2.39.5

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

[Andy Shevchenko]

On Fri, Jan 31, 2025 at 02:01:51PM +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.

...

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

Hmm... But why? It's more natural to test if the device even present before
continue to check the details of the quirk. This order looks suspicious
and unusual. At bare minimum it needs a comment. I.o.w. the Q here is "Why is
the type_from check superior to the device?"

> +		*type = int3472_gpio_map[i].type_to;
> +		*gpio_flags = int3472_gpio_map[i].polarity_low ?
> +			GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
> +		*func = int3472_gpio_map[i].func;
> +		return;
> +	}

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

Hi Andy,

On Fri, Jan 31, 2025 at 07:18:54PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 31, 2025 at 02:01:51PM +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.
> 
> ...
> 
> > +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;
> 
> Hmm... But why? It's more natural to test if the device even present before
> continue to check the details of the quirk. This order looks suspicious

From the result point of view there's no difference. It makes sense to test
an integer before a rather more elaborate acpi_dev_hid_uid_match(). I don't
think that needs a comment.

> and unusual. At bare minimum it needs a comment. I.o.w. the Q here is "Why is
> the type_from check superior to the device?"
> 
> > +		*type = int3472_gpio_map[i].type_to;
> > +		*gpio_flags = int3472_gpio_map[i].polarity_low ?
> > +			GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
> > +		*func = int3472_gpio_map[i].func;
> > +		return;
> > +	}
> 

-- 
Regards,

Sakari Ailus

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

[Ilpo Järvinen]

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!

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])
> 

-- 
 i.

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

[Sakari Ailus]

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.

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])
> > 

-- 
Regards,

Sakari Ailus

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

[Ilpo Järvinen]

[-- 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])
> > > 
> 
> 

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

[Andy Shevchenko]

On Mon, Feb 03, 2025 at 07:42:06AM +0000, Sakari Ailus wrote:
> On Fri, Jan 31, 2025 at 07:18:54PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 31, 2025 at 02:01:51PM +0200, Sakari Ailus wrote:

...

> > > +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;
> > 
> > Hmm... But why? It's more natural to test if the device even present before
> > continue to check the details of the quirk. This order looks suspicious
> 
> From the result point of view there's no difference. It makes sense to test
> an integer before a rather more elaborate acpi_dev_hid_uid_match(). I don't
> think that needs a comment.

If it doesn't need a comment, it doesn't need to be like this.
Semantically and logically it's better to read:

1. Check the device first, if not, skip the quirk.
2. For the checked device, check if the type is what we are expecting, if not, continue.
3. Perform other checks.
4. Apply a quirk.

> > and unusual. At bare minimum it needs a comment. I.o.w. the Q here is "Why is
> > the type_from check superior to the device?"
> > 
> > > +		*type = int3472_gpio_map[i].type_to;
> > > +		*gpio_flags = int3472_gpio_map[i].polarity_low ?
> > > +			GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
> > > +		*func = int3472_gpio_map[i].func;
> > > +		return;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko