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@xxxxxxxxxxx/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:
signature.asc
Description: PGP signature