Re: [PATCH 1/4] gpiolib: add support for bias pull disable

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Thu, 2022-07-14 at 20:00 +0800, Kent Gibson wrote:
> On Thu, Jul 14, 2022 at 10:47:27AM +0200, Nuno Sá wrote:
> > On Thu, 2022-07-14 at 16:27 +0800, Kent Gibson wrote:
> > > On Thu, Jul 14, 2022 at 09:14:21AM +0200, Nuno Sá wrote:
> > > > On Thu, 2022-07-14 at 12:20 +0800, Kent Gibson wrote:
> > > > > On Wed, Jul 13, 2022 at 08:36:38PM +0300, Andy Shevchenko
> > > > > wrote:
> > > > > > On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote:
> > > > > > > This change prepares the gpio core to look at firmware
> > > > > > > flags
> > > > > > > and
> > > > > > > set
> > > > > > > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way
> > > > > > > to
> > > > > > > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'.
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > >         GPIO_PULL_UP                    = (1 << 4),
> > > > > > >         GPIO_PULL_DOWN                  = (1 << 5),
> > > > > > > +       GPIO_PULL_DISABLE               = (1 << 6),
> > > > > > 
> > > > > > To me it seems superfluous. You have already two flags:
> > > > > > PUp
> > > > > > PDown
> > > > > > When none is set --> Pdisable
> > > > > > 
> > > > > 
> > > > > Agree with Andy on this.  The FLAG_BIAS_DISABLE was added, by
> > > > > me,
> > > > > to
> > > > > allow the cdev interface to support bias.  cdev requires a
> > > > > "don't
> > > > > care"
> > > > > state, distinct from an explicit BIAS_DISABLE.
> > > > > The FLAG_BIAS_DISABLE allows gpiolib-cdev to communicate that
> > > > > to
> > > > > gpiolib, without altering the interpretation of the existing
> > > > > PULL_UP
> > > > > and
> > > > > PULL_DOWN flags.
> > > > > That is not an issue on the machine interface, where the two
> > > > > GPIO_PULL
> > > > > flags suffice.
> > > > > 
> > > > 
> > > > I see, but this means we can only disable the pin BIAS through
> > > > userspace. I might be wrong but I don't see a reason why it
> > > > wouldn't be
> > > > valid to do it from an in kernel path as we do for PULL-UPS and
> > > > PULL-
> > > > DOWNS 
> > > > 
> > > 
> > > > > If you are looking for the place where FLAG_BIAS_DISABLE is
> > > > > set
> > > > > it is
> > > > > in
> > > > > gpio_v2_line_config_flags_to_desc_flags() in gpiolib-cdev.c.
> > > > > 
> > > > > Referring to gpio_set_bias(), the only place in gpiolib the
> > > > > FLAG_BIAS_DISABLE is used, if neither FLAG_PULL_UP,
> > > > > FLAG_PULL_DOWN,
> > > > > nor FLAG_BIAS_DISABLE are set then the bias configuration
> > > > > remains
> > > > > unchanged (the don't care case) - no change is passed to the
> > > > > driver.
> > > > > Otherwise the corresponding PIN_CONFIG_BIAS flag is passed to
> > > > > the
> > > > > driver.
> > > > > 
> > > > 
> > > > Exactly, but note FLAG_BIAS_DISABLE can only be set from
> > > > userspace
> > > > at
> > > > this point (IIUTC). If everyone agrees that should be case, so
> > > > be
> > > > it.
> > > > But as I said, I just don't see why it's wrong to do it within
> > > > the
> > > > kernel.
> > > > 
> > > 
> > > Believe it or not gpiolib-cdev is part of the kernel, not
> > > userspace -
> > > it
> > > just provides an interface to userspace.
> > > 
> > 
> > Yes, I do know that. But don't you still need a userspace process
> > to
> > open the cdev and do the ioctl()?
> > 
> 
> Only if you want to drive the cdev interface, so not in this case.
> You would not use cdev, you would use gpiolib directly.
> 

That's what I'm trying to do :). Without having to change gpiod
consumers to have to explicitly set this flag.

> > > Bias can be disabled by calling gpiod_direction_input() or
> > > gpiod_direction_output() after setting the FLAG_BIAS_DISABLE, as
> > > gpiolib-cdev does.
> > > 
> > > Does that work for you?
> > > 
> > 
> > I'm not seeing how would this work... We would need to make gpiod
> > consumers having to do this. Something like:
> > 
> > 
> > desc = giod_get();
> > set_bit(FLAG_BIAS_DISABLE, &desc->flags);
> > set_direction...
> > 
> > 
> 
> In a nutshell.
> 
> If that solves your immediate problem then you need to decide what
> problem your patch is trying to address.
> 
> 

So my patch is trying to solve exactly this case because (IMO), it does
not make sense for consumers drivers to have to do the above code.
Moreover, they would need some custom firmware property (eg:
devicetree) for the cases where we want to disable BIAS since we cannot
just assume we want to do it.

Well, maybe we can just assume FLAG_BIAS_DISABLE in gpiolib (when
trying to get the gpiod) if no PULL is specified. However, I do have
some concerns with it (see my conversation with Andy in the cover).

- Nuno Sá