pinctrl/nomadik: Add "ste,config" property

Commit Message

Linus Walleij Jan. 4, 2013, 4:13 p.m. UTC

From: Gabriel Fernandez <[e-mail]>

The "ste,config" property will contain the pin config node.
It will be easier to define a pin configuration and use it by
reference without duplicating lines tedious.

Signed-off-by: Gabriel Fernandez <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
---
This is made as an add-on patch to the first device-tree patch
instead of respinning it for ease-of-review. We may squash it in
the end if this approach is OK.
---
 .../devicetree/bindings/pinctrl/ste,nomadik.txt    | 56 +++++++++++++++-------
 drivers/pinctrl/pinctrl-nomadik.c                  | 24 ++++++----
 2 files changed, 53 insertions(+), 27 deletions(-)

Comments

Stephen Warren Jan. 4, 2013, 8:02 p.m. UTC | #1

On 01/04/2013 09:13 AM, Linus Walleij wrote:
> From: Gabriel Fernandez <[e-mail]>
> 
> The "ste,config" property will contain the pin config node.
> It will be easier to define a pin configuration and use it by
> reference without duplicating lines tedious.
> 
> Signed-off-by: Gabriel Fernandez <[e-mail]>
> Signed-off-by: Linus Walleij <[e-mail]>

> diff --git a/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt b/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt

> -- ste,input : <0/1/2/3>
> +- ste,config: Handle of pin configuration node (ste,config = <&in_pu>)
> +
> +- ste,input : <0/1/2>

The changes to ste,input and ste,sleep-output look like some unrelated
change.

I guess the idea of ste,input is quite neat, but ...

>  Example board file extract:
>  
> +	in_pu: input_pull_up {
> +		ste,input = <1>;
> +	};

... these nodes shouldn't be placed at the top-level of the device tree;
housing them inside the pin controller node itself makes much more sense
since the pin controller binding is able/allowed to define what goes
inside the pin controller node, but shouldn't influence the top-level of
the device tree.

> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c

> +	np_config = of_parse_phandle(np, "ste,config", 0);
> +	if (np_config) {
> +		for (i = 0; i < ARRAY_SIZE(nmk_cfg_params); i++) {
> +			unsigned long cfg = 0;
> +			int val;

Is it worth making ste,config optional, so that config properties can be
placed either into the node directly, or into a node referenced by
ste,config? That might make doing one-off unusual configurations easier
- no need to create a separate node that's only used once. Still, if
that's unlikely on your HW, it's probably no big deal.

Linus Walleij Jan. 8, 2013, 11:02 a.m. UTC | #2

On Fri, Jan 4, 2013 at 9:02 PM, Stephen Warren <[e-mail]> wrote:

>> -- ste,input : <0/1/2/3>
>> +- ste,config: Handle of pin configuration node (ste,config = <&in_pu>)
>> +
>> +- ste,input : <0/1/2>
>
> The changes to ste,input and ste,sleep-output look like some unrelated
> change.
>
> I guess the idea of ste,input is quite neat, but ...

I'll squash this whole thing into the patch with the device tree enablement,
so see it as a revieweing convenience that we only send the diff :-)

I'll send out a new version with all issues fixed.

Yours,
Linus Walleij

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt b/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt
index 8639234..8ee860c 100644
--- a/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt
+++ b/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt
@@ -26,11 +26,12 @@  Optional subnode-properties:
 - ste,function: A string containing the name of the function to mux to the
   pin or group.
 
-- ste,input : <0/1/2/3>
+- ste,config: Handle of pin configuration node (ste,config = <&in_pu>)
+
+- ste,input : <0/1/2>
 	0: input with no pull
 	1: input with pull up,
 	2: input with pull down,
-	3: input and keep last input configuration (no pull, pull up or pull down).
 
 - ste,output: <0/1/2>
 	0: output low,
@@ -41,10 +42,11 @@  Optional subnode-properties:
 	0: sleep mode disable,
 	1: sleep mode enable.
 
-- ste,sleep-input: <0/1/2>
+- ste,sleep-input: <0/1/2/3>
 	0: sleep input with no pull,
 	1: sleep input with pull up,
 	2: sleep input with pull down.
+	3: sleep input and keep last input configuration (no pull, pull up or pull down).
 
 - ste,sleep-output: <0/1/2>
 	0: sleep output low,
@@ -65,12 +67,39 @@  Optional subnode-properties:
 
 Example board file extract:
 
+	in_pu: input_pull_up {
+		ste,input = <1>;
+	};
+
+	out_hi: output_high {
+		ste,output = <1>;
+	};
+
+	slpm_in_wkup_pdis: slpm_in_wkup_pdis {
+		ste,sleep = <1>;
+		ste,sleep-input = <3>;
+		ste,sleep-wakeup = <1>;
+		ste,sleep-pull-disable = <0>;
+	};
+
+	slpm_out_hi_wkup_pdis: slpm_out_hi_wkup_pdis {
+		ste,sleep = <1>;
+		ste,sleep-output = <1>;
+		ste,sleep-wakeup = <1>;
+		ste,sleep-pull-disable = <0>;
+	};
+	slpm_out_wkup_pdis: slpm_out_wkup_pdis {
+		ste,sleep = <1>;
+		ste,sleep-output = <2>;
+		ste,sleep-wakeup = <1>;
+		ste,sleep-pull-disable = <0>;
+	};
+
 	pinctrl@80157000 {
 		compatible = "stericsson,nmk-pinctrl";
 		reg = <0x80157000 0x2000>;
 
 		pinctrl-names = "default";
-		pinctrl-0 = <&uart0_default_mode>;
 
 		uart0 {
 			uart0_default_mux: uart0_mux {
@@ -82,35 +111,26 @@  Example board file extract:
 			uart0_default_mode: uart0_default {
 				uart0_default_cfg1 {
 					ste,pins = "GPIO0", "GPIO2";
-					ste,input = <1>;
+					ste,config = <&in_pu>;
 				};
 
 				uart0_default_cfg2 {
 					ste,pins = "GPIO1", "GPIO3";
-					ste,output = <1>;
+					ste,config = <&out_hi>;
 				};
 			};
 			uart0_sleep_mode: uart0_sleep {
 				uart0_sleep_cfg1 {
 					ste,pins = "GPIO0", "GPIO2";
-					ste,sleep = <0>;
-					ste,sleep-input = <0>;
-					ste,sleep-wakeup = <1>;
-					ste,sleep-pull-disable = <0>;
+					ste,config = <&slpm_in_wkup_pdis>;
 				};
 				uart0_sleep_cfg2 {
 					ste,pins = "GPIO1";
-					ste,sleep = <0>;
-					ste,sleep-output = <1>;
-					ste,sleep-wakeup = <1>;
-					ste,sleep-pull-disable = <0>;
+					ste,config = <&slpm_out_hi_wkup_pdis>;
 				};
 				uart0_sleep_cfg3 {
 					ste,pins = "GPIO3";
-					ste,sleep = <0>;
-					ste,sleep-output = <2>;
-					ste,sleep-wakeup = <1>;
-					ste,sleep-pull-disable = <0>;
+					ste,config = <&slpm_out_wkup_pdis>;
 				};
 			};
 		};
diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
index b508bd7..0355786 100644
--- a/drivers/pinctrl/pinctrl-nomadik.c
+++ b/drivers/pinctrl/pinctrl-nomadik.c
@@ -1691,23 +1691,29 @@  int nmk_pinctrl_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 	unsigned reserve = 0;
 	struct property *prop;
 	const char *group, *gpio_name;
+	struct device_node *np_config;
 
 	ret = of_property_read_string(np, "ste,function", &function);
 	if (ret >= 0)
 		reserve = 1;
 
-	for (i = 0; i < ARRAY_SIZE(nmk_cfg_params); i++) {
-		unsigned long cfg = 0;
-		int val;
-
-		ret = of_property_read_u32(np, nmk_cfg_params[i].property, &val);
-		if (ret != -EINVAL) {
-			if (nmk_dt_pin_config(i, val, &cfg) == 0) {
-				configs |= cfg;
-				has_config = 1;
+	np_config = of_parse_phandle(np, "ste,config", 0);
+	if (np_config) {
+		for (i = 0; i < ARRAY_SIZE(nmk_cfg_params); i++) {
+			unsigned long cfg = 0;
+			int val;
+
+			ret = of_property_read_u32(np_config,
+					nmk_cfg_params[i].property, &val);
+			if (ret != -EINVAL) {
+				if (nmk_dt_pin_config(i, val, &cfg) == 0) {
+					configs |= cfg;
+					has_config = 1;
+				}
 			}
 		}
 	}
+
 	ret = of_property_count_strings(np, "ste,pins");
 	if (ret < 0)
 		goto exit;