pinctrl-sunxi: fix pin attribute handling.

PATCH New Ithamar R. Adema 2 participants 5 messages Jun 20, 2013 – Jun 23, 2013

Subsystems (2)

Arm/Allwinner sunXi SoC support Pin control

lore

Share & tools

lore

Ithamar R. Adema

author

2013-06-20 17:12 UTC #0 lore raw

PATCH 1/1 pinctrl-sunxi: fix pin attribute handling.

From: "Ithamar R. Adema" <[email protected]>

The configuration of pull up/down on pins does not allow disabling of the pins. PIN_CONFIG_BIAS_PULL_UP and PIN_CONFIG_BIAS_PULL_DOWN take an argument of 1 or 0 to specify enabling or disabling of the pull up/down, but the code does not take this into account.

Also, default the pullup/downs to disabled if not specified, so no old state from e.g. the bootloader is still active after reconfiguration by the kernel.

Signed-off-by Ithamar R. Adema <[email protected]>

---
 drivers/pinctrl/pinctrl-sunxi.c |   39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

drivers/pinctrl/pinctrl-sunxi.c +17 ~~-22~~

index b7d8c89..a652061 100644
@@ -1466,7 +1466,7 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	of_property_for_each_string(node, "allwinner,pins", prop, group) {
 		struct sunxi_pinctrl_group *grp =
 			sunxi_pinctrl_find_group_by_name(pctl, group);
-		int j = 0, configlen = 0;
+		u16 strength;
 
 		if (!grp) {
 			dev_err(pctl->dev, "unknown pin %s", group);

@@ -1490,31 +1490,23 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
 		(*map)[i].data.configs.group_or_pin = group;
 
-		if (of_find_property(node, "allwinner,drive", NULL))
-			configlen++;
-		if (of_find_property(node, "allwinner,pull", NULL))
-			configlen++;
-
-		pinconfig = kzalloc(configlen * sizeof(*pinconfig), GFP_KERNEL);
+		pinconfig = kzalloc(3 * sizeof(*pinconfig), GFP_KERNEL);
 
+		strength = 0;
 		if (!of_property_read_u32(node, "allwinner,drive", &val)) {
-			u16 strength = (val + 1) * 10;
-			pinconfig[j++] =
-				pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
-							 strength);
+			strength = (val + 1) * 10;
 		}
+		pinconfig[0] =
+			pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
+						 strength);
 
-		if (!of_property_read_u32(node, "allwinner,pull", &val)) {
-			enum pin_config_param pull = PIN_CONFIG_END;
-			if (val == 1)
-				pull = PIN_CONFIG_BIAS_PULL_UP;
-			else if (val == 2)
-				pull = PIN_CONFIG_BIAS_PULL_DOWN;
-			pinconfig[j++] = pinconf_to_config_packed(pull, 0);
-		}
+		val = 0;
+		of_property_read_u32(node, "allwinner,pull", &val);
+		pinconfig[1] = pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_UP, val == 1);
+		pinconfig[2] = pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_DOWN, val == 2);
 
 		(*map)[i].data.configs.configs = pinconfig;
-		(*map)[i].data.configs.num_configs = configlen;
+		(*map)[i].data.configs.num_configs = 3;
 
 		i++;
 	}

@@ -1563,6 +1555,7 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 {
 	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
 	struct sunxi_pinctrl_group *g = &pctl->groups[group];
+	u32 arg = pinconf_to_config_argument(config);
 	u32 val, mask;
 	u16 strength;
 	u8 dlevel;

@@ -1586,15 +1579,17 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 			pctl->membase + sunxi_dlevel_reg(g->pin));
 		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
+		arg = !!arg;
 		val = readl(pctl->membase + sunxi_pull_reg(g->pin));
 		mask = PULL_PINS_MASK << sunxi_pull_offset(g->pin);
-		writel((val & ~mask) | 1 << sunxi_pull_offset(g->pin),
+		writel((val & ~mask) | arg << sunxi_pull_offset(g->pin),
 			pctl->membase + sunxi_pull_reg(g->pin));
 		break;
 	case PIN_CONFIG_BIAS_PULL_DOWN:
+		arg = !!arg;
 		val = readl(pctl->membase + sunxi_pull_reg(g->pin));
 		mask = PULL_PINS_MASK << sunxi_pull_offset(g->pin);
-		writel((val & ~mask) | 2 << sunxi_pull_offset(g->pin),
+		writel((val & ~mask) | (arg << 1) << sunxi_pull_offset(g->pin),
 			pctl->membase + sunxi_pull_reg(g->pin));
 		break;
 	default:

Signature · 1 line

1.7.9.5

Maxime Ripard

2013-06-20 20:49 UTC #1 lore raw

PATCH 1/1 pinctrl-sunxi: fix pin attribute handling.

Hi Ithamar,

You should probably put in the recipients Linus Walleij that will probably be the one that will merge this patch anyway.

Quoted 42 lines · Ithamar R. Adema

From: "Ithamar R. Adema" <[email protected]>
The configuration of pull up/down on pins does not allow disabling of the pins. PIN_CONFIG_BIAS_PULL_UP and PIN_CONFIG_BIAS_PULL_DOWN take an argument of 1 or 0 to specify enabling or disabling of the pull up/down, but the code does not take this into account.
Also, default the pullup/downs to disabled if not specified, so no old state from e.g. the bootloader is still active after reconfiguration by the kernel.
Signed-off-by Ithamar R. Adema <[email protected]>
---
 drivers/pinctrl/pinctrl-sunxi.c |   39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)
drivers/pinctrl/pinctrl-sunxi.c +3 -7
index b7d8c89..a652061 100644
@@ -1466,7 +1466,7 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	of_property_for_each_string(node, "allwinner,pins", prop, group) {
 		struct sunxi_pinctrl_group *grp =
 			sunxi_pinctrl_find_group_by_name(pctl, group);
-		int j = 0, configlen = 0;
+		u16 strength;
 
 		if (!grp) {
 			dev_err(pctl->dev, "unknown pin %s", group);
@@ -1490,31 +1490,23 @@ static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
 		(*map)[i].data.configs.group_or_pin = group;
 
-		if (of_find_property(node, "allwinner,drive", NULL))
-			configlen++;
-		if (of_find_property(node, "allwinner,pull", NULL))
-			configlen++;
-
-		pinconfig = kzalloc(configlen * sizeof(*pinconfig), GFP_KERNEL);
+		pinconfig = kzalloc(3 * sizeof(*pinconfig), GFP_KERNEL);
 
+		strength = 0;

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev, · line 1501

Quoted 10 lines

drivers/pinctrl/pinctrl-sunxi.c +4 -4

 		if (!of_property_read_u32(node, "allwinner,drive", &val)) {
-			u16 strength = (val + 1) * 10;
-			pinconfig[j++] =
-				pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
-							 strength);
+			strength = (val + 1) * 10;
 		}
+		pinconfig[0] =
+			pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
+						 strength);

Why are you making the configuration of the strength in any cases? It's an optional property, it should be treated as such.

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev, · line 1506

Quoted 12 lines

drivers/pinctrl/pinctrl-sunxi.c +4 -8

-		if (!of_property_read_u32(node, "allwinner,pull", &val)) {
-			enum pin_config_param pull = PIN_CONFIG_END;
-			if (val == 1)
-				pull = PIN_CONFIG_BIAS_PULL_UP;
-			else if (val == 2)
-				pull = PIN_CONFIG_BIAS_PULL_DOWN;
-			pinconfig[j++] = pinconf_to_config_packed(pull, 0);
-		}
+		val = 0;
+		of_property_read_u32(node, "allwinner,pull", &val);
+		pinconfig[1] = pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_UP, val == 1);
+		pinconfig[2] = pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_DOWN, val == 2);

I'm not sure this is actually right. The pins are either configured to have a pull-up, a pull-down, or none of it, in the same register, in the same bitfield, so why adding it in any cases? we only care about at most one of the two, and those two options are mutually exclusive.

Quoted 15 lines

drivers/pinctrl/pinctrl-sunxi.c +2 -1

 		(*map)[i].data.configs.configs = pinconfig;
-		(*map)[i].data.configs.num_configs = configlen;
+		(*map)[i].data.configs.num_configs = 3;
 
 		i++;
 	}

@@ -1563,6 +1555,7 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 {
 	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
 	struct sunxi_pinctrl_group *g = &pctl->groups[group];
+	u32 arg = pinconf_to_config_argument(config);
 	u32 val, mask;
 	u16 strength;
 	u8 dlevel;

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev, · line 1582

Quoted 4 lines

drivers/pinctrl/pinctrl-sunxi.c +1 -0

@@ -1586,15 +1579,17 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 			pctl->membase + sunxi_dlevel_reg(g->pin));
 		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
+		arg = !!arg;

Ouch, nope.

Put this next to the call to pinconf_to_config_argument

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev, · line 1585

Quoted 4 lines

drivers/pinctrl/pinctrl-sunxi.c +1 -1

 		val = readl(pctl->membase + sunxi_pull_reg(g->pin));
 		mask = PULL_PINS_MASK << sunxi_pull_offset(g->pin);
-		writel((val & ~mask) | 1 << sunxi_pull_offset(g->pin),
+		writel((val & ~mask) | arg << sunxi_pull_offset(g->pin),

I'd rather see an obvious assignment of 1 here. It's the value that is documented in the datasheet, it pops to the mind. You can always put this inside an if(arg) statement if you want to.

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev, · line 1592

Quoted 8 lines

drivers/pinctrl/pinctrl-sunxi.c +2 -1

 			pctl->membase + sunxi_pull_reg(g->pin));
 		break;
 	case PIN_CONFIG_BIAS_PULL_DOWN:
+		arg = !!arg;
 		val = readl(pctl->membase + sunxi_pull_reg(g->pin));
 		mask = PULL_PINS_MASK << sunxi_pull_offset(g->pin);
-		writel((val & ~mask) | 2 << sunxi_pull_offset(g->pin),
+		writel((val & ~mask) | (arg << 1) << sunxi_pull_offset(g->pin),

Ditto.

Quoted 3 lines

drivers/pinctrl/pinctrl-sunxi.c +0 -0

 			pctl->membase + sunxi_pull_reg(g->pin));
 		break;
 	default:

Signature · 3 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Ithamar R. Adema

author

2013-06-21 07:41 UTC #2 lore raw

PATCH 1/1 pinctrl-sunxi: fix pin attribute handling.

Dear Maxime,

Quoted 4 lines · Maxime Ripard

Hi Ithamar,
You should probably put in the recipients Linus Walleij that will probably be the one that will merge this patch anyway.

Ah okay, thanks for the heads-up.

Quoted 6 lines

drivers/pinctrl/pinctrl-sunxi.c +1 -0

+		strength = 0;

		if (!of_property_read_u32(node, "allwinner,drive", &val)) {

drivers/pinctrl/pinctrl-sunxi.c +0 -4

-			u16 strength = (val + 1) * 10;
-			pinconfig[j++] =
-				pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
-							 strength);

Quoted 8 lines

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev, · line 1501
Quoted 5 lines
drivers/pinctrl/pinctrl-sunxi.c +1 -0
+			strength = (val + 1) * 10;
		}
drivers/pinctrl/pinctrl-sunxi.c +3 -0
+		pinconfig[0] =
+			pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
+						 strength);
Why are you making the configuration of the strength in any cases? It's an optional property, it should be treated as such.

My reasoning, which I can see is very subjective, is that if those properties (same for pull up/down) are not specified, you still want them to be in a defined state. Setting the drive strength (and pull up/down) to the default 0 value is just so that any configuration that might have been done the kernel started is undone.

I can understand your reasoning here, but are you sure you would want them left in undefined state?

Quoted 10 lines

drivers/pinctrl/pinctrl-sunxi.c +1 -8

-		if (!of_property_read_u32(node, "allwinner,pull", &val)) {
-			enum pin_config_param pull = PIN_CONFIG_END;
-			if (val == 1)
-				pull = PIN_CONFIG_BIAS_PULL_UP;
-			else if (val == 2)
-				pull = PIN_CONFIG_BIAS_PULL_DOWN;
-			pinconfig[j++] = pinconf_to_config_packed(pull, 0);
-		}
+		val = 0;

Quoted 8 lines

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev, · line 1506
Quoted 3 lines
drivers/pinctrl/pinctrl-sunxi.c +3 -0
+		of_property_read_u32(node, "allwinner,pull", &val);
+		pinconfig[1] = pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_UP, val == 1);
+		pinconfig[2] = pinconf_to_config_packed(PIN_CONFIG_BIAS_PULL_DOWN, val == 2);
I'm not sure this is actually right. The pins are either configured to have a pull-up, a pull-down, or none of it, in the same register, in the same bitfield, so why adding it in any cases? we only care about at most one of the two, and those two options are mutually exclusive.

True, this is my fault. I guess I thought that settings the pull up didn't clear the pull down, but after looking at the code, this is properly handled. I'll fix this properly in the next version of the patch.

Quoted 23 lines

Quoted 16 lines

		(*map)[i].data.configs.configs = pinconfig;

drivers/pinctrl/pinctrl-sunxi.c +1 -1

-		(*map)[i].data.configs.num_configs = configlen;
+		(*map)[i].data.configs.num_configs = 3;

		i++;
	}

drivers/pinctrl/pinctrl-sunxi.c +1 -0

@@ -1563,6 +1555,7 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 {
 	struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
 	struct sunxi_pinctrl_group *g = &pctl->groups[group];
+	u32 arg = pinconf_to_config_argument(config);
 	u32 val, mask;
 	u16 strength;
 	u8 dlevel;

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev, · line 1582

Quoted 4 lines

drivers/pinctrl/pinctrl-sunxi.c +1 -0

@@ -1586,15 +1579,17 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 			pctl->membase + sunxi_dlevel_reg(g->pin));
 		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
+		arg = !!arg;

Ouch, nope.

Quoted 1 line

Put this next to the call to pinconf_to_config_argument

Fair enough, will fix.

Quoted 9 lines

drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev, · line 1585
Quoted 5 lines
drivers/pinctrl/pinctrl-sunxi.c +1 -1
 		val = readl(pctl->membase + sunxi_pull_reg(g->pin));
 		mask = PULL_PINS_MASK << sunxi_pull_offset(g->pin);
-		writel((val & ~mask) | 1 << sunxi_pull_offset(g->pin),
+		writel((val & ~mask) | arg << sunxi_pull_offset(g->pin),
I'd rather see an obvious assignment of 1 here. It's the value that is documented in the datasheet, it pops to the mind. You can always put this inside an if(arg) statement if you want to.

Just to make sure I understand, are you suggesting to stretch that writel() onliner into an if (arg) writel() else writel() ?

Thanks for taking the time to review!

Regards,

Ithamar.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4163 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130621/1f7ae23f/attachment-0001.p7s>

Maxime Ripard

2013-06-23 10:20 UTC #3 lore raw

PATCH 1/1 pinctrl-sunxi: fix pin attribute handling.

Hi Ithamar,

Quoted 25 lines · Ithamar R. Adema

Dear Maxime,
Quoted 4 lines · Maxime Ripard
Hi Ithamar,
You should probably put in the recipients Linus Walleij that will probably be the one that will merge this patch anyway.
Ah okay, thanks for the heads-up.
Quoted 6 lines
Quoted 6 lines
drivers/pinctrl/pinctrl-sunxi.c +1 -0
+		strength = 0;
		if (!of_property_read_u32(node, "allwinner,drive", &val)) {
drivers/pinctrl/pinctrl-sunxi.c +0 -4
-			u16 strength = (val + 1) * 10;
-			pinconfig[j++] =
-				pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
-							 strength);
Quoted 8 lines
drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev, · line 1501
Quoted 5 lines
drivers/pinctrl/pinctrl-sunxi.c +1 -0
+			strength = (val + 1) * 10;
		}
drivers/pinctrl/pinctrl-sunxi.c +3 -0
+		pinconfig[0] =
+			pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH,
+						 strength);
Why are you making the configuration of the strength in any cases? It's an optional property, it should be treated as such.

Quoted 8 lines

My reasoning, which I can see is very subjective, is that if those properties (same for pull up/down) are not specified, you still want them to be in a defined state. Setting the drive strength (and pull up/down) to the default 0 value is just so that any configuration that might have been done the kernel started is undone.
I can understand your reasoning here, but are you sure you would want them left in undefined state?

While your reasoning is correct here, the point is that the platform is still a little obscure, with not a lot of documentation and so on. So relying on the bootloader to do some muxing could still prove useful.

Quoted 10 lines

Quoted 9 lines
drivers/pinctrl/pinctrl-sunxi.c static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev, · line 1585
Quoted 5 lines
		val = readl(pctl->membase + sunxi_pull_reg(g->pin));
		mask = PULL_PINS_MASK << sunxi_pull_offset(g->pin);
drivers/pinctrl/pinctrl-sunxi.c +1 -1
-		writel((val & ~mask) | 1 << sunxi_pull_offset(g->pin),
+		writel((val & ~mask) | arg << sunxi_pull_offset(g->pin),
I'd rather see an obvious assignment of 1 here. It's the value that is documented in the datasheet, it pops to the mind. You can always put this inside an if(arg) statement if you want to.

Quoted 2 lines

Just to make sure I understand, are you suggesting to stretch that writel() onliner into an if (arg) writel() else writel() ?

Yes, something like

case PULL_UP:

    val = readl(pctl->membase + sunxi_pull_reg(g->pin));
    mask = PULL_PINS_MASK << sunxi_pull_offset(g->pin);
    if (arg)
	writel((val & ~mask) | 1 << sunxi_pull_offset(g->pin), reg);
    else
	writel(val & ~mask, reg);

Maxime

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130623/02b42f03/attachment.sig>

Ithamar R. Adema

author

2013-06-23 10:24 UTC #4 lore raw

PATCH 1/1 pinctrl-sunxi: fix pin attribute handling.

Dear Maxime,

Quoted 3 lines · Maxime Ripard

While your reasoning is correct here, the point is that the platform is still a little obscure, with not a lot of documentation and so on. So relying on the bootloader to do some muxing could still prove useful.

Fair enough, I'll adjust this for sunxi then. The reason I'm asking is because I used this code for another architecture I hope to mainline (telechips) and I just wanted to know if this was some requirement of the subsystem or more related to the platform. Thanks for your explanation.

Quoted 5 lines

Quoted 2 lines
Just to make sure I understand, are you suggesting to stretch that writel() onliner into an if (arg) writel() else writel() ?
Yes, something like

Quoted 7 lines

case PULL_UP:

   val = readl(pctl->membase + sunxi_pull_reg(g->pin));
   mask = PULL_PINS_MASK << sunxi_pull_offset(g->pin);
   if (arg)
	writel((val & ~mask) | 1 << sunxi_pull_offset(g->pin), reg);
   else
	writel(val & ~mask, reg);

Okay, clear.

I'll send a new patch along fixes the mentioned issues as suggested.

Thanks!

Regards,

Ithamar.