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

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

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?

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


"
git grep "PIN_CONFIG_BIAS_DISABLE" drivers/gpio/

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

- Nuno Sá