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:

quoted

On Wed, Jul 13, 2022 at 03:14:17PM +0200, Nuno Sá wrote:

quoted

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?

quoted

quoted

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@analog.com/ (local)

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)

quoted

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