gpiolib: acpi: pin configuration fixes

PATCH In review Andy Shevchenko 5 participants 30 messages Nov 11, 2020 – Nov 16, 2020

Subsystems (2)

GPIO GPIO ACPI support

lore

Share & tools

lore

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #0 lore raw

v7 cover gpiolib: acpi: pin configuration fixes

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

During review Hans noted, that gpiod_set_debounce() returns -ENOTSUPP for the cases when feature is not supported either by driver or a controller.

It appears that we have slightly messy API here:

FUNC Relation with 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)

Last two functions are internal to GPIO library, while the rest is exported API. In order to be consistent with both naming schemas the series introduces gpio_set_debounce_timeout() that considers the feature optional. New API is only for internal use.

While at it, the few first patches clean up the current GPIO library code to unify it to some extend.

The above is followed by changes made in ACPI GPIO library part.

The bias patch highly depends on Intel pin control driver changes (they are material for v5.10 [1]), due to this and amount of the prerequisite changes this series 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.

Note, some patches are also depend to the code from GPIO fixes / for-next repositories. Unfortunately there is no one repository which contains all up to date for-next changes against GPIO subsystem. That's why I have merged Linus' for-next followed by Bart's for-next branches as prerequisites to the series.

Cc Jamie McClymont <[email protected]>

Cc Coiby Xu <[email protected]>

Cc Vasile-Laurentiu Stanimir <[email protected]>

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

Changelog v7:
- sent correct set of patches

Changelog v6:
- added tags (Hans, Mika, Linus)
- dropped temporary variables in couple of patches (Mika)
- dropped "Add temporary variable to gpiod_set_transitory()..." (Mika)
- added patch to move assignments outside a lock (Mika)
- added patch from Vasile
- due to above rebased accordingly the affected ones

Changelog v5:
- introduced gpio_set_debounce_timeout()
- made a prerequisite refactoring in GPIO library code
- updated the rest accordingly

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 (17):
gpiolib: Replace unsigned by unsigned int
gpiolib: add missed break statement
gpiolib: use proper API to pack pin configuration parameters
gpiolib: Extract gpio_set_config_with_argument() for future use
gpiolib: move bias related code from gpio_set_config() to

    gpio_set_bias()

gpiolib: Extract gpio_set_config_with_argument_optional() helper
gpiolib: Introduce gpio_set_debounce_timeout() for internal use
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 non-critical code outside of critical section
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

Vasile-Laurentiu Stanimir (1):
gpiolib: acpi: Set initial value for output pin based on bias and
polarity

 MAINTAINERS                   |   1 +
 drivers/gpio/gpiolib-acpi.c   | 138 +++++++++++++++++++++-------------
 drivers/gpio/gpiolib-acpi.h   |   2 +
 drivers/gpio/gpiolib.c        |  98 ++++++++++++++----------
 drivers/gpio/gpiolib.h        |   1 +
 include/linux/gpio/consumer.h |   4 +-
 6 files changed, 147 insertions(+), 97 deletions(-)

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #1 lore raw

PATCH v7 1/18 gpiolib: Replace unsigned by unsigned int

Replace unsigned by unsigned int in GPIO library code. Note, legacy API left untouched.

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib.c        | 16 ++++++++--------
 include/linux/gpio/consumer.h |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

drivers/gpio/gpiolib.c +8 -8

index bb5a9557b350..f94225d7817b 100644
@@ -211,7 +211,7 @@ static int gpiochip_find_base(int ngpio)
 int gpiod_get_direction(struct gpio_desc *desc)
 {
 	struct gpio_chip *gc;
-	unsigned offset;
+	unsigned int offset;
 	int ret;
 
 	gc = gpiod_to_chip(desc);

@@ -1333,7 +1333,7 @@ void gpiochip_irq_domain_deactivate(struct irq_domain *domain,
 }
 EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate);
 
-static int gpiochip_to_irq(struct gpio_chip *gc, unsigned offset)
+static int gpiochip_to_irq(struct gpio_chip *gc, unsigned int offset)
 {
 	struct irq_domain *domain = gc->irq.domain;

@@ -1635,7 +1635,7 @@ static inline void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gc)
  * @gc: the gpiochip owning the GPIO
  * @offset: the offset of the GPIO to request for GPIO function
  */
-int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset)
+int gpiochip_generic_request(struct gpio_chip *gc, unsigned int offset)
 {
 #ifdef CONFIG_PINCTRL
 	if (list_empty(&gc->gpiodev->pin_ranges))

@@ -1651,7 +1651,7 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_request);
  * @gc: the gpiochip to request the gpio function for
  * @offset: the offset of the GPIO to free from GPIO function
  */
-void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset)
+void gpiochip_generic_free(struct gpio_chip *gc, unsigned int offset)
 {
 	pinctrl_gpio_free(gc->gpiodev->base + offset);
 }

@@ -1663,7 +1663,7 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_free);
  * @offset: the offset of the GPIO to apply the configuration
  * @config: the configuration to be applied
  */
-int gpiochip_generic_config(struct gpio_chip *gc, unsigned offset,
+int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
 			    unsigned long config)
 {
 	return pinctrl_gpio_set_config(gc->gpiodev->base + offset, config);

@@ -1994,7 +1994,7 @@ void gpiod_free(struct gpio_desc *desc)
  * help with diagnostics, and knowing that the signal is used as a GPIO
  * can help avoid accidentally multiplexing it to another controller.
  */
-const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned offset)
+const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_desc *desc;

@@ -2098,7 +2098,7 @@ static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
 	unsigned long config;
-	unsigned arg;
+	unsigned int arg;
 
 	switch (mode) {
 	case PIN_CONFIG_BIAS_PULL_DOWN:

@@ -2354,7 +2354,7 @@ EXPORT_SYMBOL_GPL(gpiod_set_config);
  * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
  * debounce time.
  */
-int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
+int gpiod_set_debounce(struct gpio_desc *desc, unsigned int debounce)
 {
 	unsigned long config;

include/linux/gpio/consumer.h +2 -2

index 901aab89d025..ef49307611d2 100644
@@ -158,7 +158,7 @@ int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 				       unsigned long *value_bitmap);
 
 int gpiod_set_config(struct gpio_desc *desc, unsigned long config);
-int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
+int gpiod_set_debounce(struct gpio_desc *desc, unsigned int debounce);
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 void gpiod_toggle_active_low(struct gpio_desc *desc);

@@ -481,7 +481,7 @@ static inline int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
 	return -ENOSYS;
 }
 
-static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
+static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned int debounce)
 {
 	/* GPIO can never have been requested */
 	WARN_ON(desc);

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #2 lore raw

PATCH v7 2/18 gpiolib: add missed break statement

In reply to Andy Shevchenko

It's no difference in the functionality, but after the change the code is less error prone to various checkers.

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib.c | 1 +
 1 file changed, 1 insertion(+)

drivers/gpio/gpiolib.c +1 -0

index f94225d7817b..b45487aace7e 100644
@@ -2108,6 +2108,7 @@ static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
 
 	default:
 		arg = 0;
+		break;
 	}
 
 	config = PIN_CONF_PACKED(mode, arg);

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #3 lore raw

PATCH v7 3/18 gpiolib: use proper API to pack pin configuration parameters

In reply to Andy Shevchenko

Instead of open coded macro use, call pinconf_to_config_packed().

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpio/gpiolib.c +1 -1

index b45487aace7e..87bb73991337 100644
@@ -2111,7 +2111,7 @@ static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
 		break;
 	}
 
-	config = PIN_CONF_PACKED(mode, arg);
+	config = pinconf_to_config_packed(mode, arg);
 	return gpio_do_set_config(gc, gpio_chip_hwgpio(desc), config);
 }

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #4 lore raw

PATCH v7 4/18 gpiolib: Extract gpio_set_config_with_argument() for future use

In reply to Andy Shevchenko

In the future we will need to have a separate function that takes an arbitrary argument value.

Extract gpio_set_config_with_argument() for that purpose.

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

drivers/gpio/gpiolib.c +11 -3

index 87bb73991337..5210f396f0cd 100644
@@ -2094,10 +2094,19 @@ static int gpio_do_set_config(struct gpio_chip *gc, unsigned int offset,
 	return gc->set_config(gc, offset, config);
 }
 
-static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
+static int gpio_set_config_with_argument(struct gpio_desc *desc,
+					 enum pin_config_param mode,
+					 u32 argument)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
 	unsigned long config;
+
+	config = pinconf_to_config_packed(mode, argument);
+	return gpio_do_set_config(gc, gpio_chip_hwgpio(desc), config);
+}
+
+static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
+{
 	unsigned int arg;
 
 	switch (mode) {

@@ -2111,8 +2120,7 @@ static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
 		break;
 	}
 
-	config = pinconf_to_config_packed(mode, arg);
-	return gpio_do_set_config(gc, gpio_chip_hwgpio(desc), config);
+	return gpio_set_config_with_argument(desc, mode, arg);
 }
 
 static int gpio_set_bias(struct gpio_desc *desc)

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #5 lore raw

PATCH v7 5/18 gpiolib: move bias related code from gpio_set_config() to gpio_set_bias()

In reply to Andy Shevchenko

Move bias related code from gpio_set_config() to gpio_set_bias().

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

drivers/gpio/gpiolib.c +14 ~~-15~~

index 5210f396f0cd..2bfc44ca4425 100644
@@ -2107,25 +2107,13 @@ static int gpio_set_config_with_argument(struct gpio_desc *desc,
 
 static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
 {
-	unsigned int arg;
-
-	switch (mode) {
-	case PIN_CONFIG_BIAS_PULL_DOWN:
-	case PIN_CONFIG_BIAS_PULL_UP:
-		arg = 1;
-		break;
-
-	default:
-		arg = 0;
-		break;
-	}
-
-	return gpio_set_config_with_argument(desc, mode, arg);
+	return gpio_set_config_with_argument(desc, mode, 0);
 }
 
 static int gpio_set_bias(struct gpio_desc *desc)
 {
 	enum pin_config_param bias;
+	unsigned int arg;
 	int ret;
 
 	if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))

@@ -2137,7 +2125,18 @@ static int gpio_set_bias(struct gpio_desc *desc)
 	else
 		return 0;
 
-	ret = gpio_set_config(desc, bias);
+	switch (bias) {
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_PULL_UP:
+		arg = 1;
+		break;
+
+	default:
+		arg = 0;
+		break;
+	}
+
+	ret = gpio_set_config_with_argument(desc, bias, arg);
 	if (ret != -ENOTSUPP)
 		return ret;

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #6 lore raw

PATCH v7 6/18 gpiolib: Extract gpio_set_config_with_argument_optional() helper

In reply to Andy Shevchenko

This function is useful for internal use in the GPIO library. There will be new user coming, prepare a helper for the new comer and the existing ones.

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib.c | 53 +++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

drivers/gpio/gpiolib.c +27 ~~-26~~

index 2bfc44ca4425..c4f73597a54d 100644
@@ -2105,6 +2105,29 @@ static int gpio_set_config_with_argument(struct gpio_desc *desc,
 	return gpio_do_set_config(gc, gpio_chip_hwgpio(desc), config);
 }
 
+static int gpio_set_config_with_argument_optional(struct gpio_desc *desc,
+						  enum pin_config_param mode,
+						  u32 argument)
+{
+	struct device *dev = &desc->gdev->dev;
+	int gpio = gpio_chip_hwgpio(desc);
+	int ret;
+
+	ret = gpio_set_config_with_argument(desc, mode, argument);
+	if (ret != -ENOTSUPP)
+		return ret;
+
+	switch (mode) {
+	case PIN_CONFIG_PERSIST_STATE:
+		dev_dbg(dev, "Persistence not supported for GPIO %d\n", gpio);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
 {
 	return gpio_set_config_with_argument(desc, mode, 0);

@@ -2114,7 +2137,6 @@ static int gpio_set_bias(struct gpio_desc *desc)
 {
 	enum pin_config_param bias;
 	unsigned int arg;
-	int ret;
 
 	if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
 		bias = PIN_CONFIG_BIAS_DISABLE;

@@ -2136,11 +2158,7 @@ static int gpio_set_bias(struct gpio_desc *desc)
 		break;
 	}
 
-	ret = gpio_set_config_with_argument(desc, bias, arg);
-	if (ret != -ENOTSUPP)
-		return ret;
-
-	return 0;
+	return gpio_set_config_with_argument_optional(desc, bias, arg);
 }
 
 /**

@@ -2381,11 +2399,6 @@ EXPORT_SYMBOL_GPL(gpiod_set_debounce);
  */
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 {
-	struct gpio_chip *gc;
-	unsigned long packed;
-	int gpio;
-	int rc;
-
 	VALIDATE_DESC(desc);
 	/*
 	 * Handle FLAG_TRANSITORY first, enabling queries to gpiolib for

@@ -2394,21 +2407,9 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 	assign_bit(FLAG_TRANSITORY, &desc->flags, transitory);
 
 	/* If the driver supports it, set the persistence state now */
-	gc = desc->gdev->chip;
-	if (!gc->set_config)
-		return 0;
-
-	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
-					  !transitory);
-	gpio = gpio_chip_hwgpio(desc);
-	rc = gpio_do_set_config(gc, gpio, packed);
-	if (rc == -ENOTSUPP) {
-		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
-				gpio);
-		return 0;
-	}
-
-	return rc;
+	return gpio_set_config_with_argument_optional(desc,
+						      PIN_CONFIG_PERSIST_STATE,
+						      !transitory);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_transitory);

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #7 lore raw

PATCH v7 7/18 gpiolib: Introduce gpio_set_debounce_timeout() for internal use

In reply to Andy Shevchenko

In some cases we would like to have debounce setter which doesn't fail when a feature is not supported by a controller.

Cc Mika Westerberg <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

---
 drivers/gpio/gpiolib.c | 7 +++++++
 drivers/gpio/gpiolib.h | 1 +
 2 files changed, 8 insertions(+)

drivers/gpio/gpiolib.c +7 -0

index c4f73597a54d..0d691ec512a4 100644
@@ -2161,6 +2161,13 @@ static int gpio_set_bias(struct gpio_desc *desc)
 	return gpio_set_config_with_argument_optional(desc, bias, arg);
 }
 
+int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
+{
+	return gpio_set_config_with_argument_optional(desc,
+						      PIN_CONFIG_INPUT_DEBOUNCE,
+						      debounce);
+}
+
 /**
  * gpiod_direction_input - set the GPIO direction to input
  * @desc:	GPIO to set to input

drivers/gpio/gpiolib.h +1 -0

index 16bc5731673c..9b1a1c782704 100644
@@ -136,6 +136,7 @@ int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
 int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags);
+int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);

Signature · 1 line

2.28.0

Mika Westerberg

2020-11-12 08:16 UTC #8 lore raw

Re: [PATCH v7 07/18] gpiolib: Introduce gpio_set_debounce_timeout() for internal use

Quoted 16 lines · Andy Shevchenko

In some cases we would like to have debounce setter which doesn't fail when a feature is not supported by a controller.
Cc Mika Westerberg <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Hans de Goede <[email protected]>
---
 drivers/gpio/gpiolib.c | 7 +++++++
 drivers/gpio/gpiolib.h | 1 +
 2 files changed, 8 insertions(+)
drivers/gpio/gpiolib.c +0 -0
index c4f73597a54d..0d691ec512a4 100644

drivers/gpio/gpiolib.c static int gpio_set_bias(struct gpio_desc *desc) · line 2169

Quoted 9 lines

drivers/gpio/gpiolib.c +6 -0

@@ -2161,6 +2161,13 @@ static int gpio_set_bias(struct gpio_desc *desc)
 	return gpio_set_config_with_argument_optional(desc, bias, arg);
 }
 
+int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
+{
+	return gpio_set_config_with_argument_optional(desc,
+						      PIN_CONFIG_INPUT_DEBOUNCE,
+						      debounce);
+}

Sorry for commenting this late but I realized we have gpiod_set_debounce() already. At least this new function should follow the naming (gpiod as it takes descriptior):

int gpiod_set_debounce_optional(struct gpio_desc *desc, unsigned int debounce);

Andy Shevchenko

2020-11-12 10:18 UTC #9 lore raw

Re: [PATCH v7 07/18] gpiolib: Introduce gpio_set_debounce_timeout() for internal use

Quoted 1 line · Mika Westerberg

On Thu, Nov 12, 2020 at 12:19:57AM +0200, Andy Shevchenko wrote:

...

Quoted 7 lines

Quoted 1 line
+int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
Sorry for commenting this late but I realized we have gpiod_set_debounce() already. At least this new function should follow the naming (gpiod as it takes descriptior):
int gpiod_set_debounce_optional(struct gpio_desc *desc, unsigned int debounce);

Unfortunately it will be inconsistent with the naming schema. I explained the choice of name in the cover letter. I will elaborate a bit there.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Mika Westerberg

2020-11-12 11:09 UTC #10 lore raw

Re: [PATCH v7 07/18] gpiolib: Introduce gpio_set_debounce_timeout() for internal use

Quoted 12 lines · Andy Shevchenko

Quoted 1 line · Mika Westerberg
On Thu, Nov 12, 2020 at 12:19:57AM +0200, Andy Shevchenko wrote:
...
Quoted 6 lines
Quoted 1 line
+int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
Sorry for commenting this late but I realized we have gpiod_set_debounce() already. At least this new function should follow the naming (gpiod as it takes descriptior):

Quoted 5 lines

Quoted 1 line
int gpiod_set_debounce_optional(struct gpio_desc *desc, unsigned int debounce);
Unfortunately it will be inconsistent with the naming schema. I explained the choice of name in the cover letter. I will elaborate a bit there.

OK, It seems that I was not CC'd cover letter of the last two versions, or it got dropped somewhere.

Hans de Goede

2020-11-12 11:12 UTC #11 lore raw

Re: [PATCH v7 07/18] gpiolib: Introduce gpio_set_debounce_timeout() for internal use

Hi,

Quoted 13 lines · Mika Westerberg

Quoted 12 lines · Andy Shevchenko
Quoted 1 line · Mika Westerberg
On Thu, Nov 12, 2020 at 12:19:57AM +0200, Andy Shevchenko wrote:
...
Quoted 6 lines
Quoted 1 line
+int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
Sorry for commenting this late but I realized we have gpiod_set_debounce() already. At least this new function should follow the naming (gpiod as it takes descriptior):

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
int gpiod_set_debounce_optional(struct gpio_desc *desc, unsigned int debounce);
Unfortunately it will be inconsistent with the naming schema. I explained the choice of name in the cover letter. I will elaborate a bit there.
OK, It seems that I was not CC'd cover letter of the last two versions, or it got dropped somewhere.

Yes I did not get the cover-letter either, I was actually wondering if there was one...

Regards,

Hans

Andy Shevchenko

2020-11-12 12:07 UTC #12 lore raw

Re: [PATCH v7 07/18] gpiolib: Introduce gpio_set_debounce_timeout() for internal use

Quoted 20 lines · Hans de Goede

Quoted 19 lines · Mika Westerberg
Quoted 17 lines · Andy Shevchenko
Quoted 1 line · Mika Westerberg
On Thu, Nov 12, 2020 at 12:19:57AM +0200, Andy Shevchenko wrote:
...
Quoted 7 lines
Quoted 1 line
+int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
Sorry for commenting this late but I realized we have gpiod_set_debounce() already. At least this new function should follow the naming (gpiod as it takes descriptior):
int gpiod_set_debounce_optional(struct gpio_desc *desc, unsigned int debounce);
Unfortunately it will be inconsistent with the naming schema. I explained the choice of name in the cover letter. I will elaborate a bit there.

Quoted 5 lines

Quoted 2 lines
OK, It seems that I was not CC'd cover letter of the last two versions, or it got dropped somewhere.
Yes I did not get the cover-letter either, I was actually wondering if there was one...

Oops, I dropped --cc because of your Rb tags (and I have added Cc when there is no tag), but I forgot to add Cc to the cover letter.

Here we are [1].

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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #13 lore raw

PATCH v7 8/18 gpiolib: acpi: Respect bias settings for GpioInt() resource

In reply to 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 <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Acked-by Linus Walleij <[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

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #14 lore raw

PATCH v7 9/18 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 Hans de Goede <[email protected]>

Acked-by Linus Walleij <[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

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #15 lore raw

PATCH v7 10/18 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]>

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Acked-by Linus Walleij <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib-acpi.c | 18 ++++++++++++++++++
 drivers/gpio/gpiolib-acpi.h |  2 ++
 2 files changed, 20 insertions(+)

drivers/gpio/gpiolib-acpi.c +18 -0

index c127b410a7a2..a9254de964cc 100644
@@ -299,6 +299,10 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 		return AE_OK;
 	}
 
+	ret = gpio_set_debounce_timeout(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 = gpio_set_debounce_timeout(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 = gpio_set_debounce_timeout(desc, agpio->debounce_timeout);
+			if (ret) {
+				gpiochip_free_own_desc(desc);
+				mutex_unlock(&achip->conn_lock);
+				status = AE_ERROR;
+				goto out;
+			}
+
 			conn = kzalloc(sizeof(*conn), GFP_KERNEL);
 			if (!conn) {
 				status = AE_NO_MEMORY;

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

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #16 lore raw

PATCH v7 11/18 gpiolib: acpi: Move non-critical code outside of critical section

In reply to Andy Shevchenko

Mika noticed that some code is run under mutex when it doesn't require the lock, like an error code assignment.

Move non-critical code outside of critical section.

Cc Hans de Goede <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpiolib-acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpio/gpiolib-acpi.c +2 -2

index a9254de964cc..b00171d2aaf5 100644
@@ -1063,8 +1063,8 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 							 GPIO_ACTIVE_HIGH,
 							 flags);
 			if (IS_ERR(desc)) {
-				status = AE_ERROR;
 				mutex_unlock(&achip->conn_lock);
+				status = AE_ERROR;
 				goto out;
 			}

@@ -1078,9 +1078,9 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 
 			conn = kzalloc(sizeof(*conn), GFP_KERNEL);
 			if (!conn) {
-				status = AE_NO_MEMORY;
 				gpiochip_free_own_desc(desc);
 				mutex_unlock(&achip->conn_lock);
+				status = AE_NO_MEMORY;
 				goto out;
 			}

Signature · 1 line

2.28.0

Mika Westerberg

Reviewed-by on v7 11/18

2020-11-12 08:17 UTC #17 lore raw

Re: [PATCH v7 11/18] gpiolib: acpi: Move non-critical code outside of critical section

Quoted 6 lines · Andy Shevchenko

Mika noticed that some code is run under mutex when it doesn't require the lock, like an error code assignment.
Move non-critical code outside of critical section.
Suggested-by Mika Westerberg <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #18 lore raw

PATCH v7 12/18 gpiolib: acpi: Move acpi_gpio_to_gpiod_flags() upper in the code

In reply to Andy Shevchenko

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

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib-acpi.c | 66 ++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

drivers/gpio/gpiolib-acpi.c +33 ~~-33~~

index b00171d2aaf5..ac1bde0720f2 100644
@@ -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)
 {

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #19 lore raw

PATCH v7 13/18 gpiolib: acpi: Set initial value for output pin based on bias and polarity

In reply to Andy Shevchenko

From: Vasile-Laurentiu Stanimir <[email protected]>

GpioIo() resources don't contain an initial value for the output pin. Therefore instead of deducting its value solely based on bias field we should deduce that value from the polarity and the bias fields. Typical scenario is, when pin is defined in the table and its polarity, specified in _DSD or via platform code, is defined as active low, in the following call chain:

-> acpi_populate_gpio_lookup()

     -> acpi_gpio_to_gpiod_flags()

it will return GPIOD_OUT_HIGH if bias is set no matter if polarity is GPIO_ACTIVE_LOW, so it will return the current level instead of the logical level.

Cc Hans de Goede <[email protected]>

Cc Mika Westerberg <[email protected]>

Signed-off-by Vasile-Laurentiu Stanimir <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpiolib-acpi.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

drivers/gpio/gpiolib-acpi.c +12 -9

index ac1bde0720f2..b47d5e8edaeb 100644
@@ -206,7 +206,7 @@ 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)
+acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio, int polarity)
 {
 	switch (agpio->io_restriction) {
 	case ACPI_IO_RESTRICT_INPUT:

@@ -215,15 +215,17 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
 		/*
 		 * 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.
+		 * and the polarity 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. For active low
+		 * polarity values will be switched. See also
+		 * Documentation/firmware-guide/acpi/gpio-properties.rst.
 		 */
 		switch (agpio->pin_config) {
 		case ACPI_PIN_CONFIG_PULLUP:
-			return GPIOD_OUT_HIGH;
+			return polarity == GPIO_ACTIVE_LOW ? GPIOD_OUT_LOW : GPIOD_OUT_HIGH;
 		case ACPI_PIN_CONFIG_PULLDOWN:
-			return GPIOD_OUT_LOW;
+			return polarity == GPIO_ACTIVE_LOW ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
 		default:
 			break;
 		}

@@ -683,8 +685,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 			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;
+			lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio, lookup->info.polarity);
 		}
 	}

@@ -1055,12 +1057,13 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		}
 
 		if (!found) {
-			enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio);
+			int polarity = GPIO_ACTIVE_HIGH;
+			enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio, polarity);
 			const char *label = "ACPI:OpRegion";
 			int ret;
 
 			desc = gpiochip_request_own_desc(chip, pin, label,
-							 GPIO_ACTIVE_HIGH,
+							 polarity,
 							 flags);
 			if (IS_ERR(desc)) {
 				mutex_unlock(&achip->conn_lock);

Signature · 1 line

2.28.0

Mika Westerberg

Reviewed-by on v7 13/18

2020-11-12 09:10 UTC #20 lore raw

Re: [PATCH v7 13/18] gpiolib: acpi: Set initial value for output pin based on bias and polarity

Quoted 10 lines · Andy Shevchenko

From: Vasile-Laurentiu Stanimir <[email protected]>
GpioIo() resources don't contain an initial value for the output pin. Therefore instead of deducting its value solely based on bias field we should deduce that value from the polarity and the bias fields. Typical scenario is, when pin is defined in the table and its polarity, specified in _DSD or via platform code, is defined as active low, in the following call chain:
-> acpi_populate_gpio_lookup()

Quoted 8 lines

     -> acpi_gpio_to_gpiod_flags()
it will return GPIOD_OUT_HIGH if bias is set no matter if polarity is GPIO_ACTIVE_LOW, so it will return the current level instead of the logical level.
Cc Hans de Goede <[email protected]>
Cc Mika Westerberg <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #21 lore raw

PATCH v7 14/18 gpiolib: acpi: Make acpi_gpio_to_gpiod_flags() usable for GpioInt()

In reply to 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 <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib-acpi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/gpio/gpiolib-acpi.c +6 -2

index b47d5e8edaeb..644067cc0f81 100644
@@ -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, int polarity)
 {
+	/* 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;

@@ -681,13 +685,13 @@ 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.polarity = lookup->active_low;
-			lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio, lookup->info.polarity);
 		}
+
+		lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio, lookup->info.polarity);
 	}
 
 	return 1;

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #22 lore raw

PATCH v7 15/18 gpiolib: acpi: Extract acpi_request_own_gpiod() helper

In reply to 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 <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib-acpi.c | 46 +++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

drivers/gpio/gpiolib-acpi.c +24 ~~-22~~

index 644067cc0f81..c46fd51007d0 100644
@@ -244,6 +244,28 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio, int polarity)
 	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)
+{
+	int polarity = GPIO_ACTIVE_HIGH;
+	enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio, polarity);
+	unsigned int pin = agpio->pin_table[index];
+	struct gpio_desc *desc;
+	int ret;
+
+	desc = gpiochip_request_own_desc(chip, pin, label, polarity, flags);
+	if (IS_ERR(desc))
+		return desc;
+
+	ret = gpio_set_debounce_timeout(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;

@@ -329,8 +351,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",

@@ -338,10 +359,6 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 		return AE_OK;
 	}
 
-	ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout);
-	if (ret)
-		goto fail_free_desc;
-
 	ret = gpiochip_lock_as_irq(chip, pin);
 	if (ret) {
 		dev_err(chip->parent,

@@ -1061,28 +1078,13 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		}
 
 		if (!found) {
-			int polarity = GPIO_ACTIVE_HIGH;
-			enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio, polarity);
-			const char *label = "ACPI:OpRegion";
-			int ret;
-
-			desc = gpiochip_request_own_desc(chip, pin, label,
-							 polarity,
-							 flags);
+			desc = acpi_request_own_gpiod(chip, agpio, i, "ACPI:OpRegion");
 			if (IS_ERR(desc)) {
 				mutex_unlock(&achip->conn_lock);
 				status = AE_ERROR;
 				goto out;
 			}
 
-			ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout);
-			if (ret) {
-				gpiochip_free_own_desc(desc);
-				mutex_unlock(&achip->conn_lock);
-				status = AE_ERROR;
-				goto out;
-			}
-
 			conn = kzalloc(sizeof(*conn), GFP_KERNEL);
 			if (!conn) {
 				gpiochip_free_own_desc(desc);

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #23 lore raw

PATCH v7 16/18 gpiolib: acpi: Convert pin_index to be u16

In reply to Andy Shevchenko

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

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Acked-by Linus Walleij <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib-acpi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/gpio/gpiolib-acpi.c +5 -5

index c46fd51007d0..a556e2ec0a39 100644
@@ -660,7 +660,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;

@@ -676,7 +676,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++;

@@ -822,7 +822,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 {

@@ -1018,7 +1018,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;

@@ -1041,7 +1041,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

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #24 lore raw

PATCH v7 17/18 gpiolib: acpi: Use BIT() macro to increase readability

In reply to Andy Shevchenko

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

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Hans de Goede <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

---
 drivers/gpio/gpiolib-acpi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/gpio/gpiolib-acpi.c +1 -2

index a556e2ec0a39..6cc5f91bfe2e 100644
@@ -1101,8 +1101,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;
 	}

Signature · 1 line

2.28.0

Andy Shevchenko

author maintainer

2020-11-11 22:20 UTC #25 lore raw

PATCH v7 18/18 gpiolib: acpi: Make Intel GPIO tree official for GPIO ACPI work

In reply to Andy Shevchenko

Make Intel GPIO tree official for GPIO ACPI work.

Signed-off-by Andy Shevchenko <[email protected]>

---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

MAINTAINERS +1 -0

index e73636b75f29..53236b2ea0af 100644
@@ -7483,6 +7483,7 @@ M:	Andy Shevchenko <[email protected]>
 L:	[email protected]
 L:	[email protected]
 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

Signature · 1 line

2.28.0

Andy Shevchenko

2020-11-12 10:23 UTC #26 lore raw

In reply to Andy Shevchenko

Quoted 10 lines · Andy Shevchenko

There are fixes (and plenty cleanups) that allow to take into consideration more parameters in ACPI, i.e. bias for GpioInt() and debounce timeout for Operation Regions, Events and GpioInt() resources.
During review Hans noted, that gpiod_set_debounce() returns -ENOTSUPP for the cases when feature is not supported either by driver or a controller.
It appears that we have slightly messy API here:

Quoted 8 lines

FUNC Relation with 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)

Mika, the above list is all about GPIO descriptors with an exception of gpio_set_debounce(). The gpio_set_*(struct gpio_desc *desc, ...) schema is used not only for the above set of functions, but also another *internal* ones

gpio_set_open_drain_value_commit()
gpio_set_open_source_value_commit()

Hope this helps.

I think I can move gpio_set_debounce_timeout out of gpiod_* group of declarations in gpiolib.h to avoid confusion. Will it work for you?

Quoted 93 lines

Last two functions are internal to GPIO library, while the rest is exported API. In order to be consistent with both naming schemas the series introduces gpio_set_debounce_timeout() that considers the feature optional. New API is only for internal use.
While at it, the few first patches clean up the current GPIO library code to unify it to some extend.
The above is followed by changes made in ACPI GPIO library part.
The bias patch highly depends on Intel pin control driver changes (they are material for v5.10 [1]), due to this and amount of the prerequisite changes this series 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.
Note, some patches are also depend to the code from GPIO fixes / for-next repositories. Unfortunately there is no one repository which contains all up to date for-next changes against GPIO subsystem. That's why I have merged Linus' for-next followed by Bart's for-next branches as prerequisites to the series.
Cc Jamie McClymont <[email protected]>
Cc Coiby Xu <[email protected]>
Cc Vasile-Laurentiu Stanimir <[email protected]>
[1]: https://lore.kernel.org/linux-gpio/[email protected]/
Changelog v7:
- sent correct set of patches
Changelog v6:
- added tags (Hans, Mika, Linus)
- dropped temporary variables in couple of patches (Mika)
- dropped "Add temporary variable to gpiod_set_transitory()..." (Mika)
- added patch to move assignments outside a lock (Mika)
- added patch from Vasile
- due to above rebased accordingly the affected ones
Changelog v5:
- introduced gpio_set_debounce_timeout()
- made a prerequisite refactoring in GPIO library code
- updated the rest accordingly
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 (17):
gpiolib: Replace unsigned by unsigned int
gpiolib: add missed break statement
gpiolib: use proper API to pack pin configuration parameters
gpiolib: Extract gpio_set_config_with_argument() for future use
gpiolib: move bias related code from gpio_set_config() to
    gpio_set_bias()
gpiolib: Extract gpio_set_config_with_argument_optional() helper
gpiolib: Introduce gpio_set_debounce_timeout() for internal use
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 non-critical code outside of critical section
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
Vasile-Laurentiu Stanimir (1):
gpiolib: acpi: Set initial value for output pin based on bias and
polarity
 MAINTAINERS                   |   1 +
 drivers/gpio/gpiolib-acpi.c   | 138 +++++++++++++++++++++-------------
 drivers/gpio/gpiolib-acpi.h   |   2 +
 drivers/gpio/gpiolib.c        |  98 ++++++++++++++----------
 drivers/gpio/gpiolib.h        |   1 +
 include/linux/gpio/consumer.h |   4 +-
 6 files changed, 147 insertions(+), 97 deletions(-)
--
2.28.0

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2020-11-12 14:16 UTC #27 lore raw

+Cc Mika and Hans

Linus, Bart, it would be nice to hear from you about GPIO library cleanups and in general about the series...

Quoted 133 lines · Andy Shevchenko

Quoted 10 lines · Andy Shevchenko
There are fixes (and plenty cleanups) that allow to take into consideration more parameters in ACPI, i.e. bias for GpioInt() and debounce timeout for Operation Regions, Events and GpioInt() resources.
During review Hans noted, that gpiod_set_debounce() returns -ENOTSUPP for the cases when feature is not supported either by driver or a controller.
It appears that we have slightly messy API here:
Quoted 8 lines
FUNC Relation with 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)
Mika, the above list is all about GPIO descriptors with an exception of gpio_set_debounce(). The gpio_set_*(struct gpio_desc *desc, ...) schema is used not only for the above set of functions, but also another *internal* ones
gpio_set_open_drain_value_commit()
gpio_set_open_source_value_commit()
Hope this helps.
I think I can move gpio_set_debounce_timeout out of gpiod_* group of declarations in gpiolib.h to avoid confusion. Will it work for you?
Quoted 90 lines
Last two functions are internal to GPIO library, while the rest is exported API. In order to be consistent with both naming schemas the series introduces gpio_set_debounce_timeout() that considers the feature optional. New API is only for internal use.
While at it, the few first patches clean up the current GPIO library code to unify it to some extend.
The above is followed by changes made in ACPI GPIO library part.
The bias patch highly depends on Intel pin control driver changes (they are material for v5.10 [1]), due to this and amount of the prerequisite changes this series 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.
Note, some patches are also depend to the code from GPIO fixes / for-next repositories. Unfortunately there is no one repository which contains all up to date for-next changes against GPIO subsystem. That's why I have merged Linus' for-next followed by Bart's for-next branches as prerequisites to the series.
Cc Jamie McClymont <[email protected]>
Cc Coiby Xu <[email protected]>
Cc Vasile-Laurentiu Stanimir <[email protected]>
[1]: https://lore.kernel.org/linux-gpio/[email protected]/
Changelog v7:
- sent correct set of patches
Changelog v6:
- added tags (Hans, Mika, Linus)
- dropped temporary variables in couple of patches (Mika)
- dropped "Add temporary variable to gpiod_set_transitory()..." (Mika)
- added patch to move assignments outside a lock (Mika)
- added patch from Vasile
- due to above rebased accordingly the affected ones
Changelog v5:
- introduced gpio_set_debounce_timeout()
- made a prerequisite refactoring in GPIO library code
- updated the rest accordingly
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 (17):
gpiolib: Replace unsigned by unsigned int
gpiolib: add missed break statement
gpiolib: use proper API to pack pin configuration parameters
gpiolib: Extract gpio_set_config_with_argument() for future use
gpiolib: move bias related code from gpio_set_config() to
    gpio_set_bias()
gpiolib: Extract gpio_set_config_with_argument_optional() helper
gpiolib: Introduce gpio_set_debounce_timeout() for internal use
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 non-critical code outside of critical section
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
Vasile-Laurentiu Stanimir (1):
gpiolib: acpi: Set initial value for output pin based on bias and
polarity
 MAINTAINERS                   |   1 +
 drivers/gpio/gpiolib-acpi.c   | 138 +++++++++++++++++++++-------------
 drivers/gpio/gpiolib-acpi.h   |   2 +
 drivers/gpio/gpiolib.c        |  98 ++++++++++++++----------
 drivers/gpio/gpiolib.h        |   1 +
 include/linux/gpio/consumer.h |   4 +-
 6 files changed, 147 insertions(+), 97 deletions(-)
Quoted 3 lines
--
2.28.0
--
With Best Regards,
Andy Shevchenko

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

Acked-by

2020-11-15 16:04 UTC #28 lore raw

Quoted 2 lines · Andy Shevchenko

Linus, Bart, it would be nice to hear from you about GPIO library cleanups and in general about the series...

The series:

Acked-by Linus Walleij <[email protected]>

Can you send me a pull request for the contents of the v7 patch series so I can pull it in?

Yours,
Linus Walleij

Andy Shevchenko

2020-11-16 10:24 UTC #29 lore raw

Quoted 11 lines · Linus Walleij

Quoted 2 lines · Andy Shevchenko
Linus, Bart, it would be nice to hear from you about GPIO library cleanups and in general about the series...
The series:
Acked-by Linus Walleij <[email protected]>
Can you send me a pull request for the contents of the v7 patch series so I can pull it in?

Sure. Will do this week (presumably today or tomorrow).

Signature · 2 lines

With Best Regards,
Andy Shevchenko