Skip to contentSkip to conversation

gpiolib: acpi: pin configuration fixes

PATCH Superseded Andy Shevchenko 3 participants 8 messages Nov 2, 2020 – Nov 6, 2020

Subsystems (2)
GPIO GPIO ACPI support

v3 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Andy Shevchenko

author maintainer
#0 lore raw

v3 cover gpiolib: acpi: pin configuration fixes

There are two fixes (and two small cleanups) that allow to take into consideration more parameters in ACPI, i.e. bias for GpioInt() and debounce time for GpioInt() and GpioIo() resources.

The first patch highly depends on Intel pin control driver changes (for now [1], but might be more), so it's probably not supposed to be backported (at least right now).

I think the best way is to collect tags from GPIO maintainers and I can incorporate this into our Intel pin control branch which I will share with you as PR against GPIO and pin control subsystems.

I'm also all ears for alternatives.

Cc Jamie McClymont <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Hans de Goede <[email protected]>

[1]: https://lore.kernel.org/linux-gpio/[email protected]/T/

Changelog v3:
- dropped upstreamed OF patch
- added debounce fix

Andy Shevchenko (4):
gpiolib: acpi: Respect bias settings for GpioInt() resource
gpiolib: acpi: Use named item for enum gpiod_flags variable
gpiolib: acpi: Take into account debounce settings
gpiolib: acpi: Convert pin_index to be u16

 drivers/gpio/gpiolib-acpi.c | 23 ++++++++++++++++-------
 drivers/gpio/gpiolib-acpi.h |  2 ++
 2 files changed, 18 insertions(+), 7 deletions(-)
Signature · 1 line
2.28.0
author maintainer
#1 lore raw

PATCH v3 1/4 gpiolib: acpi: Respect bias settings for GpioInt() resource

In some cases the GpioInt() resource is coming with bias settings which may affect system functioning. Respect bias settings for GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API in acpi_dev_gpio_irq_get().

Reported-by Jamie McClymont <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Mika Westerberg <[email protected]>
---
 drivers/gpio/gpiolib-acpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/gpio/gpiolib-acpi.c +5 -1
index 834a12f3219e..3a39e8a93226 100644
@@ -942,6 +942,7 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 
 		if (info.gpioint && idx++ == index) {
 			unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
+			enum gpiod_flags dflags = GPIOD_ASIS;
 			char label[32];
 			int irq;
 

@@ -952,8 +953,11 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 			if (irq < 0)
 				return irq;
 
+			acpi_gpio_update_gpiod_flags(&dflags, &info);
+			acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
+
 			snprintf(label, sizeof(label), "GpioInt() %d", index);
-			ret = gpiod_configure_flags(desc, label, lflags, info.flags);
+			ret = gpiod_configure_flags(desc, label, lflags, dflags);
 			if (ret < 0)
 				return ret;
Signature · 1 line
2.28.0
author maintainer
#2 lore raw

PATCH v3 2/4 gpiolib: acpi: Use named item for enum gpiod_flags variable

In reply to Andy Shevchenko

Use named item instead of plain integer for enum gpiod_flags to make it clear that even 0 has its own meaning.

Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Mika Westerberg <[email protected]>
---
 drivers/gpio/gpiolib-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib-acpi.c +1 -1
index 3a39e8a93226..c127b410a7a2 100644
@@ -1136,7 +1136,7 @@ acpi_gpiochip_parse_own_gpio(struct acpi_gpio_chip *achip,
 	int ret;
 
 	*lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
-	*dflags = 0;
+	*dflags = GPIOD_ASIS;
 	*name = NULL;
 
 	ret = fwnode_property_read_u32_array(fwnode, "gpios", gpios,
Signature · 1 line
2.28.0
author maintainer
#3 lore raw

PATCH v3 3/4 gpiolib: acpi: Take into account debounce settings

In reply to Andy Shevchenko

We didn't take into account the debounce settings supplied by ACPI. This change is targeting the mentioned gap.

Reported-by Coiby Xu <[email protected]>
Cc Hans de Goede <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/gpio/gpiolib-acpi.c | 5 +++++
 drivers/gpio/gpiolib-acpi.h | 2 ++
 2 files changed, 7 insertions(+)
drivers/gpio/gpiolib-acpi.c +5 -0
index c127b410a7a2..ad245b2a536e 100644
@@ -664,6 +664,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
 					      agpio->pin_table[pin_index]);
 		lookup->info.pin_config = agpio->pin_config;
+		lookup->info.debounce = agpio->debounce_timeout;
 		lookup->info.gpioint = gpioint;
 
 		/*

@@ -961,6 +962,10 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 			if (ret < 0)
 				return ret;
 
+			ret = gpiod_set_debounce(desc, info.debounce);
+			if (ret)
+				return ret;
+
 			irq_flags = acpi_dev_get_irq_type(info.triggering,
 							  info.polarity);
drivers/gpio/gpiolib-acpi.h +2 -0
index 1c6d65cf0629..e2edb632b2cc 100644
@@ -18,6 +18,7 @@ struct acpi_device;
  * @pin_config: pin bias as provided by ACPI
  * @polarity: interrupt polarity as provided by ACPI
  * @triggering: triggering type as provided by ACPI
+ * @debounce: debounce timeout as provided by ACPI
  * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
  */
 struct acpi_gpio_info {

@@ -27,6 +28,7 @@ struct acpi_gpio_info {
 	int pin_config;
 	int polarity;
 	int triggering;
+	unsigned int debounce;
 	unsigned int quirks;
 };
Signature · 1 line
2.28.0
author maintainer
#4 lore raw

PATCH v3 4/4 gpiolib: acpi: Convert pin_index to be u16

In reply to Andy Shevchenko

As defined by ACPI the pin index is 16-bit unsigned integer. Define the variable, which holds it, accordingly.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/gpio/gpiolib-acpi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/gpio/gpiolib-acpi.c +5 -5
index ad245b2a536e..a571dbf2093b 100644
@@ -633,7 +633,7 @@ int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
-	int pin_index;
+	u16 pin_index;
 	bool active_low;
 	struct gpio_desc *desc;
 	int n;

@@ -649,7 +649,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 	if (!lookup->desc) {
 		const struct acpi_resource_gpio *agpio = &ares->data.gpio;
 		bool gpioint = agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
-		int pin_index;
+		u16 pin_index;
 
 		if (lookup->info.quirks & ACPI_GPIO_QUIRK_ONLY_GPIOIO && gpioint)
 			lookup->index++;

@@ -795,7 +795,7 @@ static struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
 		if (ret)
 			return ERR_PTR(ret);
 
-		dev_dbg(&adev->dev, "GPIO: _DSD returned %s %d %d %u\n",
+		dev_dbg(&adev->dev, "GPIO: _DSD returned %s %d %u %u\n",
 			dev_name(&lookup.info.adev->dev), lookup.index,
 			lookup.pin_index, lookup.active_low);
 	} else {

@@ -991,7 +991,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 	struct gpio_chip *chip = achip->chip;
 	struct acpi_resource_gpio *agpio;
 	struct acpi_resource *ares;
-	int pin_index = (int)address;
+	u16 pin_index = address;
 	acpi_status status;
 	int length;
 	int i;

@@ -1014,7 +1014,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		return AE_BAD_PARAMETER;
 	}
 
-	length = min(agpio->pin_table_length, (u16)(pin_index + bits));
+	length = min_t(u16, agpio->pin_table_length, pin_index + bits);
 	for (i = pin_index; i < length; ++i) {
 		int pin = agpio->pin_table[i];
 		struct acpi_gpio_connection *conn;
Signature · 1 line
2.28.0
Acked-by on the v3 series
#5 lore raw
In reply to Andy Shevchenko
Quoted 5 lines · Andy Shevchenko

I think the best way is to collect tags from GPIO maintainers and I can incorporate this into our Intel pin control branch which I will share with you as PR against GPIO and pin control subsystems.

I'm also all ears for alternatives.

Acked-by Linus Walleij <[email protected]>

However I definitely trust you on any changes to gpiolib-acpi.c so if you want a definitive formal control over that I do not mind if you list yourself as maintainer of this file. I would definitely pull any GPIO-ACPI-stuff from you without any hesitation anyways.

Yours,
Linus Walleij

#6 lore raw
Quoted 11 lines · Linus Walleij
Quoted 5 lines · Andy Shevchenko

I think the best way is to collect tags from GPIO maintainers and I can incorporate this into our Intel pin control branch which I will share with you as PR against GPIO and pin control subsystems.

I'm also all ears for alternatives.

Acked-by Linus Walleij <[email protected]>

Thanks!

Quoted 5 lines

However I definitely trust you on any changes to gpiolib-acpi.c so if you want a definitive formal control over that I do not mind if you list yourself as maintainer of this file. I would definitely pull any GPIO-ACPI-stuff from you without any hesitation anyways.

Let's hear what Mika says. I'm fine to use my Intel GPIO tree for that purpose.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#7 lore raw
Quoted 15 lines · Andy Shevchenko
Quoted 11 lines · Linus Walleij
Quoted 5 lines · Andy Shevchenko

I think the best way is to collect tags from GPIO maintainers and I can incorporate this into our Intel pin control branch which I will share with you as PR against GPIO and pin control subsystems.

I'm also all ears for alternatives.

Acked-by Linus Walleij <[email protected]>

Thanks!

Quoted 7 lines
Quoted 5 lines

However I definitely trust you on any changes to gpiolib-acpi.c so if you want a definitive formal control over that I do not mind if you list yourself as maintainer of this file. I would definitely pull any GPIO-ACPI-stuff from you without any hesitation anyways.

Let's hear what Mika says. I'm fine to use my Intel GPIO tree for that purpose.

Okay, I have sent v4 with updated MAINTAINERS.

Signature · 2 lines
With Best Regards,
Andy Shevchenko