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

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

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

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