[PATCH 0/8] Add proper support for Compulab CM-A510/SBC-A510

[PATCH 0/8] Add proper support for Compulab CM-A510/SBC-A510

[Sebastian Hesselbarth]

This patch set improves current mainline support for the Compulab
CM-A510 System-on-Module (SoM) and its default Compulab SBC-A510
base board. Thanks to Gabriel Dobato who agreed to remote debug and
test the provided DT changes.

On the way to proper support, we
- Rework i2c-mux-pinctrl to honor disabled sub-bus nodes
- Add missing "compulab" vendor prefix
- Fix broken uart[23] reg properties
- Beautify Dove's dtsi files by adding gpio/irq includes, node
  labels for pcie, and some additional pinctrl settings
- Add a node for the internal i2c mux mechanism on Dove SoCs

And finally add a DT include for the Compulab CM-A510 SoM and a DT
board file for the SBC-A510 base board.

Patches are based on stable v3.19 and are indended for the next
merge window (either v3.21 or v4.1). Compulab related changes have
been tested by Gabriel Dobato, I tested on SolidRun CuBox that it
does not break existing Dove boards.

For the i2c-mux-pinctrl, a Tested-by from any user of Tegra20
Seaboard, Tamonten, or Ventana would be nice to see if it is fully
compatible.

I have added MVEBU maintainers to all patches, Wolfram for i2c and
Stephen as the i2c-mux-pinctrl author i2c-mux-pinctrl related patches,
and corresponding lists. As there is no important DT work in here,
I decided to not explicitly add each of the DT maintainers except for
the vendor prefix patch.

Sebastian Hesselbarth (8):
  i2c: mux-pinctrl: Rework to honor disabled child nodes
  devicetree: vendor-prefixes: Add CompuLab to known vendors
  ARM: dts: dove: Fix uart[23] reg property
  ARM: dts: dove: Always include gpio and interrupt-controller headers
  ARM: dts: dove: Add node labels for PCIe ports 0 and 1
  ARM: dts: dove: Add some more common pinctrl settings
  ARM: dts: dove: Add internal i2c multiplexer node
  ARM: dts: dove: Add proper support for Compulab CM-A510/SBC-A510

 .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    |  28 +--
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 arch/arm/boot/dts/Makefile                         |   5 +-
 arch/arm/boot/dts/dove-cm-a510.dts                 |  38 ----
 arch/arm/boot/dts/dove-cm-a510.dtsi                | 195 +++++++++++++++++++++
 arch/arm/boot/dts/dove-sbc-a510.dts                | 182 +++++++++++++++++++
 arch/arm/boot/dts/dove.dtsi                        | 103 ++++++++++-
 drivers/i2c/muxes/i2c-mux-pinctrl.c                |  70 +++++---
 8 files changed, 537 insertions(+), 85 deletions(-)
 delete mode 100644 arch/arm/boot/dts/dove-cm-a510.dts
 create mode 100644 arch/arm/boot/dts/dove-cm-a510.dtsi
 create mode 100644 arch/arm/boot/dts/dove-sbc-a510.dts

---
Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
Cc: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Cc: Gregory Clement <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Gabriel Dobato <dobatog-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.

This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an "i2c-"
prefix.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Gabriel Dobato <dobatog@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: linux-i2c@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    | 28 ++++-----
 drivers/i2c/muxes/i2c-mux-pinctrl.c                | 70 ++++++++++++++--------
 2 files changed, 59 insertions(+), 39 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
index ae8af1694e95..24b9fdef8850 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
@@ -28,27 +28,24 @@ Also required are:
 * Standard pinctrl properties that specify the pin mux state for each child
   bus. See ../pinctrl/pinctrl-bindings.txt.
 
-* Standard I2C mux properties. See mux.txt in this directory.
+* Standard I2C mux properties. See i2c-mux.txt in this directory.
 
-* I2C child bus nodes. See mux.txt in this directory.
+* I2C child bus nodes. See i2c-mux.txt in this directory.
 
-For each named state defined in the pinctrl-names property, an I2C child bus
-will be created. I2C child bus numbers are assigned based on the index into
-the pinctrl-names property.
+For each child node that is not disabled by a status != "okay", an I2C
+child bus will be created. I2C child bus numbers are assigned based on the
+order of child nodes.
 
-The only exception is that no bus will be created for a state named "idle". If
-such a state is defined, it must be the last entry in pinctrl-names. For
-example:
-
-	pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
-	pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
-	pinctrl-names = "idle", "ddc", "pta"  ->  Invalid ("idle" not last)
+There must be a corresponding pinctrl-names entry for each enabled child
+node at the position of the child node's "reg" property.
 
 Whenever an access is made to a device on a child bus, the relevant pinctrl
 state will be programmed into hardware.
 
-If an idle state is defined, whenever an access is not being made to a device
-on a child bus, the idle pinctrl state will be programmed into hardware.
+Also, there can be an idle pinctrl state defined at the end of possible
+pinctrl states. If an idle state is defined, whenever an access is not being
+made to a device on a child bus, the idle pinctrl state will be programmed
+into hardware.
 
 If an idle state is not defined, the most recently used pinctrl state will be
 left programmed into hardware whenever no access is being made of a device on
@@ -68,6 +65,7 @@ Example:
 		pinctrl-1 = <&state_i2cmux_pta>;
 		pinctrl-2 = <&state_i2cmux_idle>;
 
+		/* Enabled child bus 0 */
 		i2c@0 {
 			reg = <0>;
 			#address-cells = <1>;
@@ -79,10 +77,12 @@ Example:
 			};
 		};
 
+		/* Disabled child bus 1 */
 		i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			status = "disabled";
 
 			eeprom {
 				compatible = "eeprom";
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index b48378c4b40d..033dacfabfdf 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -56,9 +56,12 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 				struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	int num_names, i, ret;
+	struct device_node *child;
+	struct property *prop;
+	int num_names, num_children, ret;
 	struct device_node *adapter_np;
 	struct i2c_adapter *adapter;
+	const char *state;
 
 	if (!np)
 		return 0;
@@ -77,32 +80,16 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 		return num_names;
 	}
 
-	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
-		sizeof(*mux->pdata->pinctrl_states) * num_names,
-		GFP_KERNEL);
-	if (!mux->pdata->pinctrl_states) {
-		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
-		return -ENOMEM;
+	num_children = of_get_available_child_count(np);
+	if (num_children < 0) {
+		dev_err(mux->dev, "Unable to count available children: %d\n",
+			num_children);
+		return num_children;
 	}
 
-	for (i = 0; i < num_names; i++) {
-		ret = of_property_read_string_index(np, "pinctrl-names", i,
-			&mux->pdata->pinctrl_states[mux->pdata->bus_count]);
-		if (ret < 0) {
-			dev_err(mux->dev, "Cannot parse pinctrl-names: %d\n",
-				ret);
-			return ret;
-		}
-		if (!strcmp(mux->pdata->pinctrl_states[mux->pdata->bus_count],
-			    "idle")) {
-			if (i != num_names - 1) {
-				dev_err(mux->dev, "idle state must be last\n");
-				return -EINVAL;
-			}
-			mux->pdata->pinctrl_state_idle = "idle";
-		} else {
-			mux->pdata->bus_count++;
-		}
+	if (num_names < num_children) {
+		dev_err(mux->dev, "Found less pinctrl states than children\n");
+		return -EINVAL;
 	}
 
 	adapter_np = of_parse_phandle(np, "i2c-parent", 0);
@@ -118,6 +105,39 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 	mux->pdata->parent_bus_num = i2c_adapter_id(adapter);
 	put_device(&adapter->dev);
 
+	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
+		sizeof(*mux->pdata->pinctrl_states) * num_children,
+		GFP_KERNEL);
+	if (!mux->pdata->pinctrl_states) {
+		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
+		return -ENOMEM;
+	}
+
+	of_property_for_each_string(np, "pinctrl-names", prop, state)
+		if (!strcmp(state, "idle"))
+			mux->pdata->pinctrl_state_idle = "idle";
+
+	for_each_available_child_of_node(np, child) {
+		u32 reg;
+
+		ret = of_property_read_u32(child, "reg", &reg);
+		if (ret < 0) {
+			dev_err(mux->dev, "Missing reg property for child node: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = of_property_read_string_index(np,
+				    "pinctrl-names", reg, &state);
+		if (ret < 0) {
+			dev_err(mux->dev, "Cannot parse pinctrl-names for mux %d: %d\n",
+				reg, ret);
+			return ret;
+		}
+
+		mux->pdata->pinctrl_states[mux->pdata->bus_count++] = state;
+	}
+
 	return 0;
 }
 #else
-- 
2.1.0

[PATCH 2/8] devicetree: vendor-prefixes: Add CompuLab to known vendors

[Sebastian Hesselbarth]

This adds "compulab" as a vendor-prefix for CompuLab (compulab.co.il),
an Israeli company that builds ARM-based SoMs and CoMs.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Gabriel Dobato <dobatog@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index d443279c95dc..2de628532d4b 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -35,6 +35,7 @@ chrp	Common Hardware Reference Platform
 chunghwa	Chunghwa Picture Tubes Ltd.
 cirrus	Cirrus Logic, Inc.
 cnm	Chips&Media, Inc.
+compulab	CompuLab
 cortina	Cortina Systems, Inc.
 crystalfontz	Crystalfontz America, Inc.
 dallas	Maxim Integrated Products (formerly Dallas Semiconductor)
-- 
2.1.0

[PATCH 7/8] ARM: dts: dove: Add internal i2c multiplexer node

[Sebastian Hesselbarth]

This adds a i2c-mux-pinctrl node to dove.dtsi for the internal i2c
mux found on Dove SoCs. Up to now, we had no board using any of the
two additional i2c busses, so make sure the change does not break
any existing boards.

Therefore, we rename the i2c-controller node label to "i2c" and
enable it by default. Also, the dedicated sub-bus (now "i2c0") is
enabled by default. The two optional sub-busses require additional
external pin-muxing, so disable them by default.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Wolfram,

Actually, I was hoping that default pin hog mechanism
(pinctrl-names = "default") could also be used from i2c mux nodes
and devices. Anyway, I had a look at i2c-core/mux code and failed
how to achieve that easily. Instead I decided, it would also be ok
to put the pin hog into the i2c controller node where pins will be
bound by standard platform device code.

Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Gabriel Dobato <dobatog@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: linux-i2c@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/boot/dts/dove.dtsi | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 9ad829523a13..b3340e862b0e 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -28,6 +28,42 @@
 		};
 	};
 
+	i2c-mux {
+		compatible = "i2c-mux-pinctrl";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c-parent = <&i2c>;
+
+		pinctrl-names = "i2c0", "i2c1", "i2c2";
+		pinctrl-0 = <&pmx_i2cmux_0>;
+		pinctrl-1 = <&pmx_i2cmux_1>;
+		pinctrl-2 = <&pmx_i2cmux_2>;
+
+		i2c0: i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "okay";
+		};
+
+		i2c1: i2c@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			/* Requires pmx_i2c1 on i2c controller node */
+			status = "disabled";
+		};
+
+		i2c2: i2c@2 {
+			reg = <2>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			/* Requires pmx_i2c2 on i2c controller node */
+			status = "disabled";
+		};
+	};
+
 	l2: l2-cache {
 		compatible = "marvell,tauros2-cache";
 		marvell,tauros2-cache-features = <0>;
@@ -123,7 +159,7 @@
 				status = "disabled";
 			};
 
-			i2c0: i2c-ctrl@11000 {
+			i2c: i2c-ctrl@11000 {
 				compatible = "marvell,mv64xxx-i2c";
 				reg = <0x11000 0x20>;
 				#address-cells = <1>;
@@ -132,7 +168,7 @@
 				clock-frequency = <400000>;
 				timeout-ms = <1000>;
 				clocks = <&core_clk 0>;
-				status = "disabled";
+				status = "okay";
 			};
 
 			uart0: serial@12000 {
-- 
2.1.0

Re: [PATCH 2/8] devicetree: vendor-prefixes: Add CompuLab to known vendors

[Rob Herring]

On Tue, Feb 17, 2015 at 12:52 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:
> This adds "compulab" as a vendor-prefix for CompuLab (compulab.co.il),
> an Israeli company that builds ARM-based SoMs and CoMs.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

Acked-by: Rob Herring <robh@kernel.org>

> ---
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Gabriel Dobato <dobatog@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index d443279c95dc..2de628532d4b 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -35,6 +35,7 @@ chrp  Common Hardware Reference Platform
>  chunghwa       Chunghwa Picture Tubes Ltd.
>  cirrus Cirrus Logic, Inc.
>  cnm    Chips&Media, Inc.
> +compulab       CompuLab
>  cortina        Cortina Systems, Inc.
>  crystalfontz   Crystalfontz America, Inc.
>  dallas Maxim Integrated Products (formerly Dallas Semiconductor)
> --
> 2.1.0
>

Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 02/17/2015 11:52 AM, Sebastian Hesselbarth wrote:
> I2C mux pinctrl driver currently determines the number of sub-busses by
> counting available pinctrl-names. Unfortunately, this requires each
> incarnation of the devicetree node with different available sub-busses
> to be rewritten.

Can you be more explicit about the problem here? Why does anything need 
to be re-written if a child node is disabled; presumably there's no need 
for the child bus numbers to be contiguous. In other words, with the 
example in the existing DT binding doc:

         i2cmux {
                 compatible = "i2c-mux-pinctrl";
...
                 pinctrl-names = "ddc", "pta", "idle";
                 pinctrl-0 = <&state_i2cmux_ddc>;
                 pinctrl-1 = <&state_i2cmux_pta>;
                 pinctrl-2 = <&state_i2cmux_idle>;

                 i2c@0 {
                         reg = <0>;
...
                 i2c@1 {
                         reg = <1>;
...

That would generate child busses 0 and 1. If I was to disable the i2c@0 
node, then there would still be definitions for child busses 0 and 1 in 
the DT, it's just that child bus 0 wouldn't actually exist at run-time. 
I don't see what part of DT needs to be re-written to accomodate this?

> This patch reworks i2c-mux-pinctrl driver to count the number of
> available sub-nodes instead. The rework should be compatible to the old
> way of probing for sub-busses and additionally allows to disable unused
> sub-busses with standard DT property status = "disabled".
>
> This also amends the corresponding devicetree binding documentation to
> reflect the new functionality to disable unused sub-nodes. While at it,
> also fix two references to binding documentation files that miss an "i2c-"
> prefix.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt

> -For each named state defined in the pinctrl-names property, an I2C child bus
> -will be created. I2C child bus numbers are assigned based on the index into
> -the pinctrl-names property.
> +For each child node that is not disabled by a status != "okay", an I2C
> +child bus will be created. I2C child bus numbers are assigned based on the
> +order of child nodes.

I would have assumed that disabled sub-nodes was a global concept within 
DT, and so wouldn't be mentioned in the binding. It would just be a bug 
in the driver if it didn't ignore disabled sub-nodes.




>
> -The only exception is that no bus will be created for a state named "idle". If
> -such a state is defined, it must be the last entry in pinctrl-names. For
> -example:
> -
> -	pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
> -	pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
> -	pinctrl-names = "idle", "ddc", "pta"  ->  Invalid ("idle" not last)
> +There must be a corresponding pinctrl-names entry for each enabled child
> +node at the position of the child node's "reg" property.

The addition there seems fine, but the existing text re: the idle state 
seems clearer in the original text.
>
>   Whenever an access is made to a device on a child bus, the relevant pinctrl
>   state will be programmed into hardware.
>
> -If an idle state is defined, whenever an access is not being made to a device
> -on a child bus, the idle pinctrl state will be programmed into hardware.
> +Also, there can be an idle pinctrl state defined at the end of possible
> +pinctrl states. If an idle state is defined, whenever an access is not being
> +made to a device on a child bus, the idle pinctrl state will be programmed
> +into hardware.

Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 17.02.2015 21:46, Stephen Warren wrote:
> On 02/17/2015 11:52 AM, Sebastian Hesselbarth wrote:
>> I2C mux pinctrl driver currently determines the number of sub-busses by
>> counting available pinctrl-names. Unfortunately, this requires each
>> incarnation of the devicetree node with different available sub-busses
>> to be rewritten.
>
> Can you be more explicit about the problem here? Why does anything need
> to be re-written if a child node is disabled; presumably there's no need
> for the child bus numbers to be contiguous. In other words, with the
> example in the existing DT binding doc:
>
>          i2cmux {
>                  compatible = "i2c-mux-pinctrl";
> ...
>                  pinctrl-names = "ddc", "pta", "idle";
>                  pinctrl-0 = <&state_i2cmux_ddc>;
>                  pinctrl-1 = <&state_i2cmux_pta>;
>                  pinctrl-2 = <&state_i2cmux_idle>;
>
>                  i2c@0 {
>                          reg = <0>;
> ...
>                  i2c@1 {
>                          reg = <1>;
> ...
>
> That would generate child busses 0 and 1. If I was to disable the i2c@0
> node, then there would still be definitions for child busses 0 and 1 in
> the DT, it's just that child bus 0 wouldn't actually exist at run-time.
> I don't see what part of DT needs to be re-written to accomodate this?

The way the current driver works, to disable i2c@0 you'd have to remove
the pinctrl-0 state, pinctrl-names string at position 0, and the node
itself.

So, on Dove SoC there is three sub-busses, now consider one board A with
i2c0 and i2c1 enabled but board B with i2c0 and i2c2 enabled:

board-A.dts:

i2cmux {
	pinctrl-names = "i2c0", "i2c1", "idle";
	pinctrl-0 = <&state_for_i2c0>;
	pinctrl-1 = <&state_for_i2c1>;
};

but

board-B.dts:

i2cmux {
	pinctrl-names = "i2c0", "i2c2", "idle";
	pinctrl-0 = <&state_for_i2c0>;
	pinctrl-1 = <&state_for_i2c2>;
	/* Note that this ^^^ is state_for_i2c2 */
};

while the approach with status = "disabled" allows all properties for
both board remain the same - except you'll enable either i2c1 or i2c2
sub-node on board level:

i2cmux {
	pinctrl-names = "i2c0", "i2c1", "i2c2", "idle";
	pinctrl-0 = <&state_for_i2c0>;
	pinctrl-1 = <&state_for_i2c1>;
	pinctrl-2 = <&state_for_i2c2>;
};

board-A.dts:

i2cmux {
	i2c@0 { status = "okay"; };
	i2c@1 { status = "okay"; };
};

and

board-B.dts:

i2cmux {
	i2c@0 { status = "okay"; };
	i2c@2 { status = "okay"; };
};

In general, it is less about the binding but how the driver is written:
Number of sub-busses is determined by elements in pinctrl-names not
available (enabled) sub-nodes.

>> This patch reworks i2c-mux-pinctrl driver to count the number of
>> available sub-nodes instead. The rework should be compatible to the old
>> way of probing for sub-busses and additionally allows to disable unused
>> sub-busses with standard DT property status = "disabled".
>>
>> This also amends the corresponding devicetree binding documentation to
>> reflect the new functionality to disable unused sub-nodes. While at it,
>> also fix two references to binding documentation files that miss an
>> "i2c-"
>> prefix.
>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
>
>> -For each named state defined in the pinctrl-names property, an I2C
>> child bus
>> -will be created. I2C child bus numbers are assigned based on the
>> index into
>> -the pinctrl-names property.
>> +For each child node that is not disabled by a status != "okay", an I2C
>> +child bus will be created. I2C child bus numbers are assigned based
>> on the
>> +order of child nodes.
>
> I would have assumed that disabled sub-nodes was a global concept within
> DT, and so wouldn't be mentioned in the binding. It would just be a bug
> in the driver if it didn't ignore disabled sub-nodes.

Yep, the concept is very global. It is about the current driver and this
binding changes are just to make it a little more clear that the driver
should behave different, i.e. get rid of anything that implies that
pinctrl-names has any effect on the number of sub-busses registered.

>> -The only exception is that no bus will be created for a state named
>> "idle". If
>> -such a state is defined, it must be the last entry in pinctrl-names. For
>> -example:
>> -
>> -    pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
>> -    pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
>> -    pinctrl-names = "idle", "ddc", "pta"  ->  Invalid ("idle" not last)
>> +There must be a corresponding pinctrl-names entry for each enabled child
>> +node at the position of the child node's "reg" property.
>
> The addition there seems fine, but the existing text re: the idle state
> seems clearer in the original text.

Ok, I'll have a look at how to preserve this section better.

Do you still have one of the current boards available for testing?

Sebastian

Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 02/17/2015 02:08 PM, Sebastian Hesselbarth wrote:
> On 17.02.2015 21:46, Stephen Warren wrote:
>> On 02/17/2015 11:52 AM, Sebastian Hesselbarth wrote:
>>> I2C mux pinctrl driver currently determines the number of sub-busses by
>>> counting available pinctrl-names. Unfortunately, this requires each
>>> incarnation of the devicetree node with different available sub-busses
>>> to be rewritten.
>>
>> Can you be more explicit about the problem here? Why does anything need
>> to be re-written if a child node is disabled; presumably there's no need
>> for the child bus numbers to be contiguous. In other words, with the
>> example in the existing DT binding doc:
>>
>>          i2cmux {
>>                  compatible = "i2c-mux-pinctrl";
>> ...
>>                  pinctrl-names = "ddc", "pta", "idle";
>>                  pinctrl-0 = <&state_i2cmux_ddc>;
>>                  pinctrl-1 = <&state_i2cmux_pta>;
>>                  pinctrl-2 = <&state_i2cmux_idle>;
>>
>>                  i2c@0 {
>>                          reg = <0>;
>> ...
>>                  i2c@1 {
>>                          reg = <1>;
>> ...
>>
>> That would generate child busses 0 and 1. If I was to disable the i2c@0
>> node, then there would still be definitions for child busses 0 and 1 in
>> the DT, it's just that child bus 0 wouldn't actually exist at run-time.
>> I don't see what part of DT needs to be re-written to accomodate this?
>
> The way the current driver works, to disable i2c@0 you'd have to remove
> the pinctrl-0 state, pinctrl-names string at position 0, and the node
> itself.
>
> So, on Dove SoC there is three sub-busses, now consider one board A with
> i2c0 and i2c1 enabled but board B with i2c0 and i2c2 enabled:
>
> board-A.dts:
>
> i2cmux {
>      pinctrl-names = "i2c0", "i2c1", "idle";
>      pinctrl-0 = <&state_for_i2c0>;
>      pinctrl-1 = <&state_for_i2c1>;
> };
>
> but
>
> board-B.dts:
>
> i2cmux {
>      pinctrl-names = "i2c0", "i2c2", "idle";
>      pinctrl-0 = <&state_for_i2c0>;
>      pinctrl-1 = <&state_for_i2c2>;
>      /* Note that this ^^^ is state_for_i2c2 */
> };
>
> while the approach with status = "disabled" allows all properties for
> both board remain the same - except you'll enable either i2c1 or i2c2
> sub-node on board level:
>
> i2cmux {
>      pinctrl-names = "i2c0", "i2c1", "i2c2", "idle";
>      pinctrl-0 = <&state_for_i2c0>;
>      pinctrl-1 = <&state_for_i2c1>;
>      pinctrl-2 = <&state_for_i2c2>;
> };
>
> board-A.dts:
>
> i2cmux {
>      i2c@0 { status = "okay"; };
>      i2c@1 { status = "okay"; };
> };
>
> and
>
> board-B.dts:
>
> i2cmux {
>      i2c@0 { status = "okay"; };
>      i2c@2 { status = "okay"; };
> };

OK, that all makes sense, but I don't think there's any change at all to 
the binding; this can all be fixed in the driver without affecting the 
definition of the binding at all. At most all that's needed in the 
binding is a note to the effect that if a particular child node is 
disabled, then this has no effect at all on the requirements for the 
pinctrl properties.

> In general, it is less about the binding but how the driver is written:
> Number of sub-busses is determined by elements in pinctrl-names not
> available (enabled) sub-nodes.
>
>>> This patch reworks i2c-mux-pinctrl driver to count the number of
>>> available sub-nodes instead. The rework should be compatible to the old
>>> way of probing for sub-busses and additionally allows to disable unused
>>> sub-busses with standard DT property status = "disabled".
>>>
>>> This also amends the corresponding devicetree binding documentation to
>>> reflect the new functionality to disable unused sub-nodes. While at it,
>>> also fix two references to binding documentation files that miss an
>>> "i2c-"
>>> prefix.
>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
>>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
>>
>>> -For each named state defined in the pinctrl-names property, an I2C
>>> child bus
>>> -will be created. I2C child bus numbers are assigned based on the
>>> index into
>>> -the pinctrl-names property.
>>> +For each child node that is not disabled by a status != "okay", an I2C
>>> +child bus will be created. I2C child bus numbers are assigned based
>>> on the
>>> +order of child nodes.
>>
>> I would have assumed that disabled sub-nodes was a global concept within
>> DT, and so wouldn't be mentioned in the binding. It would just be a bug
>> in the driver if it didn't ignore disabled sub-nodes.
>
> Yep, the concept is very global. It is about the current driver and this
> binding changes are just to make it a little more clear that the driver
> should behave different, i.e. get rid of anything that implies that
> pinctrl-names has any effect on the number of sub-busses registered.
>
>>> -The only exception is that no bus will be created for a state named
>>> "idle". If
>>> -such a state is defined, it must be the last entry in pinctrl-names.
>>> For
>>> -example:
>>> -
>>> -    pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
>>> -    pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
>>> -    pinctrl-names = "idle", "ddc", "pta"  ->  Invalid ("idle" not last)
>>> +There must be a corresponding pinctrl-names entry for each enabled
>>> child
>>> +node at the position of the child node's "reg" property.
>>
>> The addition there seems fine, but the existing text re: the idle state
>> seems clearer in the original text.
>
> Ok, I'll have a look at how to preserve this section better.
>
> Do you still have one of the current boards available for testing?

Yes, I have both Seaboard and Ventana still (the two Tegra boards that 
use this driver). I haven't used them in a while; I hope they still work:-)

Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 17.02.2015 22:15, Stephen Warren wrote:
> On 02/17/2015 02:08 PM, Sebastian Hesselbarth wrote:
>> On 17.02.2015 21:46, Stephen Warren wrote:
>>> Can you be more explicit about the problem here? Why does anything need
>>> to be re-written if a child node is disabled; presumably there's no need
>>> for the child bus numbers to be contiguous. In other words, with the
>>> example in the existing DT binding doc:
[...]
>>
>> The way the current driver works, to disable i2c@0 you'd have to remove
>> the pinctrl-0 state, pinctrl-names string at position 0, and the node
>> itself.
[...]
> OK, that all makes sense, but I don't think there's any change at all to
> the binding; this can all be fixed in the driver without affecting the
> definition of the binding at all. At most all that's needed in the
> binding is a note to the effect that if a particular child node is
> disabled, then this has no effect at all on the requirements for the
> pinctrl properties.

I totally agree that the binding is not affected at all. I was under the
impression that the current binding doc justifies the way the driver is
currently parsing the properties. So this was an attempt to reword it
to make it more clear what properties should influence the way
sub-busses are registered.

I'll have another look at the binding doc when the driver is fine.

[...]
>> Do you still have one of the current boards available for testing?
>
> Yes, I have both Seaboard and Ventana still (the two Tegra boards that
> use this driver). I haven't used them in a while; I hope they still work:-)

Ok, I cross my fingers too and expect a Tested-by :)

Sebastian

Re: [PATCH 7/8] ARM: dts: dove: Add internal i2c multiplexer node

[Gregory CLEMENT]

Hi Sebastian,

On 17/02/2015 19:52, Sebastian Hesselbarth wrote:
> This adds a i2c-mux-pinctrl node to dove.dtsi for the internal i2c
> mux found on Dove SoCs. Up to now, we had no board using any of the
> two additional i2c busses, so make sure the change does not break
> any existing boards.
> 
> Therefore, we rename the i2c-controller node label to "i2c" and
> enable it by default. Also, the dedicated sub-bus (now "i2c0") is
> enabled by default. The two optional sub-busses require additional
> external pin-muxing, so disable them by default.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


Thanks,

Gregory

> ---
> Wolfram,
> 
> Actually, I was hoping that default pin hog mechanism
> (pinctrl-names = "default") could also be used from i2c mux nodes
> and devices. Anyway, I had a look at i2c-core/mux code and failed
> how to achieve that easily. Instead I decided, it would also be ok
> to put the pin hog into the i2c controller node where pins will be
> bound by standard platform device code.
> 
> Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
> Cc: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> Cc: Gregory Clement <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Gabriel Dobato <dobatog-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/arm/boot/dts/dove.dtsi | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 9ad829523a13..b3340e862b0e 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -28,6 +28,42 @@
>  		};
>  	};
>  
> +	i2c-mux {
> +		compatible = "i2c-mux-pinctrl";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		i2c-parent = <&i2c>;
> +
> +		pinctrl-names = "i2c0", "i2c1", "i2c2";
> +		pinctrl-0 = <&pmx_i2cmux_0>;
> +		pinctrl-1 = <&pmx_i2cmux_1>;
> +		pinctrl-2 = <&pmx_i2cmux_2>;
> +
> +		i2c0: i2c@0 {
> +			reg = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "okay";
> +		};
> +
> +		i2c1: i2c@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			/* Requires pmx_i2c1 on i2c controller node */
> +			status = "disabled";
> +		};
> +
> +		i2c2: i2c@2 {
> +			reg = <2>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			/* Requires pmx_i2c2 on i2c controller node */
> +			status = "disabled";
> +		};
> +	};
> +
>  	l2: l2-cache {
>  		compatible = "marvell,tauros2-cache";
>  		marvell,tauros2-cache-features = <0>;
> @@ -123,7 +159,7 @@
>  				status = "disabled";
>  			};
>  
> -			i2c0: i2c-ctrl@11000 {
> +			i2c: i2c-ctrl@11000 {
>  				compatible = "marvell,mv64xxx-i2c";
>  				reg = <0x11000 0x20>;
>  				#address-cells = <1>;
> @@ -132,7 +168,7 @@
>  				clock-frequency = <400000>;
>  				timeout-ms = <1000>;
>  				clocks = <&core_clk 0>;
> -				status = "disabled";
> +				status = "okay";
>  			};
>  
>  			uart0: serial@12000 {
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/8] Add proper support for Compulab CM-A510/SBC-A510

[Gregory CLEMENT]

Hi Sebastian,

On 17/02/2015 19:52, Sebastian Hesselbarth wrote:
> This patch set improves current mainline support for the Compulab
> CM-A510 System-on-Module (SoM) and its default Compulab SBC-A510
> base board. Thanks to Gabriel Dobato who agreed to remote debug and
> test the provided DT changes.
> 
> On the way to proper support, we
> - Rework i2c-mux-pinctrl to honor disabled sub-bus nodes
> - Add missing "compulab" vendor prefix
> - Fix broken uart[23] reg properties
> - Beautify Dove's dtsi files by adding gpio/irq includes, node
>   labels for pcie, and some additional pinctrl settings
> - Add a node for the internal i2c mux mechanism on Dove SoCs
> 
> And finally add a DT include for the Compulab CM-A510 SoM and a DT
> board file for the SBC-A510 base board.
> 
> Patches are based on stable v3.19 and are indended for the next
> merge window (either v3.21 or v4.1). Compulab related changes have
> been tested by Gabriel Dobato, I tested on SolidRun CuBox that it
> does not break existing Dove boards.
> 
> For the i2c-mux-pinctrl, a Tested-by from any user of Tegra20
> Seaboard, Tamonten, or Ventana would be nice to see if it is fully
> compatible.
> 
> I have added MVEBU maintainers to all patches, Wolfram for i2c and
> Stephen as the i2c-mux-pinctrl author i2c-mux-pinctrl related patches,
> and corresponding lists. As there is no important DT work in here,
> I decided to not explicitly add each of the DT maintainers except for
> the vendor prefix patch.
> 
> Sebastian Hesselbarth (8):
>   i2c: mux-pinctrl: Rework to honor disabled child nodes
>   devicetree: vendor-prefixes: Add CompuLab to known vendors
>   ARM: dts: dove: Fix uart[23] reg property
>   ARM: dts: dove: Always include gpio and interrupt-controller headers
>   ARM: dts: dove: Add node labels for PCIe ports 0 and 1
>   ARM: dts: dove: Add some more common pinctrl settings
>   ARM: dts: dove: Add internal i2c multiplexer node
>   ARM: dts: dove: Add proper support for Compulab CM-A510/SBC-A510

Patches 3 to 6 applied on mvebu/dt

If I understood well patches 7 and 8 depend on patch 1 which had not been
taken yet.


Thanks,

Gregory


> 
>  .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    |  28 +--
>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>  arch/arm/boot/dts/Makefile                         |   5 +-
>  arch/arm/boot/dts/dove-cm-a510.dts                 |  38 ----
>  arch/arm/boot/dts/dove-cm-a510.dtsi                | 195 +++++++++++++++++++++
>  arch/arm/boot/dts/dove-sbc-a510.dts                | 182 +++++++++++++++++++
>  arch/arm/boot/dts/dove.dtsi                        | 103 ++++++++++-
>  drivers/i2c/muxes/i2c-mux-pinctrl.c                |  70 +++++---
>  8 files changed, 537 insertions(+), 85 deletions(-)
>  delete mode 100644 arch/arm/boot/dts/dove-cm-a510.dts
>  create mode 100644 arch/arm/boot/dts/dove-cm-a510.dtsi
>  create mode 100644 arch/arm/boot/dts/dove-sbc-a510.dts
> 
> ---
> Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
> Cc: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> Cc: Gregory Clement <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Gabriel Dobato <dobatog-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/8] Add proper support for Compulab CM-A510/SBC-A510

[Sebastian Hesselbarth]

On 26.02.2015 18:55, Gregory CLEMENT wrote:
> On 17/02/2015 19:52, Sebastian Hesselbarth wrote:
>> This patch set improves current mainline support for the Compulab
>> CM-A510 System-on-Module (SoM) and its default Compulab SBC-A510
>> base board. Thanks to Gabriel Dobato who agreed to remote debug and
>> test the provided DT changes.
[...]
>> Sebastian Hesselbarth (8):
>>    i2c: mux-pinctrl: Rework to honor disabled child nodes
>>    devicetree: vendor-prefixes: Add CompuLab to known vendors
>>    ARM: dts: dove: Fix uart[23] reg property
>>    ARM: dts: dove: Always include gpio and interrupt-controller headers
>>    ARM: dts: dove: Add node labels for PCIe ports 0 and 1
>>    ARM: dts: dove: Add some more common pinctrl settings
>>    ARM: dts: dove: Add internal i2c multiplexer node
>>    ARM: dts: dove: Add proper support for Compulab CM-A510/SBC-A510
>
> Patches 3 to 6 applied on mvebu/dt
>
> If I understood well patches 7 and 8 depend on patch 1 which had not been
> taken yet.

Correct. Once I get a Tested-by from Stephen, I'll resend the 4
remaining patches.

Thanks,
   Sebastian

Re: [PATCH 0/8] Add proper support for Compulab CM-A510/SBC-A510

[Stephen Warren]

On 02/26/2015 12:39 PM, Sebastian Hesselbarth wrote:
> On 26.02.2015 18:55, Gregory CLEMENT wrote:
>> On 17/02/2015 19:52, Sebastian Hesselbarth wrote:
>>> This patch set improves current mainline support for the Compulab
>>> CM-A510 System-on-Module (SoM) and its default Compulab SBC-A510
>>> base board. Thanks to Gabriel Dobato who agreed to remote debug and
>>> test the provided DT changes.
> [...]
>>> Sebastian Hesselbarth (8):
>>>    i2c: mux-pinctrl: Rework to honor disabled child nodes
>>>    devicetree: vendor-prefixes: Add CompuLab to known vendors
>>>    ARM: dts: dove: Fix uart[23] reg property
>>>    ARM: dts: dove: Always include gpio and interrupt-controller headers
>>>    ARM: dts: dove: Add node labels for PCIe ports 0 and 1
>>>    ARM: dts: dove: Add some more common pinctrl settings
>>>    ARM: dts: dove: Add internal i2c multiplexer node
>>>    ARM: dts: dove: Add proper support for Compulab CM-A510/SBC-A510
>>
>> Patches 3 to 6 applied on mvebu/dt
>>
>> If I understood well patches 7 and 8 depend on patch 1 which had not been
>> taken yet.
>
> Correct. Once I get a Tested-by from Stephen, I'll resend the 4
> remaining patches.

Me? For which patch? I thought there was going to be a v2 of the i2c mux 
driver patch given the comments on it, but perhaps I'm misremembering?

Re: [PATCH 0/8] Add proper support for Compulab CM-A510/SBC-A510

[Sebastian Hesselbarth]

On 26.02.2015 21:01, Stephen Warren wrote:
> On 02/26/2015 12:39 PM, Sebastian Hesselbarth wrote:
>> On 26.02.2015 18:55, Gregory CLEMENT wrote:
>>> On 17/02/2015 19:52, Sebastian Hesselbarth wrote:
>>>> This patch set improves current mainline support for the Compulab
>>>> CM-A510 System-on-Module (SoM) and its default Compulab SBC-A510
>>>> base board. Thanks to Gabriel Dobato who agreed to remote debug and
>>>> test the provided DT changes.
>> [...]
>>>> Sebastian Hesselbarth (8):
>>>>    i2c: mux-pinctrl: Rework to honor disabled child nodes
>>>>    devicetree: vendor-prefixes: Add CompuLab to known vendors
>>>>    ARM: dts: dove: Fix uart[23] reg property
>>>>    ARM: dts: dove: Always include gpio and interrupt-controller headers
>>>>    ARM: dts: dove: Add node labels for PCIe ports 0 and 1
>>>>    ARM: dts: dove: Add some more common pinctrl settings
>>>>    ARM: dts: dove: Add internal i2c multiplexer node
>>>>    ARM: dts: dove: Add proper support for Compulab CM-A510/SBC-A510
>>>
>>> Patches 3 to 6 applied on mvebu/dt
>>>
>>> If I understood well patches 7 and 8 depend on patch 1 which had not
>>> been
>>> taken yet.
>>
>> Correct. Once I get a Tested-by from Stephen, I'll resend the 4
>> remaining patches.
>
> Me? For which patch? I thought there was going to be a v2 of the i2c mux
> driver patch given the comments on it, but perhaps I'm misremembering?

AFAIKS, your concerns were addressed to the wording of the binding doc
changes. You don't need that ones to be taken care of before you can
give the i2c-mux-pinctrl driver a functional test, do you? ;)

FWIW, it would be great if you can give patch 1 a test run on one of
the tegra boards using i2c-mux-pinctrl. I'll address the wording
comments if there is no issues with the driver changes.

Sebastian

Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 02/17/2015 11:52 AM, Sebastian Hesselbarth wrote:
> I2C mux pinctrl driver currently determines the number of sub-busses by
> counting available pinctrl-names. Unfortunately, this requires each
> incarnation of the devicetree node with different available sub-busses
> to be rewritten.
>
> This patch reworks i2c-mux-pinctrl driver to count the number of
> available sub-nodes instead. The rework should be compatible to the old
> way of probing for sub-busses and additionally allows to disable unused
> sub-busses with standard DT property status = "disabled".
>
> This also amends the corresponding devicetree binding documentation to
> reflect the new functionality to disable unused sub-nodes. While at it,
> also fix two references to binding documentation files that miss an "i2c-"
> prefix.

Tested-by: Stephen Warren <swarren@nvidia.com>

(Both the HDMI and LCD panel DDC busses on NVIDIA Tegra 
Springbank/Seaboard, running next-20150224)

[PATCH v2 0/4] Add proper support for Compulab CM-A510/SBC-A510

[Sebastian Hesselbarth]

This is v2 of the patch set to improve current mainline support for the
Compulab CM-A510 System-on-Module (SoM) and its default Compulab SBC-A510
base board. 

Compared to v1 there have been the following changes:
- removed preparing Dove patches that already have been taken by MVEBU
  maintainers.
- added a Tested-by for i2c-mux-pinctrl changes from Stepen Warren.
- reworded i2c-mux-pinctrl devicetree doc changes
  (Suggested by Stephen Warren).
- Added an Acked-by for the vendor-prefix patch from Rob Herring.
- Added an Acked-by for the addition of i2c mux node to dove.dtsi from
  Gregory Clement.
- Rebased on top of v4.0-rc1 with preparing patches applied.

Patches are based on v4.0-rc1 with preparing patches for Dove applied
and are intended for v4.1.

Sebastian Hesselbarth (4):
  i2c: mux-pinctrl: Rework to honor disabled child nodes
  devicetree: vendor-prefixes: Add CompuLab to known vendors
  ARM: dts: dove: Add internal i2c multiplexer node
  ARM: dts: dove: Add proper support for Compulab CM-A510/SBC-A510

 .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    |  25 +--
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 arch/arm/boot/dts/Makefile                         |   4 +-
 arch/arm/boot/dts/dove-cm-a510.dts                 |  38 ----
 arch/arm/boot/dts/dove-cm-a510.dtsi                | 195 +++++++++++++++++++++
 arch/arm/boot/dts/dove-sbc-a510.dts                | 182 +++++++++++++++++++
 arch/arm/boot/dts/dove.dtsi                        |  40 ++++-
 drivers/i2c/muxes/i2c-mux-pinctrl.c                |  70 +++++---
 8 files changed, 477 insertions(+), 78 deletions(-)
 delete mode 100644 arch/arm/boot/dts/dove-cm-a510.dts
 create mode 100644 arch/arm/boot/dts/dove-cm-a510.dtsi
 create mode 100644 arch/arm/boot/dts/dove-sbc-a510.dts

---
Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
Cc: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Cc: Gregory Clement <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Gabriel Dobato <dobatog-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
-- 
2.1.0

[PATCH v2 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.

This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an "i2c-"
prefix.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
Cc: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Cc: Gregory Clement <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Gabriel Dobato <dobatog-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    | 25 ++++----
 drivers/i2c/muxes/i2c-mux-pinctrl.c                | 70 ++++++++++++++--------
 2 files changed, 59 insertions(+), 36 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
index ae8af1694e95..40cf6d86f935 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
@@ -28,21 +28,21 @@ Also required are:
 * Standard pinctrl properties that specify the pin mux state for each child
   bus. See ../pinctrl/pinctrl-bindings.txt.
 
-* Standard I2C mux properties. See mux.txt in this directory.
+* Standard I2C mux properties. See i2c-mux.txt in this directory.
 
-* I2C child bus nodes. See mux.txt in this directory.
+* I2C child bus nodes. See i2c-mux.txt in this directory.
 
-For each named state defined in the pinctrl-names property, an I2C child bus
-will be created. I2C child bus numbers are assigned based on the index into
-the pinctrl-names property.
+For each enabled child node an I2C child bus will be created. I2C child bus
+numbers are assigned based on the order of child nodes.
 
-The only exception is that no bus will be created for a state named "idle". If
-such a state is defined, it must be the last entry in pinctrl-names. For
-example:
+There must be a corresponding pinctrl-names entry for each enabled child
+node at the position of the child node's "reg" property. Also, there can be
+an idle pinctrl state defined at the end of possible pinctrl states. If such
+a state is defined, it must be the last entry in pinctrl-names. For example:
 
-	pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
-	pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
-	pinctrl-names = "idle", "ddc", "pta"  ->  Invalid ("idle" not last)
+       pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
+       pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
+       pinctrl-names = "idle", "ddc", "pta"  ->  Invalid ("idle" not last)
 
 Whenever an access is made to a device on a child bus, the relevant pinctrl
 state will be programmed into hardware.
@@ -68,6 +68,7 @@ Example:
 		pinctrl-1 = <&state_i2cmux_pta>;
 		pinctrl-2 = <&state_i2cmux_idle>;
 
+		/* Enabled child bus 0 */
 		i2c@0 {
 			reg = <0>;
 			#address-cells = <1>;
@@ -79,10 +80,12 @@ Example:
 			};
 		};
 
+		/* Disabled child bus 1 */
 		i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			status = "disabled";
 
 			eeprom {
 				compatible = "eeprom";
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index b48378c4b40d..033dacfabfdf 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -56,9 +56,12 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 				struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	int num_names, i, ret;
+	struct device_node *child;
+	struct property *prop;
+	int num_names, num_children, ret;
 	struct device_node *adapter_np;
 	struct i2c_adapter *adapter;
+	const char *state;
 
 	if (!np)
 		return 0;
@@ -77,32 +80,16 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 		return num_names;
 	}
 
-	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
-		sizeof(*mux->pdata->pinctrl_states) * num_names,
-		GFP_KERNEL);
-	if (!mux->pdata->pinctrl_states) {
-		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
-		return -ENOMEM;
+	num_children = of_get_available_child_count(np);
+	if (num_children < 0) {
+		dev_err(mux->dev, "Unable to count available children: %d\n",
+			num_children);
+		return num_children;
 	}
 
-	for (i = 0; i < num_names; i++) {
-		ret = of_property_read_string_index(np, "pinctrl-names", i,
-			&mux->pdata->pinctrl_states[mux->pdata->bus_count]);
-		if (ret < 0) {
-			dev_err(mux->dev, "Cannot parse pinctrl-names: %d\n",
-				ret);
-			return ret;
-		}
-		if (!strcmp(mux->pdata->pinctrl_states[mux->pdata->bus_count],
-			    "idle")) {
-			if (i != num_names - 1) {
-				dev_err(mux->dev, "idle state must be last\n");
-				return -EINVAL;
-			}
-			mux->pdata->pinctrl_state_idle = "idle";
-		} else {
-			mux->pdata->bus_count++;
-		}
+	if (num_names < num_children) {
+		dev_err(mux->dev, "Found less pinctrl states than children\n");
+		return -EINVAL;
 	}
 
 	adapter_np = of_parse_phandle(np, "i2c-parent", 0);
@@ -118,6 +105,39 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 	mux->pdata->parent_bus_num = i2c_adapter_id(adapter);
 	put_device(&adapter->dev);
 
+	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
+		sizeof(*mux->pdata->pinctrl_states) * num_children,
+		GFP_KERNEL);
+	if (!mux->pdata->pinctrl_states) {
+		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
+		return -ENOMEM;
+	}
+
+	of_property_for_each_string(np, "pinctrl-names", prop, state)
+		if (!strcmp(state, "idle"))
+			mux->pdata->pinctrl_state_idle = "idle";
+
+	for_each_available_child_of_node(np, child) {
+		u32 reg;
+
+		ret = of_property_read_u32(child, "reg", &reg);
+		if (ret < 0) {
+			dev_err(mux->dev, "Missing reg property for child node: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = of_property_read_string_index(np,
+				    "pinctrl-names", reg, &state);
+		if (ret < 0) {
+			dev_err(mux->dev, "Cannot parse pinctrl-names for mux %d: %d\n",
+				reg, ret);
+			return ret;
+		}
+
+		mux->pdata->pinctrl_states[mux->pdata->bus_count++] = state;
+	}
+
 	return 0;
 }
 #else
-- 
2.1.0

[PATCH v2 2/4] devicetree: vendor-prefixes: Add CompuLab to known vendors

[Sebastian Hesselbarth]

This adds "compulab" as a vendor-prefix for CompuLab (compulab.co.il),
an Israeli company that builds ARM-based SoMs and CoMs.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Gabriel Dobato <dobatog@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 389ca1347a77..03d139c1174b 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -42,6 +42,7 @@ cirrus	Cirrus Logic, Inc.
 cloudengines	Cloud Engines, Inc.
 cnm	Chips&Media, Inc.
 cnxt	Conexant Systems, Inc.
+compulab	CompuLab
 cortina	Cortina Systems, Inc.
 cosmic	Cosmic Circuits
 crystalfontz	Crystalfontz America, Inc.
-- 
2.1.0

[PATCH v2 3/4] ARM: dts: dove: Add internal i2c multiplexer node

[Sebastian Hesselbarth]

This adds a i2c-mux-pinctrl node to dove.dtsi for the internal i2c
mux found on Dove SoCs. Up to now, we had no board using any of the
two additional i2c busses, so make sure the change does not break
any existing boards.

Therefore, we rename the i2c-controller node label to "i2c" and
enable it by default. Also, the dedicated sub-bus (now "i2c0") is
enabled by default. The two optional sub-busses require additional
external pin-muxing, so disable them by default.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Gabriel Dobato <dobatog@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: linux-i2c@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/boot/dts/dove.dtsi | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 9ad829523a13..b3340e862b0e 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -28,6 +28,42 @@
 		};
 	};
 
+	i2c-mux {
+		compatible = "i2c-mux-pinctrl";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c-parent = <&i2c>;
+
+		pinctrl-names = "i2c0", "i2c1", "i2c2";
+		pinctrl-0 = <&pmx_i2cmux_0>;
+		pinctrl-1 = <&pmx_i2cmux_1>;
+		pinctrl-2 = <&pmx_i2cmux_2>;
+
+		i2c0: i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "okay";
+		};
+
+		i2c1: i2c@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			/* Requires pmx_i2c1 on i2c controller node */
+			status = "disabled";
+		};
+
+		i2c2: i2c@2 {
+			reg = <2>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			/* Requires pmx_i2c2 on i2c controller node */
+			status = "disabled";
+		};
+	};
+
 	l2: l2-cache {
 		compatible = "marvell,tauros2-cache";
 		marvell,tauros2-cache-features = <0>;
@@ -123,7 +159,7 @@
 				status = "disabled";
 			};
 
-			i2c0: i2c-ctrl@11000 {
+			i2c: i2c-ctrl@11000 {
 				compatible = "marvell,mv64xxx-i2c";
 				reg = <0x11000 0x20>;
 				#address-cells = <1>;
@@ -132,7 +168,7 @@
 				clock-frequency = <400000>;
 				timeout-ms = <1000>;
 				clocks = <&core_clk 0>;
-				status = "disabled";
+				status = "okay";
 			};
 
 			uart0: serial@12000 {
-- 
2.1.0

Re: [PATCH v2 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 02/27/2015 05:24 AM, Sebastian Hesselbarth wrote:
> I2C mux pinctrl driver currently determines the number of sub-busses by
> counting available pinctrl-names. Unfortunately, this requires each
> incarnation of the devicetree node with different available sub-busses
> to be rewritten.
>
> This patch reworks i2c-mux-pinctrl driver to count the number of
> available sub-nodes instead. The rework should be compatible to the old
> way of probing for sub-busses and additionally allows to disable unused
> sub-busses with standard DT property status = "disabled".
>
> This also amends the corresponding devicetree binding documentation to
> reflect the new functionality to disable unused sub-nodes. While at it,
> also fix two references to binding documentation files that miss an "i2c-"
> prefix.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt

> -For each named state defined in the pinctrl-names property, an I2C child bus
> -will be created. I2C child bus numbers are assigned based on the index into
> -the pinctrl-names property.
> +For each enabled child node an I2C child bus will be created. I2C child bus
> +numbers are assigned based on the order of child nodes.

I think that I2C bus numbers are an internal concept for the OS. As 
such, we should probably remove any mention re: the bus numbers from the 
binding.

> -The only exception is that no bus will be created for a state named "idle". If
> -such a state is defined, it must be the last entry in pinctrl-names. For
> -example:
> +There must be a corresponding pinctrl-names entry for each enabled child
> +node at the position of the child node's "reg" property. Also, there can be
> +an idle pinctrl state defined at the end of possible pinctrl states. If such
> +a state is defined, it must be the last entry in pinctrl-names. For example:

What about gaps in the numbering sequence? IIRC, in a situation with 5 
nodes with reg 0, 1, 2, 3, 4 but where only the nodes with reg of 1, 3 
enabled, we only want 2 entries in pinctrl-names? If so, "at the 
position of the child node's "reg" property" isn't correct, since "at 
the position" implies there must be gaps in pinctrl-names. "In the same 
order as the reg property values for enabled subnodes" might be a better 
description.

Perhaps I'm misremembering and you explicitly didn't want to remove 
entries from pinctrl-names if child nodes were disabled? If so, then 
surely then in the text above, "for each enabled child" should be 
replaced with "for each child"?

> @@ -68,6 +68,7 @@ Example:
>   		pinctrl-1 = <&state_i2cmux_pta>;
>   		pinctrl-2 = <&state_i2cmux_idle>;
>
> +		/* Enabled child bus 0 */
>   		i2c@0 {
>   			reg = <0>;
>   			#address-cells = <1>;
> @@ -79,10 +80,12 @@ Example:
>   			};
>   		};
>
> +		/* Disabled child bus 1 */
>   		i2c@1 {
>   			reg = <1>;
>   			#address-cells = <1>;
>   			#size-cells = <0>;
> +			status = "disabled";

To make the example cover more cases, perhaps make child node i2c@0 
disabled and i2c@1 enabled. Then, the example would show what happens to 
pinctrl-names when there are gaps in the reg property numbering space of 
enabled children?

Re: [PATCH v2 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 02.03.2015 21:01, Stephen Warren wrote:
> On 02/27/2015 05:24 AM, Sebastian Hesselbarth wrote:
>> I2C mux pinctrl driver currently determines the number of sub-busses by
>> counting available pinctrl-names. Unfortunately, this requires each
>> incarnation of the devicetree node with different available sub-busses
>> to be rewritten.
>>
>> This patch reworks i2c-mux-pinctrl driver to count the number of
>> available sub-nodes instead. The rework should be compatible to the old
>> way of probing for sub-busses and additionally allows to disable unused
>> sub-busses with standard DT property status = "disabled".
>>
>> This also amends the corresponding devicetree binding documentation to
>> reflect the new functionality to disable unused sub-nodes. While at it,
>> also fix two references to binding documentation files that miss an
>> "i2c-"
>> prefix.
>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
>
>> -For each named state defined in the pinctrl-names property, an I2C
>> child bus
>> -will be created. I2C child bus numbers are assigned based on the
>> index into
>> -the pinctrl-names property.
>> +For each enabled child node an I2C child bus will be created. I2C
>> child bus
>> +numbers are assigned based on the order of child nodes.
>
> I think that I2C bus numbers are an internal concept for the OS. As
> such, we should probably remove any mention re: the bus numbers from the
> binding.

Stephen,

yeah as you can see I am struggling to find a good documentation. I
agree that we should get rid of the bus number thing above.

>> -The only exception is that no bus will be created for a state named
>> "idle". If
>> -such a state is defined, it must be the last entry in pinctrl-names. For
>> -example:
>> +There must be a corresponding pinctrl-names entry for each enabled child
>> +node at the position of the child node's "reg" property. Also, there
>> can be
>> +an idle pinctrl state defined at the end of possible pinctrl states.
>> If such
>> +a state is defined, it must be the last entry in pinctrl-names. For
>> example:
>
> What about gaps in the numbering sequence? IIRC, in a situation with 5
> nodes with reg 0, 1, 2, 3, 4 but where only the nodes with reg of 1, 3
> enabled, we only want 2 entries in pinctrl-names? If so, "at the
> position of the child node's "reg" property" isn't correct, since "at
> the position" implies there must be gaps in pinctrl-names. "In the same
> order as the reg property values for enabled subnodes" might be a better
> description.

Good point. The idea was to _have_ gaps in pinctrl-names to allow to
configure the current mux layout by status properties only. The existing
implementation (and docu) suggested you have to amend pinctrl-names to
achieve a specific setup.

> Perhaps I'm misremembering and you explicitly didn't want to remove
> entries from pinctrl-names if child nodes were disabled? If so, then
> surely then in the text above, "for each enabled child" should be
> replaced with "for each child"?

True.

>> @@ -68,6 +68,7 @@ Example:
>>           pinctrl-1 = <&state_i2cmux_pta>;
>>           pinctrl-2 = <&state_i2cmux_idle>;
>>
>> +        /* Enabled child bus 0 */
>>           i2c@0 {
>>               reg = <0>;
>>               #address-cells = <1>;
>> @@ -79,10 +80,12 @@ Example:
>>               };
>>           };
>>
>> +        /* Disabled child bus 1 */
>>           i2c@1 {
>>               reg = <1>;
>>               #address-cells = <1>;
>>               #size-cells = <0>;
>> +            status = "disabled";
>
> To make the example cover more cases, perhaps make child node i2c@0
> disabled and i2c@1 enabled. Then, the example would show what happens to
> pinctrl-names when there are gaps in the reg property numbering space of
> enabled children?

The idea was to make nothing happen to pinctrl-names if you enable/
disable any of the children. But I can move the disabled status to
i2c@0 to make it more clear that there should still be a pinctrl-names
cell for it.

Sebastian

[PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.

This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an "i2c-"
prefix.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changelog:

v2->v3:
- remove mention of "I2C bus numbers" (Suggested by Stephen Warren)
- require pinctrl-names property for "each child" instead of
  "each enabled child" (Suggested by Stephen Warren)
- swap enabled/disabled child nodes (Suggested by Stephen Warren)

v1->v2:
- added a Tested-by for i2c-mux-pinctrl changes from Stepen Warren.
- reworded i2c-mux-pinctrl devicetree doc changes
  (Suggested by Stephen Warren).

Patches 2/4 - 4/4 remain unchanged.

Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
Cc: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Cc: Gregory Clement <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Gabriel Dobato <dobatog-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    | 21 ++++---
 drivers/i2c/muxes/i2c-mux-pinctrl.c                | 70 ++++++++++++++--------
 2 files changed, 58 insertions(+), 33 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
index ae8af1694e95..cd94a0f64d76 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
@@ -28,17 +28,18 @@ Also required are:
 * Standard pinctrl properties that specify the pin mux state for each child
   bus. See ../pinctrl/pinctrl-bindings.txt.
 
-* Standard I2C mux properties. See mux.txt in this directory.
+* Standard I2C mux properties. See i2c-mux.txt in this directory.
 
-* I2C child bus nodes. See mux.txt in this directory.
+* I2C child bus nodes. See i2c-mux.txt in this directory.
 
-For each named state defined in the pinctrl-names property, an I2C child bus
-will be created. I2C child bus numbers are assigned based on the index into
-the pinctrl-names property.
+For each enabled child node an I2C child bus will be created.
 
-The only exception is that no bus will be created for a state named "idle". If
-such a state is defined, it must be the last entry in pinctrl-names. For
-example:
+There must be a corresponding pinctrl-names entry for each child node at the
+position of the child node's "reg" property.
+
+Also, there can be an idle pinctrl state defined at the end of possible pinctrl
+states. If such a state is defined, it must be the last entry in pinctrl-names.
+For example:
 
 	pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
 	pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
@@ -68,10 +69,12 @@ Example:
 		pinctrl-1 = <&state_i2cmux_pta>;
 		pinctrl-2 = <&state_i2cmux_idle>;
 
+		/* Disabled child bus 0 */
 		i2c@0 {
 			reg = <0>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			status = "disabled";
 
 			eeprom {
 				compatible = "eeprom";
@@ -79,10 +82,12 @@ Example:
 			};
 		};
 
+		/* Enabled child bus 1 */
 		i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			status = "okay";
 
 			eeprom {
 				compatible = "eeprom";
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index b48378c4b40d..033dacfabfdf 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -56,9 +56,12 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 				struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	int num_names, i, ret;
+	struct device_node *child;
+	struct property *prop;
+	int num_names, num_children, ret;
 	struct device_node *adapter_np;
 	struct i2c_adapter *adapter;
+	const char *state;
 
 	if (!np)
 		return 0;
@@ -77,32 +80,16 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 		return num_names;
 	}
 
-	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
-		sizeof(*mux->pdata->pinctrl_states) * num_names,
-		GFP_KERNEL);
-	if (!mux->pdata->pinctrl_states) {
-		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
-		return -ENOMEM;
+	num_children = of_get_available_child_count(np);
+	if (num_children < 0) {
+		dev_err(mux->dev, "Unable to count available children: %d\n",
+			num_children);
+		return num_children;
 	}
 
-	for (i = 0; i < num_names; i++) {
-		ret = of_property_read_string_index(np, "pinctrl-names", i,
-			&mux->pdata->pinctrl_states[mux->pdata->bus_count]);
-		if (ret < 0) {
-			dev_err(mux->dev, "Cannot parse pinctrl-names: %d\n",
-				ret);
-			return ret;
-		}
-		if (!strcmp(mux->pdata->pinctrl_states[mux->pdata->bus_count],
-			    "idle")) {
-			if (i != num_names - 1) {
-				dev_err(mux->dev, "idle state must be last\n");
-				return -EINVAL;
-			}
-			mux->pdata->pinctrl_state_idle = "idle";
-		} else {
-			mux->pdata->bus_count++;
-		}
+	if (num_names < num_children) {
+		dev_err(mux->dev, "Found less pinctrl states than children\n");
+		return -EINVAL;
 	}
 
 	adapter_np = of_parse_phandle(np, "i2c-parent", 0);
@@ -118,6 +105,39 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
 	mux->pdata->parent_bus_num = i2c_adapter_id(adapter);
 	put_device(&adapter->dev);
 
+	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
+		sizeof(*mux->pdata->pinctrl_states) * num_children,
+		GFP_KERNEL);
+	if (!mux->pdata->pinctrl_states) {
+		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
+		return -ENOMEM;
+	}
+
+	of_property_for_each_string(np, "pinctrl-names", prop, state)
+		if (!strcmp(state, "idle"))
+			mux->pdata->pinctrl_state_idle = "idle";
+
+	for_each_available_child_of_node(np, child) {
+		u32 reg;
+
+		ret = of_property_read_u32(child, "reg", &reg);
+		if (ret < 0) {
+			dev_err(mux->dev, "Missing reg property for child node: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = of_property_read_string_index(np,
+				    "pinctrl-names", reg, &state);
+		if (ret < 0) {
+			dev_err(mux->dev, "Cannot parse pinctrl-names for mux %d: %d\n",
+				reg, ret);
+			return ret;
+		}
+
+		mux->pdata->pinctrl_states[mux->pdata->bus_count++] = state;
+	}
+
 	return 0;
 }
 #else
-- 
2.1.0

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 03/09/2015 06:21 AM, Sebastian Hesselbarth wrote:
> I2C mux pinctrl driver currently determines the number of sub-busses by
> counting available pinctrl-names. Unfortunately, this requires each
> incarnation of the devicetree node with different available sub-busses
> to be rewritten.
>
> This patch reworks i2c-mux-pinctrl driver to count the number of
> available sub-nodes instead. The rework should be compatible to the old
> way of probing for sub-busses and additionally allows to disable unused
> sub-busses with standard DT property status = "disabled".
>
> This also amends the corresponding devicetree binding documentation to
> reflect the new functionality to disable unused sub-nodes. While at it,
> also fix two references to binding documentation files that miss an "i2c-"
> prefix.

The DT binding changes at least,
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 10.03.2015 17:28, Stephen Warren wrote:
> On 03/09/2015 06:21 AM, Sebastian Hesselbarth wrote:
>> I2C mux pinctrl driver currently determines the number of sub-busses by
>> counting available pinctrl-names. Unfortunately, this requires each
>> incarnation of the devicetree node with different available sub-busses
>> to be rewritten.
>>
>> This patch reworks i2c-mux-pinctrl driver to count the number of
>> available sub-nodes instead. The rework should be compatible to the old
>> way of probing for sub-busses and additionally allows to disable unused
>> sub-busses with standard DT property status = "disabled".
>>
>> This also amends the corresponding devicetree binding documentation to
>> reflect the new functionality to disable unused sub-nodes. While at it,
>> also fix two references to binding documentation files that miss an
>> "i2c-"
>> prefix.
>
> The DT binding changes at least,
> Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Wolfram,

are you going to pick this patch through your tree now that Stephen
ack'ed the binding documentation change, too?

I can also split the patch up into driver/doc changes if you prefer.

Sebastian

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 7819 bytes --]

On Mon, Mar 09, 2015 at 01:21:05PM +0100, Sebastian Hesselbarth wrote:
> I2C mux pinctrl driver currently determines the number of sub-busses by
> counting available pinctrl-names. Unfortunately, this requires each
> incarnation of the devicetree node with different available sub-busses
> to be rewritten.
> 
> This patch reworks i2c-mux-pinctrl driver to count the number of
> available sub-nodes instead. The rework should be compatible to the old
> way of probing for sub-busses and additionally allows to disable unused
> sub-busses with standard DT property status = "disabled".

Not sure about this change. With DYNAMIC_OF these days, you can't rely
that 'disabled' stays disabled all the time. My gut feeling tells me
that people will want to use this someday.

> 
> This also amends the corresponding devicetree binding documentation to
> reflect the new functionality to disable unused sub-nodes. While at it,
> also fix two references to binding documentation files that miss an "i2c-"
> prefix.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> ---
> Changelog:
> 
> v2->v3:
> - remove mention of "I2C bus numbers" (Suggested by Stephen Warren)
> - require pinctrl-names property for "each child" instead of
>   "each enabled child" (Suggested by Stephen Warren)
> - swap enabled/disabled child nodes (Suggested by Stephen Warren)
> 
> v1->v2:
> - added a Tested-by for i2c-mux-pinctrl changes from Stepen Warren.
> - reworded i2c-mux-pinctrl devicetree doc changes
>   (Suggested by Stephen Warren).
> 
> Patches 2/4 - 4/4 remain unchanged.
> 
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Gabriel Dobato <dobatog@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: linux-i2c@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    | 21 ++++---
>  drivers/i2c/muxes/i2c-mux-pinctrl.c                | 70 ++++++++++++++--------
>  2 files changed, 58 insertions(+), 33 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
> index ae8af1694e95..cd94a0f64d76 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
> @@ -28,17 +28,18 @@ Also required are:
>  * Standard pinctrl properties that specify the pin mux state for each child
>    bus. See ../pinctrl/pinctrl-bindings.txt.
>  
> -* Standard I2C mux properties. See mux.txt in this directory.
> +* Standard I2C mux properties. See i2c-mux.txt in this directory.
>  
> -* I2C child bus nodes. See mux.txt in this directory.
> +* I2C child bus nodes. See i2c-mux.txt in this directory.
>  
> -For each named state defined in the pinctrl-names property, an I2C child bus
> -will be created. I2C child bus numbers are assigned based on the index into
> -the pinctrl-names property.
> +For each enabled child node an I2C child bus will be created.
>  
> -The only exception is that no bus will be created for a state named "idle". If
> -such a state is defined, it must be the last entry in pinctrl-names. For
> -example:
> +There must be a corresponding pinctrl-names entry for each child node at the
> +position of the child node's "reg" property.
> +
> +Also, there can be an idle pinctrl state defined at the end of possible pinctrl
> +states. If such a state is defined, it must be the last entry in pinctrl-names.
> +For example:
>  
>  	pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
>  	pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
> @@ -68,10 +69,12 @@ Example:
>  		pinctrl-1 = <&state_i2cmux_pta>;
>  		pinctrl-2 = <&state_i2cmux_idle>;
>  
> +		/* Disabled child bus 0 */
>  		i2c@0 {
>  			reg = <0>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> +			status = "disabled";
>  
>  			eeprom {
>  				compatible = "eeprom";
> @@ -79,10 +82,12 @@ Example:
>  			};
>  		};
>  
> +		/* Enabled child bus 1 */
>  		i2c@1 {
>  			reg = <1>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> +			status = "okay";
>  
>  			eeprom {
>  				compatible = "eeprom";
> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> index b48378c4b40d..033dacfabfdf 100644
> --- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> @@ -56,9 +56,12 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
>  				struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> -	int num_names, i, ret;
> +	struct device_node *child;
> +	struct property *prop;
> +	int num_names, num_children, ret;
>  	struct device_node *adapter_np;
>  	struct i2c_adapter *adapter;
> +	const char *state;
>  
>  	if (!np)
>  		return 0;
> @@ -77,32 +80,16 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
>  		return num_names;
>  	}
>  
> -	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
> -		sizeof(*mux->pdata->pinctrl_states) * num_names,
> -		GFP_KERNEL);
> -	if (!mux->pdata->pinctrl_states) {
> -		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
> -		return -ENOMEM;
> +	num_children = of_get_available_child_count(np);
> +	if (num_children < 0) {
> +		dev_err(mux->dev, "Unable to count available children: %d\n",
> +			num_children);
> +		return num_children;
>  	}
>  
> -	for (i = 0; i < num_names; i++) {
> -		ret = of_property_read_string_index(np, "pinctrl-names", i,
> -			&mux->pdata->pinctrl_states[mux->pdata->bus_count]);
> -		if (ret < 0) {
> -			dev_err(mux->dev, "Cannot parse pinctrl-names: %d\n",
> -				ret);
> -			return ret;
> -		}
> -		if (!strcmp(mux->pdata->pinctrl_states[mux->pdata->bus_count],
> -			    "idle")) {
> -			if (i != num_names - 1) {
> -				dev_err(mux->dev, "idle state must be last\n");
> -				return -EINVAL;
> -			}
> -			mux->pdata->pinctrl_state_idle = "idle";
> -		} else {
> -			mux->pdata->bus_count++;
> -		}
> +	if (num_names < num_children) {
> +		dev_err(mux->dev, "Found less pinctrl states than children\n");
> +		return -EINVAL;
>  	}
>  
>  	adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> @@ -118,6 +105,39 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
>  	mux->pdata->parent_bus_num = i2c_adapter_id(adapter);
>  	put_device(&adapter->dev);
>  
> +	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
> +		sizeof(*mux->pdata->pinctrl_states) * num_children,
> +		GFP_KERNEL);
> +	if (!mux->pdata->pinctrl_states) {
> +		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
> +		return -ENOMEM;
> +	}
> +
> +	of_property_for_each_string(np, "pinctrl-names", prop, state)
> +		if (!strcmp(state, "idle"))
> +			mux->pdata->pinctrl_state_idle = "idle";
> +
> +	for_each_available_child_of_node(np, child) {
> +		u32 reg;
> +
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		if (ret < 0) {
> +			dev_err(mux->dev, "Missing reg property for child node: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = of_property_read_string_index(np,
> +				    "pinctrl-names", reg, &state);
> +		if (ret < 0) {
> +			dev_err(mux->dev, "Cannot parse pinctrl-names for mux %d: %d\n",
> +				reg, ret);
> +			return ret;
> +		}
> +
> +		mux->pdata->pinctrl_states[mux->pdata->bus_count++] = state;
> +	}
> +
>  	return 0;
>  }
>  #else
> -- 
> 2.1.0
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 18.03.2015 13:30, Wolfram Sang wrote:
> On Mon, Mar 09, 2015 at 01:21:05PM +0100, Sebastian Hesselbarth wrote:
>> I2C mux pinctrl driver currently determines the number of sub-busses by
>> counting available pinctrl-names. Unfortunately, this requires each
>> incarnation of the devicetree node with different available sub-busses
>> to be rewritten.
>>
>> This patch reworks i2c-mux-pinctrl driver to count the number of
>> available sub-nodes instead. The rework should be compatible to the old
>> way of probing for sub-busses and additionally allows to disable unused
>> sub-busses with standard DT property status = "disabled".
>
> Not sure about this change. With DYNAMIC_OF these days, you can't rely
> that 'disabled' stays disabled all the time. My gut feeling tells me
> that people will want to use this someday.

Possible. But this change just makes i2c-mux-pinctrl honor status
property at all. There is no functional change except it now allows
you to disable any of the sub-busses.

I agree that this driver still does not cope well with DYNAMIC_OF but
neither did the former implementation. How about we settle this driver
to this implementation now and wait for any maniac that wants to use it
the way you are suggesting above?

The other option would be to leave the driver as is - but at least on
Dove where the muxing-options are not used often, it will always create
4 i2c-busses (controller plus the three muxing options) on any board
even though the pins are not accessible at all. I think that will just
create massive confusion from the user point-of-view?

BTW, I have received a patchwork update notification - it may be
unrelated but I prefer the Dove dts/dtsi changes to go through mvebu
tree.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

On Wed, Mar 18, 2015 at 02:23:18PM +0100, Sebastian Hesselbarth wrote:
> On 18.03.2015 13:30, Wolfram Sang wrote:
> >On Mon, Mar 09, 2015 at 01:21:05PM +0100, Sebastian Hesselbarth wrote:
> >>I2C mux pinctrl driver currently determines the number of sub-busses by
> >>counting available pinctrl-names. Unfortunately, this requires each
> >>incarnation of the devicetree node with different available sub-busses
> >>to be rewritten.
> >>
> >>This patch reworks i2c-mux-pinctrl driver to count the number of
> >>available sub-nodes instead. The rework should be compatible to the old
> >>way of probing for sub-busses and additionally allows to disable unused
> >>sub-busses with standard DT property status = "disabled".
> >
> >Not sure about this change. With DYNAMIC_OF these days, you can't rely
> >that 'disabled' stays disabled all the time. My gut feeling tells me
> >that people will want to use this someday.
> 
> Possible. But this change just makes i2c-mux-pinctrl honor status
> property at all. There is no functional change except it now allows
> you to disable any of the sub-busses.

Actually, this is the feature I like. However, I wonder if we shouldn't
have that in the core, say in of_i2c_register_devices()?

> I agree that this driver still does not cope well with DYNAMIC_OF but
> neither did the former implementation. How about we settle this driver
> to this implementation now and wait for any maniac that wants to use it
> the way you are suggesting above?

Sure. I don't want you to make this driver OF_DYNAMIC compatible. I just
thought it makes it harder, though, e.g. you allocate memory for the
number of active busses not the number of possibilities, so that would
have to be reverted by the "maniac". I am still at the glimpse level,
but what if we let the mux-pinctrl parse all the data (even for disabled
busses), but only the enabled ones will get a muxed adapter because this
is handled in of_i2c_register_devices()?

> BTW, I have received a patchwork update notification - it may be
> unrelated but I prefer the Dove dts/dtsi changes to go through mvebu
> tree.

Yes, I only take dts patches in rare cases.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 18.03.2015 15:00, Wolfram Sang wrote:
> On Wed, Mar 18, 2015 at 02:23:18PM +0100, Sebastian Hesselbarth wrote:
>> Possible. But this change just makes i2c-mux-pinctrl honor status
>> property at all. There is no functional change except it now allows
>> you to disable any of the sub-busses.
>
> Actually, this is the feature I like. However, I wonder if we shouldn't
> have that in the core, say in of_i2c_register_devices()?

Hmm, looking at of_i2c_register_devices():

	for_each_available_child_of_node(adap->dev.of_node, node)
		of_i2c_register_device(adap, node);

already honors status properties by using for_each_available_foo.
Therefore, i2c-core will also skip i2c device nodes disabled by
status property.

>> I agree that this driver still does not cope well with DYNAMIC_OF but
>> neither did the former implementation. How about we settle this driver
>> to this implementation now and wait for any maniac that wants to use it
>> the way you are suggesting above?
>
> Sure. I don't want you to make this driver OF_DYNAMIC compatible. I just
> thought it makes it harder, though, e.g. you allocate memory for the
> number of active busses not the number of possibilities, so that would
> have to be reverted by the "maniac". I am still at the glimpse level,
> but what if we let the mux-pinctrl parse all the data (even for disabled
> busses), but only the enabled ones will get a muxed adapter because this
> is handled in of_i2c_register_devices()?

I am not too deep into i2c-core, but AFAIKS i2c-mux-pinctrl is not an
i2c device but an i2c mux that is dealt with differently? It is not
probed with of_i2c_register_devices() but as a separate platform_device
with a reference to the parent i2c bus.

About the memory allocation for the maximum potential number of muxes:
We would need some way to distinguish disabled from enabled muxes in
i2c-mux-pinctrl's platform_data.

i2c_mux_pinctrl_probe() is basically DT-agnostic and it should
definitely stay that way. Currently, each mux within pd->bus_count
requires a non-NULL pd->pinctrl_states[i] otherwise _probe() will bail
out for all sub-busses.

We could rework it to

(a) deal with each sub-bus individually with respect to 
pinctrl_lookup_state() and i2c_add_mux_adapter()

and

(b) allow (and skip) muxes with pinctrl_states[i] == NULL for now and
let the "maniac" deal with storing/re-probing the corresponding
pinctrl_state name once it gets dynamically enabled.

I am still not too eager working on it but if you insist, I can see
what I can do as long as Stephen sticks with testing it on Tegra. ;)

Sebastian

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3495 bytes --]


> >>Possible. But this change just makes i2c-mux-pinctrl honor status
> >>property at all. There is no functional change except it now allows
> >>you to disable any of the sub-busses.
> >
> >Actually, this is the feature I like. However, I wonder if we shouldn't
> >have that in the core, say in of_i2c_register_devices()?
> 
> Hmm, looking at of_i2c_register_devices():
> 
> 	for_each_available_child_of_node(adap->dev.of_node, node)
> 		of_i2c_register_device(adap, node);
> 
> already honors status properties by using for_each_available_foo.
> Therefore, i2c-core will also skip i2c device nodes disabled by
> status property.

Yes, but only child nodes, not the complete bus. Here is an RFC of what
I mean:

From: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Subject: [RFC] i2c: of: always check if busses are enabled

Allow all busses to have a "status" property which allows busses to not
be probed when set to "disabled". Needed for DTS overlays with i2c mux
scenarios.

Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index fe80f85896e267..d9a3ad2149332e 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1305,8 +1305,8 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 {
 	struct device_node *node;
 
-	/* Only register child devices if the adapter has a node pointer set */
-	if (!adap->dev.of_node)
+	/* Only register childs if adapter has a node pointer with enabled status */
+	if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node))
 		return;
 
 	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");

> I am not too deep into i2c-core, but AFAIKS i2c-mux-pinctrl is not an
> i2c device but an i2c mux that is dealt with differently? It is not
> probed with of_i2c_register_devices() but as a separate platform_device
> with a reference to the parent i2c bus.

Yes, but the busses have seperate nodes. Those nodes with the 'reg =
<0>' properties. And those are matched with of_i2c_register_devices when
the reg-property and the chan_id match.

> About the memory allocation for the maximum potential number of muxes:
> We would need some way to distinguish disabled from enabled muxes in
> i2c-mux-pinctrl's platform_data.

Do we? Can't we claim that every described bus needs a pinctrl entry.
Still, the disabled busses won't be probed? So, we could also think
about putting the above code snippet into i2c-mux when registering
busses, so not only the childs will be skipped but also the whole
bus will not be created.

> i2c_mux_pinctrl_probe() is basically DT-agnostic and it should
> definitely stay that way. Currently, each mux within pd->bus_count

I agree.

> requires a non-NULL pd->pinctrl_states[i] otherwise _probe() will bail
> out for all sub-busses.

And I think that makes sense.

> (b) allow (and skip) muxes with pinctrl_states[i] == NULL for now and
> let the "maniac" deal with storing/re-probing the corresponding
> pinctrl_state name once it gets dynamically enabled.

Why do you need the NULL?

> I am still not too eager working on it but if you insist, I can see
> what I can do as long as Stephen sticks with testing it on Tegra. ;)

Please decide if you want to work on it. Remember, I am not short of
patches to deal with.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

> > I am still not too eager working on it but if you insist, I can see
> > what I can do as long as Stephen sticks with testing it on Tegra. ;)
> 
> Please decide if you want to work on it. Remember, I am not short of
> patches to deal with.

Maybe sounds more harsh than it was meant. I just want to know if I need
to schedule time for this issue.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 03/19/2015 04:09 AM, Wolfram Sang wrote:
>
>>>> Possible. But this change just makes i2c-mux-pinctrl honor status
>>>> property at all. There is no functional change except it now allows
>>>> you to disable any of the sub-busses.
>>>
>>> Actually, this is the feature I like. However, I wonder if we shouldn't
>>> have that in the core, say in of_i2c_register_devices()?
>>
>> Hmm, looking at of_i2c_register_devices():
>>
>> 	for_each_available_child_of_node(adap->dev.of_node, node)
>> 		of_i2c_register_device(adap, node);
>>
>> already honors status properties by using for_each_available_foo.
>> Therefore, i2c-core will also skip i2c device nodes disabled by
>> status property.
>
> Yes, but only child nodes, not the complete bus. Here is an RFC of what
> I mean:
>
> From: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> Subject: [RFC] i2c: of: always check if busses are enabled
>
> Allow all busses to have a "status" property which allows busses to not
> be probed when set to "disabled". Needed for DTS overlays with i2c mux
> scenarios.

> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c

> @@ -1305,8 +1305,8 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
>   {
>   	struct device_node *node;
>
> -	/* Only register child devices if the adapter has a node pointer set */
> -	if (!adap->dev.of_node)
> +	/* Only register childs if adapter has a node pointer with enabled status */
> +	if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node))
>   		return;

That feels a bit odd to me. For a regular non-mux I2C controller, that 
extra case would never trigger if the controller node was disabled, 
since the device core would never probe the controller device itself. 
So, we'd end up with inconsistent paths through the I2C core for regular 
controllers and muxes.

Perhaps better would be to have a mux-specific function to iterate over 
a mux's child nodes and instantiate buses for those. That function would 
check whether each bus node was disabled or not. That'd isolate the 
special case into the place where it was relevant.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

> >-	/* Only register child devices if the adapter has a node pointer set */
> >-	if (!adap->dev.of_node)
> >+	/* Only register childs if adapter has a node pointer with enabled status */
> >+	if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node))
> >  		return;
> 
> That feels a bit odd to me. For a regular non-mux I2C controller, that extra
> case would never trigger if the controller node was disabled, since the
> device core would never probe the controller device itself. So, we'd end up
> with inconsistent paths through the I2C core for regular controllers and
> muxes.

I first thought the no-op for the non-mux case wouldn't hurt, but I
agree about the consistent code path. I mentioned in my previous mail
that i2c-mux might be a better place for this...

> Perhaps better would be to have a mux-specific function to iterate over a
> mux's child nodes and instantiate buses for those. That function would check
> whether each bus node was disabled or not. That'd isolate the special case
> into the place where it was relevant.

... so I wonder what you think about putting the
of_device_is_available() check into i2c_add_mux_adapter() once the
reg-property and chan_id have been matched?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren]

On 03/19/2015 10:02 AM, Wolfram Sang wrote:
>>> -	/* Only register child devices if the adapter has a node pointer set */
>>> -	if (!adap->dev.of_node)
>>> +	/* Only register childs if adapter has a node pointer with enabled status */
>>> +	if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node))
>>>   		return;
>>
>> That feels a bit odd to me. For a regular non-mux I2C controller, that extra
>> case would never trigger if the controller node was disabled, since the
>> device core would never probe the controller device itself. So, we'd end up
>> with inconsistent paths through the I2C core for regular controllers and
>> muxes.
>
> I first thought the no-op for the non-mux case wouldn't hurt, but I
> agree about the consistent code path. I mentioned in my previous mail
> that i2c-mux might be a better place for this...
>
>> Perhaps better would be to have a mux-specific function to iterate over a
>> mux's child nodes and instantiate buses for those. That function would check
>> whether each bus node was disabled or not. That'd isolate the special case
>> into the place where it was relevant.
>
> ... so I wonder what you think about putting the
> of_device_is_available() check into i2c_add_mux_adapter() once the
> reg-property and chan_id have been matched?

I think that looks like a good place, yes.

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 19.03.2015 17:02, Wolfram Sang wrote:
>> Perhaps better would be to have a mux-specific function to iterate over a
>> mux's child nodes and instantiate buses for those. That function would check
>> whether each bus node was disabled or not. That'd isolate the special case
>> into the place where it was relevant.
>
> ... so I wonder what you think about putting the
> of_device_is_available() check into i2c_add_mux_adapter() once the
> reg-property and chan_id have been matched?
>

Ok, I see what you mean. I had a look at the place in question and
wonder what to return from i2c_add_mux_adapter() in the disabled
case so that i2c-mux-pinctrl is still happy with the returned value.

I guess what you want to have is that i2c_add_adapter() is not called
for the disabled case, right?

Is the i2c_adapter struct prepared in i2c_mux_add_adapter() still valid
if i2c_add_adapter() is not called?

Sorry, I am not too deep into i2c subsystem, I just reworked i2c-mux-
pinctrl to make it work on Dove. If you are fine with giving me some
guidance how you prefer to have it done, I can try to free some spare
time. Unfortunately there is already little of it, so please don't
expect a quick tested patch.

Sebastian

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]


> Ok, I see what you mean. I had a look at the place in question and
> wonder what to return from i2c_add_mux_adapter() in the disabled
> case so that i2c-mux-pinctrl is still happy with the returned value.

Ouch, you are right. The crux of interfaces returning NULL instead of an
ERR_PTR :( I'll have a look, I maybe started to fix this somewhen.

> I guess what you want to have is that i2c_add_adapter() is not called
> for the disabled case, right?

I think that makes sense.

> Is the i2c_adapter struct prepared in i2c_mux_add_adapter() still valid
> if i2c_add_adapter() is not called?

I will have a closer look to the issue this weekend.

> Sorry, I am not too deep into i2c subsystem, I just reworked i2c-mux-
> pinctrl to make it work on Dove. If you are fine with giving me some
> guidance how you prefer to have it done, I can try to free some spare
> time.

Cool, thanks. Learning by doing is a good way to get such knowledge :)

> Unfortunately there is already little of it, so please don't
> expect a quick tested patch.

I understand.

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 359 bytes --]


> > I guess what you want to have is that i2c_add_adapter() is not called
> > for the disabled case, right?
> 
> I think that makes sense.

But maybe we should just start simple and keep calling i2c_add_adapter()
for the disabled case. We will just skip probing devices on the bus.
Would that help the issue you were originally trying to solve?



[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 21.03.2015 22:00, Wolfram Sang wrote:
>>> I guess what you want to have is that i2c_add_adapter() is not called
>>> for the disabled case, right?
>>
>> I think that makes sense.
>
> But maybe we should just start simple and keep calling i2c_add_adapter()
> for the disabled case. We will just skip probing devices on the bus.
> Would that help the issue you were originally trying to solve?

It is not about probing devices on the mux sub-busses, I'd expect no
devices on the optional sub-busses in DT on boards where those pins
are not used as i2c.

The idea was to hide those busses completely in particular from
userspace on boards where they'll never be available.

If modifying i2c-mux-pinctrl to respect the sub-bus status property is
such a big issue, I'd rather leave the driver as is and expose all
sub-busses to userspace.

Sebastian

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

> If modifying i2c-mux-pinctrl to respect the sub-bus status property is
> such a big issue, I'd rather leave the driver as is and expose all
> sub-busses to userspace.

Well, dunno what 'big issue' is in your book :) What definately needs to
be done is:

* handle "status" at mux-core level, not mux-driver
* that probably needs us to convert i2c_add_mux_adapter() to return
  ERR_PTRs instead of NULL, so we can distinguish the "disabled" case
* that would mean to fix all existing users

That's all not groundbreaking stuff, but needs caution and thoroughness.
There might be some gory details left, though...

Regards,

   Wolfram

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Sebastian Hesselbarth]

On 23.03.2015 19:32, Wolfram Sang wrote:
>> If modifying i2c-mux-pinctrl to respect the sub-bus status property is
>> such a big issue, I'd rather leave the driver as is and expose all
>> sub-busses to userspace.
>
> Well, dunno what 'big issue' is in your book :) What definately needs to
> be done is:

Wolfram,

I had a look at the code in question again and prepared some patches
to return ERR_PTRs from i2c_add_mux_adapter(), rework users to check
for IS_ERR() and finally skip i2c_add_adapter for the OF disabled case.

I have them compile-tested but I don't have any hardware available
right now.

Anyway, I still have some questions.

> * handle "status" at mux-core level, not mux-driver

I get that.. but:

> * that probably needs us to convert i2c_add_mux_adapter() to return
>    ERR_PTRs instead of NULL, so we can distinguish the "disabled" case

What do you want to return from i2c_add_mux_adapter() if you find an OF
disabled child node?

AFAIKS, there is no explicit errno for a disabled device (node) so all
I can imagine here is to return either the &priv->adap before actually
calling i2c_add_adapter on it or NULL now that we explicitly check for
errors.

> * that would mean to fix all existing users

If we choose to return NULL, those users who can deal with a
missing/disabled sub-bus on the mux can check with IS_ERR()
the others should check with IS_ERR_OR_NULL().

We could also choose to return some other errno (-ENODEV maybe)
but still we'd have to double-check that return value on i2c-mux-pinctrl
and the others too if we don't care that i2c_add_adapter() wasn't
called for a mux.

> That's all not groundbreaking stuff, but needs caution and thoroughness.
> There might be some gory details left, though...

As I said before, the intention was not disable a possible i2c bus that
can be dynamically added/removed on that specific Dove SBC/CM-A510
board but to have a single i2c-mux-pinctrl in the SoC dtsi just because
the SoC has the optional i2c bus muxing.

While thinking about it (and I still think of it as a 'big issue'
compared to the intention of the initial patch) I came to the
conclusion that I should maybe just go for a board-specific
i2c-mux-pinctrl node instead of adding it to the SoC dtsi. That will
also avoid doubled i2c busses on boards with just the default i2c
option.

Sebastian

Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]


> While thinking about it (and I still think of it as a 'big issue'
> compared to the intention of the initial patch) I came to the
> conclusion that I should maybe just go for a board-specific
> i2c-mux-pinctrl node instead of adding it to the SoC dtsi. That will
> also avoid doubled i2c busses on boards with just the default i2c
> option.

Ehrm, then please let me know what you decided on. If you chose the
above road, then I don't need to think about the other questions :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]