[v2,2/7] gpio: regmap: Add configurable dir/value order

Message ID d5f294489d31a80b69169f358da89bb7f70d1328.1621279162.git.sander@svanheule.net
State New
Headers show
Series
  • [v2,1/7] regmap: Add MDIO bus support
Related show

Commit Message

Sander Vanheule May 17, 2021, 7:28 p.m.
GPIO chips may not support setting the output value when a pin is
configured as an input, although the current implementation assumes this
is always possible.

Add support for setting pin direction before value. The order defaults
to setting the value first, but this can be reversed by setting the
regmap_config.no_set_on_input flag, similar to the corresponding flag in
the gpio-mmio driver.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/gpio/gpio-regmap.c  | 20 +++++++++++++++++---
 include/linux/gpio/regmap.h |  3 +++
 2 files changed, 20 insertions(+), 3 deletions(-)

Comments

Michael Walle May 18, 2021, 8:39 a.m. | #1
Hi,

Am 2021-05-17 21:28, schrieb Sander Vanheule:
> GPIO chips may not support setting the output value when a pin is
> configured as an input, although the current implementation assumes 
> this
> is always possible.
> 
> Add support for setting pin direction before value. The order defaults
> to setting the value first, but this can be reversed by setting the
> regmap_config.no_set_on_input flag, similar to the corresponding flag 
> in
> the gpio-mmio driver.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  drivers/gpio/gpio-regmap.c  | 20 +++++++++++++++++---
>  include/linux/gpio/regmap.h |  3 +++
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 134cedf151a7..1cdb20f8f8b4 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -170,14 +170,25 @@ static int gpio_regmap_direction_input(struct
> gpio_chip *chip,
>  	return gpio_regmap_set_direction(chip, offset, false);
>  }
> 
> -static int gpio_regmap_direction_output(struct gpio_chip *chip,
> -					unsigned int offset, int value)
> +static int gpio_regmap_dir_out_val_first(struct gpio_chip *chip,
> +					 unsigned int offset, int value)

Can we leave the name as is? TBH I find these two similar names
super confusing. Maybe its just me, though.

>  {
>  	gpio_regmap_set(chip, offset, value);
> 
>  	return gpio_regmap_set_direction(chip, offset, true);
>  }
> 
> +static int gpio_regmap_dir_out_dir_first(struct gpio_chip *chip,
> +					 unsigned int offset, int value)
> +{
> +	int err;

use ret for consistency here

> +
> +	err = gpio_regmap_set_direction(chip, offset, true);
> +	gpio_regmap_set(chip, offset, value);
> +
> +	return err;
> +}
> +

Instead of adding a new one, we can also just check no_set_on_input
in gpio_regmap_direction_output(), which I'd prefer.

static int gpio_regmap_direction_output(struct gpio_chip *chip,
					unsigned int offset, int value)
{
	struct gpio_regmap *gpio = gpiochip_get_data(chip);
	int ret;

	if (gpio->no_set_on_input) {
		/* some smart comment here, also mention gliches */
		ret = gpio_regmap_set_direction(chip, offset, true);
		gpio_regmap_set(chip, offset, value);
	} else {
		gpio_regmap_set(chip, offset, value);
		ret = gpio_regmap_set_direction(chip, offset, true);
	}

	return ret;
}

>  void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data)
>  {
>  	gpio->driver_data = data;
> @@ -277,7 +288,10 @@ struct gpio_regmap *gpio_regmap_register(const
> struct gpio_regmap_config *config
>  	if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
>  		chip->get_direction = gio_regmap_get_direction;
>  		chip->direction_input = gpio_regmap_direction_input;
> -		chip->direction_output = gpio_regmap_direction_output;
> +		if (config->no_set_on_input)
> +			chip->direction_output = gpio_regmap_dir_out_dir_first;
> +		else
> +			chip->direction_output = gpio_regmap_dir_out_val_first;
>  	}
> 
>  	ret = gpiochip_add_data(chip, gpio);
> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> index 334dd928042b..2a732f8f23be 100644
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
> @@ -30,6 +30,8 @@ struct regmap;
>   * @reg_dir_out_base:	(Optional) out setting register base address
>   * @reg_stride:		(Optional) May be set if the registers (of the
>   *			same type, dat, set, etc) are not consecutive.
> + * @no_set_on_input:	Set if output value can only be set when the 
> direction
> + *			is configured as output.

set_direction_first ?

>   * @ngpio_per_reg:	Number of GPIOs per register
>   * @irq_domain:		(Optional) IRQ domain if the controller is
>   *			interrupt-capable
> @@ -73,6 +75,7 @@ struct gpio_regmap_config {
>  	unsigned int reg_dir_out_base;
>  	int reg_stride;
>  	int ngpio_per_reg;
> +	bool no_set_on_input;
>  	struct irq_domain *irq_domain;
> 
>  	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,

-michael
Andy Shevchenko May 18, 2021, 10:39 a.m. | #2
+Matti

On Tue, May 18, 2021 at 11:39 AM Michael Walle <michael@walle.cc> wrote:
> Am 2021-05-17 21:28, schrieb Sander Vanheule:

...

> Instead of adding a new one, we can also just check no_set_on_input
> in gpio_regmap_direction_output(), which I'd prefer.

+! here.

> static int gpio_regmap_direction_output(struct gpio_chip *chip,
>                                         unsigned int offset, int value)
> {
>         struct gpio_regmap *gpio = gpiochip_get_data(chip);
>         int ret;
>
>         if (gpio->no_set_on_input) {
>                 /* some smart comment here, also mention gliches */
>                 ret = gpio_regmap_set_direction(chip, offset, true);
>                 gpio_regmap_set(chip, offset, value);
>         } else {
>                 gpio_regmap_set(chip, offset, value);
>                 ret = gpio_regmap_set_direction(chip, offset, true);
>         }
>
>         return ret;
> }

...

> > + * @no_set_on_input: Set if output value can only be set when the
> > direction
> > + *                   is configured as output.
>
> set_direction_first ?

Perhaps we need to establish rather something like

/* Broken hardware can't set value on input pin, we have to set it to
output first */
#define GPIO_REGMAP_QUIRK_...  BIT(0)

unsigned int quirks;

?
Sander Vanheule May 18, 2021, 11:39 a.m. | #3
Hi Andrew,

On Tue, 2021-05-18 at 03:40 +0200, Andrew Lunn wrote:
> On Mon, May 17, 2021 at 09:28:04PM +0200, Sander Vanheule wrote:
> > GPIO chips may not support setting the output value when a pin is
> > configured as an input
> 
> Could you describe what happens with the hardware you are playing
> with. Not being able to do this means you will get glitches when
> enabling the output so you should not use these GPIOs with bit banging
> busses like i2c.

As I was testing this driver, I noticed that output settings for GPIO LEDs,
connected to the RTL8231, weren't being properly set. The actual LED brightness
didn't correspond to the one reported by sysfs. Changing the operation order
fixed this.

However, the vendor code uses I2C bitbanging quite extensively on these chips,
so I decided to have another look.
Sander Vanheule May 23, 2021, 9:19 p.m. | #4
Hi Michael,

On Tue, 2021-05-18 at 10:39 +0200, Michael Walle wrote:
> Hi,

> 

> Am 2021-05-17 21:28, schrieb Sander Vanheule:

> > GPIO chips may not support setting the output value when a pin is

> > configured as an input, although the current implementation assumes 

> > this

> > is always possible.

> > 

> > Add support for setting pin direction before value. The order defaults

> > to setting the value first, but this can be reversed by setting the

> > regmap_config.no_set_on_input flag, similar to the corresponding flag 

> > in

> > the gpio-mmio driver.

> > 

> > Signed-off-by: Sander Vanheule <sander@svanheule.net>

> > ---

> >  drivers/gpio/gpio-regmap.c  | 20 +++++++++++++++++---

> >  include/linux/gpio/regmap.h |  3 +++

> >  2 files changed, 20 insertions(+), 3 deletions(-)

> > 

> > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c

> > index 134cedf151a7..1cdb20f8f8b4 100644

> > --- a/drivers/gpio/gpio-regmap.c

> > +++ b/drivers/gpio/gpio-regmap.c

> > @@ -170,14 +170,25 @@ static int gpio_regmap_direction_input(struct

> > gpio_chip *chip,

> >         return gpio_regmap_set_direction(chip, offset, false);

> >  }

> > 

> > -static int gpio_regmap_direction_output(struct gpio_chip *chip,

> > -                                       unsigned int offset, int value)

> > +static int gpio_regmap_dir_out_val_first(struct gpio_chip *chip,

> > +                                        unsigned int offset, int value)

> 

> Can we leave the name as is? TBH I find these two similar names

> super confusing. Maybe its just me, though.


Sure. This is the implementation used in gpio-mmio.c to provide the same
functionality, so I had used that for consistenty between the two drivers.

> >  {

> >         gpio_regmap_set(chip, offset, value);

> > 

> >         return gpio_regmap_set_direction(chip, offset, true);

> >  }

> > 

> > +static int gpio_regmap_dir_out_dir_first(struct gpio_chip *chip,

> > +                                        unsigned int offset, int value)

> > +{

> > +       int err;

> 

> use ret for consistency here

> 

> > +

> > +       err = gpio_regmap_set_direction(chip, offset, true);

> > +       gpio_regmap_set(chip, offset, value);

> > +

> > +       return err;

> > +}

> > +

> 

> Instead of adding a new one, we can also just check no_set_on_input

> in gpio_regmap_direction_output(), which I'd prefer.

> 

> static int gpio_regmap_direction_output(struct gpio_chip *chip,

>                                         unsigned int offset, int value)

> {

>         struct gpio_regmap *gpio = gpiochip_get_data(chip);

>         int ret;

> 

>         if (gpio->no_set_on_input) {

>                 /* some smart comment here, also mention gliches */

>                 ret = gpio_regmap_set_direction(chip, offset, true);

>                 gpio_regmap_set(chip, offset, value);

>         } else {

>                 gpio_regmap_set(chip, offset, value);

>                 ret = gpio_regmap_set_direction(chip, offset, true);

>         }

> 

>         return ret;

> }

> 


This would certainly make the code a bit easier to follow when you're not
familiar with it :-)
I also see the other functions do checks on static values too, so I'll bring
this function in line with that style.


> >  void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data)

> >  {

> >         gpio->driver_data = data;

> > @@ -277,7 +288,10 @@ struct gpio_regmap *gpio_regmap_register(const

> > struct gpio_regmap_config *config

> >         if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {

> >                 chip->get_direction = gio_regmap_get_direction;

> >                 chip->direction_input = gpio_regmap_direction_input;

> > -               chip->direction_output = gpio_regmap_direction_output;

> > +               if (config->no_set_on_input)

> > +                       chip->direction_output =

> > gpio_regmap_dir_out_dir_first;

> > +               else

> > +                       chip->direction_output =

> > gpio_regmap_dir_out_val_first;

> >         }

> > 

> >         ret = gpiochip_add_data(chip, gpio);

> > diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h

> > index 334dd928042b..2a732f8f23be 100644

> > --- a/include/linux/gpio/regmap.h

> > +++ b/include/linux/gpio/regmap.h

> > @@ -30,6 +30,8 @@ struct regmap;

> >   * @reg_dir_out_base:  (Optional) out setting register base address

> >   * @reg_stride:                (Optional) May be set if the registers (of

> > the

> >   *                     same type, dat, set, etc) are not consecutive.

> > + * @no_set_on_input:   Set if output value can only be set when the 

> > direction

> > + *                     is configured as output.

> 

> set_direction_first ?


This negation can indeed be a bit confusing, I'll change this. As Andy
suggested, I just went for a 'quirks' field, with currently only one defined
flag.

Best,
Sander
Sander Vanheule May 23, 2021, 10:21 p.m. | #5
Hi Adrew,

On Tue, 2021-05-18 at 03:40 +0200, Andrew Lunn wrote:
> On Mon, May 17, 2021 at 09:28:04PM +0200, Sander Vanheule wrote:
> > GPIO chips may not support setting the output value when a pin is
> > configured as an input
> 
> Could you describe what happens with the hardware you are playing
> with. Not being able to do this means you will get glitches when
> enabling the output so you should not use these GPIOs with bit banging
> busses like i2c.

As I reported earlier, using value-before-direction breaks the GPIO driven LEDs
on one of my devices.

I've tried to use another device to test if I could reproduce this. Using the
gpioset and gpioget utilities, I can't seem to reproduce this however. Whether I
enable the (new) quirk or not, doesn't seem to make any difference.

The documentation we have on this chip is quite scarce, so I'm unaware of
different chip revisions, or how I would distinguish between revisions. As far
as I can see, Realtek's code (present in the GPL archives we got from some
vendors) set the pin direction before setting the value.

For now, I've made the implementation so that the quirk is always applied. Like
the behaviour that is implicit in the origal code. If prefered, I could also
supply a separate devicetree compatible or extra devictree flag.

Regarding bit banged I2C, glitches may not actually be an issue. If a pull-up
resistor is used for HIGH values, and an open drain for LOW values, the GPIO pin
doesn't actually have to change value, only direction (IN for HIGH, OUT for
LOW). A configuration like this would perhaps glitch once on the initial OUT-LOW
configuration. Like I mentioned, bit banged I2C is frequently used in ethernet
switches with these chips to talk to SFP modules.


Best,
Sander

Patch

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 134cedf151a7..1cdb20f8f8b4 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -170,14 +170,25 @@  static int gpio_regmap_direction_input(struct gpio_chip *chip,
 	return gpio_regmap_set_direction(chip, offset, false);
 }
 
-static int gpio_regmap_direction_output(struct gpio_chip *chip,
-					unsigned int offset, int value)
+static int gpio_regmap_dir_out_val_first(struct gpio_chip *chip,
+					 unsigned int offset, int value)
 {
 	gpio_regmap_set(chip, offset, value);
 
 	return gpio_regmap_set_direction(chip, offset, true);
 }
 
+static int gpio_regmap_dir_out_dir_first(struct gpio_chip *chip,
+					 unsigned int offset, int value)
+{
+	int err;
+
+	err = gpio_regmap_set_direction(chip, offset, true);
+	gpio_regmap_set(chip, offset, value);
+
+	return err;
+}
+
 void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data)
 {
 	gpio->driver_data = data;
@@ -277,7 +288,10 @@  struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
 		chip->get_direction = gpio_regmap_get_direction;
 		chip->direction_input = gpio_regmap_direction_input;
-		chip->direction_output = gpio_regmap_direction_output;
+		if (config->no_set_on_input)
+			chip->direction_output = gpio_regmap_dir_out_dir_first;
+		else
+			chip->direction_output = gpio_regmap_dir_out_val_first;
 	}
 
 	ret = gpiochip_add_data(chip, gpio);
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index 334dd928042b..2a732f8f23be 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -30,6 +30,8 @@  struct regmap;
  * @reg_dir_out_base:	(Optional) out setting register base address
  * @reg_stride:		(Optional) May be set if the registers (of the
  *			same type, dat, set, etc) are not consecutive.
+ * @no_set_on_input:	Set if output value can only be set when the direction
+ *			is configured as output.
  * @ngpio_per_reg:	Number of GPIOs per register
  * @irq_domain:		(Optional) IRQ domain if the controller is
  *			interrupt-capable
@@ -73,6 +75,7 @@  struct gpio_regmap_config {
 	unsigned int reg_dir_out_base;
 	int reg_stride;
 	int ngpio_per_reg;
+	bool no_set_on_input;
 	struct irq_domain *irq_domain;
 
 	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,