[PATCH 0/4] Proposal to support pull-up/pull-down GPIO configuration

[PATCH 0/4] Proposal to support pull-up/pull-down GPIO 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 set of patches implements what I understood of Linus option (2),
i.e extend the ->set_config() callback, and provide additional flags
that can be used in the Device Tree: GPIO_PULL_UP and GPIO_PULL_DOWN.

Let me know what you think about this proposal.

Thanks for your feedback.

Thomas

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

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

 .../devicetree/bindings/gpio/gpio.txt         |  4 ++
 drivers/gpio/gpio-pca953x.c                   | 59 ++++++++++++++++++-
 drivers/gpio/gpiolib-of.c                     |  5 ++
 drivers/gpio/gpiolib.c                        | 40 ++++++++-----
 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, 104 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCH 1/4] 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index f0ba154b5723..03d8adf67c16 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -67,6 +67,10 @@ 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
+- Bit 5: 0 means no pull-down resistor should be enabled
+         1 means a pull-down resistor should be enabled
 
 1.1) GPIO specifier best practices
 ----------------------------------
-- 
2.20.1

[PATCH 2/4] 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 a2cbb474901c..b07e4f6ee641 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2530,6 +2530,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
@@ -2583,14 +2591,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;
@@ -2687,8 +2687,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 */
@@ -2696,16 +2696,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 3/4] 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          | 12 ++++++++++++
 drivers/gpio/gpiolib.h          |  2 ++
 include/dt-bindings/gpio/gpio.h |  6 ++++++
 include/linux/gpio/machine.h    |  2 ++
 include/linux/of_gpio.h         |  2 ++
 6 files changed, 29 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 7f1260c78270..6518dc8c7c4c 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -323,6 +323,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;
+	else 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 b07e4f6ee641..20887c62fbb3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2585,6 +2585,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;
@@ -4054,6 +4061,11 @@ 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)
+		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 087d865286a0..0b0cf213c45a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -225,6 +225,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/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
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 4/4] 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 | 59 +++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 82d69ab51976..f14cfb42681f 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,54 @@ 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;
+
+	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 +540,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 3/4] gpio: add core support for pull-up/pull-down configuration

[Jan Kundrát]

> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 7f1260c78270..6518dc8c7c4c 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -323,6 +323,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;
> +	else if (of_flags & OF_GPIO_PULL_DOWN)
> +		*flags |= GPIO_PULL_DOWN;
> +
>  	return desc;
>  }
>  

Hi Thomas,
my recommendation is to add an explicit "error handling" code here to warn 
when the DT specifies both pull-up and pull-down bits. It's outside of any 
hot path, and it will help identify mistakes instead of silently prefering 
a random bit choice.

> +/* Bit 4 express pull up */
> +#define GPIO_PULL_UP 16
> +
> +/* Bit 5 express pull down */
> +#define GPIO_PULL_DOWN 32
> +

> +	GPIO_PULL_UP = (1 << 4),
> +	GPIO_PULL_DOWN = (1 << 5),

> +	OF_GPIO_PULL_UP = 0x10,
> +	OF_GPIO_PULL_DOWN = 0x20,

I understand that it's already there, but I wonder if this duplication can 
be removed. Am I missing something, perhaps a reason why 
include/linux/of_gpio.h and include/dt-bindings/gpio/gpio.h are separate 
files?

With kind regards,
Jan

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

[Thomas Petazzoni]

Hello Jan,

Thanks for your feedback.

On Tue, 08 Jan 2019 15:00:22 +0100, Jan Kundrát wrote:
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 7f1260c78270..6518dc8c7c4c 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -323,6 +323,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;
> > +	else if (of_flags & OF_GPIO_PULL_DOWN)
> > +		*flags |= GPIO_PULL_DOWN;
> > +
> >  	return desc;
> >  }
> >    
> 
> Hi Thomas,
> my recommendation is to add an explicit "error handling" code here to warn 
> when the DT specifies both pull-up and pull-down bits. It's outside of any 
> hot path, and it will help identify mistakes instead of silently prefering 
> a random bit choice.

Sure.

> > +/* Bit 4 express pull up */
> > +#define GPIO_PULL_UP 16
> > +
> > +/* Bit 5 express pull down */
> > +#define GPIO_PULL_DOWN 32
> > +  
> 
> > +	GPIO_PULL_UP = (1 << 4),
> > +	GPIO_PULL_DOWN = (1 << 5),  
> 
> > +	OF_GPIO_PULL_UP = 0x10,
> > +	OF_GPIO_PULL_DOWN = 0x20,  
> 
> I understand that it's already there, but I wonder if this duplication can 
> be removed. Am I missing something, perhaps a reason why 
> include/linux/of_gpio.h and include/dt-bindings/gpio/gpio.h are separate 
> files?

I also wondered why there was such duplication when writing the
patches. I've assumed it was done so that
include/dt-bindings/gpio/gpio.h is "clean" enough to be included in DT,
while include/linux/of_gpio.h some more elaborate definitions. But
indeed <linux/of_gpio.h> could include <dt-bindings/gpio/gpio.h>.
Perhaps there's a good reason for this duplication? Let's see the
feedback of GPIO maintainers about this.

Best regards,

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

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

[Linus Walleij]

On Thu, Jan 3, 2019 at 5:41 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> 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 | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index f0ba154b5723..03d8adf67c16 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -67,6 +67,10 @@ 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
> +- Bit 5: 0 means no pull-down resistor should be enabled
> +         1 means a pull-down resistor should be enabled

As per the commit message this should be more verbose IMO,
add something like "this setting will only apply to hardware
with a simple on/off approach to pull up/down: if the hardware
has more elaborate pull up/down settings such as specifying a
specific resistance, the driver should use pin control bindings.

Yours,
Linus Walleij

Re: [PATCH 2/4] gpio: rename gpio_set_drive_single_ended() to gpio_set_config()

[Linus Walleij]

On Thu, Jan 3, 2019 at 5:41 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> 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>

This is fine, but if you're factoring this config setting out then also
change the lines in gpiod_set_debounce() and gpiod_set_transitory()
to call the same helper.

Yours,
Linus Walleij

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

[Linus Walleij]

On Thu, Jan 3, 2019 at 5:41 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> 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>

I'm overall happy with this approach.

> +/* Bit 4 express pull up */
> +#define GPIO_PULL_UP 16
> +
> +/* Bit 5 express pull down */
> +#define GPIO_PULL_DOWN 32

Please use
#define GPIO_PULL_UP BIT(5)
#define GPIO_PULL_DOWN BIT(6)

> @@ -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),

You can keep this to follow the pattern set by the others.

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

Same here: this is fine.

Yours,
Linus Walleij

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

[Linus Walleij]

On Tue, Jan 8, 2019 at 3:04 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> > > +/* Bit 4 express pull up */
> > > +#define GPIO_PULL_UP 16
> > > +
> > > +/* Bit 5 express pull down */
> > > +#define GPIO_PULL_DOWN 32
> > > +
> >
> > > +   GPIO_PULL_UP = (1 << 4),
> > > +   GPIO_PULL_DOWN = (1 << 5),
> >
> > > +   OF_GPIO_PULL_UP = 0x10,
> > > +   OF_GPIO_PULL_DOWN = 0x20,
> >
> > I understand that it's already there, but I wonder if this duplication can
> > be removed. Am I missing something, perhaps a reason why
> > include/linux/of_gpio.h and include/dt-bindings/gpio/gpio.h are separate
> > files?
>
> I also wondered why there was such duplication when writing the
> patches. I've assumed it was done so that
> include/dt-bindings/gpio/gpio.h is "clean" enough to be included in DT,
> while include/linux/of_gpio.h some more elaborate definitions. But
> indeed <linux/of_gpio.h> could include <dt-bindings/gpio/gpio.h>.
> Perhaps there's a good reason for this duplication? Let's see the
> feedback of GPIO maintainers about this.

There is a good reason for this.

The reason is that DT ABI is written in clay, userspace ABI is
written in stone and kernel internal API is written in water.

For the last one see Documentation/process/stable-api-nonsense.rst

For the DT the clay tablet hardens when devices are mass produced
and shipped with a DT in flash.

For the userspace ABI, that will never change until the chardevice
disappears.

So these ABIs are essentially versioned according to completely
different rules and for this reason they are kept separate.

They overlap considerably, but for this reason we are keeping the
definitions separately and also explicitly translating between them
so that we can still change the internal kernel representation of
these flags if we need.

Yours,
Linus Walleij

Re: [PATCH 4/4] gpio: pca953x: add ->set_config implementation

[Linus Walleij]

On Thu, Jan 3, 2019 at 5:41 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> 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>

This looks OK to me, but IIUC not all versions of the PCA953x supports
this?

So we need to make sure that you return -ENOTSUPP if the device
does not support setting pull up/down.

Also there are configs for debounce too, right? I suggested to add
that first but I have no strong opinion on it.

Yours,
Linus Walleij

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

[Thomas Petazzoni]

Hello Linus,

On Fri, 11 Jan 2019 11:05:03 +0100, Linus Walleij wrote:

> > 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>  
> 
> I'm overall happy with this approach.

Thanks for this review and feedback, very useful.

> 
> > +/* Bit 4 express pull up */
> > +#define GPIO_PULL_UP 16
> > +
> > +/* Bit 5 express pull down */
> > +#define GPIO_PULL_DOWN 32  
> 
> Please use
> #define GPIO_PULL_UP BIT(5)
> #define GPIO_PULL_DOWN BIT(6)

Here, I did exactly like below: keep the existing approach used in the
file. Today it has:

/* Bit 0 express polarity */
#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

/* Bit 1 express single-endedness */
#define GPIO_PUSH_PULL 0
#define GPIO_SINGLE_ENDED 2

/* Bit 2 express Open drain or open source */
#define GPIO_LINE_OPEN_SOURCE 0
#define GPIO_LINE_OPEN_DRAIN 4

[...]

/* Bit 3 express GPIO suspend/resume and reset persistence */
#define GPIO_PERSISTENT 0
#define GPIO_TRANSITORY 8

So I kept the same logic and used 16 and 32 to define the
GPIO_PULL_UP/GPIO_PULL_DOWN values instead of BIT(x). BIT(x) would have
been more logical.

However, we are in the dt-bindings includes here, and apparently, there
is no definition for the BIT() macro in those headers.

Best regards,

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

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

[Linus Walleij]

On Fri, Jan 11, 2019 at 1:58 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
> On Fri, 11 Jan 2019 11:05:03 +0100, Linus Walleij wrote:

> > > +/* Bit 4 express pull up */
> > > +#define GPIO_PULL_UP 16
> > > +
> > > +/* Bit 5 express pull down */
> > > +#define GPIO_PULL_DOWN 32
> >
> > Please use
> > #define GPIO_PULL_UP BIT(5)
> > #define GPIO_PULL_DOWN BIT(6)
(...)
> However, we are in the dt-bindings includes here, and apparently, there
> is no definition for the BIT() macro in those headers.

Ah you nailed it :D

OK keep it like this.

Yours,
Linus Walleij

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

[Rob Herring]

On Thu, Jan 03, 2019 at 05:41:01PM +0100, Thomas Petazzoni wrote:
> 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          | 12 ++++++++++++
>  drivers/gpio/gpiolib.h          |  2 ++
>  include/dt-bindings/gpio/gpio.h |  6 ++++++

It's fine, but really this one should be part of the binding patch.

>  include/linux/gpio/machine.h    |  2 ++
>  include/linux/of_gpio.h         |  2 ++
>  6 files changed, 29 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 4/4] gpio: pca953x: add ->set_config implementation

[Thomas Petazzoni]

Hello Linus,

On Fri, 11 Jan 2019 11:14:49 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> > 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>  
> 
> This looks OK to me, but IIUC not all versions of the PCA953x supports
> this?
> 
> So we need to make sure that you return -ENOTSUPP if the device
> does not support setting pull up/down.

Indeed, I've added a check for this.

> Also there are configs for debounce too, right? I suggested to add
> that first but I have no strong opinion on it.

The particular PCA variant that I have does not have any debounce
related capability it seems, so I won't be able to implement this
aspect.

Best regards,

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