Re: [PATCH 0/4] add support for bias pull-disable

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Jul 15, 2022 at 2:19 PM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> On Fri, 2022-07-15 at 15:05 +0300, Andy Shevchenko wrote:
> > On Fri, Jul 15, 2022 at 12:20:56PM +0200, Nuno Sá wrote:
> > > On Thu, 2022-07-14 at 21:57 +0300, Andy Shevchenko wrote:
> > > > On Thu, Jul 14, 2022 at 05:43:41PM +0200, Nuno Sá wrote:
> > > > > On Thu, 2022-07-14 at 17:58 +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:
> > > > > > > The gpio core looks at 'FLAG_BIAS_DISABLE' in preparation
> > > > > > > of
> > > > > > > calling the
> > > > > > > gpiochip 'set_config()' hook. However, AFAICT, there's no
> > > > > > > way
> > > > > > > that
> > > > > > > this
> > > > > > > flag is set because there's no support for it in firwmare
> > > > > > > code.
> > > > > > > Moreover,
> > > > > > > in 'gpiod_configure_flags()', only pull-ups and pull-downs
> > > > > > > are
> > > > > > > being
> > > > > > > handled.
> > > > > > >
> > > > > > > On top of this, there are some users that are looking at
> > > > > > > 'PIN_CONFIG_BIAS_DISABLE' in the 'set_config()' hook. So,
> > > > > > > unless
> > > > > > > I'm
> > > > > > > missing something, it looks like this was never working for
> > > > > > > these
> > > > > > > chips.
> > > > > > >
> > > > > > > Note that the ACPI case is only compiled tested. At first
> > > > > > > glance,
> > > > > > > it seems
> > > > > > > the current patch is enough but i'm not really sure...
> > > > > >
> > > > > > So, I looked closer to the issue you are trying to describe
> > > > > > here.
> > > > > >
> > > > > > As far as I understand we have 4 state of BIAS in the
> > > > > > hardware:
> > > > > > 1/ AS IS (defined by firnware)
> > > > > > 2/ Disabled (neither PU, not PD)
> > > > > > 3/ PU
> > > > > > 4/ PD
> > > > > >
> > > > > > The case when the default of bias is not disabled (for
> > > > > > example
> > > > > > specific, and I
> > > > > > think very special, hardware may reset it to PD or PU), it's
> > > > > > a
> > > > > > hardware driver
> > > > > > responsibility to inform the framework about the real state
> > > > > > of
> > > > > > the
> > > > > > lines and
> > > > > > synchronize it.
> > > > > >
> > > > > > Another case is when the firmware sets the line in non-
> > > > > > disabled
> > > > > > state
> > > > > > and
> > > > > > by some reason you need to disable it. The question is, why?
> > > > >
> > > > > Not getting this point...
> > > >
> > > > I understand that in your case "firmware" is what DTB provides.
> > > > So taking into account that the default of hardware is PU, it
> > > > needs
> > > > a mechanism to override that, correct?
> > > >
> > >
> > > Exactly...
> > >
> > > > > > > As a side note, this came to my attention during this
> > > > > > > patchset
> > > > > > > [1]
> > > > > > > (and, ofr OF,  was tested with it).
> > > > > > >
> > > > > > > [1]:
> > > > > > > https://lore.kernel.org/linux-input/20220708093448.42617-5-nuno.sa@xxxxxxxxxx/
> > > > > >
> > > > > > Since this provides a GPIO chip, correct?, it's
> > > > > > responsibility of
> > > > > > the
> > > > > > driver to
> > > > > > synchronize it, no? Basically if you really don't trust
> > > > > > firmware,
> > > > > > you
> > > > > > may
> > > > >
> > > > > What do you mean by synchronize?
> > > >
> > > > Full duplex sync, i.e. setting flag to PU for the pins that
> > > > should
> > > > stay PU:ed
> > > > and disabling bias for the ones, that want it to be disabled. (PD
> > > > accordingly)
> > > >
> > > > > > go via all GPIO lines and switch them to the known (in
> > > > > > software)
> > > > > > state. This
> > > > > > approach on the other hand is error prone, because firmware
> > > > > > should
> > > > > > know better
> > > > > > which pin is used for which purpose, no? If you don't trust
> > > > > > firwmare
> > > > > > (in some
> > > > > > cases), then it's a matter of buggy platform that has to be
> > > > > > quirked
> > > > > > out.
> > > > >
> > > > > I'm not getting what you mean by "firmware should know better"?
> > > > > So,
> > > > > basically, and let's take OF as example, you can request a GPIO
> > > > > in
> > > > > OF
> > > > > by doing something like:
> > > > >
> > > > >         foo-gpios = <&gpio 1 GPIO_PULL_UP>;
> > > > >
> > > > > In this way, when the consumer driver gets the gpio, all the
> > > > > flags
> > > > > will
> > > > > be properly set so that when we set a direction (for example)
> > > > > the
> > > > > gpiochip's 'set_config()' will be called and the driver does
> > > > > what
> > > > > it
> > > > > needs to setup the pull-up. If we want BIAS_DISABLED on the
> > > > > pin,
> > > > > there's no way to the same as the above. So basically, this can
> > > > > ever
> > > > > happen:
> > > > >
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2227
> > > > >
> > > > > (only possible from the gpiochip cdev interface)
> > > > >
> > > > > So, what I'm proposing is to be possible to do from OF:
> > > > >
> > > > >         foo-gpios = <&gpio 1 GPIO_PULL_DISABLE>;
> > > > >
> > > > > And then we will get into the gpiochip's 'set_config()' to
> > > > > disable
> > > > > the
> > > > > pull-up or pull-down.
> > > > >
> > > > > As I said, my device is an input keymap that can export pins as
> > > > > GPIOs
> > > > > (to be consumed by gpio_keys). The pins by default have pull-
> > > > > ups
> > > > > that
> > > > > can be disabled by doing a device i2c write. I'm just trying to
> > > > > use
> > > > > the
> > > > > infrastructure that already exists in gpiolib (for pull-
> > > > > up|down) to
> > > > > accomplish this. There's no pinctrl driver controlling the
> > > > > pins.
> > > > > The
> > > > > device itself controls them and having this device as a pinctrl
> > > > > one
> > > > > is
> > > > > not really applicable.
> > > >
> > > > Yes, I have got it eventually. The root cause is that after reset
> > > > you
> > > > have a
> > > > hardware that doesn't disable bias.
> > > >
> > > > Now, we have DT properties for PD and PU, correct?
> > > > For each requested pin you decide either to leave the state as it
> > > > is,
> > > > or
> > > > apply bias.
> > > >
> > > > in ->probe() of your GPIO you reset hardware and for each GPIO
> > > > descriptor you
> > > > set PU flag.
> > > > In ->request(), don;t know the name by heart, you disable BIAS
> > > > based
> > > > on absence
> > > > of flags, it can be done without an additional properties, purely
> > > > in
> > > > the GPIO
> > > > OF code. Do I understand this correctly?
> > > >
> > >
> > > Alright, I think now you got it and we are on the same page. If I
> > > understood your suggestion, users would just use GPIO_PULL_UP in
> > > dtb if
> > > wanting the default behavior. I would then use the gpiochip
> > > 'request()'
> > > callback to test the for pull-up flag right?
> >
> > Something like this, yes.
> >
> > > If I'm getting this right, there's a problem with it...
> > > gpiod_configure_flags() is called after gpiod_request() which means
> > > that the gpiod descriptor won't still have the BIAS flags set. And
> > > I
> > > don't think there's a way (at least clean and easy) to get the
> > > firmware
> > > lookup flags from the request callback?
> > >
> > > So, honestly the only option I see to do it without changing
> > > gpioblib
> > > would be to hook this change in output/input callbacks which is far
> > > from being optimal...
> > >
> > > So, in the end having this explicitly like this feels the best
> > > option
> > > to me. Sure, I can find some workaround in my driver but that does
> > > not
> > > change this...
> >
> > Ok, let me think about it. Meanwhile, maybe others have better ideas
> > already?
> >
>
> Sure, I'm still thinking that having this extra property and explicitly
> set it from OF is not that bad :)
>
> > > "
> > > git grep "PIN_CONFIG_BIAS_DISABLE" drivers/gpio/
> >
> > Hint: `git grep -lw "PIN_CONFIG_BIAS_DISABLE" -- drivers/gpio`
> >
>
> nice..
>
> > > drivers/gpio/gpio-aspeed.c:963: else if (param ==
> > > PIN_CONFIG_BIAS_DISABLE ||
> > > drivers/gpio/gpio-merrifield.c:197:     if
> > > ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) ||
> > > drivers/gpio/gpio-omap.c:903:   case PIN_CONFIG_BIAS_DISABLE:
> > > drivers/gpio/gpio-pca953x.c:573:        if (config ==
> > > PIN_CONFIG_BIAS_DISABLE)
> > > drivers/gpio/gpio-pca953x.c:592:        case
> > > PIN_CONFIG_BIAS_DISABLE:
> > > "
> > >
> > > AFAICT, the only way this path is possible for these drivers is
> > > through
> > > gpiolib cdev which might not be what the authors of the drivers
> > > were
> > > expecting...
> >
> > gpio-merrifield is bad example, it has a pin control.
> > gpio-pca953x as I said should effectively be a pin control driver.
> >
> > For the two left it might be the case.
> >
>
> Well the thing is that even if we have pinctrl like for example,
> gpio-omap, it is still true that there's no way to get into
> 'omap_gpio_set_config()' for 'PIN_CONFIG_BIAS_DISABLE' and call
> 'gpiochip_generic_config()'.
>
> (naturally in this case, one can directly use pinctrl properties to
> control the pin but still...)
>
>
> - Nuno Sá
>

Ideologically I don't have anything against adding this flag (except
that it should be called BIAS_DISABLE not PULL_DISABLE IMO). Nuno is
right in that the character device is the only way to set this mode
ATM and. However I would like to see the first user added together
with the series because adding features nobody uses in the mainline
kernel tree is generally frowned upon and it's also not clear that
anyone actually wants to use it.

Bart