On Fri, Jul 15, 2022 at 2:19 PM Nuno Sá [off-list ref] wrote:

On Fri, 2022-07-15 at 15:05 +0300, Andy Shevchenko wrote:

quoted

On Fri, Jul 15, 2022 at 12:20:56PM +0200, Nuno Sá wrote:

quoted

On Thu, 2022-07-14 at 21:57 +0300, Andy Shevchenko wrote:

quoted

On Thu, Jul 14, 2022 at 05:43:41PM +0200, Nuno Sá wrote:

quoted

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?

Exactly...

quoted

quoted

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

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?

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.

quoted

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 :)

quoted

quoted

"
git grep "PIN_CONFIG_BIAS_DISABLE" drivers/gpio/

Hint: `git grep -lw "PIN_CONFIG_BIAS_DISABLE" -- drivers/gpio`

nice..

quoted

quoted

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

Bart