diff mbox series

[v4,2/3] platform/x86: int3472: Call "reset" GPIO "enable" for INT347E

Message ID 20250131073707.1082931-3-sakari.ailus@linux.intel.com (mailing list archive)
State New
Headers show
Series int3472: Support GPIO con_id based on _HID | expand

Commit Message

Sakari Ailus Jan. 31, 2025, 7:37 a.m. UTC 
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>
---
 drivers/platform/x86/intel/int3472/discrete.c | 45 +++++++++++++++++--
 1 file changed, 41 insertions(+), 4 deletions(-)

Comments

Andy Shevchenko Jan. 31, 2025, 11:22 a.m. UTC | #1 
On Fri, Jan 31, 2025 at 09:37:06AM +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 ||
> +		    !acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> +			continue;

I think in a split form it is easier to read and maintain:

		if (!acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
			continue;

		if (*type != int3472_gpio_map[i].type_from)
			continue;

> +		*type = int3472_gpio_map[i].type_to;

> +		*gpio_flags = int3472_gpio_map[i].polarity_low ?
> +			GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;

Still can be one line (100 characters). But in this case I have no strong
preference. Up to Hans.

> +		*func = int3472_gpio_map[i].func;
> +		return;
> +	}
Sakari Ailus Jan. 31, 2025, 11:57 a.m. UTC | #2 
Hi Andy,

On Fri, Jan 31, 2025 at 01:22:19PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 31, 2025 at 09:37:06AM +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 ||
> > +		    !acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> > +			continue;
> 
> I think in a split form it is easier to read and maintain:
> 
> 		if (!acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> 			continue;
> 
> 		if (*type != int3472_gpio_map[i].type_from)
> 			continue;

Works for me, with the order reverted. I'll send v5.

> 
> > +		*type = int3472_gpio_map[i].type_to;
> 
> > +		*gpio_flags = int3472_gpio_map[i].polarity_low ?
> > +			GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
> 
> Still can be one line (100 characters). But in this case I have no strong
> preference. Up to Hans.
> 
> > +		*func = int3472_gpio_map[i].func;
> > +		return;
> > +	}
>
Andy Shevchenko Jan. 31, 2025, 5:20 p.m. UTC | #3 
On Fri, Jan 31, 2025 at 11:57:20AM +0000, Sakari Ailus wrote:
> On Fri, Jan 31, 2025 at 01:22:19PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 31, 2025 at 09:37:06AM +0200, Sakari Ailus wrote:

...

> > > +		if (*type != int3472_gpio_map[i].type_from ||
> > > +		    !acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> > > +			continue;
> > 
> > I think in a split form it is easier to read and maintain:
> > 
> > 		if (!acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
> > 			continue;
> > 
> > 		if (*type != int3472_gpio_map[i].type_from)
> > 			continue;
> 
> Works for me, with the order reverted. I'll send v5.

Hmm... I don't think the provided order is understandable.
But let's continue discussion in v5 thread.
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 3f7624714869..3f3f50ce431c 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,46 @@  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 ||
+		    !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 +255,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])