Re: [PATCH] gpiolib: fix up emulated open drain outputs

[Kent Gibson <warthog618@xxxxxxxxx>]

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@xxxxxxxxxx>
> > > > > Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > >  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.