[PATCH v2 0/5] gpio: add support for pull-up/pull-down configuration

[PATCH v2 0/5] gpio: add support for pull-up/pull-down configuration

[Thomas Petazzoni]

Hello,

As we started discussing in [1], it would be useful to have a way to
configure pull-up/pull-down resistors for simple GPIO controllers that
don't have any pinmuxing capability, and therefore no interaction with
the pinctrl subsystem.

This is a second iteration of the patches (the v1 was posted at
https://patchwork.ozlabs.org/cover/1020392/).

Changes since v1:

 - Add a comment in the binding that explicitly states that the new
   bit 4 and 5 should only be used for HW with a simple on/off
   pull-up/pull-down configuration. More complex HW should use a pin
   control binding. Suggested by Linus Walleij.

 - Change gpiod_set_debounce() and gpiod_set_transitory() to use the
   new gpio_set_config() helper. This is done in a new, separate
   patch. Suggested by Linus Walleij.

 - Add error checking in gpiod_configure_flags() to make sure pull-up
   and pull-down are not both enabled on the same GPIO. Suggested by
   Jan Kundrát. Jan suggested to put this in the code converting from
   DT properties to internal flags, but it actually makes more sense
   to do it in gpiod_configure_flags(), independently from DT parsing.

 - Add a check in gpio-pca953x driver that pull-up/pull-down is indeed
   supported by the underlying HW in the
   pca953x_gpio_set_pull_up_down() function. Noticed by Linus Walleij.

 - Move the changes in include/dt-bindings/gpio/gpio.h to the
   dt-bindings patch, as suggested by Rob Herring.

Rob: due to the changes in the dt-bindings patch, I have not kept your
Acked-by.

Thomas

[1] https://marc.info/?l=linux-gpio&m=154491873506701&w=2

Thomas Petazzoni (5):
  dt-bindings: gpio: document the new pull-up/pull-down flags
  gpio: rename gpio_set_drive_single_ended() to gpio_set_config()
  gpio: use new gpio_set_config() helper in more places
  gpio: add core support for pull-up/pull-down configuration
  gpio: pca953x: add ->set_config implementation

 .../devicetree/bindings/gpio/gpio.txt         | 12 ++++
 drivers/gpio/gpio-pca953x.c                   | 66 ++++++++++++++++++-
 drivers/gpio/gpiolib-of.c                     |  5 ++
 drivers/gpio/gpiolib.c                        | 50 +++++++++-----
 drivers/gpio/gpiolib.h                        |  2 +
 include/dt-bindings/gpio/gpio.h               |  6 ++
 include/linux/gpio/machine.h                  |  2 +
 include/linux/of_gpio.h                       |  2 +
 8 files changed, 127 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH v2 1/5] dt-bindings: gpio: document the new pull-up/pull-down flags

[Thomas Petazzoni]

This commit extends the flags that can be used in GPIO specifiers to
indicate if a pull-up resistor or pull-down resistor should be
enabled.

While some pinctrl DT bindings already offer the capability of
configuring pull-up/pull-down resistors at the pin level, a number of
simple GPIO controllers don't have any pinmuxing capability, and
therefore do not rely on the pinctrl DT bindings.

Such simple GPIO controllers however sometimes allow to configure
pull-up and pull-down resistors on a per-pin basis, and whether such
resistors should be enabled or not is a highly board-specific HW
characteristic.

By using two additional bits of the GPIO flag specifier, we can easily
allow the Device Tree to describe which GPIOs should have their
pull-up or pull-down resistors enabled. Even though the two options
are mutually exclusive, we still need two bits to encode at least
three states: no pull-up/pull-down, pull-up, pull-down.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 12 ++++++++++++
 include/dt-bindings/gpio/gpio.h                 |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index f0ba154b5723..a8895d339bfe 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -67,6 +67,18 @@ Optional standard bitfield specifiers for the last cell:
            https://en.wikipedia.org/wiki/Open_collector
 - Bit 3: 0 means the output should be maintained during sleep/low-power mode
          1 means the output state can be lost during sleep/low-power mode
+- Bit 4: 0 means no pull-up resistor should be enabled
+         1 means a pull-up resistor should be enabled
+         This setting only applies to hardware with a simple on/off
+         control for pull-up configuration. If the hardware has more
+         elaborate pull-up configuration, it should be represented
+         using a pin control binding.
+- Bit 5: 0 means no pull-down resistor should be enabled
+         1 means a pull-down resistor should be enabled
+         This setting only applies to hardware with a simple on/off
+         control for pull-down configuration. If the hardware has more
+         elaborate pull-down configuration, it should be represented
+         using a pin control binding.
 
 1.1) GPIO specifier best practices
 ----------------------------------
diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index 2cc10ae4bbb7..c029467e828b 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -33,4 +33,10 @@
 #define GPIO_PERSISTENT 0
 #define GPIO_TRANSITORY 8
 
+/* Bit 4 express pull up */
+#define GPIO_PULL_UP 16
+
+/* Bit 5 express pull down */
+#define GPIO_PULL_DOWN 32
+
 #endif
-- 
2.20.1

[PATCH v2 2/5] gpio: rename gpio_set_drive_single_ended() to gpio_set_config()

[Thomas Petazzoni]

This commit simply renames gpio_set_drive_single_ended() to
gpio_set_config(), as the function is not specific to setting the GPIO
drive type, and will be used for other purposes in followup commits.

In addition, it moves the function above gpiod_direction_input(), as
it will be used from gpiod_direction_input().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 drivers/gpio/gpiolib.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 361a09c8138a..cf8a4402fef1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2555,6 +2555,14 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
  * rely on gpio_request() having been called beforehand.
  */
 
+static int gpio_set_config(struct gpio_chip *gc, unsigned offset,
+			   enum pin_config_param mode)
+{
+	unsigned long config = { PIN_CONF_PACKED(mode, 0) };
+
+	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
+}
+
 /**
  * gpiod_direction_input - set the GPIO direction to input
  * @desc:	GPIO to set to input
@@ -2608,14 +2616,6 @@ int gpiod_direction_input(struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_input);
 
-static int gpio_set_drive_single_ended(struct gpio_chip *gc, unsigned offset,
-				       enum pin_config_param mode)
-{
-	unsigned long config = { PIN_CONF_PACKED(mode, 0) };
-
-	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
-}
-
 static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
@@ -2712,8 +2712,8 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	gc = desc->gdev->chip;
 	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
 		/* First see if we can enable open drain in hardware */
-		ret = gpio_set_drive_single_ended(gc, gpio_chip_hwgpio(desc),
-						  PIN_CONFIG_DRIVE_OPEN_DRAIN);
+		ret = gpio_set_config(gc, gpio_chip_hwgpio(desc),
+				      PIN_CONFIG_DRIVE_OPEN_DRAIN);
 		if (!ret)
 			goto set_output_value;
 		/* Emulate open drain by not actively driving the line high */
@@ -2721,16 +2721,16 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 			return gpiod_direction_input(desc);
 	}
 	else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
-		ret = gpio_set_drive_single_ended(gc, gpio_chip_hwgpio(desc),
-						  PIN_CONFIG_DRIVE_OPEN_SOURCE);
+		ret = gpio_set_config(gc, gpio_chip_hwgpio(desc),
+				      PIN_CONFIG_DRIVE_OPEN_SOURCE);
 		if (!ret)
 			goto set_output_value;
 		/* Emulate open source by not actively driving the line low */
 		if (!value)
 			return gpiod_direction_input(desc);
 	} else {
-		gpio_set_drive_single_ended(gc, gpio_chip_hwgpio(desc),
-					    PIN_CONFIG_DRIVE_PUSH_PULL);
+		gpio_set_config(gc, gpio_chip_hwgpio(desc),
+				PIN_CONFIG_DRIVE_PUSH_PULL);
 	}
 
 set_output_value:
-- 
2.20.1

[PATCH v2 3/5] gpio: use new gpio_set_config() helper in more places

[Thomas Petazzoni]

As suggested by Linus Walleij, let's use the new gpio_set_config()
helper in gpiod_set_debounce() and gpiod_set_transitory().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cf8a4402fef1..9762a836fec9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2762,7 +2762,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 	}
 
 	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
+	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
@@ -2799,7 +2799,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
 					  !transitory);
 	gpio = gpio_chip_hwgpio(desc);
-	rc = chip->set_config(chip, gpio, packed);
+	rc = gpio_set_config(chip, gpio, packed);
 	if (rc == -ENOTSUPP) {
 		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
 				gpio);
-- 
2.20.1

[PATCH v2 4/5] gpio: add core support for pull-up/pull-down configuration

[Thomas Petazzoni]

This commit adds support for configuring the pull-up and pull-down
resistors available in some GPIO controllers. While configuring
pull-up/pull-down is already possible through the pinctrl subsystem,
some GPIO controllers, especially simple ones such as GPIO expanders
on I2C, don't have any pinmuxing capability and therefore do not use
the pinctrl subsystem.

This commit implements the GPIO_PULL_UP and GPIO_PULL_DOWN flags,
which can be used from the Device Tree, to enable a pull-up or
pull-down resistor on a given GPIO.

The flag is simply propagated all the way to the core GPIO subsystem,
where it is used to call the gpio_chip ->set_config callback with the
appropriate existing PIN_CONFIG_BIAS_* values.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 drivers/gpio/gpiolib-of.c    |  5 +++++
 drivers/gpio/gpiolib.c       | 18 ++++++++++++++++++
 drivers/gpio/gpiolib.h       |  2 ++
 include/linux/gpio/machine.h |  2 ++
 include/linux/of_gpio.h      |  2 ++
 5 files changed, 29 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index a6e1891217e2..9a8b78477f79 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -345,6 +345,11 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 	if (of_flags & OF_GPIO_TRANSITORY)
 		*flags |= GPIO_TRANSITORY;
 
+	if (of_flags & OF_GPIO_PULL_UP)
+		*flags |= GPIO_PULL_UP;
+	if (of_flags & OF_GPIO_PULL_DOWN)
+		*flags |= GPIO_PULL_DOWN;
+
 	return desc;
 }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9762a836fec9..9be4c9401172 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2610,6 +2610,13 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	if (status == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
 
+	if (test_bit(FLAG_PULL_UP, &desc->flags))
+		gpio_set_config(chip, gpio_chip_hwgpio(desc),
+				PIN_CONFIG_BIAS_PULL_UP);
+	else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+		gpio_set_config(chip, gpio_chip_hwgpio(desc),
+				PIN_CONFIG_BIAS_PULL_DOWN);
+
 	trace_gpio_direction(desc_to_gpio(desc), 1, status);
 
 	return status;
@@ -4087,6 +4094,17 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 	if (lflags & GPIO_OPEN_SOURCE)
 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
+	if ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) {
+		gpiod_err(desc,
+			  "both pull-up and pull-down enabled, invalid configuration\n");
+		return -EINVAL;
+	}
+
+	if (lflags & GPIO_PULL_UP)
+		set_bit(FLAG_PULL_UP, &desc->flags);
+	else if (lflags & GPIO_PULL_DOWN)
+		set_bit(FLAG_PULL_DOWN, &desc->flags);
+
 	status = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
 	if (status < 0)
 		return status;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index bc57f0dc5953..078ab17b96bf 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -219,6 +219,8 @@ struct gpio_desc {
 #define FLAG_IRQ_IS_ENABLED 10	/* GPIO is connected to an enabled IRQ */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 #define FLAG_TRANSITORY 12	/* GPIO may lose value in sleep or reset */
+#define FLAG_PULL_UP    13	/* GPIO has pull up enabled */
+#define FLAG_PULL_DOWN  14	/* GPIO has pull down enabled */
 
 	/* Connection label */
 	const char		*label;
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index daa44eac9241..69673be10213 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -12,6 +12,8 @@ enum gpio_lookup_flags {
 	GPIO_OPEN_SOURCE = (1 << 2),
 	GPIO_PERSISTENT = (0 << 3),
 	GPIO_TRANSITORY = (1 << 3),
+	GPIO_PULL_UP = (1 << 4),
+	GPIO_PULL_DOWN = (1 << 5),
 };
 
 /**
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 163b79ecd01a..f9737dea9d1f 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -28,6 +28,8 @@ enum of_gpio_flags {
 	OF_GPIO_SINGLE_ENDED = 0x2,
 	OF_GPIO_OPEN_DRAIN = 0x4,
 	OF_GPIO_TRANSITORY = 0x8,
+	OF_GPIO_PULL_UP = 0x10,
+	OF_GPIO_PULL_DOWN = 0x20,
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.20.1

[PATCH v2 5/5] gpio: pca953x: add ->set_config implementation

[Thomas Petazzoni]

This commit adds a minimal implementation of the ->set_config() hook,
with support for the PIN_CONFIG_BIAS_PULL_UP and
PIN_CONFIG_BIAS_PULL_DOWN configurations.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 drivers/gpio/gpio-pca953x.c | 66 +++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 83d45f00e4d0..1e5579acd275 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -179,6 +179,8 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
 #define PCA957x_BANK_OUTPUT	BIT(5)
 
 #define PCAL9xxx_BANK_IN_LATCH	BIT(8 + 2)
+#define PCAL9xxx_BANK_PULL_EN	BIT(8 + 3)
+#define PCAL9xxx_BANK_PULL_SEL	BIT(8 + 4)
 #define PCAL9xxx_BANK_IRQ_MASK	BIT(8 + 5)
 #define PCAL9xxx_BANK_IRQ_STAT	BIT(8 + 6)
 
@@ -200,6 +202,8 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
  * - Extended set, above 0x40, often chip specific.
  *   - PCAL6524/PCAL9555A with custom PCAL IRQ handling:
  *     Input latch register		0x40 + 2 * bank_size	RW
+ *     Pull-up/pull-down enable reg	0x40 + 3 * bank_size    RW
+ *     Pull-up/pull-down select reg	0x40 + 4 * bank_size    RW
  *     Interrupt mask register		0x40 + 5 * bank_size	RW
  *     Interrupt status register	0x40 + 6 * bank_size	R
  *
@@ -248,7 +252,8 @@ static bool pca953x_readable_register(struct device *dev, unsigned int reg)
 	}
 
 	if (chip->driver_data & PCA_PCAL) {
-		bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_IRQ_MASK |
+		bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN |
+			PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK |
 			PCAL9xxx_BANK_IRQ_STAT;
 	}
 
@@ -269,7 +274,8 @@ static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
 	}
 
 	if (chip->driver_data & PCA_PCAL)
-		bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_IRQ_MASK;
+		bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN |
+			PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK;
 
 	return pca953x_check_register(chip, reg, bank);
 }
@@ -474,6 +480,61 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 	mutex_unlock(&chip->i2c_lock);
 }
 
+static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
+					 unsigned int offset,
+					 unsigned long config)
+{
+	u8 pull_en_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_EN, offset,
+					     true, false);
+	u8 pull_sel_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_SEL, offset,
+					      true, false);
+	u8 bit = BIT(offset % BANK_SZ);
+	int ret;
+
+	/*
+	 * pull-up/pull-down configuration requires PCAL extended
+	 * registers
+	 */
+	if (!(chip->driver_data & PCA_PCAL))
+		return -ENOTSUPP;
+
+	mutex_lock(&chip->i2c_lock);
+
+	/* Disable pull-up/pull-down */
+	ret = regmap_write_bits(chip->regmap, pull_en_reg, bit, 0);
+	if (ret)
+		goto exit;
+
+	/* Configure pull-up/pull-down */
+	if (config == PIN_CONFIG_BIAS_PULL_UP)
+		ret = regmap_write_bits(chip->regmap, pull_sel_reg, bit, bit);
+	else if (config == PIN_CONFIG_BIAS_PULL_DOWN)
+		ret = regmap_write_bits(chip->regmap, pull_sel_reg, bit, 0);
+	if (ret)
+		goto exit;
+
+	/* Enable pull-up/pull-down */
+	ret = regmap_write_bits(chip->regmap, pull_en_reg, bit, bit);
+
+exit:
+	mutex_unlock(&chip->i2c_lock);
+	return ret;
+}
+
+static int pca953x_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
+				   unsigned long config)
+{
+	struct pca953x_chip *chip = gpiochip_get_data(gc);
+
+	switch (config) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		return pca953x_gpio_set_pull_up_down(chip, offset, config);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
 static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
 {
 	struct gpio_chip *gc;
@@ -486,6 +547,7 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
 	gc->set = pca953x_gpio_set_value;
 	gc->get_direction = pca953x_gpio_get_direction;
 	gc->set_multiple = pca953x_gpio_set_multiple;
+	gc->set_config = pca953x_gpio_set_config;
 	gc->can_sleep = true;
 
 	gc->base = chip->gpio_start;
-- 
2.20.1

Re: [PATCH v2 0/5] gpio: add support for pull-up/pull-down configuration

[Linus Walleij]

On Thu, Feb 7, 2019 at 5:29 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> As we started discussing in [1], it would be useful to have a way to
> configure pull-up/pull-down resistors for simple GPIO controllers that
> don't have any pinmuxing capability, and therefore no interaction with
> the pinctrl subsystem.
>
> This is a second iteration of the patches (the v1 was posted at
> https://patchwork.ozlabs.org/cover/1020392/).

I don't see any problem with this patch set so I have merged it
into an immutable branch in my GPIO tree, and if it builds fine
I will merge it for devel (for-v5.1).

Notably we do not add a userspace ABI in this patch series.

I guess that is fine for now, but I think we might see userspace
support requests soon. But that can be a different patch
and should be driven by actual need for that.

Yours,
Linus Walleij

Re: [PATCH v2 3/5] gpio: use new gpio_set_config() helper in more places

[Guenter Roeck]

On Thu, Feb 07, 2019 at 05:28:57PM +0100, Thomas Petazzoni wrote:
> As suggested by Linus Walleij, let's use the new gpio_set_config()
> helper in gpiod_set_debounce() and gpiod_set_transitory().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  drivers/gpio/gpiolib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index cf8a4402fef1..9762a836fec9 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2762,7 +2762,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
>  	}
>  
>  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> -	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
> +	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);

Are you sure this is correct ? This patch results in a number of tracebacks
in mainline. Reverting it fixes the problem.

gpio_set_config() seems to pack config, but it is already packed above.
That seems a bit suspicious.

>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_debounce);
>  
> @@ -2799,7 +2799,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
>  	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
>  					  !transitory);
>  	gpio = gpio_chip_hwgpio(desc);
> -	rc = chip->set_config(chip, gpio, packed);
> +	rc = gpio_set_config(chip, gpio, packed);

Same here.

Guenter

---
Bisect log:

# bad: [f261c4e529dac5608a604d3dd3ae1cd2adf23c89] Merge branch 'akpm' (patches from Andrew)
# good: [1c163f4c7b3f621efff9b28a47abb36f7378d783] Linux 5.0
git bisect start 'HEAD' 'v5.0'
# good: [45763bf4bc1ebdf8eb95697607e1fd042a3e1221] Merge tag 'char-misc-5.1-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc
git bisect good 45763bf4bc1ebdf8eb95697607e1fd042a3e1221
# good: [cf2e8c544cd3b33e9e403b7b72404c221bf888d1] Merge tag 'mfd-next-5.1' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd
git bisect good cf2e8c544cd3b33e9e403b7b72404c221bf888d1
# bad: [d6075262969321bcb5d795de25595fc2a141ac02] Merge tag 'nios2-v5.1-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/lftan/nios2
git bisect bad d6075262969321bcb5d795de25595fc2a141ac02
# bad: [96a6de1a541c86e9e67b9c310c14db4099bd1cbc] Merge tag 'media/v5.1-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
git bisect bad 96a6de1a541c86e9e67b9c310c14db4099bd1cbc
# bad: [d1cae94871330cb9f5fdcea34529abf7917e682e] Merge tag 'fscrypt-for-linus' of git://git.kernel.org/pub/scm/fs/fscrypt/fscrypt
git bisect bad d1cae94871330cb9f5fdcea34529abf7917e682e
# bad: [80201fe175cbf7f3e372f53eba0a881a702ad926] Merge tag 'for-5.1/block-20190302' of git://git.kernel.dk/linux-block
git bisect bad 80201fe175cbf7f3e372f53eba0a881a702ad926
# bad: [4221b807d1f73c03d22543416d303b60a5d1ef31] Merge tag 'for-5.1/libata-20190301' of git://git.kernel.dk/linux-block
git bisect bad 4221b807d1f73c03d22543416d303b60a5d1ef31
# bad: [8fab3d713ca36bf4ad4dadec0bf38f5e70b8999d] Merge tag 'gpio-v5.1-updates-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux into devel
git bisect bad 8fab3d713ca36bf4ad4dadec0bf38f5e70b8999d
# good: [b868db94a6a704755a33a362cfcf4625abdda115] gpio: tqmx86: Add GPIO from for this IO controller
git bisect good b868db94a6a704755a33a362cfcf4625abdda115
# good: [5340f23df8fe27a270af3fa1a93cd07293d23dd9] gpio: sprd: Add missing break in switch statement
git bisect good 5340f23df8fe27a270af3fa1a93cd07293d23dd9
# bad: [7f2f787c10596f486644d730a0a23e78abe8cbe0] gpio: pcf857x: Simpify wake-up handling
git bisect bad 7f2f787c10596f486644d730a0a23e78abe8cbe0
# bad: [6581eaf0e890756e093e2f44916edb5e7e6558ca] gpio: use new gpio_set_config() helper in more places
git bisect bad 6581eaf0e890756e093e2f44916edb5e7e6558ca
# good: [71479789851bdd9d56cd9e82892b0a3bee0a4c2a] gpio: rename gpio_set_drive_single_ended() to gpio_set_config()
git bisect good 71479789851bdd9d56cd9e82892b0a3bee0a4c2a
# first bad commit: [6581eaf0e890756e093e2f44916edb5e7e6558ca] gpio: use new gpio_set_config() helper in more places

Sample traceback:

[   15.965307] ------------[ cut here ]------------
[   15.965655] WARNING: CPU: 0 PID: 1 at drivers/gpio/gpio-aspeed.c:834 unregister_allocated_timer+0x60/0x98
[   15.965884] No timer allocated to offset 125
[   15.966177] CPU: 0 PID: 1 Comm: swapper Not tainted 5.0.0-11515-g3b319ee220a8 #1
[   15.966360] Hardware name: Generic DT based system
[   15.966586] Backtrace: 
[   15.966815] [<80106f9c>] (dump_backtrace) from [<80107248>] (show_stack+0x20/0x24)
[   15.967112]  r7:00000009 r6:00000000 r5:80bed4a0 r4:878a1c74
[   15.967704] [<80107228>] (show_stack) from [<8093f830>] (dump_stack+0x20/0x28)
[   15.967925] [<8093f810>] (dump_stack) from [<801147c8>] (__warn+0xf0/0x118)
[   15.968133] [<801146d8>] (__warn) from [<80114844>] (warn_slowpath_fmt+0x54/0x74)
[   15.968370]  r9:00000001 r8:60000113 r7:879f5ed0 r6:879f5e20 r5:80bed4bc r4:80e0b028
[   15.968612] [<801147f4>] (warn_slowpath_fmt) from [<805a83ac>] (unregister_allocated_timer+0x60/0x98)
[   15.968854]  r3:0000007d r2:80bed4bc
[   15.968989]  r5:00000000 r4:0000007d
[   15.969141] [<805a834c>] (unregister_allocated_timer) from [<805a8538>] (aspeed_gpio_set_config+0x154/0x444)
[   15.969419] [<805a83e4>] (aspeed_gpio_set_config) from [<805a0e6c>] (gpiod_set_debounce+0x60/0xcc)
[   15.969696]  r10:8718e8a0 r9:00000001 r8:84346f4c r7:00000000 r6:879bc010 r5:871606a8
[   15.969893]  r4:87a987d0
[   15.970008] [<805a0e0c>] (gpiod_set_debounce) from [<807462c8>] (gpio_keys_probe+0x4d8/0x924)
[   15.970211]  r4:8718e8bc
[   15.970326] [<80745df0>] (gpio_keys_probe) from [<80633fec>] (platform_drv_probe+0x58/0xa8)

Re: [PATCH v2 3/5] gpio: use new gpio_set_config() helper in more places

[Thomas Petazzoni]

Hello,

On Fri, 15 Mar 2019 18:43:52 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index cf8a4402fef1..9762a836fec9 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2762,7 +2762,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
> >  	}
> >  
> >  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> > -	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
> > +	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);  
> 
> Are you sure this is correct ? This patch results in a number of tracebacks
> in mainline. Reverting it fixes the problem.
> 
> gpio_set_config() seems to pack config, but it is already packed above.
> That seems a bit suspicious.

I'll have a look. In the mean time, I'm fine with the patch being
reverted.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 3/5] gpio: use new gpio_set_config() helper in more places

[Guenter Roeck]

On Sat, Mar 16, 2019 at 02:45:19PM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Fri, 15 Mar 2019 18:43:52 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index cf8a4402fef1..9762a836fec9 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -2762,7 +2762,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
> > >  	}
> > >  
> > >  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> > > -	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
> > > +	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);  
> > 
> > Are you sure this is correct ? This patch results in a number of tracebacks
> > in mainline. Reverting it fixes the problem.
> > 
> > gpio_set_config() seems to pack config, but it is already packed above.
> > That seems a bit suspicious.
> 
> I'll have a look. In the mean time, I'm fine with the patch being
> reverted.
> 

I don't know the code well enough to make a recommendation one way or another
(meaning I am not entirely sure if the aspeed code generating the traceback code
is correct). However, the attached does fix the problem for me, suggesting that
gpio_set_config() might be missing an argument.

Guenter

---

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 144af0733581..e972836e8d8f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2563,9 +2563,9 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
  */
 
 static int gpio_set_config(struct gpio_chip *gc, unsigned offset,
-			   enum pin_config_param mode)
+			   enum pin_config_param mode, u32 argument)
 {
-	unsigned long config = { PIN_CONF_PACKED(mode, 0) };
+	unsigned long config = { PIN_CONF_PACKED(mode, argument) };
 
 	return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
 }
@@ -2619,10 +2619,10 @@ int gpiod_direction_input(struct gpio_desc *desc)
 
 	if (test_bit(FLAG_PULL_UP, &desc->flags))
 		gpio_set_config(chip, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_BIAS_PULL_UP);
+				PIN_CONFIG_BIAS_PULL_UP, 0);
 	else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
 		gpio_set_config(chip, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_BIAS_PULL_DOWN);
+				PIN_CONFIG_BIAS_PULL_DOWN, 0);
 
 	trace_gpio_direction(desc_to_gpio(desc), 1, status);
 
@@ -2727,7 +2727,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
 		/* First see if we can enable open drain in hardware */
 		ret = gpio_set_config(gc, gpio_chip_hwgpio(desc),
-				      PIN_CONFIG_DRIVE_OPEN_DRAIN);
+				      PIN_CONFIG_DRIVE_OPEN_DRAIN, 0);
 		if (!ret)
 			goto set_output_value;
 		/* Emulate open drain by not actively driving the line high */
@@ -2736,7 +2736,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	}
 	else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
 		ret = gpio_set_config(gc, gpio_chip_hwgpio(desc),
-				      PIN_CONFIG_DRIVE_OPEN_SOURCE);
+				      PIN_CONFIG_DRIVE_OPEN_SOURCE, 0);
 		if (!ret)
 			goto set_output_value;
 		/* Emulate open source by not actively driving the line low */
@@ -2744,7 +2744,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 			return gpiod_direction_input(desc);
 	} else {
 		gpio_set_config(gc, gpio_chip_hwgpio(desc),
-				PIN_CONFIG_DRIVE_PUSH_PULL);
+				PIN_CONFIG_DRIVE_PUSH_PULL, 0);
 	}
 
 set_output_value:
@@ -2764,19 +2764,11 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output);
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 {
 	struct gpio_chip	*chip;
-	unsigned long		config;
 
 	VALIDATE_DESC(desc);
 	chip = desc->gdev->chip;
-	if (!chip->set || !chip->set_config) {
-		gpiod_dbg(desc,
-			  "%s: missing set() or set_config() operations\n",
-			  __func__);
-		return -ENOTSUPP;
-	}
-
-	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);
+	return gpio_set_config(chip, gpio_chip_hwgpio(desc),
+			       PIN_CONFIG_INPUT_DEBOUNCE, debounce);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
@@ -2791,7 +2783,6 @@ EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 {
 	struct gpio_chip *chip;
-	unsigned long packed;
 	int gpio;
 	int rc;
 
@@ -2807,13 +2798,8 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 
 	/* If the driver supports it, set the persistence state now */
 	chip = desc->gdev->chip;
-	if (!chip->set_config)
-		return 0;
-
-	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
-					  !transitory);
 	gpio = gpio_chip_hwgpio(desc);
-	rc = gpio_set_config(chip, gpio, packed);
+	rc = gpio_set_config(chip, gpio, PIN_CONFIG_PERSIST_STATE, !transitory);
 	if (rc == -ENOTSUPP) {
 		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
 				gpio);

Re: [PATCH v2 3/5] gpio: use new gpio_set_config() helper in more places

[Guenter Roeck]

On Sat, Mar 16, 2019 at 02:45:19PM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Fri, 15 Mar 2019 18:43:52 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index cf8a4402fef1..9762a836fec9 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -2762,7 +2762,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
> > >  	}
> > >  
> > >  	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
> > > -	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
> > > +	return gpio_set_config(chip, gpio_chip_hwgpio(desc), config);  
> > 
> > Are you sure this is correct ? This patch results in a number of tracebacks
> > in mainline. Reverting it fixes the problem.
> > 
> > gpio_set_config() seems to pack config, but it is already packed above.
> > That seems a bit suspicious.
> 
> I'll have a look. In the mean time, I'm fine with the patch being
> reverted.
> 

The problem is still seen in the latest kernel as of last night, and
I did not see any further activities. Should I send a revert request ?

Guenter

Re: [PATCH v2 3/5] gpio: use new gpio_set_config() helper in more places

[Thomas Petazzoni]

Hello Guenter,

On Tue, 26 Mar 2019 13:03:02 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > I'll have a look. In the mean time, I'm fine with the patch being
> > reverted.
> 
> The problem is still seen in the latest kernel as of last night, and
> I did not see any further activities. Should I send a revert request ?

Sorry for not replying. I have not had the chance to investigate this
further due to lack of time. Linus Walleij initially suggested to use
gpio_set_config() in more places, but that doesn't to work as expected.

A revert patch has already been sent:

  https://lore.kernel.org/lkml/20190326044954.18671-1-andrew@aj.id.au/

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com