On Tue, Mar 19, 2024 at 12:06:34PM +0100, Matthijs Kooijman wrote:
> The pinctrl-single driver handles pin_config_set by looking up the
> requested setting in a DT-defined lookup table, which defines what bits
> correspond to each setting. There is no way to add
> PIN_CONFIG_BIAS_DISABLE entries to the table, since there is instead
> code to disable the bias by applying the disable values of both the
> pullup and pulldown entries in the table.
>
> However, this code is inside the table-lookup loop, so it would only
> execute if there is an entry for PIN_CONFIG_BIAS_DISABLE in the table,
> which can never exist, so this code never runs.
>
> This commit lifts the offending code out of the loop, so it just
> executes directly whenever PIN_CONFIG_BIAS_DISABLE is requested,
> skippipng the table lookup loop.
>
> This also introduces a new `param` variable to make the code slightly
> more readable.
>
> This bug seems to have existed when this code was first merged in commit
> 9dddb4df90d13 ("pinctrl: single: support generic pinconf"). Earlier
> versions of this patch did have an entry for PIN_CONFIG_BIAS_DISABLE in
> the lookup table, but that was removed, which is probably how this bug
> was introduced.
>
> Signed-off-by: Matthijs Kooijman <matthijs@xxxxxxxx>
> ---
> drivers/pinctrl/pinctrl-single.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 19cc0db771a5a..c7a03b63fa812 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -554,21 +554,30 @@ static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
> unsigned offset = 0, shift = 0, i, data, ret;
> u32 arg;
> int j;
> + enum pin_config_param param;
>
> ret = pcs_get_function(pctldev, pin, &func);
> if (ret)
> return ret;
>
> for (j = 0; j < num_configs; j++) {
> + param = pinconf_to_config_param(configs[j]);
> +
> + /* BIAS_DISABLE has no entry in the func->conf table */
> + if (param == PIN_CONFIG_BIAS_DISABLE) {
> + /* This just disables all bias entries */
> + pcs_pinconf_clear_bias(pctldev, pin);
> + continue;
> + }
> +
> for (i = 0; i < func->nconfs; i++) {
> - if (pinconf_to_config_param(configs[j])
> - != func->conf[i].param)
> + if (param != func->conf[i].param)
> continue;
>
> offset = pin * (pcs->width / BITS_PER_BYTE);
> data = pcs->read(pcs->base + offset);
> arg = pinconf_to_config_argument(configs[j]);
> - switch (func->conf[i].param) {
> + switch (param) {
> /* 2 parameters */
> case PIN_CONFIG_INPUT_SCHMITT:
> case PIN_CONFIG_DRIVE_STRENGTH:
> @@ -580,9 +589,6 @@ static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
> data |= (arg << shift) & func->conf[i].mask;
> break;
> /* 4 parameters */
> - case PIN_CONFIG_BIAS_DISABLE:
> - pcs_pinconf_clear_bias(pctldev, pin);
> - break;
> case PIN_CONFIG_BIAS_PULL_DOWN:
> case PIN_CONFIG_BIAS_PULL_UP:
> if (arg)
> --
> 2.40.1
>
Reviewed-by: Haojian Zhuang <haojian.zhuang@xxxxxxxxxx>
