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

[Kent Gibson <warthog618@gmail.com>]

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.

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

Cheers,
Kent.