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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Oct 18, 2019 at 09:48:09AM +0200, Bartosz Golaszewski wrote:
> śr., 16 paź 2019 o 03:01 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> >
> > On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote:
> > > 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.
> > >
> > Ok. And, since gpiolib has no knowledge of what combinations are
> > appropriate for a given chip, we can't provide a higher level
> > abstraction and have no option but to expose that pinconf
> > complexity in the GPIO uapi?
> >
> > In fact, pinconf doesn't just define 3 bias bits - it defines 6:
> >
> > enum pin_config_param {
> >         PIN_CONFIG_BIAS_BUS_HOLD,
> >         PIN_CONFIG_BIAS_DISABLE,
> >         PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> >         PIN_CONFIG_BIAS_PULL_DOWN,
> >         PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> >         PIN_CONFIG_BIAS_PULL_UP,
> >
> > Do we need to support any of the remaining 3 in the GPIO uapi, either
> > now or possibly in the future?
> >
> > And what about the other PIN_CONFIG flags?  Some of these might be
> > candidates for controlling via SET_CONFIG_IOCTL, if not in the request
> > itself? (again this is contemplating the future, not suggesting being part
> > of this patch)
> >
> 
> Is anyone asking for these though? Users have been asking for adding
> support for PULL-UP/DOWN and since it seems like a reasonable
> use-case, we're now open to adding it. IMO this is all we need for now
> (plus direction changes on taken lines but it's unrelated to this
> series IMO) but we should make sure that adding additional options in
> the future will be feasible should we need it. Having a hidden
> functionality of disabling bias when pull-up and pull-down bits are
> set quickly becomes obscure once you add more flags.
> 

The point of the question was whether there may be other bias modes we
want to support in the future. That would put another nail in the
coffin of encoding the 4 values we currently want to support, and give
more reason to mirror the pinctrl API.

> > > > 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.
> > >
> > And for some reason I didn't get 0001, yet all 7 parts made it to the mailing
> > list. Spam filters kicking in? Though it isn't in my spam folder either.
> > Something odd going on.
> >
> > > 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.
> > >
> > Cos it makes no sense to me to encode 4 values into 3 bits when 2 will
> > do.  But if you want to expose part of the pinconf API within the GPIO
> > uapi then that goes out the window - it's not 4 values anymore.
> >
> 
> It does make perfect sense if you want to stay as explicit as possible
> on the border between the kernel and user-space. And no, I don't want
> to expose the pinctrl API - I just want to follow its example.
> 

I'm not sure I see the distinction.
I said part of the pinctrl API, and the flags you want to follow
explicitly are part of it.

But we agree on the end result so moving on...

> > And partly cos I'm frustrated that I'd asked questions regarding how the
> > API should look earlier and got no reply.  This is the sort of thing I
> > usually deal with in the design stage, not review.
> >
> 
> I don't really have good news here. This is what kernel development is
> like: it's easier for maintainers to discuss existing code than
> hypothetical design concepts. Just to give you an idea: it recently
> took me 11 iterations of patches and several significant changes to
> get a relatively simple and - what I assumed was a rather -
> non-controversial pmic driver merged upstream. This is simply how it
> usually works: after some initial high-level discussion, there's an
> expectation that a patch series will be posted which can then be
> discussed and worked upon until a consensus is reached.
> 

I understand that, but conversely there are times when the may be clear
preferences or even hard rules as to what is acceptable or unacceptable,
and it wouldn't be that hard for a maintainer to just say a word to stop
development on a branch which is sure to be dead.  I thought this was
one of those cases, rather than the case you cite.

But anyway, moving on...

> Also remember that an idea that sounds good in theory can turn out to
> be wrong once the code is ready and there often is no way of
> predicting it.
> 
> > I realise you guys are busy, but a little time spent clarifying design
> > would save a whole lot more time in coding, testing and review.
> >
> > > 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.
> > >
> > That makes sense, if we are exposing the pinctrl API here.
> >
> 
> But we're not - we're just following the example of the in-kernel
> pinctrl API, making it easy on user-space programs and making this
> future-proof in that adding new options will be painless.
> 

Again, I'm not sure I see the distinction, but I agree going with the
pinctrl flags (PULL_UP, PULL_DOWN and DISABLE) is the way to go.

> > Wrt the patch series, before I take another go at it, I'd like some
> > guidance as to how you would like it structured.
> > Are you good, in principle, with the feature set in the existing patches?
> 
> Yes, as I understand it: the patches should add support for
> pull-up/down on inputs, the ability to disable bias and then
> additional support for pull-up/downs on outputs. This sounds like a
> self-contained set of changes.
> 
> > What branch would you like it based on?
> 
> For now rebase it on top of my gpio/for-next branch[1].
> 
> > It will begin with Drew's patch, either his original or with your
> > additions.
> 
> I don't want a series in which some things are added by earlier
> patches only to be removed by subsequent ones - this is nonsensical.
> If you know you'll change some parts of Drew's patch later, then
> change it right away: leave only the parts that will make it to the
> end and that make logical sense as a single patch while keeping the
> code bisectable (for instance flags definitions etc.) to give Drew
> credit and build on top of that. Be creative and try to imagine what
> code you'd want to review.
> 

Ironically I think it is your additions that are later removed.
So I'll start with Drew's original patch and see how that goes.

> > Other than that I'm open to your suggestions on number, ordering,
> > content, comments etc.
> >
> 
> Patch 5 is wrong to me, I want an explicit DISABLE flag. Patch 4
> should be part of an earlier patch. I'm fine with separate patches for
> lineevent and linehandle as long as the kernel never breaks when
> applying them one-by-one. Adding support for outputs separately is
> fine too.
> 

I do try to ensure that the kernel is never broken by any of my patches.
I believe that is the case with the patches in this series.

Wrt Patch 5 - agreed - DISABLE will be a separate flag as per pinctrl.
And I'll squash 4 into 2.

Cheers,
Kent.