Re: [PATCH v2 2/6] pinctrl: Ingenic: Add support for read the pin configuration of X1830.
Hi Paul,
On 2021/3/12 下午9:31, Paul Cercueil wrote:
Hi Zhou,
Le jeu. 11 mars 2021 à 23:21, 周琰杰 (Zhou Yanjie)
a écrit :
Add X1830 support in "ingenic_pinconf_get()", so that it can read the
configuration of X1830 SoC correctly.
Signed-off-by: 周琰杰 (Zhou Yanjie)
This is a fix, so it needs a Fixes: tag, and you need to Cc linux-stable.
Sure.
---
Notes:
v2:
New patch.
drivers/pinctrl/pinctrl-ingenic.c | 76
+--
1 file changed, 57 insertions(+), 19 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-ingenic.c
b/drivers/pinctrl/pinctrl-ingenic.c
index 05dfa0a..0a88aab 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -2109,31 +2109,69 @@ static int ingenic_pinconf_get(struct
pinctrl_dev *pctldev,
enum pin_config_param param = pinconf_to_config_param(*config);
unsigned int idx = pin % PINS_PER_GPIO_CHIP;
unsigned int offt = pin / PINS_PER_GPIO_CHIP;
+ unsigned int bias;
bool pull;
- if (jzpc->info->version >= ID_JZ4770)
- pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN);
- else
- pull = !ingenic_get_pin_config(jzpc, pin,
JZ4740_GPIO_PULL_DIS);
+ if (jzpc->info->version >= ID_X1830) {
+ unsigned int half = PINS_PER_GPIO_CHIP / 2;
+ unsigned int idxh = pin % half * 2;
- switch (param) {
- case PIN_CONFIG_BIAS_DISABLE:
- if (pull)
- return -EINVAL;
- break;
+ if (idx < half)
+ regmap_read(jzpc->map, offt * jzpc->info->reg_offset +
+ X1830_GPIO_PEL, &bias);
+ else
+ regmap_read(jzpc->map, offt * jzpc->info->reg_offset +
+ X1830_GPIO_PEH, &bias);
- case PIN_CONFIG_BIAS_PULL_UP:
- if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx)))
- return -EINVAL;
- break;
+ bias = (bias >> idxh) & 3;
You can do:
u32 mask = GENMASK(idxh + 1, idxh);
bias = FIELD_GET(mask, bias);
(macros in )
Sure.
- case PIN_CONFIG_BIAS_PULL_DOWN:
- if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx)))
- return -EINVAL;
- break;
+ switch (param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ if (bias)
+ return -EINVAL;
+ break;
- default:
- return -ENOTSUPP;
+ case PIN_CONFIG_BIAS_PULL_UP:
+ if ((bias != PIN_CONFIG_BIAS_PULL_UP) ||
+ !(jzpc->info->pull_ups[offt] & BIT(idx)))
"bias" is a 2-bit value (because of the & 3 mask), and
PIN_CONFIG_BIAS_PULL_UP == 5.
So this clearly won't work. You are comparing hardware values with
public API enums.
OK, I will fix it in the next version.
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ if ((bias != PIN_CONFIG_BIAS_PULL_DOWN) ||
+ !(jzpc->info->pull_downs[offt] & BIT(idx)))
+ return -EINVAL;
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ } else {
+ if (jzpc->info->version >= ID_JZ4770)
+ pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN);
+ else
+ pull = !ingenic_get_pin_config(jzpc, pin,
JZ4740_GPIO_PULL_DIS);
I think you can keep the switch outside the if/else block, if you use
pullup/pulldown variables.
These can be initialized (in the non-X1830 case) to:
pullup = pull && (jzpc->info->pull_ups[offt] & BIT(idx));
pulldown = pull && (jzpc->info->pull_downs[offt] & BIT(idx));
In the X1830 case you'd initialize these variables from 'bias'.
Sure, I will do this in the next version.
+
+ switch (param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ if (pull)
Here would change to if (pullup || pulldown)
OK.
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_UP:
+ if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx)))
if (!pullup)
Sure.
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx)))
if (!pulldown)
Sure.
Thanks and best regards!
Cheers,
-Paul
+ return -EINVAL;
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
}
*config = pinconf_to_config_packed(param, 1);
--
2.7.4
Re: [PATCH v2 2/6] pinctrl: Ingenic: Add support for read the pin configuration of X1830.
Hi Zhou,
Le jeu. 11 mars 2021 à 23:21, 周琰杰 (Zhou Yanjie)
a écrit :
Add X1830 support in "ingenic_pinconf_get()", so that it can read the
configuration of X1830 SoC correctly.
Signed-off-by: 周琰杰 (Zhou Yanjie)
This is a fix, so it needs a Fixes: tag, and you need to Cc
linux-stable.
---
Notes:
v2:
New patch.
drivers/pinctrl/pinctrl-ingenic.c | 76
+--
1 file changed, 57 insertions(+), 19 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-ingenic.c
b/drivers/pinctrl/pinctrl-ingenic.c
index 05dfa0a..0a88aab 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -2109,31 +2109,69 @@ static int ingenic_pinconf_get(struct
pinctrl_dev *pctldev,
enum pin_config_param param = pinconf_to_config_param(*config);
unsigned int idx = pin % PINS_PER_GPIO_CHIP;
unsigned int offt = pin / PINS_PER_GPIO_CHIP;
+ unsigned int bias;
bool pull;
- if (jzpc->info->version >= ID_JZ4770)
- pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN);
- else
- pull = !ingenic_get_pin_config(jzpc, pin, JZ4740_GPIO_PULL_DIS);
+ if (jzpc->info->version >= ID_X1830) {
+ unsigned int half = PINS_PER_GPIO_CHIP / 2;
+ unsigned int idxh = pin % half * 2;
- switch (param) {
- case PIN_CONFIG_BIAS_DISABLE:
- if (pull)
- return -EINVAL;
- break;
+ if (idx < half)
+ regmap_read(jzpc->map, offt * jzpc->info->reg_offset +
+ X1830_GPIO_PEL, &bias);
+ else
+ regmap_read(jzpc->map, offt * jzpc->info->reg_offset +
+ X1830_GPIO_PEH, &bias);
- case PIN_CONFIG_BIAS_PULL_UP:
- if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx)))
- return -EINVAL;
- break;
+ bias = (bias >> idxh) & 3;
You can do:
u32 mask = GENMASK(idxh + 1, idxh);
bias = FIELD_GET(mask, bias);
(macros in )
- case PIN_CONFIG_BIAS_PULL_DOWN:
- if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx)))
- return -EINVAL;
- break;
+ switch (param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ if (bias)
+ return -EINVAL;
+ break;
- default:
- return -ENOTSUPP;
+ case PIN_CONFIG_BIAS_PULL_UP:
+ if ((bias != PIN_CONFIG_BIAS_PULL_UP) ||
+ !(jzpc->info->pull_ups[offt] &
BIT(idx)))
"bias" is a 2-bit value (because of the & 3 mask), and
PIN_CONFIG_BIAS_PULL_UP == 5.
So this clearly won't work. You are comparing hardware values with
public API enums.
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ if ((bias != PIN_CONFIG_BIAS_PULL_DOWN) ||
+ !(jzpc->info->pull_downs[offt] &
BIT(idx)))
+ return -EINVAL;
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ } else {
+ if (jzpc->info->version >= ID_JZ4770)
+ pull = !ingenic_get_pin_config(jzpc, pin,
JZ4770_GPIO_PEN);
+ else
+ pull = !ingenic_get_pin_config(jzpc, pin,
JZ4740_GPIO_PULL_DIS);
I think you can keep the switch outside the if/else block, if you use
pullup/pulldown variables.
These can be initialized (in the non-X1830 case) to:
pullup = pull && (jzpc->info->pull_ups[offt] & BIT(idx));
pulldown = pull && (jzpc->info->pull_downs[offt] & BIT(idx));
In the X1830 case you'd initialize these variables from 'bias'.
+
+ switch (param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ if (pull)
Here would change to if (pullup || pulldown)
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_UP:
+ if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx)))
if (!pullup)
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx)))
if (!pulldown)
Cheers,
-Paul
+ return -EINVAL;
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
}
*config = pinconf_to_config_packed(param, 1);
--
2.7.4
[PATCH v2 2/6] pinctrl: Ingenic: Add support for read the pin configuration of X1830.
Add X1830 support in "ingenic_pinconf_get()", so that it can read the
configuration of X1830 SoC correctly.
Signed-off-by: 周琰杰 (Zhou Yanjie)
---
Notes:
v2:
New patch.
drivers/pinctrl/pinctrl-ingenic.c | 76 +--
1 file changed, 57 insertions(+), 19 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-ingenic.c
b/drivers/pinctrl/pinctrl-ingenic.c
index 05dfa0a..0a88aab 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -2109,31 +2109,69 @@ static int ingenic_pinconf_get(struct pinctrl_dev
*pctldev,
enum pin_config_param param = pinconf_to_config_param(*config);
unsigned int idx = pin % PINS_PER_GPIO_CHIP;
unsigned int offt = pin / PINS_PER_GPIO_CHIP;
+ unsigned int bias;
bool pull;
- if (jzpc->info->version >= ID_JZ4770)
- pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN);
- else
- pull = !ingenic_get_pin_config(jzpc, pin, JZ4740_GPIO_PULL_DIS);
+ if (jzpc->info->version >= ID_X1830) {
+ unsigned int half = PINS_PER_GPIO_CHIP / 2;
+ unsigned int idxh = pin % half * 2;
- switch (param) {
- case PIN_CONFIG_BIAS_DISABLE:
- if (pull)
- return -EINVAL;
- break;
+ if (idx < half)
+ regmap_read(jzpc->map, offt * jzpc->info->reg_offset +
+ X1830_GPIO_PEL, &bias);
+ else
+ regmap_read(jzpc->map, offt * jzpc->info->reg_offset +
+ X1830_GPIO_PEH, &bias);
- case PIN_CONFIG_BIAS_PULL_UP:
- if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx)))
- return -EINVAL;
- break;
+ bias = (bias >> idxh) & 3;
- case PIN_CONFIG_BIAS_PULL_DOWN:
- if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx)))
- return -EINVAL;
- break;
+ switch (param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ if (bias)
+ return -EINVAL;
+ break;
- default:
- return -ENOTSUPP;
+ case PIN_CONFIG_BIAS_PULL_UP:
+ if ((bias != PIN_CONFIG_BIAS_PULL_UP) ||
+ !(jzpc->info->pull_ups[offt] &
BIT(idx)))
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ if ((bias != PIN_CONFIG_BIAS_PULL_DOWN) ||
+ !(jzpc->info->pull_downs[offt] &
BIT(idx)))
+ return -EINVAL;
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ } else {
+ if (jzpc->info->version >= ID_JZ4770)
+ pull = !ingenic_get_pin_config(jzpc, pin,
JZ4770_GPIO_PEN);
+ else
+ pull = !ingenic_get_pin_config(jzpc, pin,
JZ4740_GPIO_PULL_DIS);
+
+ switch (param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ if (pull)
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_UP:
+ if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx)))
+ return -EINVAL;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx)))
+ return -EINVAL;
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
}
*config = pinconf_to_config_packed(param, 1);
--
2.7.4
