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

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

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

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

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



- Nuno Sá