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

[Kent Gibson <warthog618@gmail.com>]

On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote:
> sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> 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@gmail.com>
> > ---
> >  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.

Kent.
> Also: I don't like that
> this patch deletes code added by earlier patches in this series. If we
> really need this, then it should be rearranged.
> 
> Bart
> 
> >                 gpio_set_config(chip, gpio_chip_hwgpio(desc),
> >                                 PIN_CONFIG_BIAS_PULL_UP);
> >         else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
> > @@ -4462,7 +4454,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> >
> >         if (lflags & GPIO_PULL_UP)
> >                 set_bit(FLAG_PULL_UP, &desc->flags);
> > -       else if (lflags & GPIO_PULL_DOWN)
> > +       if (lflags & GPIO_PULL_DOWN)
> >                 set_bit(FLAG_PULL_DOWN, &desc->flags);
> >
> >         ret = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY));
> > --
> > 2.23.0
> >