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

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

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.

       Andrew

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

?

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

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