Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <c5c7e071-a645-39a2-c3dc-897173e8c971@postmarketos.org>
Date:   Wed, 8 Jun 2022 11:25:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH 5.18 579/879] pinctrl/rockchip: support deferring other
 gpio params
Content-Language: en-US
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
Cc:     stable@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>,
        Sasha Levin <sashal@kernel.org>
References: <20220607165002.659942637@linuxfoundation.org>
 <20220607165019.660801561@linuxfoundation.org>
From:   Caleb Connolly <kc@postmarketos.org>
In-Reply-To: <20220607165019.660801561@linuxfoundation.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;
>   };
>