Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set

[Bartosz Golaszewski <brgl@xxxxxxxx>]

wt., 15 paź 2019 o 02:58 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
>
> On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> > pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> > >
> > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> > > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> > > > >
> > > > > This patch allows pull up/down bias to be disabled, allowing
> > > > > the line to float or to be biased only by external circuitry.
> > > > > Use case is for where the bias has been applied previously,
> > > > > either by default or by the user, but that setting may
> > > > > conflict with the current use of the line.
> > > > >
> > > > > Signed-off-by: Kent Gibson <warthog618@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/gpio/gpiolib.c | 22 +++++++---------------
> > > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > index 647334f53622..f90b20d548b9 100644
> > > > > --- a/drivers/gpio/gpiolib.c
> > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> > > > >             (lflags & GPIOHANDLE_REQUEST_OUTPUT))
> > > > >                 return -EINVAL;
> > > > >
> > > > > -       /* Same with pull-up and pull-down. */
> > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > -               return -EINVAL;
> > > > > -
> > > > >         /*
> > > > >          * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
> > > > >          * the hardware actually supports enabling both at the same time the
> > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > > > >              (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
> > > > >                 return -EINVAL;
> > > > >
> > > > > -       /*
> > > > > -        * Do not allow both pull-up and pull-down flags to be set as they
> > > > > -        *  are contradictory.
> > > > > -        */
> > > > > -       if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
> > > > > -           (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
> > > > > -               return -EINVAL;
> > > > > -
> > > > >         le = kzalloc(sizeof(*le), GFP_KERNEL);
> > > > >         if (!le)
> > > > >                 return -ENOMEM;
> > > > > @@ -2931,6 +2918,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned offset,
> > > > >         unsigned arg;
> > > > >
> > > > >         switch (mode) {
> > > > > +       case PIN_CONFIG_BIAS_DISABLE:
> > > > >         case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > >         case PIN_CONFIG_BIAS_PULL_UP:
> > > > >                 arg = 1;
> > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc)
> > > > >         if (ret == 0)
> > > > >                 clear_bit(FLAG_IS_OUT, &desc->flags);
> > > > >
> > > > > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > > > +       if (test_bit(FLAG_PULL_UP, &desc->flags) &&
> > > > > +               test_bit(FLAG_PULL_DOWN, &desc->flags))
> > > > > +               gpio_set_config(chip, gpio_chip_hwgpio(desc),
> > > > > +                               PIN_CONFIG_BIAS_DISABLE);
> > > > > +       else if (test_bit(FLAG_PULL_UP, &desc->flags))
> > > >
> > > > From looking at the code: user-space can disable bias when setting
> > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in
> > > > this implicit way? Why not a separate flag?
> > >
> > > An extra flag would waste a bit and add nothing but more sanity checking.
> > >
> >
> > I disagree. The user API needs to be very explicit. Sanity checking is
> > alright - if there'll be too many ifdefs, we can start thinking about
> > adding some core library helpers for sanitizing conflicting flags, I'm
> > sure other frameworks could use something like this as well.
> >
> > Especially in this context: setting PULL_UP and PULL_DOWN together
> > disables bias - this doesn't make sense logically.
> >
> In a way it does make a weird kind of sense - they cancel.  Physically.
>

Yes, on some devices we set both bits to disable bias, but on others
the pull-up and pull-down bits need to be cleared and yet others have
a dedicated bit for that. It's not standardized and the pinctrl
framework defines all three values as separate bits to expose a common
programming interface.

> Did you read the cover letter?  The problem, as I see it,
> is that we're stuck using a flag field to encode a two bit enum.
> That fact the we only have a flag field to play with can't be
> changed due to ABI.

For some reason I haven't received the cover letter on my inbox. I'm
only now seeing it on linux-gpio archives.

Anyway: I don't understand why you insist on using two instead of
three bits. You have 32 bits in total that can be used and only 5 are
used so far. There's plenty left.

I'd prefer to see:

GPIOHANDLE_REQUEST_PULL_UP
GPIOHANDLE_REQUEST_PULL_DOWN
GPIOHANDLE_REQUEST_PULL_DISABLED

or maybe even

GPIOHANDLE_REQUEST_BIAS_PULL_UP
GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
GPIOHANDLE_REQUEST_BIAS_DISABLED

to stay consistent with the pinctrl flags. No bit set among these
three would mean AS_IS.

Bart

> I'd be happier adding utils to pull bit fields out of flags.
>
> It makes no sense to me to add another flag, so I wont be doing that.
>
> Cheers,
> Kent.
>