| Message ID | 20220308181847.3276-1-kris@embeddedTS.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] gpio: ts4900: Do not set DAT and OE together | expand |
On Tue, Mar 8, 2022 at 7:19 PM Kris Bahnsen <kris@embeddedts.com> wrote: > > From: Mark Featherston <mark@embeddedTS.com> > > This works around an issue with the hardware where both OE and > DAT are exposed in the same register. If both are updated > simultaneously, the harware makes no guarantees that OE or DAT > will actually change in any given order and may result in a > glitch of a few ns on a GPIO pin when changing direction and value > in a single write. > > Setting direction to input now only affects OE bit. Setting > direction to output updates DAT first, then OE. > > Fixes: 9c6686322d74 ("gpio: add Technologic I2C-FPGA gpio support") > > Signed-off-by: Mark Featherston <mark@embeddedTS.com> > Signed-off-by: Kris Bahnsen <kris@embeddedTS.com> > --- > V1 -> V2: Add Fixes tag > > drivers/gpio/gpio-ts4900.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpio/gpio-ts4900.c b/drivers/gpio/gpio-ts4900.c > index d885032cf814..fbabfca030c0 100644 > --- a/drivers/gpio/gpio-ts4900.c > +++ b/drivers/gpio/gpio-ts4900.c > @@ -1,7 +1,8 @@ > +// SPDX-License-Identifier: GPL-2.0 This is not part of the fix, please send a separate patch that comes after the fix adding the SPDX identifier. > /* > * Digital I/O driver for Technologic Systems I2C FPGA Core > * > - * Copyright (C) 2015 Technologic Systems > + * Copyright (C) 2015-2018 Technologic Systems > * Copyright (C) 2016 Savoir-Faire Linux > * > * This program is free software; you can redistribute it and/or If you're adding the SPDX identifier, you can drop the license boilerplate here. Bart > @@ -55,19 +56,33 @@ static int ts4900_gpio_direction_input(struct gpio_chip *chip, > { > struct ts4900_gpio_priv *priv = gpiochip_get_data(chip); > > - /* > - * This will clear the output enable bit, the other bits are > - * dontcare when this is cleared > + /* Only clear the OE bit here, requires a RMW. Prevents potential issue > + * with OE and data getting to the physical pin at different times. > */ > - return regmap_write(priv->regmap, offset, 0); > + return regmap_update_bits(priv->regmap, offset, TS4900_GPIO_OE, 0); > } > > static int ts4900_gpio_direction_output(struct gpio_chip *chip, > unsigned int offset, int value) > { > struct ts4900_gpio_priv *priv = gpiochip_get_data(chip); > + unsigned int reg; > int ret; > > + /* If changing from an input to an output, we need to first set the > + * proper data bit to what is requested and then set OE bit. This > + * prevents a glitch that can occur on the IO line > + */ > + regmap_read(priv->regmap, offset, ®); > + if (!(reg & TS4900_GPIO_OE)) { > + if (value) > + reg = TS4900_GPIO_OUT; > + else > + reg &= ~TS4900_GPIO_OUT; > + > + regmap_write(priv->regmap, offset, reg); > + } > + > if (value) > ret = regmap_write(priv->regmap, offset, TS4900_GPIO_OE | > TS4900_GPIO_OUT); > -- > 2.11.0 >
On Wed, 2022-03-09 at 09:32 +0100, Bartosz Golaszewski wrote: > On Tue, Mar 8, 2022 at 7:19 PM Kris Bahnsen <kris@embeddedts.com> wrote: > > > > diff --git a/drivers/gpio/gpio-ts4900.c b/drivers/gpio/gpio-ts4900.c > > index d885032cf814..fbabfca030c0 100644 > > --- a/drivers/gpio/gpio-ts4900.c > > +++ b/drivers/gpio/gpio-ts4900.c > > @@ -1,7 +1,8 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > This is not part of the fix, please send a separate patch that comes > after the fix adding the SPDX identifier. > > > /* > > * Digital I/O driver for Technologic Systems I2C FPGA Core > > * > > - * Copyright (C) 2015 Technologic Systems > > + * Copyright (C) 2015-2018 Technologic Systems > > * Copyright (C) 2016 Savoir-Faire Linux > > * > > * This program is free software; you can redistribute it and/or > > If you're adding the SPDX identifier, you can drop the license boilerplate here. > > Bart > Thanks for the feedback. I'll get a v3 series out soon to address these. Kris
Bart, side note: I can't see your for-current in Linux Next for a few days, is everything okay? On Wed, Mar 9, 2022 at 1:47 AM Kris Bahnsen <kris@embeddedts.com> wrote: > > From: Mark Featherston <mark@embeddedTS.com> > > This works around an issue with the hardware where both OE and > DAT are exposed in the same register. If both are updated > simultaneously, the harware makes no guarantees that OE or DAT hardware the OE > will actually change in any given order and may result in a > glitch of a few ns on a GPIO pin when changing direction and value > in a single write. > > Setting direction to input now only affects OE bit. Setting the OE bit > direction to output updates DAT first, then OE. > > Fixes: 9c6686322d74 ("gpio: add Technologic I2C-FPGA gpio support") > There must be no blank lines in the tag block. > Signed-off-by: Mark Featherston <mark@embeddedTS.com> > Signed-off-by: Kris Bahnsen <kris@embeddedTS.com> ... > + * Copyright (C) 2015-2018 Technologic Systems Not sure it's a valid change for a simple fix. ... > - /* > - * This will clear the output enable bit, the other bits are > - * dontcare when this is cleared > + /* Only clear the OE bit here, requires a RMW. Prevents potential issue > + * with OE and data getting to the physical pin at different times. > */ Keep the proper style for multi-line comments. ... > + /* If changing from an input to an output, we need to first set the > + * proper data bit to what is requested and then set OE bit. This the OE bit > + * prevents a glitch that can occur on the IO line > + */ Keep the proper style.
diff --git a/drivers/gpio/gpio-ts4900.c b/drivers/gpio/gpio-ts4900.c index d885032cf814..fbabfca030c0 100644 --- a/drivers/gpio/gpio-ts4900.c +++ b/drivers/gpio/gpio-ts4900.c @@ -1,7 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0 /* * Digital I/O driver for Technologic Systems I2C FPGA Core * - * Copyright (C) 2015 Technologic Systems + * Copyright (C) 2015-2018 Technologic Systems * Copyright (C) 2016 Savoir-Faire Linux * * This program is free software; you can redistribute it and/or @@ -55,19 +56,33 @@ static int ts4900_gpio_direction_input(struct gpio_chip *chip, { struct ts4900_gpio_priv *priv = gpiochip_get_data(chip); - /* - * This will clear the output enable bit, the other bits are - * dontcare when this is cleared + /* Only clear the OE bit here, requires a RMW. Prevents potential issue + * with OE and data getting to the physical pin at different times. */ - return regmap_write(priv->regmap, offset, 0); + return regmap_update_bits(priv->regmap, offset, TS4900_GPIO_OE, 0); } static int ts4900_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value) { struct ts4900_gpio_priv *priv = gpiochip_get_data(chip); + unsigned int reg; int ret; + /* If changing from an input to an output, we need to first set the + * proper data bit to what is requested and then set OE bit. This + * prevents a glitch that can occur on the IO line + */ + regmap_read(priv->regmap, offset, ®); + if (!(reg & TS4900_GPIO_OE)) { + if (value) + reg = TS4900_GPIO_OUT; + else + reg &= ~TS4900_GPIO_OUT; + + regmap_write(priv->regmap, offset, reg); + } + if (value) ret = regmap_write(priv->regmap, offset, TS4900_GPIO_OE | TS4900_GPIO_OUT);