2023-10-27 07:17:29

by Linhua Xu

Subject: [PATCH V3 2/6] pinctrl: sprd: Fix the incorrect mask and shift definition

From: Linhua Xu <[email protected]>

Pull-up and pull-down are mutually exclusive. When setting one of them,
the bit of the other needs to be clear. Now, there are cases where pull-up
and pull-down are set at the same time in the code, thus fix them.

Fixes:<1fb4b907e808> ("pinctrl: sprd: Add Spreadtrum pin control driver")

Signed-off-by: Linhua Xu <[email protected]>
---
drivers/pinctrl/sprd/pinctrl-sprd.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/sprd/pinctrl-sprd.c b/drivers/pinctrl/sprd/pinctrl-sprd.c
index 74d8f8c3b9b6..b7a3cb9e7a61 100644
--- a/drivers/pinctrl/sprd/pinctrl-sprd.c
+++ b/drivers/pinctrl/sprd/pinctrl-sprd.c
@@ -60,22 +60,22 @@
#define DRIVE_STRENGTH_SHIFT 19

#define SLEEP_PULL_DOWN BIT(2)
-#define SLEEP_PULL_DOWN_MASK 0x1
+#define SLEEP_PULL_DOWN_MASK GENMASK(1, 0)
#define SLEEP_PULL_DOWN_SHIFT 2

#define PULL_DOWN BIT(6)
-#define PULL_DOWN_MASK 0x1
+#define PULL_DOWN_MASK (GENMASK(1, 0) | BIT(6))
#define PULL_DOWN_SHIFT 6

#define SLEEP_PULL_UP BIT(3)
-#define SLEEP_PULL_UP_MASK 0x1
-#define SLEEP_PULL_UP_SHIFT 3
+#define SLEEP_PULL_UP_MASK GENMASK(1, 0)
+#define SLEEP_PULL_UP_SHIFT 2

#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 0x21
-#define PULL_UP_SHIFT 7
+#define PULL_UP_MASK (GENMASK(1, 0) | BIT(6))
+#define PULL_UP_SHIFT 6

#define INPUT_SCHMITT BIT(11)
#define INPUT_SCHMITT_MASK 0x1
--
2.17.1

2023-10-27 11:33:13

by Andy Shevchenko

Subject: Re: [PATCH V3 2/6] pinctrl: sprd: Fix the incorrect mask and shift definition

On Fri, Oct 27, 2023 at 03:14:22PM +0800, Linhua Xu wrote:
> From: Linhua Xu <[email protected]>
>
> Pull-up and pull-down are mutually exclusive. When setting one of them,
> the bit of the other needs to be clear. Now, there are cases where pull-up
> and pull-down are set at the same time in the code, thus fix them.

...

> Fixes:<1fb4b907e808> ("pinctrl: sprd: Add Spreadtrum pin control driver")
>
> Signed-off-by: Linhua Xu <[email protected]>

Same comment about the Fixes: tag.

...

> -#define SLEEP_PULL_DOWN_MASK 0x1
> +#define SLEEP_PULL_DOWN_MASK GENMASK(1, 0)
> #define SLEEP_PULL_DOWN_SHIFT 2

No, this is an incorrect (prone to errors and confusion) change.
You need to introduce new mask for both of them and use in the code.

#define SLEEP_PULL_UP_DOWN_MASK GENMASK(1, 0)
#define SLEEP_PULL_UP_DOWN_SHIFT 2

...

> -#define PULL_DOWN_MASK 0x1
> +#define PULL_DOWN_MASK (GENMASK(1, 0) | BIT(6))
> #define PULL_DOWN_SHIFT 6

Ditto.

#define PULL_UP_DOWN_MASK (GENMASK(1, 0) | BIT(6))
#define PULL_UP_DOWN_SHIFT 6

--
With Best Regards,
Andy Shevchenko