linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kent Gibson <warthog618@gmail.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bamvor Jian Zhang <bamv2005@gmail.com>,
	Drew Fustini <drew@pdp7.com>
Subject: Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set
Date: Tue, 15 Oct 2019 08:58:49 +0800	[thread overview]
Message-ID: <20191015005849.GA7970@sol> (raw)
In-Reply-To: <CAMRc=Md75GmWhi+KBtVTeLJ=efB6cDr8dxsEbZ2A6vc4z6LAwA@mail.gmail.com>

On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote:
> pon., 14 paź 2019 o 15:04 Kent Gibson <warthog618@gmail.com> 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@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.
> >
> 
> 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.

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.
I'd be happier adding utils to pull bit fields out of flags.

It makes no sense to me to add another flag, so I wont be doing that.

Cheers,
Kent.


  reply	other threads:[~2019-10-15  0:58 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-12  1:56 [PATCH v2 0/6] gpio: expose pull-up/pull-down line flags to userspace Kent Gibson
2019-10-12  1:56 ` [PATCH v2 1/6] " Kent Gibson
2019-10-12  1:56 ` [PATCH v2 2/6] gpiolib: add support for pull up/down to lineevent_create Kent Gibson
2019-10-14 12:35   ` Bartosz Golaszewski
2019-10-14 12:58     ` Kent Gibson
2019-10-14 16:44       ` Bartosz Golaszewski
2019-10-12  1:56 ` [PATCH v2 3/6] gpio: mockup: add set_config to support pull up/down Kent Gibson
2019-10-12  1:56 ` [PATCH v2 4/6] gpiolib: pull requires explicit input mode Kent Gibson
2019-10-14 12:38   ` Bartosz Golaszewski
2019-10-14 12:54     ` Kent Gibson
2019-10-14 17:00       ` Bartosz Golaszewski
2019-10-15  0:52         ` Kent Gibson
2019-10-12  1:56 ` [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set Kent Gibson
2019-10-14 12:43   ` Bartosz Golaszewski
2019-10-14 13:04     ` Kent Gibson
2019-10-14 16:50       ` Bartosz Golaszewski
2019-10-15  0:58         ` Kent Gibson [this message]
2019-10-15 12:51           ` Bartosz Golaszewski
2019-10-16  1:01             ` Kent Gibson
2019-10-17  5:06               ` Kent Gibson
2019-10-18  8:03                 ` Bartosz Golaszewski
2019-10-18 10:13                   ` Kent Gibson
2019-10-21 14:57                     ` Bartosz Golaszewski
2019-10-21 23:14                       ` Kent Gibson
2019-10-22  3:11                         ` Kent Gibson
2019-10-18  7:48               ` Bartosz Golaszewski
2019-10-18  9:42                 ` Kent Gibson
2019-10-12  1:56 ` [PATCH v2 6/6] gpiolib: allow pull up/down on outputs Kent Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191015005849.GA7970@sol \
    --to=warthog618@gmail.com \
    --cc=bamv2005@gmail.com \
    --cc=brgl@bgdev.pl \
    --cc=drew@pdp7.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).