| Message ID | d5f294489d31a80b69169f358da89bb7f70d1328.1621279162.git.sander@svanheule.net |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > 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. But it's broken hardware. Can it be rather marked as a quirk? > 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.
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
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; ? -- With Best Regards, Andy Shevchenko
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. From u-boot on my device, I can manipulate the RTL8231 registeres relatively easily. I performed the following short tests: * Set pin to input, pull pin high, write output low, change direction: pin output changes to low value * Set pin to input pull pin low, write output high, change direction: pin output changes to high value Which seems to indicate that I _can_ set output values on input pins... I'll need to look into this in more detail when I have a bit more time, later this week. 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(-)