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 |
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
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
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
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 --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;