Re: [PATCH] gpio: add new SET_CONFIG ioctl() to gpio chardev

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Oct 31, 2019 at 08:11:00AM +0100, Bartosz Golaszewski wrote:
> pon., 28 paź 2019 o 08:39 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> >
> > Add the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev.
> > The ioctl allows some of the configuration of a requested handle to be
> > changed without having to release the line.
> > The primary use case is the changing of direction for bi-directional
> > lines.
> >
> > Based on initial work by Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> >
> > Signed-off-by: Kent Gibson <warthog618@xxxxxxxxx>
> > ---
> >
> > The patch is against Bart's gpio/for-next branch[1] with v4 of my bias
> > patch series "gpio: expose line bias flags to userspace" applied.
> > This is necessary to support setting of the bias flags introduced
> > there.
> >
> > The patch has been successfully tested against gpio-mockup using the
> > feature/pud_set_config branch of my Go gpiod library[2], as libgpiod has
> > not yet been updated with the uAPI change.
> >
> >  drivers/gpio/gpiolib.c    | 163 ++++++++++++++++++++++++++------------
> >  include/uapi/linux/gpio.h |  18 +++++
> >  2 files changed, 132 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index df27f05f9b8d..62a74e27ad02 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -427,23 +427,74 @@ struct linehandle_state {
> >         GPIOHANDLE_REQUEST_OPEN_DRAIN | \
> >         GPIOHANDLE_REQUEST_OPEN_SOURCE)
> >
> > +static int linehandle_validate_flags(u32 flags)
> > +{
> > +       /* Return an error if an unknown flag is set */
> > +       if (flags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * Do not allow both INPUT & OUTPUT flags to be set as they are
> > +        * contradictory.
> > +        */
> > +       if ((flags & GPIOHANDLE_REQUEST_INPUT) &&
> > +           (flags & GPIOHANDLE_REQUEST_OUTPUT))
> > +               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
> > +        * electrical result would be disastrous.
> > +        */
> > +       if ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
> > +           (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
> > +               return -EINVAL;
> > +
> > +       /* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
> > +       if (!(flags & GPIOHANDLE_REQUEST_OUTPUT) &&
> > +           ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> > +            (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
> > +               return -EINVAL;
> > +
> > +       /* Bias flags only allowed for input or output mode. */
> > +       if (!((flags & GPIOHANDLE_REQUEST_INPUT) ||
> > +             (flags & GPIOHANDLE_REQUEST_OUTPUT)) &&
> > +           ((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
> > +            (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
> > +            (flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
> > +               return -EINVAL;
> > +
> > +       /* Only one bias flag can be set. */
> > +       if (((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
> > +            (flags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
> > +                       GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
> > +           ((flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
> > +            (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
> > +               return -EINVAL;
> > +
> > +       return 0;
> > +}
> > +
> 
> This is great but I'd prefer that it be put into a separate patch with
> its own description that would come before the other changes.
> 

Will do.

> >  static long linehandle_ioctl(struct file *filep, unsigned int cmd,
> >                              unsigned long arg)
> >  {
> >         struct linehandle_state *lh = filep->private_data;
> >         void __user *ip = (void __user *)arg;
> >         struct gpiohandle_data ghd;
> > +       struct gpiohandle_config gcnf;
> > +       struct gpio_desc *desc;
> >         DECLARE_BITMAP(vals, GPIOHANDLES_MAX);
> > -       int i;
> > +       int i, ret;
> > +       u32 lflags;
> >
> >         if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
> >                 /* NOTE: It's ok to read values of output lines. */
> > -               int ret = gpiod_get_array_value_complex(false,
> > -                                                       true,
> > -                                                       lh->numdescs,
> > -                                                       lh->descs,
> > -                                                       NULL,
> > -                                                       vals);
> > +               ret = gpiod_get_array_value_complex(false,
> > +                                                   true,
> > +                                                   lh->numdescs,
> > +                                                   lh->descs,
> > +                                                   NULL,
> > +                                                   vals);
> >                 if (ret)
> >                         return ret;
> >
> > @@ -477,6 +528,59 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
> >                                               lh->descs,
> >                                               NULL,
> >                                               vals);
> > +       } else if (cmd == GPIOHANDLE_SET_CONFIG_IOCTL) {
> > +               if (copy_from_user(&gcnf, ip, sizeof(gcnf)))
> > +                       return -EFAULT;
> > +
> > +               lflags = gcnf.flags;
> > +               ret = linehandle_validate_flags(lflags);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               for (i = 0; i < lh->numdescs; i++) {
> > +                       desc = lh->descs[i];
> > +                       if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
> > +                               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > +                       else
> > +                               clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > +                       if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
> > +                               set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > +                       else
> > +                               clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > +                       if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
> > +                               set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > +                       else
> > +                               clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > +                       if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
> > +                               set_bit(FLAG_PULL_UP, &desc->flags);
> > +                       else
> > +                               clear_bit(FLAG_PULL_UP, &desc->flags);
> > +                       if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
> > +                               set_bit(FLAG_PULL_DOWN, &desc->flags);
> > +                       else
> > +                               clear_bit(FLAG_PULL_DOWN, &desc->flags);
> > +                       if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
> > +                               set_bit(FLAG_BIAS_DISABLE, &desc->flags);
> > +                       else
> > +                               clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
> 
> Could you add some new lines between the if elses? I think it'll be
> more readable.
> 

Agreed.  I would prefer to call a function that atomically sets or clears
the flag bit based on the requested state, but there doesn't seem to be one.

Cheers,
Kent.