Re: [PATCH] pinctrl: single: Fix PIN_CONFIG_BIAS_DISABLE handling

[Haojian Zhuang <haojian.zhuang@xxxxxxxxxx>]

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>