| Message ID | d5f294489d31a80b69169f358da89bb7f70d1328.1621279162.git.sander@svanheule.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/7] regmap: Add MDIO bus support | expand |
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
+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 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.
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
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,
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(-)