* Re: [PATCH] pinctrl: single: Fix PIN_CONFIG_BIAS_DISABLE handling
2024-03-19 11:06 ` [PATCH] pinctrl: single: Fix PIN_CONFIG_BIAS_DISABLE handling Matthijs Kooijman
@ 2024-03-28 21:02 ` Linus Walleij
2024-04-03 8:59 ` Tony Lindgren
2024-03-29 2:05 ` Haojian Zhuang
2024-04-04 12:00 ` Linus Walleij
2 siblings, 1 reply; 5+ messages in thread
From: Linus Walleij @ 2024-03-28 21:02 UTC (permalink / raw)
To: Matthijs Kooijman; +Cc: Haojian Zhuang, Tony Lindgren, linux-gpio, linux-omap
On Tue, Mar 19, 2024 at 12:07 PM Matthijs Kooijman <matthijs@stdin.nl> 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@stdin.nl>
This looks reasonable to me, but I need Tony to review it before applying.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] pinctrl: single: Fix PIN_CONFIG_BIAS_DISABLE handling
2024-03-28 21:02 ` Linus Walleij
@ 2024-04-03 8:59 ` Tony Lindgren
0 siblings, 0 replies; 5+ messages in thread
From: Tony Lindgren @ 2024-04-03 8:59 UTC (permalink / raw)
To: Linus Walleij; +Cc: Matthijs Kooijman, Haojian Zhuang, linux-gpio, linux-omap
* Linus Walleij <linus.walleij@linaro.org> [240328 21:02]:
> On Tue, Mar 19, 2024 at 12:07 PM Matthijs Kooijman <matthijs@stdin.nl> 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@stdin.nl>
>
> This looks reasonable to me, but I need Tony to review it before applying.
Looks good to me:
Reviewed-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pinctrl: single: Fix PIN_CONFIG_BIAS_DISABLE handling
2024-03-19 11:06 ` [PATCH] pinctrl: single: Fix PIN_CONFIG_BIAS_DISABLE handling Matthijs Kooijman
2024-03-28 21:02 ` Linus Walleij
@ 2024-03-29 2:05 ` Haojian Zhuang
2024-04-04 12:00 ` Linus Walleij
2 siblings, 0 replies; 5+ messages in thread
From: Haojian Zhuang @ 2024-03-29 2:05 UTC (permalink / raw)
To: Matthijs Kooijman; +Cc: Tony Lindgren, linux-gpio, Linus Walleij, linux-omap
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@stdin.nl>
> ---
> 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@linaro.org>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] pinctrl: single: Fix PIN_CONFIG_BIAS_DISABLE handling
2024-03-19 11:06 ` [PATCH] pinctrl: single: Fix PIN_CONFIG_BIAS_DISABLE handling Matthijs Kooijman
2024-03-28 21:02 ` Linus Walleij
2024-03-29 2:05 ` Haojian Zhuang
@ 2024-04-04 12:00 ` Linus Walleij
2 siblings, 0 replies; 5+ messages in thread
From: Linus Walleij @ 2024-04-04 12:00 UTC (permalink / raw)
To: Matthijs Kooijman; +Cc: Haojian Zhuang, Tony Lindgren, linux-gpio, linux-omap
On Tue, Mar 19, 2024 at 12:07 PM Matthijs Kooijman <matthijs@stdin.nl> 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@stdin.nl>
Patch applied!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 5+ messages in thread