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

[Nuno Sá <noname.nuno@xxxxxxxxx>]

On Fri, 2022-07-15 at 21:31 +0200, Bartosz Golaszewski wrote:
> 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

It makes sense, yes.

> 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.

Hmm, you mean something like a system's devicetree needing this flag?
If so, I don't really have such a thing. I did all my testing on a rpi
using overlays.

- Nuno Sá