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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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?

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


-- 
With Best Regards,
Andy Shevchenko