diff mbox series

gpio: sifive: use the correct register to read output values

Message ID 20220204130142.21727-1-Niklas.Cassel@wdc.com
State Accepted
Commit cc38ef936840ac29204d806deb4d1836ec509594
Headers show
Series gpio: sifive: use the correct register to read output values | expand

Commit Message

Niklas Cassel Feb. 4, 2022, 1:02 p.m. UTC 
From: Niklas Cassel <niklas.cassel@wdc.com>

Setting the output of a GPIO to 1 using gpiod_set_value(), followed by
reading the same GPIO using gpiod_get_value(), will currently yield an
incorrect result.

This is because the SiFive GPIO device stores the output values in reg_set,
not reg_dat.

Supply the flag BGPIOF_READ_OUTPUT_REG_SET to bgpio_init() so that the
generic driver reads the correct register.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
The patch was tested on a canaan,k210 board (canaan,k210-gpiohs compatible
string). It would be nice with a Tested-by from someone with a SiFive board.

However, the u-boot driver for this device already behaves exactly the same
as this driver does after my patch, for all platforms using the driver.

 drivers/gpio/gpio-sifive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Linus Walleij Feb. 5, 2022, 4:39 p.m. UTC | #1 
On Fri, Feb 4, 2022 at 2:02 PM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:

> From: Niklas Cassel <niklas.cassel@wdc.com>

Hi Niklas, long time no C!

> Setting the output of a GPIO to 1 using gpiod_set_value(), followed by
> reading the same GPIO using gpiod_get_value(), will currently yield an
> incorrect result.
>
> This is because the SiFive GPIO device stores the output values in reg_set,
> not reg_dat.
>
> Supply the flag BGPIOF_READ_OUTPUT_REG_SET to bgpio_init() so that the
> generic driver reads the correct register.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

Looks like a straight-forward fix so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Bartosz Golaszewski Feb. 8, 2022, 9:44 a.m. UTC | #2 
On Sat, Feb 5, 2022 at 12:33 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 2/4/22 22:02, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > Setting the output of a GPIO to 1 using gpiod_set_value(), followed by
> > reading the same GPIO using gpiod_get_value(), will currently yield an
> > incorrect result.
> >
> > This is because the SiFive GPIO device stores the output values in reg_set,
> > not reg_dat.
> >
> > Supply the flag BGPIOF_READ_OUTPUT_REG_SET to bgpio_init() so that the
> > generic driver reads the correct register.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>
> This probably needs a Fixes tag. I have not checked which patch though.
>

I added the tag and queued this for fixes, thanks!

Bart

> > ---
> > The patch was tested on a canaan,k210 board (canaan,k210-gpiohs compatible
> > string). It would be nice with a Tested-by from someone with a SiFive board.
> >
> > However, the u-boot driver for this device already behaves exactly the same
> > as this driver does after my patch, for all platforms using the driver.
> >
> >  drivers/gpio/gpio-sifive.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
> > index 403f9e833d6a..7d82388b4ab7 100644
> > --- a/drivers/gpio/gpio-sifive.c
> > +++ b/drivers/gpio/gpio-sifive.c
> > @@ -223,7 +223,7 @@ static int sifive_gpio_probe(struct platform_device *pdev)
> >                        NULL,
> >                        chip->base + SIFIVE_GPIO_OUTPUT_EN,
> >                        chip->base + SIFIVE_GPIO_INPUT_EN,
> > -                      0);
> > +                      BGPIOF_READ_OUTPUT_REG_SET);
> >       if (ret) {
> >               dev_err(dev, "unable to init generic GPIO\n");
> >               return ret;
>
>
> --
> Damien Le Moal
> Western Digital Research
Niklas Cassel Feb. 8, 2022, 10 a.m. UTC | #3 
On Tue, Feb 08, 2022 at 10:44:41AM +0100, Bartosz Golaszewski wrote:
> On Sat, Feb 5, 2022 at 12:33 AM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
> >
> > On 2/4/22 22:02, Niklas Cassel wrote:
> > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > >
> > > Setting the output of a GPIO to 1 using gpiod_set_value(), followed by
> > > reading the same GPIO using gpiod_get_value(), will currently yield an
> > > incorrect result.
> > >
> > > This is because the SiFive GPIO device stores the output values in reg_set,
> > > not reg_dat.
> > >
> > > Supply the flag BGPIOF_READ_OUTPUT_REG_SET to bgpio_init() so that the
> > > generic driver reads the correct register.
> > >
> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > This probably needs a Fixes tag. I have not checked which patch though.
> >
> 
> I added the tag and queued this for fixes, thanks!

Hello Bart,

Thank you for queueing and amending the Fixes tag!


Kind regards,
Niklas


> 
> Bart
> 
> > > ---
> > > The patch was tested on a canaan,k210 board (canaan,k210-gpiohs compatible
> > > string). It would be nice with a Tested-by from someone with a SiFive board.
> > >
> > > However, the u-boot driver for this device already behaves exactly the same
> > > as this driver does after my patch, for all platforms using the driver.
> > >
> > >  drivers/gpio/gpio-sifive.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
> > > index 403f9e833d6a..7d82388b4ab7 100644
> > > --- a/drivers/gpio/gpio-sifive.c
> > > +++ b/drivers/gpio/gpio-sifive.c
> > > @@ -223,7 +223,7 @@ static int sifive_gpio_probe(struct platform_device *pdev)
> > >                        NULL,
> > >                        chip->base + SIFIVE_GPIO_OUTPUT_EN,
> > >                        chip->base + SIFIVE_GPIO_INPUT_EN,
> > > -                      0);
> > > +                      BGPIOF_READ_OUTPUT_REG_SET);
> > >       if (ret) {
> > >               dev_err(dev, "unable to init generic GPIO\n");
> > >               return ret;
> >
> >
> > --
> > Damien Le Moal
> > Western Digital Research
Niklas Cassel Feb. 8, 2022, 10:13 a.m. UTC | #4 
On Sat, Feb 05, 2022 at 05:39:35PM +0100, Linus Walleij wrote:
> On Fri, Feb 4, 2022 at 2:02 PM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:
> 
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Hi Niklas, long time no C!

I C what you did there ;)

I've been working mostly on spdk and fio, unfortunately,
both projects are written in C :)

Learning Rust is still on the todo list...


> 
> > Setting the output of a GPIO to 1 using gpiod_set_value(), followed by
> > reading the same GPIO using gpiod_get_value(), will currently yield an
> > incorrect result.
> >
> > This is because the SiFive GPIO device stores the output values in reg_set,
> > not reg_dat.
> >
> > Supply the flag BGPIOF_READ_OUTPUT_REG_SET to bgpio_init() so that the
> > generic driver reads the correct register.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Looks like a straight-forward fix so:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for the review!

Indeed, I can't understand how no one has caught this bug before.

I guess most drivers simply use gpiod_set_value() to control a pin,
e.g. chip select, but rarely care about the existing value of the pin,
so gpiod_get_value() being broken does not really matter to most drivers.


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
index 403f9e833d6a..7d82388b4ab7 100644
--- a/drivers/gpio/gpio-sifive.c
+++ b/drivers/gpio/gpio-sifive.c
@@ -223,7 +223,7 @@  static int sifive_gpio_probe(struct platform_device *pdev)
 			 NULL,
 			 chip->base + SIFIVE_GPIO_OUTPUT_EN,
 			 chip->base + SIFIVE_GPIO_INPUT_EN,
-			 0);
+			 BGPIOF_READ_OUTPUT_REG_SET);
 	if (ret) {
 		dev_err(dev, "unable to init generic GPIO\n");
 		return ret;