| Message ID | 20180616125636.4210-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [RFC] i2c: gpio: initialize SCL to HIGH again | expand |
Hi Wolfram, On Sat, Jun 16, 2018 at 2:57 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > It seems that during the conversion from gpio* to gpiod*, the initial > state of SCL was wrongly switched to LOW. Fix it to be HIGH again. > > Fixes: 7bb75029ef34 ("i2c: gpio: Enforce open drain through gpiolib") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Seems to have no ill-effect on the boot log of armadillo800eva, so Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> It's not clear to me if this depends on "[PATCH 0/2] i2c: algos: bit: revert to fix regression (#200045)" or not? So far I haven't tried that one. I'm still on v4.17+, but probably I have all i2c bugs/features due to renesas-drivers ;-) Gr{oetje,eeting}s, Geert
On Sat, Jun 16, 2018 at 09:56:36PM +0900, Wolfram Sang wrote: > It seems that during the conversion from gpio* to gpiod*, the initial > state of SCL was wrongly switched to LOW. Fix it to be HIGH again. > > Fixes: 7bb75029ef34 ("i2c: gpio: Enforce open drain through gpiolib") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Linus: Or was it intentional? But then I am missing something... I found this > by trying to fix a regression. I can't scope my use case now because I am on > the road. I recall SCL being unexpectedly LOW when using the GPIO fault > injector the last time, though. This made me do a fix in the underlying > i2c-algo-bit algorithm, but that sadly caused a regression :( So, this may be > the proper fix. RFC for now, until I have my scope back. But discussion is > welcome. Linus: any comment? > > drivers/i2c/busses/i2c-gpio.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c > index 005e6e0330c2..66f85bbf3591 100644 > --- a/drivers/i2c/busses/i2c-gpio.c > +++ b/drivers/i2c/busses/i2c-gpio.c > @@ -279,9 +279,9 @@ static int i2c_gpio_probe(struct platform_device *pdev) > * required for an I2C bus. > */ > if (pdata->scl_is_open_drain) > - gflags = GPIOD_OUT_LOW; > + gflags = GPIOD_OUT_HIGH; > else > - gflags = GPIOD_OUT_LOW_OPEN_DRAIN; > + gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; > priv->scl = i2c_gpio_get_desc(dev, "scl", 1, gflags); > if (IS_ERR(priv->scl)) > return PTR_ERR(priv->scl); > -- > 2.11.0 >
On Mon, Jun 25, 2018 at 4:30 PM Wolfram Sang <wsa@the-dreams.de> wrote: > On Sat, Jun 16, 2018 at 09:56:36PM +0900, Wolfram Sang wrote: > > It seems that during the conversion from gpio* to gpiod*, the initial > > state of SCL was wrongly switched to LOW. Fix it to be HIGH again. > > > > Fixes: 7bb75029ef34 ("i2c: gpio: Enforce open drain through gpiolib") > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > > > Linus: Or was it intentional? But then I am missing something... I found this > > by trying to fix a regression. I can't scope my use case now because I am on > > the road. I recall SCL being unexpectedly LOW when using the GPIO fault > > injector the last time, though. This made me do a fix in the underlying > > i2c-algo-bit algorithm, but that sadly caused a regression :( So, this may be > > the proper fix. RFC for now, until I have my scope back. But discussion is > > welcome. > > Linus: any comment? Looks like a mistake on my part. Mea culpa :( Luckily it is functionally mostly harmless, but to conserve power, it is better to initialize this as high (lest we dissipate power throught the pull-up resistor), so please queue this as a fix and also tag for stable. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
> Looks like a mistake on my part. Mea culpa :( No worries... > Luckily it is functionally mostly harmless, but to > conserve power, it is better to initialize this as high (lest > we dissipate power throught the pull-up resistor), so please queue > this as a fix and also tag for stable. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> ... and thanks for the heads up!
On Sat, Jun 16, 2018 at 09:56:36PM +0900, Wolfram Sang wrote: > It seems that during the conversion from gpio* to gpiod*, the initial > state of SCL was wrongly switched to LOW. Fix it to be HIGH again. > > Fixes: 7bb75029ef34 ("i2c: gpio: Enforce open drain through gpiolib") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c index 005e6e0330c2..66f85bbf3591 100644 --- a/drivers/i2c/busses/i2c-gpio.c +++ b/drivers/i2c/busses/i2c-gpio.c @@ -279,9 +279,9 @@ static int i2c_gpio_probe(struct platform_device *pdev) * required for an I2C bus. */ if (pdata->scl_is_open_drain) - gflags = GPIOD_OUT_LOW; + gflags = GPIOD_OUT_HIGH; else - gflags = GPIOD_OUT_LOW_OPEN_DRAIN; + gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; priv->scl = i2c_gpio_get_desc(dev, "scl", 1, gflags); if (IS_ERR(priv->scl)) return PTR_ERR(priv->scl);
It seems that during the conversion from gpio* to gpiod*, the initial state of SCL was wrongly switched to LOW. Fix it to be HIGH again. Fixes: 7bb75029ef34 ("i2c: gpio: Enforce open drain through gpiolib") Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Linus: Or was it intentional? But then I am missing something... I found this by trying to fix a regression. I can't scope my use case now because I am on the road. I recall SCL being unexpectedly LOW when using the GPIO fault injector the last time, though. This made me do a fix in the underlying i2c-algo-bit algorithm, but that sadly caused a regression :( So, this may be the proper fix. RFC for now, until I have my scope back. But discussion is welcome. drivers/i2c/busses/i2c-gpio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)