Re: [PATCH 5.18 579/879] pinctrl/rockchip: support deferring other gpio params

Caleb Connolly
2022-06-08T10:44:27Z

Hi Greg,

This commit contains a bug which was fixed in commit
42d90a1e5caf ("pinctrl/rockchip: support setting input-enable param")

It uses bitwise OR to check non-bitwise values (see below).

On 07/06/2022 18:01, Greg Kroah-Hartman wrote:
> From: Caleb Connolly <kc@postmarketos.org>
>
> [ Upstream commit 8ce5ef64546850294b021497046588a7abcebe96 ]
>
> Add support for deferring other params like PIN_CONFIG_INPUT_ENABLE.
> This will be used to add support for PIN_CONFIG_INPUT_ENABLE to the
> driver.
>
> Fixes: e7165b1dff06 ("pinctrl/rockchip: add a queue for deferred pin output settings on probe")
> Fixes: 59dd178e1d7c ("gpio/rockchip: fetch deferred output settings on probe")
> Signed-off-by: Caleb Connolly <kc@postmarketos.org>
> Link: https://lore.kernel.org/r/20220328005005.72492-2-kc@postmarketos.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/gpio/gpio-rockchip.c | 24 ++++++++-----
> drivers/pinctrl/pinctrl-rockchip.c | 54 ++++++++++++++++--------------
> drivers/pinctrl/pinctrl-rockchip.h | 7 ++--
> 3 files changed, 50 insertions(+), 35 deletions(-)

[snip]

> @@ -2143,6 +2144,25 @@ static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> param = pinconf_to_config_param(configs[i]);
> arg = pinconf_to_config_argument(configs[i]);
>
> + if (param == (PIN_CONFIG_OUTPUT | PIN_CONFIG_INPUT_ENABLE)) {
Should be if (param == PIN_CONFIG_OUTPUT || param == PIN_CONFIG_INPUT_ENABLE) {
> + /*
> + * Check for gpio driver not being probed yet.
> + * The lock makes sure that either gpio-probe has completed
> + * or the gpio driver hasn't probed yet.
> + */
> + mutex_lock(&bank->deferred_lock);
> + if (!gpio || !gpio->direction_output) {
> + rc = rockchip_pinconf_defer_pin(bank, pin - bank->pin_base, param,
> + arg);
> + mutex_unlock(&bank->deferred_lock);
> + if (rc)
> + return rc;
> +
> + break;
> + }
> + mutex_unlock(&bank->deferred_lock);
> + }
> +
> switch (param) {
> case PIN_CONFIG_BIAS_DISABLE:
> rc = rockchip_set_pull(bank, pin - bank->pin_base,
> @@ -2171,22 +2191,6 @@ static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> if (rc != RK_FUNC_GPIO)
> return -EINVAL;
>
> - /*
> - * Check for gpio driver not being probed yet.
> - * The lock makes sure that either gpio-probe has completed
> - * or the gpio driver hasn't probed yet.
> - */
> - mutex_lock(&bank->deferred_lock);
> - if (!gpio || !gpio->direction_output) {
> - rc = rockchip_pinconf_defer_output(bank, pin - bank->pin_base, arg);
> - mutex_unlock(&bank->deferred_lock);
> - if (rc)
> - return rc;
> -
> - break;
> - }
> - mutex_unlock(&bank->deferred_lock);
> -
> rc = gpio->direction_output(gpio, pin - bank->pin_base,
> arg);
> if (rc)
> @@ -2500,7 +2504,7 @@ static int rockchip_pinctrl_register(struct platform_device *pdev,
> pdesc++;
> }
>
> - INIT_LIST_HEAD(&pin_bank->deferred_output);
> + INIT_LIST_HEAD(&pin_bank->deferred_pins);
> mutex_init(&pin_bank->deferred_lock);
> }
>
> @@ -2763,7 +2767,7 @@ static int rockchip_pinctrl_remove(struct platform_device *pdev)
> {
> struct rockchip_pinctrl *info = platform_get_drvdata(pdev);
> struct rockchip_pin_bank *bank;
> - struct rockchip_pin_output_deferred *cfg;
> + struct rockchip_pin_deferred *cfg;
> int i;
>
> of_platform_depopulate(&pdev->dev);
> @@ -2772,9 +2776,9 @@ static int rockchip_pinctrl_remove(struct platform_device *pdev)
> bank = &info->ctrl->pin_banks[i];
>
> mutex_lock(&bank->deferred_lock);
> - while (!list_empty(&bank->deferred_output)) {
> - cfg = list_first_entry(&bank->deferred_output,
> - struct rockchip_pin_output_deferred, head);
> + while (!list_empty(&bank->deferred_pins)) {
> + cfg = list_first_entry(&bank->deferred_pins,
> + struct rockchip_pin_deferred, head);
> list_del(&cfg->head);
> kfree(cfg);
> }
> diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
> index 91f10279d084..98a01a616da6 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.h
> +++ b/drivers/pinctrl/pinctrl-rockchip.h
> @@ -171,7 +171,7 @@ struct rockchip_pin_bank {
> u32 toggle_edge_mode;
> u32 recalced_mask;
> u32 route_mask;
> - struct list_head deferred_output;
> + struct list_head deferred_pins;
> struct mutex deferred_lock;
> };
>
> @@ -247,9 +247,12 @@ struct rockchip_pin_config {
> unsigned int nconfigs;
> };
>
> -struct rockchip_pin_output_deferred {
> +enum pin_config_param;
> +
> +struct rockchip_pin_deferred {
> struct list_head head;
> unsigned int pin;
> + enum pin_config_param param;
> u32 arg;
> };
>