On Wed, 2022-07-13 at 20:39 +0300, Andy Shevchenko wrote:

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.

Isn't it enough?

I might be missing something but don't think so. Look at this driver
which seems a lot like the reference i put in the cover:

https://elixir.bootlin.com/linux/v5.19-rc6/source/drivers/gpio/gpio-pca953x.c#L573

I just don't see an in-kernel path (I'm aware now that we can get here
through gpio cdev) to get to the point where we want to disable the pin
BIAS.

quoted

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.

It seems you are looking into wrong source of issues. Isn't it a
issue of
particular pin control driver?

Think about gpio expanders on, eg, an i2c bus which don't really have
any pinmuxing capability [1]. For example, my device is an i2c keyboard
which has the capability of exposing pins as gpios (to be consumed by
gpio_keys). The pins, by default are PULL-UPs but we can disable them
doing an i2c write on the device. So to me, the way to do it is via the
gpiochip 'set_config()' hook but as things are, there's no way to get
into the callback with 'PIN_CONFIG_BIAS_DISABLE'. And the driver cannot
just assume that the default case is to disable bias...

Now taking your words (on patch 1 comments)

"
To me it seems superfluous. You have already two flags:
PUp
PDown
When none is set --> Pdisable
"

I guess we could do that assumption in 'gpiod_configure_flags()' and
extend the following code:


if (lflags & GPIO_PULL_UP)
	set_bit(FLAG_PULL_UP, &desc->flags);
else if (lflags & GPIO_PULL_DOWN)
	set_bit(FLAG_PULL_DOWN, &desc->flags);

with an else clause where we do 'set_bit(FLAG_BIAS_DISABLE, &desc-

flags)' by default. As gpiolib does not consider '-ENOTSUPP' as an

error, this would not "explicitly" break existing drivers.

But I do have some concerns with making such an assumption. This
*might* change behavior on existing systems. Think on a system using
for example gpio-pca953x I linked before. If the default state of the
pins is PULL-UP (or down), it's legit to think that, for example,
devicetrees of such a system are not explicitly setting 'GPIO_PULL_UP'.
That's it, this change would break it because now the pins will have
BIAS disabled by default...

Note the above is just me speculating but might be a valid concern.
 
1: https://github.com/torvalds/linux/commit/ede033e1e863c


- Nuno Sá