[PATCH v4 0/9] gpiolib: acpi: pin configuration fixes

[PATCH v4 0/9] gpiolib: acpi: pin configuration fixes

[Andy Shevchenko]

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

The first patch highly depends on Intel pin control driver changes
(they are material for v5.10 [1]), so this is probably not supposed
to be backported (at least right now).

The last patch adds Intel GPIO tree as official one for ACPI GPIO
changes.

Assuming [1] makes v5.10 this series can be sent as PR to Linus
for v5.11 cycle.

Cc: Jamie McClymont <jamie@kwiius.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>

[1]: https://lore.kernel.org/linux-gpio/20201106181938.GA41213@black.fi.intel.com/

Changelog v4:
- extended debounce setting to ACPI events and Operation Regions
- added Ack (Linus)
- added few more cleanup patches, including MAINTAINERS update

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

Andy Shevchenko (9):
  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: Move acpi_gpio_to_gpiod_flags() upper in the code
  gpiolib: acpi: Make acpi_gpio_to_gpiod_flags() usable for GpioInt()
  gpiolib: acpi: Extract acpi_request_own_gpiod() helper
  gpiolib: acpi: Convert pin_index to be u16
  gpiolib: acpi: Use BIT() macro to increase readability
  gpiolib: acpi: Make Intel GPIO tree official for GPIO ACPI work

 MAINTAINERS                 |   1 +
 drivers/gpio/gpiolib-acpi.c | 130 +++++++++++++++++++++---------------
 drivers/gpio/gpiolib-acpi.h |   2 +
 3 files changed, 81 insertions(+), 52 deletions(-)

-- 
2.28.0

[PATCH v4 1/9] gpiolib: acpi: Respect bias settings for GpioInt() resource

[Andy Shevchenko]

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 <jamie@kwiius.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 834a12f3219e..3a39e8a93226 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -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;
 
-- 
2.28.0

[PATCH v4 2/9] gpiolib: acpi: Use named item for enum gpiod_flags variable

[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 <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 3a39e8a93226..c127b410a7a2 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -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,
-- 
2.28.0

[PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[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 <coiby.xu@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-acpi.c | 18 ++++++++++++++++++
 drivers/gpio/gpiolib-acpi.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index c127b410a7a2..b4a0decfeac2 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -299,6 +299,10 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 		return AE_OK;
 	}
 
+	ret = gpiod_set_debounce(desc, agpio->debounce_timeout);
+	if (ret)
+		goto fail_free_desc;
+
 	ret = gpiochip_lock_as_irq(chip, pin);
 	if (ret) {
 		dev_err(chip->parent,
@@ -664,6 +668,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 +966,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);
 
@@ -1048,6 +1057,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		if (!found) {
 			enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio);
 			const char *label = "ACPI:OpRegion";
+			int ret;
 
 			desc = gpiochip_request_own_desc(chip, pin, label,
 							 GPIO_ACTIVE_HIGH,
@@ -1058,6 +1068,14 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 				goto out;
 			}
 
+			ret = gpiod_set_debounce(desc, agpio->debounce_timeout);
+			if (ret) {
+				status = AE_ERROR;
+				gpiochip_free_own_desc(desc);
+				mutex_unlock(&achip->conn_lock);
+				goto out;
+			}
+
 			conn = kzalloc(sizeof(*conn), GFP_KERNEL);
 			if (!conn) {
 				status = AE_NO_MEMORY;
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index 1c6d65cf0629..e2edb632b2cc 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -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;
 };
 
-- 
2.28.0

[PATCH v4 4/9] gpiolib: acpi: Move acpi_gpio_to_gpiod_flags() upper in the code

[Andy Shevchenko]

Move acpi_gpio_to_gpiod_flags() upper in the code to allow further refactoring.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 66 ++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index b4a0decfeac2..42c0605e3910 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -205,6 +205,39 @@ static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
 		acpi_gpiochip_request_irq(acpi_gpio, event);
 }
 
+static enum gpiod_flags
+acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
+{
+	switch (agpio->io_restriction) {
+	case ACPI_IO_RESTRICT_INPUT:
+		return GPIOD_IN;
+	case ACPI_IO_RESTRICT_OUTPUT:
+		/*
+		 * ACPI GPIO resources don't contain an initial value for the
+		 * GPIO. Therefore we deduce that value from the pull field
+		 * instead. If the pin is pulled up we assume default to be
+		 * high, if it is pulled down we assume default to be low,
+		 * otherwise we leave pin untouched.
+		 */
+		switch (agpio->pin_config) {
+		case ACPI_PIN_CONFIG_PULLUP:
+			return GPIOD_OUT_HIGH;
+		case ACPI_PIN_CONFIG_PULLDOWN:
+			return GPIOD_OUT_LOW;
+		default:
+			break;
+		}
+	default:
+		break;
+	}
+
+	/*
+	 * Assume that the BIOS has configured the direction and pull
+	 * accordingly.
+	 */
+	return GPIOD_ASIS;
+}
+
 static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)
 {
 	const char *controller, *pin_str;
@@ -530,39 +563,6 @@ static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
 	return false;
 }
 
-static enum gpiod_flags
-acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
-{
-	switch (agpio->io_restriction) {
-	case ACPI_IO_RESTRICT_INPUT:
-		return GPIOD_IN;
-	case ACPI_IO_RESTRICT_OUTPUT:
-		/*
-		 * ACPI GPIO resources don't contain an initial value for the
-		 * GPIO. Therefore we deduce that value from the pull field
-		 * instead. If the pin is pulled up we assume default to be
-		 * high, if it is pulled down we assume default to be low,
-		 * otherwise we leave pin untouched.
-		 */
-		switch (agpio->pin_config) {
-		case ACPI_PIN_CONFIG_PULLUP:
-			return GPIOD_OUT_HIGH;
-		case ACPI_PIN_CONFIG_PULLDOWN:
-			return GPIOD_OUT_LOW;
-		default:
-			break;
-		}
-	default:
-		break;
-	}
-
-	/*
-	 * Assume that the BIOS has configured the direction and pull
-	 * accordingly.
-	 */
-	return GPIOD_ASIS;
-}
-
 static int
 __acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
 {
-- 
2.28.0

[PATCH v4 5/9] gpiolib: acpi: Make acpi_gpio_to_gpiod_flags() usable for GpioInt()

[Andy Shevchenko]

GpioInt() implies input configuration of the pin. Add this to
the acpi_gpio_to_gpiod_flags() and make usable for GpioInt().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 42c0605e3910..20bc6921457f 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -208,6 +208,10 @@ static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
 static enum gpiod_flags
 acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
 {
+	/* GpioInt() implies input configuration */
+	if (agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT)
+		return GPIOD_IN;
+
 	switch (agpio->io_restriction) {
 	case ACPI_IO_RESTRICT_INPUT:
 		return GPIOD_IN;
@@ -669,6 +673,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 					      agpio->pin_table[pin_index]);
 		lookup->info.pin_config = agpio->pin_config;
 		lookup->info.debounce = agpio->debounce_timeout;
+		lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio);
 		lookup->info.gpioint = gpioint;
 
 		/*
@@ -679,11 +684,9 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		 * - ACPI_ACTIVE_HIGH == GPIO_ACTIVE_HIGH
 		 */
 		if (lookup->info.gpioint) {
-			lookup->info.flags = GPIOD_IN;
 			lookup->info.polarity = agpio->polarity;
 			lookup->info.triggering = agpio->triggering;
 		} else {
-			lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio);
 			lookup->info.polarity = lookup->active_low;
 		}
 	}
-- 
2.28.0

[PATCH v4 6/9] gpiolib: acpi: Extract acpi_request_own_gpiod() helper

[Andy Shevchenko]

It appears that we are using similar code excerpts for ACPI OpRegion
and event handling. Deduplicate those excerpts by extracting a new
acpi_request_own_gpiod() helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 44 +++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 20bc6921457f..65fa38d467db 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -242,6 +242,27 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
 	return GPIOD_ASIS;
 }
 
+static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
+						struct acpi_resource_gpio *agpio,
+						unsigned int index,
+						const char *label)
+{
+	enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio);
+	unsigned int pin = agpio->pin_table[index];
+	struct gpio_desc *desc;
+	int ret;
+
+	desc = gpiochip_request_own_desc(chip, pin, label, GPIO_ACTIVE_HIGH, flags);
+	if (IS_ERR(desc))
+		return desc;
+
+	ret = gpiod_set_debounce(desc, agpio->debounce_timeout);
+	if (ret)
+		gpiochip_free_own_desc(desc);
+
+	return ret ? ERR_PTR(ret) : desc;
+}
+
 static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)
 {
 	const char *controller, *pin_str;
@@ -327,8 +348,7 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 	if (!handler)
 		return AE_OK;
 
-	desc = gpiochip_request_own_desc(chip, pin, "ACPI:Event",
-					 GPIO_ACTIVE_HIGH, GPIOD_IN);
+	desc = acpi_request_own_gpiod(chip, agpio, 0, "ACPI:Event");
 	if (IS_ERR(desc)) {
 		dev_err(chip->parent,
 			"Failed to request GPIO for pin 0x%04X, err %ld\n",
@@ -336,10 +356,6 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 		return AE_OK;
 	}
 
-	ret = gpiod_set_debounce(desc, agpio->debounce_timeout);
-	if (ret)
-		goto fail_free_desc;
-
 	ret = gpiochip_lock_as_irq(chip, pin);
 	if (ret) {
 		dev_err(chip->parent,
@@ -1058,27 +1074,13 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		}
 
 		if (!found) {
-			enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio);
-			const char *label = "ACPI:OpRegion";
-			int ret;
-
-			desc = gpiochip_request_own_desc(chip, pin, label,
-							 GPIO_ACTIVE_HIGH,
-							 flags);
+			desc = acpi_request_own_gpiod(chip, agpio, i, "ACPI:OpRegion");
 			if (IS_ERR(desc)) {
 				status = AE_ERROR;
 				mutex_unlock(&achip->conn_lock);
 				goto out;
 			}
 
-			ret = gpiod_set_debounce(desc, agpio->debounce_timeout);
-			if (ret) {
-				status = AE_ERROR;
-				gpiochip_free_own_desc(desc);
-				mutex_unlock(&achip->conn_lock);
-				goto out;
-			}
-
 			conn = kzalloc(sizeof(*conn), GFP_KERNEL);
 			if (!conn) {
 				status = AE_NO_MEMORY;
-- 
2.28.0

[PATCH v4 7/9] gpiolib: acpi: Convert pin_index to be u16

[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 <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-acpi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 65fa38d467db..bcb76b4e82aa 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -657,7 +657,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;
@@ -673,7 +673,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++;
@@ -818,7 +818,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 {
@@ -1014,7 +1014,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;
@@ -1037,7 +1037,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;
-- 
2.28.0

[PATCH v4 8/9] gpiolib: acpi: Use BIT() macro to increase readability

[Andy Shevchenko]

We may use BIT() macro to increase readability in
acpi_gpio_adr_space_handler().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index bcb76b4e82aa..64a898c8719e 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1097,8 +1097,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		mutex_unlock(&achip->conn_lock);
 
 		if (function == ACPI_WRITE)
-			gpiod_set_raw_value_cansleep(desc,
-						     !!((1 << i) & *value));
+			gpiod_set_raw_value_cansleep(desc, !!(*value & BIT(i)));
 		else
 			*value |= (u64)gpiod_get_raw_value_cansleep(desc) << i;
 	}
-- 
2.28.0

[PATCH v4 9/9] gpiolib: acpi: Make Intel GPIO tree official for GPIO ACPI work

[Andy Shevchenko]

Make Intel GPIO tree official for GPIO ACPI work.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e73636b75f29..53236b2ea0af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7483,6 +7483,7 @@ M:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
 L:	linux-gpio@vger.kernel.org
 L:	linux-acpi@vger.kernel.org
 S:	Maintained
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git
 F:	Documentation/firmware-guide/acpi/gpio-properties.rst
 F:	drivers/gpio/gpiolib-acpi.c
 F:	drivers/gpio/gpiolib-acpi.h
-- 
2.28.0

Re: [PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[Hans de Goede]

Hi,

On 11/6/20 8:22 PM, Andy Shevchenko wrote:
> We didn't take into account the debounce settings supplied by ACPI.
> This change is targeting the mentioned gap.
> 
> Reported-by: Coiby Xu <coiby.xu@gmail.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

I added an older version of this (which only modified acpi_dev_gpio_irq_get())
for testing and when booting a kernel with that version applied to it,
on a Cherry Trail device I noticed that a whole bunch of devices where no
longer seen by the kernel because of acpi_dev_gpio_irq_get() returning
errors now (-ENOTSUPP).

Quoting from the gpiod_set_debounce docs:

/**
 * gpiod_set_debounce - sets @debounce time for a GPIO
 * @desc: descriptor of the GPIO for which to set debounce time
 * @debounce: debounce time in microseconds
 *
 * Returns:
 * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
 * debounce time.
 */

This is expected on GPIO chips where setting the debounce time
is not supported. So the error handling should be modified to
ignore -ENOTSUPP errors here.

This certainly MUST NOT be merged as is because it breaks a lot
of things as is.

Regards,

Hans




> ---
>  drivers/gpio/gpiolib-acpi.c | 18 ++++++++++++++++++
>  drivers/gpio/gpiolib-acpi.h |  2 ++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c127b410a7a2..b4a0decfeac2 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -299,6 +299,10 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
>  		return AE_OK;
>  	}
>  
> +	ret = gpiod_set_debounce(desc, agpio->debounce_timeout);
> +	if (ret)
> +		goto fail_free_desc;
> +
>  	ret = gpiochip_lock_as_irq(chip, pin);
>  	if (ret) {
>  		dev_err(chip->parent,
> @@ -664,6 +668,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 +966,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);
>  
> @@ -1048,6 +1057,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  		if (!found) {
>  			enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio);
>  			const char *label = "ACPI:OpRegion";
> +			int ret;
>  
>  			desc = gpiochip_request_own_desc(chip, pin, label,
>  							 GPIO_ACTIVE_HIGH,
> @@ -1058,6 +1068,14 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  				goto out;
>  			}
>  
> +			ret = gpiod_set_debounce(desc, agpio->debounce_timeout);
> +			if (ret) {
> +				status = AE_ERROR;
> +				gpiochip_free_own_desc(desc);
> +				mutex_unlock(&achip->conn_lock);
> +				goto out;
> +			}
> +
>  			conn = kzalloc(sizeof(*conn), GFP_KERNEL);
>  			if (!conn) {
>  				status = AE_NO_MEMORY;
> diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
> index 1c6d65cf0629..e2edb632b2cc 100644
> --- a/drivers/gpio/gpiolib-acpi.h
> +++ b/drivers/gpio/gpiolib-acpi.h
> @@ -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;
>  };
>  
> 

Re: [PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[Andy Shevchenko]

On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/6/20 8:22 PM, Andy Shevchenko wrote:

...

> I added an older version of this (which only modified acpi_dev_gpio_irq_get())
> for testing and when booting a kernel with that version applied to it,
> on a Cherry Trail device I noticed that a whole bunch of devices where no
> longer seen by the kernel because of acpi_dev_gpio_irq_get() returning
> errors now (-ENOTSUPP).
>
> Quoting from the gpiod_set_debounce docs:
>
> /**
>  * gpiod_set_debounce - sets @debounce time for a GPIO
>  * @desc: descriptor of the GPIO for which to set debounce time
>  * @debounce: debounce time in microseconds
>  *
>  * Returns:
>  * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
>  * debounce time.
>  */
>
> This is expected on GPIO chips where setting the debounce time
> is not supported. So the error handling should be modified to
> ignore -ENOTSUPP errors here.
>
> This certainly MUST NOT be merged as is because it breaks a lot
> of things as is.

Thank you very much for the testing! I remember that I fixed debounce
for BayTrail, but it seems I have yet to fix Cherry Trail pin control
as a prerequisite to this patch.

And like I said this series is definitely not for backporting.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[Hans de Goede]

Hi,

On 11/7/20 4:26 PM, Andy Shevchenko wrote:
> On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 11/6/20 8:22 PM, Andy Shevchenko wrote:
> 
> ...
> 
>> I added an older version of this (which only modified acpi_dev_gpio_irq_get())
>> for testing and when booting a kernel with that version applied to it,
>> on a Cherry Trail device I noticed that a whole bunch of devices where no
>> longer seen by the kernel because of acpi_dev_gpio_irq_get() returning
>> errors now (-ENOTSUPP).
>>
>> Quoting from the gpiod_set_debounce docs:
>>
>> /**
>>  * gpiod_set_debounce - sets @debounce time for a GPIO
>>  * @desc: descriptor of the GPIO for which to set debounce time
>>  * @debounce: debounce time in microseconds
>>  *
>>  * Returns:
>>  * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
>>  * debounce time.
>>  */
>>
>> This is expected on GPIO chips where setting the debounce time
>> is not supported. So the error handling should be modified to
>> ignore -ENOTSUPP errors here.
>>
>> This certainly MUST NOT be merged as is because it breaks a lot
>> of things as is.
> 
> Thank you very much for the testing! I remember that I fixed debounce
> for BayTrail, but it seems I have yet to fix Cherry Trail pin control
> as a prerequisite to this patch.
> 
> And like I said this series is definitely not for backporting.

Independent of fixing the CherryTrail pinctrl driver to support this,
I strongly believe that -ENOTSUPP should be ignored (treated as success)
by this patch. Remember ACPI is not only used on x86 but also on ARM
now a days. We simply cannot guarantee that all pinctrls will support
(let alone implement) debounce settings. E.g. I'm pretty sure that
the pinctrl on the popular Allwinner A64 does not support debouncing
and there are builts using a combination of uboot + EDK2 to boot!

The documentation for gpiod_set_debounce even explicitly mentioned that
-ENOTSUPP is an error which one may expect (and thus treat specially).

The same goes for the bias stuff too.

Regards,

Hans

Re: [PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[Hans de Goede]

Hi,

On 11/7/20 4:26 PM, Andy Shevchenko wrote:
> On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 11/6/20 8:22 PM, Andy Shevchenko wrote:
> 
> ...
> 
>> I added an older version of this (which only modified acpi_dev_gpio_irq_get())
>> for testing and when booting a kernel with that version applied to it,
>> on a Cherry Trail device I noticed that a whole bunch of devices where no
>> longer seen by the kernel because of acpi_dev_gpio_irq_get() returning
>> errors now (-ENOTSUPP).
>>
>> Quoting from the gpiod_set_debounce docs:
>>
>> /**
>>  * gpiod_set_debounce - sets @debounce time for a GPIO
>>  * @desc: descriptor of the GPIO for which to set debounce time
>>  * @debounce: debounce time in microseconds
>>  *
>>  * Returns:
>>  * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
>>  * debounce time.
>>  */
>>
>> This is expected on GPIO chips where setting the debounce time
>> is not supported. So the error handling should be modified to
>> ignore -ENOTSUPP errors here.
>>
>> This certainly MUST NOT be merged as is because it breaks a lot
>> of things as is.
> 
> Thank you very much for the testing! I remember that I fixed debounce
> for BayTrail, but it seems I have yet to fix Cherry Trail pin control
> as a prerequisite to this patch.
> 
> And like I said this series is definitely not for backporting.

Independent of fixing the CherryTrail pinctrl driver to support this,
I strongly believe that -ENOTSUPP should be ignored (treated as success)
by this patch. Remember ACPI is not only used on x86 but also on ARM
now a days. We simply cannot guarantee that all pinctrls will support
(let alone implement) debounce settings. E.g. I'm pretty sure that
the pinctrl on the popular Allwinner A64 does not support debouncing
and there are builts using a combination of uboot + EDK2 to boot!

The documentation for gpiod_set_debounce even explicitly mentioned that
-ENOTSUPP is an error which one may expect (and thus treat specially).

The same goes for the bias stuff too.

Regards,

Hans

Re: [PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[Andy Shevchenko]

On Sun, Nov 08, 2020 at 10:31:32AM +0100, Hans de Goede wrote:
> On 11/7/20 4:26 PM, Andy Shevchenko wrote:
> > On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 11/6/20 8:22 PM, Andy Shevchenko wrote:

...

> > Thank you very much for the testing! I remember that I fixed debounce
> > for BayTrail, but it seems I have yet to fix Cherry Trail pin control
> > as a prerequisite to this patch.
> > 
> > And like I said this series is definitely not for backporting.
> 
> Independent of fixing the CherryTrail pinctrl driver to support this,
> I strongly believe that -ENOTSUPP should be ignored (treated as success)
> by this patch. Remember ACPI is not only used on x86 but also on ARM
> now a days. We simply cannot guarantee that all pinctrls will support
> (let alone implement) debounce settings. E.g. I'm pretty sure that
> the pinctrl on the popular Allwinner A64 does not support debouncing
> and there are builts using a combination of uboot + EDK2 to boot!
> 
> The documentation for gpiod_set_debounce even explicitly mentioned that
> -ENOTSUPP is an error which one may expect (and thus treat specially).
> 
> The same goes for the bias stuff too.

While for debounce I absolutely agree with you I don't think it applies to
bias. ACPI table is coupled with a platform and setting bias == !PullNone
implies that bias is supported.

If we break something with this it means:
- ACPI table is broken and we need a quirk
- GPIO library is broken on architectural level and needs not to return
  ENOTSUPP for the flags configuration.

I will update this with taking debounce optional support into account.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[Hans de Goede]

Hi,

On 11/9/20 12:45 PM, Andy Shevchenko wrote:
> On Sun, Nov 08, 2020 at 10:31:32AM +0100, Hans de Goede wrote:
>> On 11/7/20 4:26 PM, Andy Shevchenko wrote:
>>> On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 11/6/20 8:22 PM, Andy Shevchenko wrote:
> 
> ...
> 
>>> Thank you very much for the testing! I remember that I fixed debounce
>>> for BayTrail, but it seems I have yet to fix Cherry Trail pin control
>>> as a prerequisite to this patch.
>>>
>>> And like I said this series is definitely not for backporting.
>>
>> Independent of fixing the CherryTrail pinctrl driver to support this,
>> I strongly believe that -ENOTSUPP should be ignored (treated as success)
>> by this patch. Remember ACPI is not only used on x86 but also on ARM
>> now a days. We simply cannot guarantee that all pinctrls will support
>> (let alone implement) debounce settings. E.g. I'm pretty sure that
>> the pinctrl on the popular Allwinner A64 does not support debouncing
>> and there are builts using a combination of uboot + EDK2 to boot!
>>
>> The documentation for gpiod_set_debounce even explicitly mentioned that
>> -ENOTSUPP is an error which one may expect (and thus treat specially).
>>
>> The same goes for the bias stuff too.
> 
> While for debounce I absolutely agree with you I don't think it applies to
> bias. ACPI table is coupled with a platform and setting bias == !PullNone
> implies that bias is supported.

What about PullDefault ? I can easily see DSDT writers using PullDefault
on platforms where bias setting is not supported.

> If we break something with this it means:
> - ACPI table is broken and we need a quirk

Broken ACPI tables are as common as rain in the Netherlands, where ever
possible we want to deal with these / workaround the brokenness
without requiring per device quirks. Requiring a per device quirk for
every broken ACPI table out there does not scale.

> - GPIO library is broken on architectural level and needs not to return
>   ENOTSUPP for the flags configuration.

Usually we handle features not being implemented gracefully. E.g chances
are good that whatever bias is required was already setup by the firmware
(or bootloader in the uboot + EDK2 case). Making bias set failures a
critical error will likely regress working platforms in various cases.

Keep in mind we have an existing userbase where things is working fine
without taking the bias settings into account now. So if we hit the
-ENOTSUPP case on any platform out there, then that is a regression
plain and simple and as you know regressions are a big red flag.

Since it is really easy to avoid the regression here we really
should avoid it IMHO. What about just printing a warning on ENOTSUPP
when setting the bias instead ?

Regards,

Hans

Re: [PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[Andy Shevchenko]

+Cc: my work address back (by some reason Gmail(?) dropped it)

On Mon, Nov 9, 2020 at 1:53 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/9/20 12:45 PM, Andy Shevchenko wrote:
> > On Sun, Nov 08, 2020 at 10:31:32AM +0100, Hans de Goede wrote:
> >> On 11/7/20 4:26 PM, Andy Shevchenko wrote:
> >>> On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>> On 11/6/20 8:22 PM, Andy Shevchenko wrote:
> >
> > ...
> >
> >>> Thank you very much for the testing! I remember that I fixed debounce
> >>> for BayTrail, but it seems I have yet to fix Cherry Trail pin control
> >>> as a prerequisite to this patch.
> >>>
> >>> And like I said this series is definitely not for backporting.
> >>
> >> Independent of fixing the CherryTrail pinctrl driver to support this,
> >> I strongly believe that -ENOTSUPP should be ignored (treated as success)
> >> by this patch. Remember ACPI is not only used on x86 but also on ARM
> >> now a days. We simply cannot guarantee that all pinctrls will support
> >> (let alone implement) debounce settings. E.g. I'm pretty sure that
> >> the pinctrl on the popular Allwinner A64 does not support debouncing
> >> and there are builts using a combination of uboot + EDK2 to boot!
> >>
> >> The documentation for gpiod_set_debounce even explicitly mentioned that
> >> -ENOTSUPP is an error which one may expect (and thus treat specially).
> >>
> >> The same goes for the bias stuff too.
> >
> > While for debounce I absolutely agree with you I don't think it applies to
> > bias. ACPI table is coupled with a platform and setting bias == !PullNone

s/None/Default/

> > implies that bias is supported.
>
> What about PullDefault ? I can easily see DSDT writers using PullDefault
> on platforms where bias setting is not supported.

PullDefault is ASIS in terms of BIAS, it's always supported.
I mistakenly took None in the upper paragraph, but I got your point.

> > If we break something with this it means:
> > - ACPI table is broken and we need a quirk
>
> Broken ACPI tables are as common as rain in the Netherlands, where ever
> possible we want to deal with these / workaround the brokenness
> without requiring per device quirks. Requiring a per device quirk for
> every broken ACPI table out there does not scale.

Okay.

> > - GPIO library is broken on architectural level and needs not to return
> >   ENOTSUPP for the flags configuration.
>
> Usually we handle features not being implemented gracefully. E.g chances
> are good that whatever bias is required was already setup by the firmware
> (or bootloader in the uboot + EDK2 case). Making bias set failures a
> critical error will likely regress working platforms in various cases.
>
> Keep in mind we have an existing userbase where things is working fine
> without taking the bias settings into account now. So if we hit the
> -ENOTSUPP case on any platform out there, then that is a regression
> plain and simple and as you know regressions are a big red flag.

Yeah, probably the best is to have these things optional and thus
return only real error cases.

> Since it is really easy to avoid the regression here we really
> should avoid it IMHO. What about just printing a warning on ENOTSUPP
> when setting the bias instead ?

So, looking into this deeper I have got the following:

FUNC            Relation to ENOTSUPP

gpiod_set_config()  returns if not supported
gpiod_set_debounce()  as gpiod_set_config() above
gpio_set_debounce()  legacy wrapper on top of gpiod_set_debounce()
gpiod_set_transitory()  skips it (returns okay) with debug message
gpio_set_config()  returns if not supported
gpio_set_bias()  skips it (returns okay)

Now, what the heck are the last ones? Why do we have this difference?
(I meant APIs)

So, I will try to unify it. Not sure that we need to issue a message
in case something is not supported. Debug one? Might be, but don't
think we need it right now.


-- 
With Best Regards,
Andy Shevchenko