* [PATCH] gpio: sifive: use the correct register to read output values
@ 2022-02-04 13:02 Niklas Cassel
2022-02-04 23:33 ` Damien Le Moal
2022-02-05 16:39 ` Linus Walleij
0 siblings, 2 replies; 6+ messages in thread
From: Niklas Cassel @ 2022-02-04 13:02 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Palmer Dabbelt, Paul Walmsley
Cc: damien.lemoal@opensource.wdc.com, Niklas Cassel,
linux-gpio@vger.kernel.org, linux-riscv@lists.infradead.org
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(-)
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;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: sifive: use the correct register to read output values
2022-02-04 13:02 [PATCH] gpio: sifive: use the correct register to read output values Niklas Cassel
@ 2022-02-04 23:33 ` Damien Le Moal
2022-02-08 9:44 ` Bartosz Golaszewski
2022-02-05 16:39 ` Linus Walleij
1 sibling, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2022-02-04 23:33 UTC (permalink / raw)
To: Niklas Cassel, Linus Walleij, Bartosz Golaszewski, Palmer Dabbelt,
Paul Walmsley
Cc: linux-gpio@vger.kernel.org, linux-riscv@lists.infradead.org
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.
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: sifive: use the correct register to read output values
2022-02-04 13:02 [PATCH] gpio: sifive: use the correct register to read output values Niklas Cassel
2022-02-04 23:33 ` Damien Le Moal
@ 2022-02-05 16:39 ` Linus Walleij
2022-02-08 10:13 ` Niklas Cassel
1 sibling, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2022-02-05 16:39 UTC (permalink / raw)
To: Niklas Cassel
Cc: Bartosz Golaszewski, Palmer Dabbelt, Paul Walmsley,
damien.lemoal@opensource.wdc.com, linux-gpio@vger.kernel.org,
linux-riscv@lists.infradead.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: sifive: use the correct register to read output values
2022-02-04 23:33 ` Damien Le Moal
@ 2022-02-08 9:44 ` Bartosz Golaszewski
2022-02-08 10:00 ` Niklas Cassel
0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2022-02-08 9:44 UTC (permalink / raw)
To: Damien Le Moal
Cc: Niklas Cassel, Linus Walleij, Palmer Dabbelt, Paul Walmsley,
linux-gpio@vger.kernel.org, linux-riscv@lists.infradead.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: sifive: use the correct register to read output values
2022-02-08 9:44 ` Bartosz Golaszewski
@ 2022-02-08 10:00 ` Niklas Cassel
0 siblings, 0 replies; 6+ messages in thread
From: Niklas Cassel @ 2022-02-08 10:00 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Damien Le Moal, Linus Walleij, Palmer Dabbelt, Paul Walmsley,
linux-gpio@vger.kernel.org, linux-riscv@lists.infradead.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio: sifive: use the correct register to read output values
2022-02-05 16:39 ` Linus Walleij
@ 2022-02-08 10:13 ` Niklas Cassel
0 siblings, 0 replies; 6+ messages in thread
From: Niklas Cassel @ 2022-02-08 10:13 UTC (permalink / raw)
To: Linus Walleij
Cc: Bartosz Golaszewski, Palmer Dabbelt, Paul Walmsley,
damien.lemoal@opensource.wdc.com, linux-gpio@vger.kernel.org,
linux-riscv@lists.infradead.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-08 11:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-04 13:02 [PATCH] gpio: sifive: use the correct register to read output values Niklas Cassel
2022-02-04 23:33 ` Damien Le Moal
2022-02-08 9:44 ` Bartosz Golaszewski
2022-02-08 10:00 ` Niklas Cassel
2022-02-05 16:39 ` Linus Walleij
2022-02-08 10:13 ` Niklas Cassel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox