* [PATCH] gpiolib: fix up emulated open drain outputs @ 2019-12-07 16:20 Russell King 2019-12-08 3:43 ` Kent Gibson 0 siblings, 1 reply; 7+ messages in thread From: Russell King @ 2019-12-07 16:20 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski; +Cc: linux-gpio gpiolib has a corner case with open drain outputs that are emulated. When such outputs are outputting a logic 1, emulation will set the hardware to input mode, which will cause gpiod_get_direction() to report that it is in input mode. This is different from the behaviour with a true open-drain output. Unify the semantics here. Suggested-by: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/gpio/gpiolib.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 32c2048deb8c..70c60aac41cc 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -220,6 +220,14 @@ int gpiod_get_direction(struct gpio_desc *desc) chip = gpiod_to_chip(desc); offset = gpio_chip_hwgpio(desc); + /* + * Open drain emulation using input mode may incorrectly report + * input here, fix that up. + */ + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags) && + test_bit(FLAG_IS_OUT, &desc->flags)) + return 0; + if (!chip->get_direction) return -ENOTSUPP; -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] gpiolib: fix up emulated open drain outputs 2019-12-07 16:20 [PATCH] gpiolib: fix up emulated open drain outputs Russell King @ 2019-12-08 3:43 ` Kent Gibson 2019-12-08 7:18 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 7+ messages in thread From: Kent Gibson @ 2019-12-08 3:43 UTC (permalink / raw) To: Russell King; +Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio On Sat, Dec 07, 2019 at 04:20:18PM +0000, Russell King wrote: > gpiolib has a corner case with open drain outputs that are emulated. > When such outputs are outputting a logic 1, emulation will set the > hardware to input mode, which will cause gpiod_get_direction() to > report that it is in input mode. This is different from the behaviour > with a true open-drain output. > > Unify the semantics here. > > Suggested-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/gpio/gpiolib.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 32c2048deb8c..70c60aac41cc 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -220,6 +220,14 @@ int gpiod_get_direction(struct gpio_desc *desc) > chip = gpiod_to_chip(desc); > offset = gpio_chip_hwgpio(desc); > > + /* > + * Open drain emulation using input mode may incorrectly report > + * input here, fix that up. > + */ > + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags) && > + test_bit(FLAG_IS_OUT, &desc->flags)) > + return 0; > + What about the FLAG_OPEN_SOURCE case? Kent. > if (!chip->get_direction) > return -ENOTSUPP; > > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpiolib: fix up emulated open drain outputs 2019-12-08 3:43 ` Kent Gibson @ 2019-12-08 7:18 ` Russell King - ARM Linux admin 2019-12-08 7:51 ` Kent Gibson 0 siblings, 1 reply; 7+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-08 7:18 UTC (permalink / raw) To: Kent Gibson; +Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio On Sun, Dec 08, 2019 at 11:43:40AM +0800, Kent Gibson wrote: > On Sat, Dec 07, 2019 at 04:20:18PM +0000, Russell King wrote: > > gpiolib has a corner case with open drain outputs that are emulated. > > When such outputs are outputting a logic 1, emulation will set the > > hardware to input mode, which will cause gpiod_get_direction() to > > report that it is in input mode. This is different from the behaviour > > with a true open-drain output. > > > > Unify the semantics here. > > > > Suggested-by: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > drivers/gpio/gpiolib.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 32c2048deb8c..70c60aac41cc 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -220,6 +220,14 @@ int gpiod_get_direction(struct gpio_desc *desc) > > chip = gpiod_to_chip(desc); > > offset = gpio_chip_hwgpio(desc); > > > > + /* > > + * Open drain emulation using input mode may incorrectly report > > + * input here, fix that up. > > + */ > > + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags) && > > + test_bit(FLAG_IS_OUT, &desc->flags)) > > + return 0; > > + > > What about the FLAG_OPEN_SOURCE case? do you have a scenario you can test? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpiolib: fix up emulated open drain outputs 2019-12-08 7:18 ` Russell King - ARM Linux admin @ 2019-12-08 7:51 ` Kent Gibson 2019-12-08 13:02 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 7+ messages in thread From: Kent Gibson @ 2019-12-08 7:51 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio On Sun, Dec 08, 2019 at 07:18:23AM +0000, Russell King - ARM Linux admin wrote: > On Sun, Dec 08, 2019 at 11:43:40AM +0800, Kent Gibson wrote: > > On Sat, Dec 07, 2019 at 04:20:18PM +0000, Russell King wrote: > > > gpiolib has a corner case with open drain outputs that are emulated. > > > When such outputs are outputting a logic 1, emulation will set the > > > hardware to input mode, which will cause gpiod_get_direction() to > > > report that it is in input mode. This is different from the behaviour > > > with a true open-drain output. > > > > > > Unify the semantics here. > > > > > > Suggested-by: Linus Walleij <linus.walleij@linaro.org> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > --- > > > drivers/gpio/gpiolib.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > index 32c2048deb8c..70c60aac41cc 100644 > > > --- a/drivers/gpio/gpiolib.c > > > +++ b/drivers/gpio/gpiolib.c > > > @@ -220,6 +220,14 @@ int gpiod_get_direction(struct gpio_desc *desc) > > > chip = gpiod_to_chip(desc); > > > offset = gpio_chip_hwgpio(desc); > > > > > > + /* > > > + * Open drain emulation using input mode may incorrectly report > > > + * input here, fix that up. > > > + */ > > > + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags) && > > > + test_bit(FLAG_IS_OUT, &desc->flags)) > > > + return 0; > > > + > > > > What about the FLAG_OPEN_SOURCE case? > > do you have a scenario you can test? > No I don't - if I had a scenario that had tripped over this problem then I would've submitted a patch already ;-). I'm simply pointing out that the logic that applies to emulating OPEN_DRAIN also applies to emulating OPEN_SOURCE. IMHO if you are fixing this for one then it should be fixed for both. I'd love to see some test cases for the gpiolib driver-api, including this function. But I'm relatively new here, and am generally coming from the userspace side, so will have to admit ignorance to anything related to testing APIs within the kernel. Kent. > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpiolib: fix up emulated open drain outputs 2019-12-08 7:51 ` Kent Gibson @ 2019-12-08 13:02 ` Russell King - ARM Linux admin 2019-12-08 13:34 ` Kent Gibson 0 siblings, 1 reply; 7+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-08 13:02 UTC (permalink / raw) To: Kent Gibson; +Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio On Sun, Dec 08, 2019 at 03:51:43PM +0800, Kent Gibson wrote: > On Sun, Dec 08, 2019 at 07:18:23AM +0000, Russell King - ARM Linux admin wrote: > > On Sun, Dec 08, 2019 at 11:43:40AM +0800, Kent Gibson wrote: > > > On Sat, Dec 07, 2019 at 04:20:18PM +0000, Russell King wrote: > > > > gpiolib has a corner case with open drain outputs that are emulated. > > > > When such outputs are outputting a logic 1, emulation will set the > > > > hardware to input mode, which will cause gpiod_get_direction() to > > > > report that it is in input mode. This is different from the behaviour > > > > with a true open-drain output. > > > > > > > > Unify the semantics here. > > > > > > > > Suggested-by: Linus Walleij <linus.walleij@linaro.org> > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > --- > > > > drivers/gpio/gpiolib.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > index 32c2048deb8c..70c60aac41cc 100644 > > > > --- a/drivers/gpio/gpiolib.c > > > > +++ b/drivers/gpio/gpiolib.c > > > > @@ -220,6 +220,14 @@ int gpiod_get_direction(struct gpio_desc *desc) > > > > chip = gpiod_to_chip(desc); > > > > offset = gpio_chip_hwgpio(desc); > > > > > > > > + /* > > > > + * Open drain emulation using input mode may incorrectly report > > > > + * input here, fix that up. > > > > + */ > > > > + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags) && > > > > + test_bit(FLAG_IS_OUT, &desc->flags)) > > > > + return 0; > > > > + > > > > > > What about the FLAG_OPEN_SOURCE case? > > > > do you have a scenario you can test? > > > > No I don't - if I had a scenario that had tripped over this problem > then I would've submitted a patch already ;-). > > I'm simply pointing out that the logic that applies to > emulating OPEN_DRAIN also applies to emulating OPEN_SOURCE. > IMHO if you are fixing this for one then it should be fixed for both. That would be nice, but it would also be nice to be sure that the fix works there _and_ it doesn't break anything by fixing it. I regard this as a risky change: with open drain/open source "outputs" it is quite obvious when the pin is being driven, it is in output mode. When the driver is off though, it is debatable whether it should be regarded as in input or output mode. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpiolib: fix up emulated open drain outputs 2019-12-08 13:02 ` Russell King - ARM Linux admin @ 2019-12-08 13:34 ` Kent Gibson 2019-12-11 9:39 ` Bartosz Golaszewski 0 siblings, 1 reply; 7+ messages in thread From: Kent Gibson @ 2019-12-08 13:34 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio On Sun, Dec 08, 2019 at 01:02:56PM +0000, Russell King - ARM Linux admin wrote: > On Sun, Dec 08, 2019 at 03:51:43PM +0800, Kent Gibson wrote: > > On Sun, Dec 08, 2019 at 07:18:23AM +0000, Russell King - ARM Linux admin wrote: > > > On Sun, Dec 08, 2019 at 11:43:40AM +0800, Kent Gibson wrote: > > > > On Sat, Dec 07, 2019 at 04:20:18PM +0000, Russell King wrote: > > > > > gpiolib has a corner case with open drain outputs that are emulated. > > > > > When such outputs are outputting a logic 1, emulation will set the > > > > > hardware to input mode, which will cause gpiod_get_direction() to > > > > > report that it is in input mode. This is different from the behaviour > > > > > with a true open-drain output. > > > > > > > > > > Unify the semantics here. > > > > > > > > > > Suggested-by: Linus Walleij <linus.walleij@linaro.org> > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > > --- > > > > > drivers/gpio/gpiolib.c | 8 ++++++++ > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > > index 32c2048deb8c..70c60aac41cc 100644 > > > > > --- a/drivers/gpio/gpiolib.c > > > > > +++ b/drivers/gpio/gpiolib.c > > > > > @@ -220,6 +220,14 @@ int gpiod_get_direction(struct gpio_desc *desc) > > > > > chip = gpiod_to_chip(desc); > > > > > offset = gpio_chip_hwgpio(desc); > > > > > > > > > > + /* > > > > > + * Open drain emulation using input mode may incorrectly report > > > > > + * input here, fix that up. > > > > > + */ > > > > > + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags) && > > > > > + test_bit(FLAG_IS_OUT, &desc->flags)) > > > > > + return 0; > > > > > + > > > > > > > > What about the FLAG_OPEN_SOURCE case? > > > > > > do you have a scenario you can test? > > > > > > > No I don't - if I had a scenario that had tripped over this problem > > then I would've submitted a patch already ;-). > > > > I'm simply pointing out that the logic that applies to > > emulating OPEN_DRAIN also applies to emulating OPEN_SOURCE. > > IMHO if you are fixing this for one then it should be fixed for both. > > That would be nice, but it would also be nice to be sure that the fix > works there _and_ it doesn't break anything by fixing it. > > I regard this as a risky change: with open drain/open source "outputs" > it is quite obvious when the pin is being driven, it is in output mode. > When the driver is off though, it is debatable whether it should be > regarded as in input or output mode. > Higher powers can make the call on that. I just wanted to point out that the fix only deals with one of the two cases that need to be fixed - in case that slipped by. Cheers, Kent. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gpiolib: fix up emulated open drain outputs 2019-12-08 13:34 ` Kent Gibson @ 2019-12-11 9:39 ` Bartosz Golaszewski 0 siblings, 0 replies; 7+ messages in thread From: Bartosz Golaszewski @ 2019-12-11 9:39 UTC (permalink / raw) To: Kent Gibson; +Cc: Russell King - ARM Linux admin, Linus Walleij, linux-gpio niedz., 8 gru 2019 o 14:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > What about the FLAG_OPEN_SOURCE case? > > > > > > > > do you have a scenario you can test? > > > > > > > > > > No I don't - if I had a scenario that had tripped over this problem > > > then I would've submitted a patch already ;-). > > > > > > I'm simply pointing out that the logic that applies to > > > emulating OPEN_DRAIN also applies to emulating OPEN_SOURCE. > > > IMHO if you are fixing this for one then it should be fixed for both. > > > > That would be nice, but it would also be nice to be sure that the fix > > works there _and_ it doesn't break anything by fixing it. > > > > I regard this as a risky change: with open drain/open source "outputs" > > it is quite obvious when the pin is being driven, it is in output mode. > > When the driver is off though, it is debatable whether it should be > > regarded as in input or output mode. > > > > Higher powers can make the call on that. > > I just wanted to point out that the fix only deals with one of the two > cases that need to be fixed - in case that slipped by. > I'll apply this patch as is and if someone reports problems with the opposite condition, then we'll fix that too. Bartosz ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-12-11 9:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-07 16:20 [PATCH] gpiolib: fix up emulated open drain outputs Russell King 2019-12-08 3:43 ` Kent Gibson 2019-12-08 7:18 ` Russell King - ARM Linux admin 2019-12-08 7:51 ` Kent Gibson 2019-12-08 13:02 ` Russell King - ARM Linux admin 2019-12-08 13:34 ` Kent Gibson 2019-12-11 9:39 ` Bartosz Golaszewski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).