Re: [PATCH v2 2/6] gpiolib: add support for pull up/down to lineevent_create

[Bartosz Golaszewski <brgl@bgdev.pl>]

pon., 14 paź 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Oct 14, 2019 at 02:35:38PM +0200, Bartosz Golaszewski wrote:
> > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > This patch adds support for pull up/down to lineevent_create.
> > > Use cases include receiving asynchronous presses from a
> > > push button without an external pull up/down.
> > >
> > > Move all the flags sanitization before any memory allocation in
> > > lineevent_create() in order to remove a couple unneeded gotos.
> > > (from Bartosz Golaszewski <bgolaszewski@baylibre.com>)
> > >
> >
> > The patch you pulled in into your commit already sits in my for-next branch.
> > I didn't know you would be modifying the code I touched earlier. In this case
> > you can rebase the series on top of gpio/for-next from my tree[1]
> >
> You explicitly told me to rebase it onto the latest release candidate,

Yes, because - as you can find out in
Documentation/process/5.Posting.rst - the general rule is to rebase
the series against the latest release candidate tag from mainline, but
it may become necessary to rebase against the maintainer's tree, which
is the case here.

> and to squash the incomplete changes from your branch into my changes.
> How did you think that was going to pan out?
>

When saying that you can squash my change if you like, I referred to
the unfinished patch[1], not an applied, self-contained change
from[2].

[1] https://github.com/brgl/linux/commit/99b85d1c26ea51b7b6841b2f2971c31d1d544c2f
[2] https://github.com/brgl/linux/commit/f6cfbbe2950b2f7ae6a99d5e13285c8fad5975d2

> > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > ---
> > >  drivers/gpio/gpiolib.c | 60 +++++++++++++++++++++++++-----------------
> > >  1 file changed, 36 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 9d2a5e2f6e77..053847b6187f 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -905,6 +905,38 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > >         if (copy_from_user(&eventreq, ip, sizeof(eventreq)))
> > >                 return -EFAULT;
> > >
> > > +       offset = eventreq.lineoffset;
> > > +       lflags = eventreq.handleflags;
> > > +       eflags = eventreq.eventflags;
> > > +
> > > +       if (offset >= gdev->ngpio)
> > > +               return -EINVAL;
> > > +
> > > +       /* Return an error if a unknown flag is set */
> > > +       if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
> > > +           (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS))
> > > +               return -EINVAL;
> > > +
> > > +       /* This is just wrong: we don't look for events on output lines */
> > > +       if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
> > > +           (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> > > +           (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
> > > +               return -EINVAL;
> > > +
> > > +       /* PULL_UP and PULL_DOWN flags only make sense for input mode. */
> > > +       if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
> > > +           ((lflags & GPIOHANDLE_REQUEST_PULL_UP) ||
> > > +            (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;
> > > @@ -922,30 +954,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > >                 }
> > >         }
> > >
> > > -       offset = eventreq.lineoffset;
> > > -       lflags = eventreq.handleflags;
> > > -       eflags = eventreq.eventflags;
> > > -
> > > -       if (offset >= gdev->ngpio) {
> > > -               ret = -EINVAL;
> > > -               goto out_free_label;
> > > -       }
> > > -
> > > -       /* Return an error if a unknown flag is set */
> > > -       if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
> > > -           (eflags & ~GPIOEVENT_REQUEST_VALID_FLAGS)) {
> > > -               ret = -EINVAL;
> > > -               goto out_free_label;
> > > -       }
> > > -
> > > -       /* This is just wrong: we don't look for events on output lines */
> > > -       if ((lflags & GPIOHANDLE_REQUEST_OUTPUT) ||
> > > -           (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> > > -           (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)) {
> > > -               ret = -EINVAL;
> > > -               goto out_free_label;
> > > -       }
> > > -
> > >         desc = &gdev->descs[offset];
> > >         ret = gpiod_request(desc, le->label);
> > >         if (ret)
> > > @@ -955,6 +963,10 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> > >
> > >         if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
> > >                 set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > > +       if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)
> > > +               set_bit(FLAG_PULL_DOWN, &desc->flags);
> > > +       if (lflags & GPIOHANDLE_REQUEST_PULL_UP)
> > > +               set_bit(FLAG_PULL_UP, &desc->flags);
> > >
> >
> > Correct me if I'm wrong but this looks like it should be part of
> > Drew's patch (1/6) in this series right? You can modify it and add
> > your Signed-off-by tag. In fact your Signed-off-by is needed anyway if
> > you're sending someone else's patches.
> >
> No problem - you are wrong.  Drew's patch added support on handle requests.
> This patch adds support on event requests.
>
> Or are you do you want me to squash the whole series down to 2 - gpiolib
> and mockup?
>