[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <06791ae0-a4fb-d230-da0c-ced84d8ebe93@linux.alibaba.com>
Date: Tue, 12 Sep 2023 16:22:01 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Linhua Xu <Linhua.xu@...soc.com>,
Linus Walleij <linus.walleij@...aro.org>
Cc: Orson Zhai <orsonzhai@...il.com>,
Chunyan Zhang <zhang.lyra@...il.com>,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
lh xu <xulh0829@...il.com>,
Zhirong Qiu <zhirong.qiu@...soc.com>,
Xiongpeng Wu <xiongpeng.wu@...soc.com>
Subject: Re: [PATCH V2 3/6] pinctrl: sprd: Modify pull-up parameters
On 9/8/2023 1:51 PM, Linhua Xu wrote:
> From: Linhua Xu <Linhua.Xu@...soc.com>
>
> For UNISOC pin controller, there are three different configurations of
> pull-up drive current. Modify the 20K pull-up resistor configuration and
> add the 1.8K pull-up resistor configuration.
>
> Signed-off-by: Linhua Xu <Linhua.Xu@...soc.com>
Please add a Fixes tag.
> ---
> drivers/pinctrl/sprd/pinctrl-sprd.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/sprd/pinctrl-sprd.c b/drivers/pinctrl/sprd/pinctrl-sprd.c
> index 5b9126b2cde2..6c75e6b8d2ca 100644
> --- a/drivers/pinctrl/sprd/pinctrl-sprd.c
> +++ b/drivers/pinctrl/sprd/pinctrl-sprd.c
> @@ -69,7 +69,8 @@
> #define SLEEP_PULL_UP_MASK GENMASK(1, 0)
> #define SLEEP_PULL_UP_SHIFT 2
>
> -#define PULL_UP_4_7K (BIT(12) | BIT(7))
> +#define PULL_UP_1_8K (BIT(12) | BIT(7))
> +#define PULL_UP_4_7K BIT(12)
> #define PULL_UP_20K BIT(7)
> #define PULL_UP_MASK (GENMASK(1, 0) | BIT(6))
> #define PULL_UP_SHIFT 6
> @@ -499,7 +500,7 @@ static int sprd_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin_id,
> break;
> case PIN_CONFIG_BIAS_DISABLE:
> if ((reg & (SLEEP_PULL_DOWN | SLEEP_PULL_UP)) ||
> - (reg & (PULL_DOWN | PULL_UP_4_7K | PULL_UP_20K)))
> + (reg & (PULL_DOWN | PULL_UP_1_8K)))
The math logic is correct, but the code is not readable now. Since we
should mask all PULL UP configration, but the code only mask 'PULL_UP_1_8K'.
So changing to below will be more clear:
(reg & (PULL_DOWN | PULL_UP_4_7K | PULL_UP_20K | PULL_UP_1_8K)
> return -EINVAL;
>
> arg = 1;
> @@ -701,6 +702,8 @@ static int sprd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin_id,
> val |= PULL_UP_20K;
> else if (arg == 4700)
> val |= PULL_UP_4_7K;
> + else if (arg == 1800)
> + val |= PULL_UP_1_8K;
>
> mask = PULL_UP_MASK;
> shift = PULL_UP_SHIFT;
> @@ -712,8 +715,7 @@ static int sprd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin_id,
> mask = SLEEP_PULL_DOWN | SLEEP_PULL_UP;
> } else {
> val = shift = 0;
> - mask = PULL_DOWN | PULL_UP_20K |
> - PULL_UP_4_7K;
> + mask = PULL_DOWN | PULL_UP_1_8K;
ditto.
> }
> break;
> case PIN_CONFIG_SLEEP_HARDWARE_STATE:
Powered by blists - more mailing lists