All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver
@ 2015-03-17  7:15 Wolfram Sang
  2015-03-18 13:18 ` Laurent Pinchart
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Wolfram Sang @ 2015-03-17  7:15 UTC (permalink / raw)
  To: linux-sh

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

This is the driver which maps n different I2C IP cores to the same bus.
The approach needs discussion.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Changes since last version:

- rebased to 4.0-rc4
- improved documentation
- improved code comments
- added all error paths
- added locking
- bugfixes

 .../devicetree/bindings/i2c/i2c-demux-pinctrl.txt  |  79 ++++++++
 drivers/i2c/muxes/Kconfig                          |   6 +
 drivers/i2c/muxes/Makefile                         |   2 +
 drivers/i2c/muxes/i2c-demux-pinctrl.c              | 220 +++++++++++++++++++++
 4 files changed, 307 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
 create mode 100644 drivers/i2c/muxes/i2c-demux-pinctrl.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
new file mode 100644
index 00000000000000..a740f1d5681fab
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
@@ -0,0 +1,79 @@
+Pinctrl-based I2C Bus DeMux
+
+This binding describes an I2C bus demultiplexer that uses pin multiplexing to
+route the I2C signals, and represents the pin multiplexing configuration using
+the pinctrl device tree bindings. This may be used to select one I2C IP core at
+runtime which may have a better feature set for a given task than another I2C
+IP core on the SoC.
+
+    +------------------------------+
+    | SoC                          |
+    |                              |   +-----+  +-----+
+    |   +------------+             |   | dev |  | dev |
+    |   |I2C IP Core1|--\          |   +-----+  +-----+
+    |   +------------+   \------+  |      |        |
+    |                    |Pinmux|--|------+--------+
+    |   +------------+   +------+  |
+    |   |I2C IP Core2|--/          |
+    |   +------------+             |
+    |                              |
+    +------------------------------+
+
+For n parents, n+1 new busses are created:
+  1st new bus is the demuxed bus with the first parent as the master
+  Nth new bus is the demuxed bus with the N-th parent as the master
+  N+1st new bus is the demuxed bus using the last active master
+  (needed because kernel client drivers need a static bus number for the client)
+
+Example:
+  i2c-0 is native I2C IP core A and first parent of the demuxed bus
+  i2c-1 is native I2C IP core B and second parent of the demuxed bus
+  i2c-2 accesses the demuxed bus with IP core A
+  i2c-3 accesses the demuxed bus with IP core B
+  i2c-4 accesses the demuxed bus with the last used IP core
+
+i2c-0 and i2c-1 should not be used directly. Their pins might not be muxed.
+All kernel I2C clients should be attached to i2c-4, so they have static bus number.
+
+Switching a master currently needs some access to either i2c-2 or i2c-3.
+
+Required properties:
+- compatible: "i2c-demux-pinctrl"
+- i2c-parent: List of phandles of I2C cores to be selected
+
+Also I2C mux properties and child nodes. See mux.txt in this directory.
+The child bus nodes must be connected to sub-bus number 0 as shown in the
+example below.
+
+Also required:
+
+- pinctrl properties for the parent I2C controllers need their pinctrl state
+  for actually driving the bus named "active", not "default"!
+
+Example:
+
+	i2c-demux {
+		compatible = "i2c-demux-pinctrl";
+		i2c-parent = <&iic2>, <&i2c2>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			composite-in@20 {
+				compatible = "adi,adv7180";
+				reg = <0x20>;
+				remote = <&vin1>;
+
+				port {
+					adv7180: endpoint {
+						bus-width = <8>;
+						remote-endpoint = <&vin1ep0>;
+					};
+				};
+			};
+		};
+	};
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index f6d313e528de34..281747d8cf4895 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -60,4 +60,10 @@ config I2C_MUX_PINCTRL
 	  This driver can also be built as a module. If so, the module will be
 	  called pinctrl-i2cmux.
 
+config I2C_DEMUX_PINCTRL
+	tristate "pinctrl-based I2C demultiplexer"
+	depends on PINCTRL
+	help
+	  PROTOYPE! Understand the docs!
+
 endmenu
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 465778b5d5dc86..4d14cb88d83438 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -3,6 +3,8 @@
 
 obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE)	+= i2c-arb-gpio-challenge.o
 
+obj-$(CONFIG_I2C_DEMUX_PINCTRL)		+= i2c-demux-pinctrl.o
+
 obj-$(CONFIG_I2C_MUX_GPIO)	+= i2c-mux-gpio.o
 obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
 obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
new file mode 100644
index 00000000000000..2065e83e857f9a
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
@@ -0,0 +1,220 @@
+/*
+ * Pinctrl based I2C DeMultiplexer PROTOTYPE!
+ *
+ * Copyright (C) 2014 by Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
+ * Copyright (C) 2014 by Renesas Electronics Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; version 2 of the License.
+ *
+ * See the bindings doc for DTS setup.
+ *
+ * For n parents, n+1 new busses are created:
+ *   1st new bus is the demuxed bus with the first parent as the master
+ *   Nth new bus is the demuxed bus with the N-th parent as the master
+ *   N+1st new bus is the demuxed bus using the last active master
+ *   (needed because kernel client drivers need a static bus number for the client)
+ *
+ * Example:
+ *   i2c-0 is native I2C IP core A and first parent of the demuxed bus
+ *   i2c-1 is native I2C IP core B and second parent of the demuxed bus
+ *   i2c-2 accesses the demuxed bus with IP core A
+ *   i2c-3 accesses the demuxed bus with IP core B
+ *   i2c-4 accesses the demuxed bus with the last used IP core
+ *
+ * i2c-0 and i2c-1 should not be used directly. Their pins might not be muxed.
+ * All kernel I2C clients should be attached to i2c-4, so they have static bus number.
+ *
+ * Switching a master currently needs some access to either i2c-2 or i2c-3.
+ * Switching could also be done via sysfs or any other config mechanism.
+ * For this proof-of-concept, extra busses have been used since it simplifies
+ * locking a little.
+ */
+
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct i2c_demux_pinctrl_chan {
+	struct i2c_adapter *parent;
+	struct i2c_adapter *child;
+	struct pinctrl *p;
+	struct pinctrl_state *p_state;
+};
+
+struct i2c_demux_pinctrl_priv {
+	struct mutex mux_lock;
+	u32 last_chan;
+	int num_chan;
+	struct i2c_demux_pinctrl_chan chan[];
+};
+
+static int i2c_demux_select_state(struct i2c_adapter *adap, void *data, u32 new_chan)
+{
+	struct i2c_demux_pinctrl_priv *priv = data;
+	int err;
+
+	mutex_lock(&priv->mux_lock);
+
+	if (new_chan = priv->last_chan)
+		return 0;
+
+	pinctrl_deselect_state(priv->chan[priv->last_chan].p);
+	err = pinctrl_select_state(priv->chan[new_chan].p,
+				   priv->chan[new_chan].p_state);
+	if (!err) {
+		priv->last_chan = new_chan;
+		i2c_mux_reparent(priv->chan[0].child, priv->chan[new_chan].parent);
+	}
+
+	return err;
+}
+
+static int i2c_demux_lock_mux(struct i2c_adapter *adap, void *data, u32 new_chan)
+{
+	struct i2c_demux_pinctrl_priv *priv = data;
+
+	mutex_lock(&priv->mux_lock);
+	return 0;
+}
+
+static int i2c_demux_unlock_mux(struct i2c_adapter *adap, void *data, u32 new_chan)
+{
+	struct i2c_demux_pinctrl_priv *priv = data;
+
+	mutex_unlock(&priv->mux_lock);
+	return 0;
+}
+
+static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
+{
+	struct i2c_demux_pinctrl_priv *priv;
+	struct device_node *np = pdev->dev.of_node;
+	int num_chan, i, j, err;
+
+	num_chan = of_count_phandle_with_args(np, "i2c-parent", NULL);
+	if (num_chan < 2) {
+		dev_err(&pdev->dev, "Need at least two I2C masters to switch\n");
+		return -EINVAL;
+	}
+
+	/* we need an extra channel for the bus with the last active master */
+	num_chan++;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv)
+			   + num_chan * sizeof(struct i2c_demux_pinctrl_chan), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	for (i = 1; i < num_chan; i++) {
+		struct device_node *adap_np;
+		struct i2c_adapter *adap;
+
+		adap_np = of_parse_phandle(np, "i2c-parent", i - 1);
+		if (!adap_np) {
+			dev_err(&pdev->dev, "can't get phandle for parent %d\n", i);
+			err = -ENOENT;
+			goto err_rollback;
+		}
+
+		adap = of_find_i2c_adapter_by_node(adap_np);
+		if (!adap) {
+			dev_dbg(&pdev->dev, "can't find parent bus %d, deferring\n", i);
+			err = -EPROBE_DEFER;
+			goto err_rollback;
+		}
+
+		priv->chan[i].p = devm_pinctrl_get(adap->dev.parent);
+		if (IS_ERR(priv->chan[i].p)) {
+			dev_err(&pdev->dev, "can't get pinctrl for parent %d\n", i);
+			err = PTR_ERR(priv->chan[i].p);
+			goto err_rollback;
+		}
+
+		priv->chan[i].p_state = pinctrl_lookup_state(priv->chan[i].p, "active");
+		if (IS_ERR(priv->chan[i].p_state)) {
+			dev_err(&pdev->dev, "no demux pinctrl state 'active' for parent %d\n", i);
+			err = PTR_ERR(priv->chan[i].p_state);
+			goto err_rollback;
+		}
+
+		priv->chan[i].child = i2c_add_mux_adapter(adap, &pdev->dev, priv,
+						0, i, 0, i2c_demux_select_state, i2c_demux_unlock_mux);
+		if (!priv->chan[i].child) {
+			dev_err(&pdev->dev, "cannot create mux bus %d\n", i);
+			err = -ENXIO;
+			goto err_rollback;
+		}
+
+		priv->chan[i].parent = adap;
+	}
+
+	priv->chan[0].child = i2c_add_mux_adapter(priv->chan[1].parent, &pdev->dev, priv,
+					0, 0, 0, i2c_demux_lock_mux, i2c_demux_unlock_mux);
+	if (!priv->chan[0].child) {
+		dev_err(&pdev->dev, "cannot create mux bus %d\n", 0);
+		err = -ENXIO;
+		goto err_rollback;
+	}
+
+	priv->num_chan = num_chan;
+	mutex_init(&priv->mux_lock);
+
+	platform_set_drvdata(pdev, priv);
+
+	/* switch to first parent as active master */
+	pinctrl_select_state(priv->chan[1].p, priv->chan[1].p_state);
+	priv->last_chan = 1;
+
+	return 0;
+
+err_rollback:
+	for (j = 1; j < i; j++) {
+		i2c_del_mux_adapter(priv->chan[j].child);
+		i2c_put_adapter(priv->chan[j].parent);
+	}
+
+	return err;
+}
+
+static int i2c_demux_pinctrl_remove(struct platform_device *pdev)
+{
+	struct i2c_demux_pinctrl_priv *priv = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < priv->num_chan; i++) {
+		i2c_del_mux_adapter(priv->chan[i].child);
+		if (i)
+			i2c_put_adapter(priv->chan[i].parent);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id i2c_demux_pinctrl_of_match[] = {
+	{ .compatible = "i2c-demux-pinctrl", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, i2c_demux_pinctrl_of_match);
+
+static struct platform_driver i2c_demux_pinctrl_driver = {
+	.driver	= {
+		.name = "i2c-demux-pinctrl",
+		.of_match_table = i2c_demux_pinctrl_of_match,
+	},
+	.probe	= i2c_demux_pinctrl_probe,
+	.remove	= i2c_demux_pinctrl_remove,
+};
+module_platform_driver(i2c_demux_pinctrl_driver);
+
+MODULE_DESCRIPTION("pinctrl-based I2C demux driver");
+MODULE_AUTHOR("Wolfram Sang <wsa@sang-engineering.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:i2c-demux-pinctrl");
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver
  2015-03-17  7:15 [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver Wolfram Sang
@ 2015-03-18 13:18 ` Laurent Pinchart
  2015-03-19 15:53 ` Wolfram Sang
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2015-03-18 13:18 UTC (permalink / raw)
  To: linux-sh

Hi Wolfram,

Thank you for the patch.

On Tuesday 17 March 2015 08:15:24 Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> This is the driver which maps n different I2C IP cores to the same bus.
> The approach needs discussion.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Changes since last version:
> 
> - rebased to 4.0-rc4
> - improved documentation
> - improved code comments
> - added all error paths
> - added locking
> - bugfixes
> 
>  .../devicetree/bindings/i2c/i2c-demux-pinctrl.txt  |  79 ++++++++
>  drivers/i2c/muxes/Kconfig                          |   6 +
>  drivers/i2c/muxes/Makefile                         |   2 +
>  drivers/i2c/muxes/i2c-demux-pinctrl.c              | 220 ++++++++++++++++++
>  4 files changed, 307 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt create mode
> 100644 drivers/i2c/muxes/i2c-demux-pinctrl.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
> b/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt new file mode
> 100644
> index 00000000000000..a740f1d5681fab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
> @@ -0,0 +1,79 @@
> +Pinctrl-based I2C Bus DeMux
> +
> +This binding describes an I2C bus demultiplexer that uses pin multiplexing
> to
> +route the I2C signals, and represents the pin multiplexing configuration
> using
> +the pinctrl device tree bindings. This may be used to select one I2C IP
> core at
> +runtime which may have a better feature set for a given task than another
> I2C
> +IP core on the SoC.
> +
> +    +------------------------------+
> +    | SoC                          |
> +    |                              |   +-----+  +-----+
> +    |   +------------+             |   | dev |  | dev |
> +    |   |I2C IP Core1|--\          |   +-----+  +-----+
> +    |   +------------+   \------+  |      |        |
> +    |                    |Pinmux|--|------+--------+
> +    |   +------------+   +------+  |
> +    |   |I2C IP Core2|--/          |
> +    |   +------------+             |
> +    |                              |
> +    +------------------------------+
> +
> +For n parents, n+1 new busses are created:
> +  1st new bus is the demuxed bus with the first parent as the master
> +  Nth new bus is the demuxed bus with the N-th parent as the master
> +  N+1st new bus is the demuxed bus using the last active master
> +  (needed because kernel client drivers need a static bus number for the
> client)
> +
> +Example:
> +  i2c-0 is native I2C IP core A and first parent of the demuxed bus
> +  i2c-1 is native I2C IP core B and second parent of the demuxed bus
> +  i2c-2 accesses the demuxed bus with IP core A
> +  i2c-3 accesses the demuxed bus with IP core B
> +  i2c-4 accesses the demuxed bus with the last used IP core
> +
> +i2c-0 and i2c-1 should not be used directly. Their pins might not be muxed.
> +All kernel I2C clients should be attached to i2c-4, so they have static
> bus number.
> +
> +Switching a master currently needs some access to either i2c-2 or i2c-3.
> +
> +Required properties:
> +- compatible: "i2c-demux-pinctrl"
> +- i2c-parent: List of phandles of I2C cores to be selected
> +
> +Also I2C mux properties and child nodes. See mux.txt in this directory.
> +The child bus nodes must be connected to sub-bus number 0 as shown in the
> +example below.
> +
> +Also required:
> +
> +- pinctrl properties for the parent I2C controllers need their pinctrl
> state
> +  for actually driving the bus named "active", not "default"!
> +
> +Example:
> +
> +	i2c-demux {
> +		compatible = "i2c-demux-pinctrl";
> +		i2c-parent = <&iic2>, <&i2c2>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		i2c@0 {
> +			reg = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			composite-in@20 {
> +				compatible = "adi,adv7180";
> +				reg = <0x20>;
> +				remote = <&vin1>;
> +
> +				port {
> +					adv7180: endpoint {
> +						bus-width = <8>;
> +						remote-endpoint = <&vin1ep0>;
> +					};
> +				};
> +			};
> +		};
> +	};
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index f6d313e528de34..281747d8cf4895 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -60,4 +60,10 @@ config I2C_MUX_PINCTRL
>  	  This driver can also be built as a module. If so, the module will be
>  	  called pinctrl-i2cmux.
> 
> +config I2C_DEMUX_PINCTRL
> +	tristate "pinctrl-based I2C demultiplexer"
> +	depends on PINCTRL
> +	help
> +	  PROTOYPE! Understand the docs!
> +
>  endmenu
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 465778b5d5dc86..4d14cb88d83438 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -3,6 +3,8 @@
> 
>  obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE)	+= i2c-arb-gpio-challenge.o
> 
> +obj-$(CONFIG_I2C_DEMUX_PINCTRL)		+= i2c-demux-pinctrl.o
> +
>  obj-$(CONFIG_I2C_MUX_GPIO)	+= i2c-mux-gpio.o
>  obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
>  obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> b/drivers/i2c/muxes/i2c-demux-pinctrl.c new file mode 100644
> index 00000000000000..2065e83e857f9a
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -0,0 +1,220 @@
> +/*
> + * Pinctrl based I2C DeMultiplexer PROTOTYPE!
> + *
> + * Copyright (C) 2014 by Wolfram Sang, Sang Engineering
> <wsa@sang-engineering.com> + * Copyright (C) 2014 by Renesas Electronics
> Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; version 2 of the License.
> + *
> + * See the bindings doc for DTS setup.
> + *
> + * For n parents, n+1 new busses are created:
> + *   1st new bus is the demuxed bus with the first parent as the master
> + *   Nth new bus is the demuxed bus with the N-th parent as the master
> + *   N+1st new bus is the demuxed bus using the last active master
> + *   (needed because kernel client drivers need a static bus number for the
> client)
> + *
> + * Example:
> + *   i2c-0 is native I2C IP core A and first parent of the demuxed bus
> + *   i2c-1 is native I2C IP core B and second parent of the demuxed bus
> + *   i2c-2 accesses the demuxed bus with IP core A
> + *   i2c-3 accesses the demuxed bus with IP core B
> + *   i2c-4 accesses the demuxed bus with the last used IP core
> + *
> + * i2c-0 and i2c-1 should not be used directly. Their pins might not be
> muxed.
> + * All kernel I2C clients should be attached to i2c-4, so they have static
> bus number.
> + *
> + * Switching a master currently needs some access to either i2c-2 or i2c-3.
> + * Switching could also be done via sysfs or any other config mechanism.
> + * For this proof-of-concept, extra busses have been used since it
> simplifies
> + * locking a little.

I have mixed feelings to be honest. When using n internal masters muxed on the 
same pins, with pin muxing used as a selector, I could agree that we are 
dealing with n+1 busses, with n busses between the masters and the demux, and 
one external bus. The two extra virtual busses in your example above bother 
me.

This being said, I see this as an attempt to keep the traditional model of I2C 
slaves being children of an I2C master while still departing from it at the 
hardware level. Wouldn't it be better to depart from it from a software point 
of view as well ? This would allow supporting real multi-master 
configurations, but would come with a high refactoring cost in kernel code.

I'm also worried about power management, how do you envision its 
implementation ?

> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/i2c-mux.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +struct i2c_demux_pinctrl_chan {
> +	struct i2c_adapter *parent;
> +	struct i2c_adapter *child;
> +	struct pinctrl *p;
> +	struct pinctrl_state *p_state;
> +};
> +
> +struct i2c_demux_pinctrl_priv {
> +	struct mutex mux_lock;
> +	u32 last_chan;
> +	int num_chan;
> +	struct i2c_demux_pinctrl_chan chan[];
> +};
> +
> +static int i2c_demux_select_state(struct i2c_adapter *adap, void *data, u32
> new_chan) +{
> +	struct i2c_demux_pinctrl_priv *priv = data;
> +	int err;
> +
> +	mutex_lock(&priv->mux_lock);
> +
> +	if (new_chan = priv->last_chan)
> +		return 0;
> +
> +	pinctrl_deselect_state(priv->chan[priv->last_chan].p);
> +	err = pinctrl_select_state(priv->chan[new_chan].p,
> +				   priv->chan[new_chan].p_state);
> +	if (!err) {
> +		priv->last_chan = new_chan;
> +		i2c_mux_reparent(priv->chan[0].child, priv->chan[new_chan].parent);
> +	}
> +
> +	return err;
> +}
> +
> +static int i2c_demux_lock_mux(struct i2c_adapter *adap, void *data, u32
> new_chan) +{
> +	struct i2c_demux_pinctrl_priv *priv = data;
> +
> +	mutex_lock(&priv->mux_lock);
> +	return 0;
> +}
> +
> +static int i2c_demux_unlock_mux(struct i2c_adapter *adap, void *data, u32
> new_chan) +{
> +	struct i2c_demux_pinctrl_priv *priv = data;
> +
> +	mutex_unlock(&priv->mux_lock);
> +	return 0;
> +}
> +
> +static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct i2c_demux_pinctrl_priv *priv;
> +	struct device_node *np = pdev->dev.of_node;
> +	int num_chan, i, j, err;
> +
> +	num_chan = of_count_phandle_with_args(np, "i2c-parent", NULL);
> +	if (num_chan < 2) {
> +		dev_err(&pdev->dev, "Need at least two I2C masters to switch\n");
> +		return -EINVAL;
> +	}
> +
> +	/* we need an extra channel for the bus with the last active master */
> +	num_chan++;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv)
> +			   + num_chan * sizeof(struct i2c_demux_pinctrl_chan), 
GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	for (i = 1; i < num_chan; i++) {
> +		struct device_node *adap_np;
> +		struct i2c_adapter *adap;
> +
> +		adap_np = of_parse_phandle(np, "i2c-parent", i - 1);
> +		if (!adap_np) {
> +			dev_err(&pdev->dev, "can't get phandle for parent %d\n", i);
> +			err = -ENOENT;
> +			goto err_rollback;
> +		}
> +
> +		adap = of_find_i2c_adapter_by_node(adap_np);
> +		if (!adap) {
> +			dev_dbg(&pdev->dev, "can't find parent bus %d, deferring\n", i);
> +			err = -EPROBE_DEFER;
> +			goto err_rollback;
> +		}
> +
> +		priv->chan[i].p = devm_pinctrl_get(adap->dev.parent);
> +		if (IS_ERR(priv->chan[i].p)) {
> +			dev_err(&pdev->dev, "can't get pinctrl for parent %d\n", i);
> +			err = PTR_ERR(priv->chan[i].p);
> +			goto err_rollback;
> +		}
> +
> +		priv->chan[i].p_state = pinctrl_lookup_state(priv->chan[i].p, 
"active");
> +		if (IS_ERR(priv->chan[i].p_state)) {
> +			dev_err(&pdev->dev, "no demux pinctrl state 'active' for parent 
%d\n",
> i); +			err = PTR_ERR(priv->chan[i].p_state);
> +			goto err_rollback;
> +		}
> +
> +		priv->chan[i].child = i2c_add_mux_adapter(adap, &pdev->dev, priv,
> +						0, i, 0, i2c_demux_select_state, 
i2c_demux_unlock_mux);
> +		if (!priv->chan[i].child) {
> +			dev_err(&pdev->dev, "cannot create mux bus %d\n", i);
> +			err = -ENXIO;
> +			goto err_rollback;
> +		}
> +
> +		priv->chan[i].parent = adap;
> +	}
> +
> +	priv->chan[0].child = i2c_add_mux_adapter(priv->chan[1].parent,
> &pdev->dev, priv, +					0, 0, 0, i2c_demux_lock_mux, 
i2c_demux_unlock_mux);
> +	if (!priv->chan[0].child) {
> +		dev_err(&pdev->dev, "cannot create mux bus %d\n", 0);
> +		err = -ENXIO;
> +		goto err_rollback;
> +	}
> +
> +	priv->num_chan = num_chan;
> +	mutex_init(&priv->mux_lock);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* switch to first parent as active master */
> +	pinctrl_select_state(priv->chan[1].p, priv->chan[1].p_state);
> +	priv->last_chan = 1;
> +
> +	return 0;
> +
> +err_rollback:
> +	for (j = 1; j < i; j++) {
> +		i2c_del_mux_adapter(priv->chan[j].child);
> +		i2c_put_adapter(priv->chan[j].parent);
> +	}
> +
> +	return err;
> +}
> +
> +static int i2c_demux_pinctrl_remove(struct platform_device *pdev)
> +{
> +	struct i2c_demux_pinctrl_priv *priv = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < priv->num_chan; i++) {
> +		i2c_del_mux_adapter(priv->chan[i].child);
> +		if (i)
> +			i2c_put_adapter(priv->chan[i].parent);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id i2c_demux_pinctrl_of_match[] = {
> +	{ .compatible = "i2c-demux-pinctrl", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, i2c_demux_pinctrl_of_match);
> +
> +static struct platform_driver i2c_demux_pinctrl_driver = {
> +	.driver	= {
> +		.name = "i2c-demux-pinctrl",
> +		.of_match_table = i2c_demux_pinctrl_of_match,
> +	},
> +	.probe	= i2c_demux_pinctrl_probe,
> +	.remove	= i2c_demux_pinctrl_remove,
> +};
> +module_platform_driver(i2c_demux_pinctrl_driver);
> +
> +MODULE_DESCRIPTION("pinctrl-based I2C demux driver");
> +MODULE_AUTHOR("Wolfram Sang <wsa@sang-engineering.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:i2c-demux-pinctrl");

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver
  2015-03-17  7:15 [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver Wolfram Sang
  2015-03-18 13:18 ` Laurent Pinchart
@ 2015-03-19 15:53 ` Wolfram Sang
  2015-04-21 17:55 ` Laurent Pinchart
  2015-04-21 18:47 ` Wolfram Sang
  3 siblings, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2015-03-19 15:53 UTC (permalink / raw)
  To: linux-sh

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


> > + * Switching a master currently needs some access to either i2c-2 or i2c-3.
> > + * Switching could also be done via sysfs or any other config mechanism.
> > + * For this proof-of-concept, extra busses have been used since it
> > simplifies
> > + * locking a little.
> 
> I have mixed feelings to be honest. When using n internal masters muxed on the 
> same pins, with pin muxing used as a selector, I could agree that we are 
> dealing with n+1 busses, with n busses between the masters and the demux, and 
> one external bus. The two extra virtual busses in your example above bother 
> me.

That was chosen so the access to the "virtual" bus would automatically
do the pinctrl change. As I said somewhere, this could be handled
differently.

> This being said, I see this as an attempt to keep the traditional model of I2C 
> slaves being children of an I2C master while still departing from it at the 
> hardware level. Wouldn't it be better to depart from it from a software point 
> of view as well ? This would allow supporting real multi-master 
> configurations, but would come with a high refactoring cost in kernel code.

You mean the slaves belong to an i2c-bus and this bus can be connected
to masters? Yeah, that would be quite a change. And I am still not
convinced if that would solve the issue that the driver model does not
support re-parenting but rather suggests to delete and recreate the
device. From device_add():

 * Do not call this routine or device_register() more than once for
 * any device structure.  The driver model core is not designed to work
 * with devices that get unregistered and then spring back to life.
 * (Among other things, it's very hard to guarantee that all references
 * to the previous incarnation of @dev have been dropped.)  Allocate
 * and register a fresh new struct device instead.

> I'm also worried about power management, how do you envision its 
> implementation ?

Haven't thought about it so far. That is another reason not to break the
driver model, I guess.

Thanks for your input!

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver
  2015-03-17  7:15 [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver Wolfram Sang
  2015-03-18 13:18 ` Laurent Pinchart
  2015-03-19 15:53 ` Wolfram Sang
@ 2015-04-21 17:55 ` Laurent Pinchart
  2015-04-21 18:47 ` Wolfram Sang
  3 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2015-04-21 17:55 UTC (permalink / raw)
  To: linux-sh

Hi Wolfram,

On Thursday 19 March 2015 16:53:18 Wolfram Sang wrote:
> >> + * Switching a master currently needs some access to either i2c-2 or
> >> i2c-3.
> >> + * Switching could also be done via sysfs or any other config mechanism.
> >> + * For this proof-of-concept, extra busses have been used since it
> >> simplifies
> >> + * locking a little.
> > 
> > I have mixed feelings to be honest. When using n internal masters muxed on
> > the same pins, with pin muxing used as a selector, I could agree that we
> > are dealing with n+1 busses, with n busses between the masters and the
> > demux, and one external bus. The two extra virtual busses in your example
> > above bother me.
> 
> That was chosen so the access to the "virtual" bus would automatically
> do the pinctrl change. As I said somewhere, this could be handled
> differently.
> 
> > This being said, I see this as an attempt to keep the traditional model of
> > I2C slaves being children of an I2C master while still departing from it
> > at the hardware level. Wouldn't it be better to depart from it from a
> > software point of view as well ? This would allow supporting real
> > multi-master configurations, but would come with a high refactoring cost
> > in kernel code.
> 
> You mean the slaves belong to an i2c-bus and this bus can be connected
> to masters? Yeah, that would be quite a change.

Yes, that's what I meant. I believe it would describe the hardware topology 
better for multi-master systems.

> And I am still not convinced if that would solve the issue that the driver
> model does not support re-parenting but rather suggests to delete and
> recreate the device. From device_add():
> 
>  * Do not call this routine or device_register() more than once for
>  * any device structure.  The driver model core is not designed to work
>  * with devices that get unregistered and then spring back to life.
>  * (Among other things, it's very hard to guarantee that all references
>  * to the previous incarnation of @dev have been dropped.)  Allocate
>  * and register a fresh new struct device instead.

Maybe it's time to implement proper reparenting in the driver core code then ? 
:-) That's opening Pandora's box though...

> > I'm also worried about power management, how do you envision its
> > implementation ?
> 
> Haven't thought about it so far. That is another reason not to break the
> driver model, I guess.
> 
> Thanks for your input!

You're welcome. I'm afraid it has mostly been destructive input, I don't have 
any good solution to the problem for now :-(

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver
  2015-03-17  7:15 [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver Wolfram Sang
                   ` (2 preceding siblings ...)
  2015-04-21 17:55 ` Laurent Pinchart
@ 2015-04-21 18:47 ` Wolfram Sang
  3 siblings, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2015-04-21 18:47 UTC (permalink / raw)
  To: linux-sh

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

> > Thanks for your input!
> 
> You're welcome. I'm afraid it has mostly been destructive input, I don't have 
> any good solution to the problem for now :-(

Well, actually, I have another version based on OF_DYNAMIC mostly ready.
It is not capable of keeping the I2C clients between an IP core switch,
but it is MUCH closer to the driver model. Could be good enough for our
use-case. Tests so far have been promising, but I get an OOPS when the
platform_devices should get destroyed. This looks like an OF_DYNAMIC
problem to me and I haven't got feedback from Pantelis yet.


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-04-21 18:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-17  7:15 [RFC V2 4/4] i2c: mux: demux-pinctrl: add driver Wolfram Sang
2015-03-18 13:18 ` Laurent Pinchart
2015-03-19 15:53 ` Wolfram Sang
2015-04-21 17:55 ` Laurent Pinchart
2015-04-21 18:47 ` Wolfram Sang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.