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á
