From: Mark Featherston <[email protected]>
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 <[email protected]>
Signed-off-by: Kris Bahnsen <[email protected]>
---
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
/*
* 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);
--
2.11.0
On Tue, Mar 8, 2022 at 7:19 PM Kris Bahnsen <[email protected]> wrote:
>
> From: Mark Featherston <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Kris Bahnsen <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> wrote:
>
> From: Mark Featherston <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Kris Bahnsen <[email protected]>
...
> + * 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.
--
With Best Regards,
Andy Shevchenko