pinctrl: single: Cannot disable bias

* pinctrl: single: Cannot disable bias - PIN_CONFIG_BIAS_DISABLE not handled
@ 2024-02-28 12:04 Matthijs Kooijman
  2024-02-29  7:32 ` Tony Lindgren
  0 siblings, 1 reply; 13+ messages in thread
From: Matthijs Kooijman @ 2024-02-28 12:04 UTC (permalink / raw)
  To: linux-gpio; +Cc: Haojian Zhuang, Tony Lindgren

[-- Attachment #1: Type: text/plain, Size: 6189 bytes --]

Hi,

While figuring out why bias-setting on the beaglebone black did not
work, I noticed what looks like a bug in the pinctrl-single driver,
preventing it from disabling a bias once enabled.

The short version is: It handles pin_config_set by looking up the requested
setting in a DT-defined lookup table, which defines what bits correspond to
each setting. For PIN_CONFIG_BIAS_DISABLE, this never works since that setting
is never defined in the lookup table (so the code that exists to handle this
setting is never actually executed).

For the longer version: The pcs_pinconf_set() function handles setting
various settings, including PIN_CONFIG_BIAS_DISABLE,
PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP.

The relevant bits of code look like this (full code at [1]):

	static int pcs_pinconf_set(struct pinctrl_dev *pctldev, unsigned pin, unsigned long *configs, unsigned num_configs) {
		...
		ret = pcs_get_function(pctldev, pin, &func);
		...
		for (j = 0; j < num_configs; j++) {
			for (i = 0; i < func->nconfs; i++) {
				if (pinconf_to_config_param(configs[j]) != func->conf[i].param)
					continue;

				...
				switch (func->conf[i].param) {
				...
				case PIN_CONFIG_BIAS_DISABLE:
					pcs_pinconf_clear_bias(pctldev, pin);
					break;
				case PIN_CONFIG_BIAS_PULL_DOWN:
				case PIN_CONFIG_BIAS_PULL_UP:
				...
				}
				pcs->write(data, pcs->base + offset);

				break;
			}
			...

Functionally, this requests the table of supported configurations for
the to-be-configured pin using pcs_get_function(). This per-pin (group) table
is populated through setting e.g. pinctrl-single,bias-pullup in the device tree
(see [2]). However, this table never contains entries for
PIN_CONFIG_BIAS_DISABLE (see pcs_parse_pinconf() which populates the table), so
the `if (pinconf_to_config_param(configs[j]) != func->conf[i].param)` line
above is the never true, and the handling for PIN_CONFIG_BIAS_DISABLE never
runs. This is probably because in an earlier version of this code, there
was a separate DT entry for PIN_CONFIG_BIAS_DISABLE, see discussion
links below.

It seems this code has been broken since it was first introduced in
commit 9dddb4df90d1 (pinctrl: single: support generic pinconf).

An obvious fix for this would be to lift the handling for
PIN_CONFIG_BIAS_DISABLE out of the inner loop, running that instead of
the inner loop in that case.

Note that I have not verified my analysis of the code by testing. I did
confirm that if I define the (what I think is) proper DT definitions for
bias control for the beaglebone black board, I can enable bias, but not
disable them, but I cannot rule out that that problem has a different
cause. I also do not have a good toolchain setup for compiling custom
kernels for this setup (and limited time for setting up one), so I do
not expect to be able to provide more testing or a patch anytime soon.

I also wonder about the design in general - currently the DT defines 4 values
for both pullup and pulldown: value, enable, disable, mask. Of these, I think
value is not actually used. Mask is used to clear all relevant bits before
setting a value and enable/disable are bits to set for either state.
PIN_CONFIG_BIAS_DISABLE is implemented by disabling both the pullup and
the pulldown.

This expressivity allows expressing a situation where the pullup and
pulldown can be both enabled at the same time, and a pullup can be
disabled by passing 0 to PIN_CONFIG_BIAS_PULL_UP or using
PIN_CONFIG_BIAS_DISABLE. However, in practice, enabling both is often
not supported by hardware, and even if it is, it makes no sense
electrically. Also, I think that in practice this API is never used as
such, for example gpiolib.c enforces only one of these is set, and
always uses PIN_CONFIG_BIAS_DISABLE to disable a bias. The only docs
I could find about this, was in pinconf-generic.h, which says:

   * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
   *      transition from say pull-up to pull-down implies that you disable
   *      pull-up in the process, this setting disables all biasing.

Suggesting that it is indeed never possible to have pullup and pulldown
enabled at the same time.

For the pinctrl-single driver, it would IMHO make more sense to define
three DT properties, for pullup, pulldown and bias-disabled with a value
and mask each, that can be used when each of these settings is enabled.
If a pullup or pulldown is separately disabled (by calling e.g.
PIN_CONFIG_BIAS_PULL_UP with a zero value), this would just apply the
"bias-disabled" value. This is slightly different from the current
behavior for hardware that has separate pullup/pulldown bits (and no
shared enable bit), but matches the behavior of hardware with a shared
pullup/down enable bit (so callers cannot rely on independent state for
the pullup and pulldown anyway).

Looking at the history of the patch that added this code, it seems an
earlier vdrsion has exactly the implementation that I suggested above
(with a pullup/pulldown/disable attribute with a value and mask, and
additonally also a match value for getting):

https://lore.kernel.org/all/20130121171436.GI15361@atomide.com/T/#ma87c4da5183ca8a10c0d6fe72c9a1fffd206e40e

Then it was changed to the current implementation, seemingly with an
argument that having a "PIN_CONFIG_BIAS_ENABLE" API would be more
consistent than "PIN_CONFIG_BIAS_DISABLE", but then also concluding then
that the enable API is actually redundant if you can also disable pullup
and pulldown separately, and finally settling for
a PIN_CONFIG_BIAS_DISABLE after all (since an existing pinctrl driver
turned out to have it), but without the DT configuration for that.

It could very well be that I'm missing some more complex behavior or API that
needs this, so consider this an open question from a somewhat outsider
perspective, and feel free to ignore it because changing this will need
effort and does not add much except maybe some clarity in code.

[1]: https://github.com/torvalds/linux/blob/45ec2f5f6ed3ec3a79ba1329ad585497cdcbe663/drivers/pinctrl/pinctrl-single.c#L548
[2]: https://www.kernel.org/doc/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt

Gr.

Matthijs

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread