[PATCH] driver: gpio: fix gpio glitch for output-high gpio-hog

[PATCH] driver: gpio: fix gpio glitch for output-high gpio-hog

[Tim Harvey]

A gpio-hog can be specified as output-low, output-high, or input where
output-low means 'de-asserted' and 'output-high' means asserted vs
voltage levels.

When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags()
which ends up setting the GPIO as an output with a driven 'de-asserted'
value prior to setting the desired value the hog was configured for.
While I'm not sure it makes sense to set the output level while simply
'requesting' a GPIO the result of this is that if the hog is configured
for output-high the request call sets it first as output low before
gpio_hog_probe() sets it to the configured value causing the gpio to
'glitch' which may be undesired for certain applications.

Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as
output-high.

This was tested with the following hogs:

        /* active-high output-low (de-asserted) GPIO should drive 0 */
        gpio1 {
                gpio-hog;
                output-low;
                gpios = <1 GPIO_ACTIVE_HIGH>;
                line-name = "gpio1";
        };

        /* active-high output-high (asserted) GPIO should drive 1 */
	/* before patch this would first drive 0 then 1 */
        gpio2 {
                gpio-hog;
                output-high;
                gpios = <2 GPIO_ACTIVE_HIGH>;
                line-name = "gpio2";
        };

        /* active-low output-low (de-asserted) GPIO should drive 1 */
        gpio3 {
                gpio-hog;
                output-low;
                gpios = <3 GPIO_ACTIVE_LOW>;
                line-name = "gpio3#";
        };

        /* active-low output-high (asserted) GPIO should drive 0 */
	/* before patch this would first drive 0 then 1 */
        gpio4 {
                gpio-hog;
                output-high;
                gpios = <4 GPIO_ACTIVE_LOW>;
                line-name = "gpio4#";
        };

Cc: Sean Anderson <sean.anderson@seco.com>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/gpio/gpio-uclass.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 125ae53d612f..baf9d451c19d 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -294,6 +294,8 @@ static int gpio_hog_probe(struct udevice *dev)
 	struct gpio_hog_priv *priv = dev_get_priv(dev);
 	int ret;
 
+	if (plat->gpiod_flags == GPIOD_IS_OUT && plat->value)
+		plat->gpiod_flags |= GPIOD_IS_OUT_ACTIVE;
 	ret = gpio_dev_request_index(dev->parent, dev->name, "gpio-hog",
 				     plat->val[0], plat->gpiod_flags,
 				     plat->val[1], &priv->gpiod);
-- 
2.17.1

Re: [PATCH] driver: gpio: fix gpio glitch for output-high gpio-hog

[Tim Harvey]

On Wed, Mar 2, 2022 at 6:01 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> A gpio-hog can be specified as output-low, output-high, or input where
> output-low means 'de-asserted' and 'output-high' means asserted vs
> voltage levels.
>
> When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags()
> which ends up setting the GPIO as an output with a driven 'de-asserted'
> value prior to setting the desired value the hog was configured for.
> While I'm not sure it makes sense to set the output level while simply
> 'requesting' a GPIO the result of this is that if the hog is configured
> for output-high the request call sets it first as output low before
> gpio_hog_probe() sets it to the configured value causing the gpio to
> 'glitch' which may be undesired for certain applications.
>
> Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as
> output-high.
>
> This was tested with the following hogs:
>
>         /* active-high output-low (de-asserted) GPIO should drive 0 */
>         gpio1 {
>                 gpio-hog;
>                 output-low;
>                 gpios = <1 GPIO_ACTIVE_HIGH>;
>                 line-name = "gpio1";
>         };
>
>         /* active-high output-high (asserted) GPIO should drive 1 */
>         /* before patch this would first drive 0 then 1 */
>         gpio2 {
>                 gpio-hog;
>                 output-high;
>                 gpios = <2 GPIO_ACTIVE_HIGH>;
>                 line-name = "gpio2";
>         };
>
>         /* active-low output-low (de-asserted) GPIO should drive 1 */
>         gpio3 {
>                 gpio-hog;
>                 output-low;
>                 gpios = <3 GPIO_ACTIVE_LOW>;
>                 line-name = "gpio3#";
>         };
>
>         /* active-low output-high (asserted) GPIO should drive 0 */
>         /* before patch this would first drive 0 then 1 */
>         gpio4 {
>                 gpio-hog;
>                 output-high;
>                 gpios = <4 GPIO_ACTIVE_LOW>;
>                 line-name = "gpio4#";
>         };
>
> Cc: Sean Anderson <sean.anderson@seco.com>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/gpio/gpio-uclass.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index 125ae53d612f..baf9d451c19d 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -294,6 +294,8 @@ static int gpio_hog_probe(struct udevice *dev)
>         struct gpio_hog_priv *priv = dev_get_priv(dev);
>         int ret;
>
> +       if (plat->gpiod_flags == GPIOD_IS_OUT && plat->value)
> +               plat->gpiod_flags |= GPIOD_IS_OUT_ACTIVE;
>         ret = gpio_dev_request_index(dev->parent, dev->name, "gpio-hog",
>                                      plat->val[0], plat->gpiod_flags,
>                                      plat->val[1], &priv->gpiod);
> --
> 2.17.1
>

Widening the net here to those who were involved in the gpio-hog support:

Cc: Heiko Schocher <hs@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>

Best Regards,

Tim

Re: [PATCH] driver: gpio: fix gpio glitch for output-high gpio-hog

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

On Wed, Mar 02, 2022 at 06:01:08PM -0800, Tim Harvey wrote:

> A gpio-hog can be specified as output-low, output-high, or input where
> output-low means 'de-asserted' and 'output-high' means asserted vs
> voltage levels.
> 
> When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags()
> which ends up setting the GPIO as an output with a driven 'de-asserted'
> value prior to setting the desired value the hog was configured for.
> While I'm not sure it makes sense to set the output level while simply
> 'requesting' a GPIO the result of this is that if the hog is configured
> for output-high the request call sets it first as output low before
> gpio_hog_probe() sets it to the configured value causing the gpio to
> 'glitch' which may be undesired for certain applications.
> 
> Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as
> output-high.
> 
> This was tested with the following hogs:
> 
>         /* active-high output-low (de-asserted) GPIO should drive 0 */
>         gpio1 {
>                 gpio-hog;
>                 output-low;
>                 gpios = <1 GPIO_ACTIVE_HIGH>;
>                 line-name = "gpio1";
>         };
> 
>         /* active-high output-high (asserted) GPIO should drive 1 */
> 	/* before patch this would first drive 0 then 1 */
>         gpio2 {
>                 gpio-hog;
>                 output-high;
>                 gpios = <2 GPIO_ACTIVE_HIGH>;
>                 line-name = "gpio2";
>         };
> 
>         /* active-low output-low (de-asserted) GPIO should drive 1 */
>         gpio3 {
>                 gpio-hog;
>                 output-low;
>                 gpios = <3 GPIO_ACTIVE_LOW>;
>                 line-name = "gpio3#";
>         };
> 
>         /* active-low output-high (asserted) GPIO should drive 0 */
> 	/* before patch this would first drive 0 then 1 */
>         gpio4 {
>                 gpio-hog;
>                 output-high;
>                 gpios = <4 GPIO_ACTIVE_LOW>;
>                 line-name = "gpio4#";
>         };
> 
> Cc: Sean Anderson <sean.anderson@seco.com>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

This breaks the ut_dm_dm_test_gpio test which I guess needs to be
updated.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] driver: gpio: fix gpio glitch for output-high gpio-hog

[Tim Harvey]

On Wed, Apr 6, 2022 at 5:27 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 02, 2022 at 06:01:08PM -0800, Tim Harvey wrote:
>
> > A gpio-hog can be specified as output-low, output-high, or input where
> > output-low means 'de-asserted' and 'output-high' means asserted vs
> > voltage levels.
> >
> > When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags()
> > which ends up setting the GPIO as an output with a driven 'de-asserted'
> > value prior to setting the desired value the hog was configured for.
> > While I'm not sure it makes sense to set the output level while simply
> > 'requesting' a GPIO the result of this is that if the hog is configured
> > for output-high the request call sets it first as output low before
> > gpio_hog_probe() sets it to the configured value causing the gpio to
> > 'glitch' which may be undesired for certain applications.
> >
> > Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as
> > output-high.
> >
> > This was tested with the following hogs:
> >
> >         /* active-high output-low (de-asserted) GPIO should drive 0 */
> >         gpio1 {
> >                 gpio-hog;
> >                 output-low;
> >                 gpios = <1 GPIO_ACTIVE_HIGH>;
> >                 line-name = "gpio1";
> >         };
> >
> >         /* active-high output-high (asserted) GPIO should drive 1 */
> >       /* before patch this would first drive 0 then 1 */
> >         gpio2 {
> >                 gpio-hog;
> >                 output-high;
> >                 gpios = <2 GPIO_ACTIVE_HIGH>;
> >                 line-name = "gpio2";
> >         };
> >
> >         /* active-low output-low (de-asserted) GPIO should drive 1 */
> >         gpio3 {
> >                 gpio-hog;
> >                 output-low;
> >                 gpios = <3 GPIO_ACTIVE_LOW>;
> >                 line-name = "gpio3#";
> >         };
> >
> >         /* active-low output-high (asserted) GPIO should drive 0 */
> >       /* before patch this would first drive 0 then 1 */
> >         gpio4 {
> >                 gpio-hog;
> >                 output-high;
> >                 gpios = <4 GPIO_ACTIVE_LOW>;
> >                 line-name = "gpio4#";
> >         };
> >
> > Cc: Sean Anderson <sean.anderson@seco.com>
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>
> This breaks the ut_dm_dm_test_gpio test which I guess needs to be
> updated.
>

Tom,

That's kind of funny I suppose because the behavior is clearly wrong.

Is fixing the test something that needs to be done for the patch to be
applied, or needs to be done by me? I'm not familiar with the test
framework or how to execute it.

Tim

Re: [PATCH] driver: gpio: fix gpio glitch for output-high gpio-hog

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

On Wed, Apr 06, 2022 at 08:30:31AM -0700, Tim Harvey wrote:
> On Wed, Apr 6, 2022 at 5:27 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 02, 2022 at 06:01:08PM -0800, Tim Harvey wrote:
> >
> > > A gpio-hog can be specified as output-low, output-high, or input where
> > > output-low means 'de-asserted' and 'output-high' means asserted vs
> > > voltage levels.
> > >
> > > When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags()
> > > which ends up setting the GPIO as an output with a driven 'de-asserted'
> > > value prior to setting the desired value the hog was configured for.
> > > While I'm not sure it makes sense to set the output level while simply
> > > 'requesting' a GPIO the result of this is that if the hog is configured
> > > for output-high the request call sets it first as output low before
> > > gpio_hog_probe() sets it to the configured value causing the gpio to
> > > 'glitch' which may be undesired for certain applications.
> > >
> > > Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as
> > > output-high.
> > >
> > > This was tested with the following hogs:
> > >
> > >         /* active-high output-low (de-asserted) GPIO should drive 0 */
> > >         gpio1 {
> > >                 gpio-hog;
> > >                 output-low;
> > >                 gpios = <1 GPIO_ACTIVE_HIGH>;
> > >                 line-name = "gpio1";
> > >         };
> > >
> > >         /* active-high output-high (asserted) GPIO should drive 1 */
> > >       /* before patch this would first drive 0 then 1 */
> > >         gpio2 {
> > >                 gpio-hog;
> > >                 output-high;
> > >                 gpios = <2 GPIO_ACTIVE_HIGH>;
> > >                 line-name = "gpio2";
> > >         };
> > >
> > >         /* active-low output-low (de-asserted) GPIO should drive 1 */
> > >         gpio3 {
> > >                 gpio-hog;
> > >                 output-low;
> > >                 gpios = <3 GPIO_ACTIVE_LOW>;
> > >                 line-name = "gpio3#";
> > >         };
> > >
> > >         /* active-low output-high (asserted) GPIO should drive 0 */
> > >       /* before patch this would first drive 0 then 1 */
> > >         gpio4 {
> > >                 gpio-hog;
> > >                 output-high;
> > >                 gpios = <4 GPIO_ACTIVE_LOW>;
> > >                 line-name = "gpio4#";
> > >         };
> > >
> > > Cc: Sean Anderson <sean.anderson@seco.com>
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >
> > This breaks the ut_dm_dm_test_gpio test which I guess needs to be
> > updated.
> >
> 
> Tom,
> 
> That's kind of funny I suppose because the behavior is clearly wrong.
> 
> Is fixing the test something that needs to be done for the patch to be
> applied, or needs to be done by me? I'm not familiar with the test
> framework or how to execute it.

For running the tests, please see:
https://u-boot.readthedocs.io/en/latest/develop/testing.html
and you're going to need to take a look at test/dm/gpio.c as where to
fix what.  And yes, a v2 that addresses the test as well is what I need.
Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]