[v2,4/6] gpiolib: pull requires explicit input mode

This patch prevents pull up/down flags being applied to as-is line
requests, which should be left as-is, and for output mode for which
setting pulls is not currently supported.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib.c | 6 ++++++
 1 file changed, 6 insertions(+)

sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> This patch prevents pull up/down flags being applied to as-is line
> requests, which should be left as-is, and for output mode for which
> setting pulls is not currently supported.
>

This again looks like it should be done right in patch 1/6 instead of
being fixed later in the same series. Or is there some reason to do it
this way I'm not seeing?

Bart

> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 053847b6187f..647334f53622 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -559,6 +559,12 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>              (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;
> +
>         lh = kzalloc(sizeof(*lh), GFP_KERNEL);
>         if (!lh)
>                 return -ENOMEM;
> --
> 2.23.0
>

On Mon, Oct 14, 2019 at 02:38:38PM +0200, Bartosz Golaszewski wrote:
> sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > This patch prevents pull up/down flags being applied to as-is line
> > requests, which should be left as-is, and for output mode for which
> > setting pulls is not currently supported.
> >
> 
> This again looks like it should be done right in patch 1/6 instead of
> being fixed later in the same series. Or is there some reason to do it
> this way I'm not seeing?
> 
The patch series adds full support for pull up/down in stages - in order
of increasing level of controversy, at least IMHO.
That way you can drop the more contraversial components if you disagree
with them by rejecting individual patches, and most likely all the
ones that follow.

And I certainly wasn't going to bundle everything into Drew's patch.

Cheers,
Kent.

pon., 14 paź 2019 o 14:55 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Oct 14, 2019 at 02:38:38PM +0200, Bartosz Golaszewski wrote:
> > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > This patch prevents pull up/down flags being applied to as-is line
> > > requests, which should be left as-is, and for output mode for which
> > > setting pulls is not currently supported.
> > >
> >
> > This again looks like it should be done right in patch 1/6 instead of
> > being fixed later in the same series. Or is there some reason to do it
> > this way I'm not seeing?
> >
> The patch series adds full support for pull up/down in stages - in order
> of increasing level of controversy, at least IMHO.
> That way you can drop the more contraversial components if you disagree
> with them by rejecting individual patches, and most likely all the
> ones that follow.
>

I will not - and I think Linus will agree on that - apply half a
series when it addresses the user API. We need to be very precise
about what changes will be merged and the patches must be well
organized logically.

For instance: the commit message of this patch makes me think that it
fixes an issue introduced in an earlier patch in this very series.
Unless that's not true - in which case the commit message should be
reworded - it's not acceptable.

Bart

> And I certainly wasn't going to bundle everything into Drew's patch.
>
> Cheers,
> Kent.
>

On Mon, Oct 14, 2019 at 07:00:37PM +0200, Bartosz Golaszewski wrote:
> pon., 14 paź 2019 o 14:55 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Mon, Oct 14, 2019 at 02:38:38PM +0200, Bartosz Golaszewski wrote:
> > > sob., 12 paź 2019 o 03:57 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > This patch prevents pull up/down flags being applied to as-is line
> > > > requests, which should be left as-is, and for output mode for which
> > > > setting pulls is not currently supported.
> > > >
> > >
> > > This again looks like it should be done right in patch 1/6 instead of
> > > being fixed later in the same series. Or is there some reason to do it
> > > this way I'm not seeing?
> > >
> > The patch series adds full support for pull up/down in stages - in order
> > of increasing level of controversy, at least IMHO.
> > That way you can drop the more contraversial components if you disagree
> > with them by rejecting individual patches, and most likely all the
> > ones that follow.
> >
> 
> I will not - and I think Linus will agree on that - apply half a
> series when it addresses the user API. We need to be very precise
> about what changes will be merged and the patches must be well
> organized logically.

And that is fine.  The series was structured so I could easily cull the
more contraversial aspects, such as the Pull None, and issue an updated
series.

Conversely it is easy enough for you to squash patches together if you
want all of them, but don't want the intermediate states.
> 
> For instance: the commit message of this patch makes me think that it
> fixes an issue introduced in an earlier patch in this very series.
> Unless that's not true - in which case the commit message should be
> reworded - it's not acceptable.

> 

It fixes what I consider an oversight in a patch that I thought you had
already signed off on. Much the same as my recent patch to reject both 
INPUT and OUTPUT flags being set.  Is that an issue?  Given no one will
even be using the new flags until the feature is added to the kernel 
and libgpiod is updated?

I don't see it as relevant that the oversight it addresses was
introduced in an earlier patch (Drew's).  So what?  You just said 
that you will only apply the series all or nothing, so why does it
matter?

As the problem begins in Drew's patch, that would require changing
Drew's patch itself.  I'm happy to build on top of other's patches,
but I'm not happy to do that. It just doesn't feel right to me.
Doesn't doing that muddle who has changed what, and confuse
attribution?

I obviously don't understand how this whole patch series thing works.

Cheers,
Kent.