[PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[Alexander Sverdlin]

of: i2c: Add idle-disconnect DT property to PCA954x mux driver

Add idle-disconnect device tree property to PCA954x mux driver. The new property
forces the multiplexer to disconnect child buses in idle state. This is used, for
example, when there are several multiplexers on the same bus and the devices on
the underlying buses might have same I2C addresses.

At the same time old (and not used in the tree) platform data binding
deselect_on_exit is removed to simplify the implementation. Old binding has
different (per-channel) semantics and doesn't fit well in the new concept.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-OYasijW0DpE@public.gmane.org>
---

Changes in v2:
- dropped idle-state binding
- headers in alphabetical order
- removed old platform data deselect_on_exit flag

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |    3 +++
 drivers/i2c/muxes/i2c-mux-pca954x.c                |    9 +++++++--
 include/linux/i2c/pca954x.h                        |    3 ---
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index 34a3fb6..0272463 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -16,6 +16,9 @@ Required Properties:
 Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the reset input.
+  - idle-disconnect: Boolean; if defined, forces mux to disconnect all children
+    in idle state. This is necessary, for example, if there are several
+    multiplexers on the bus and the devices behind them use same I2C addresses.
 
 
 Example:
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index ec11b40..3fe966f 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,6 +41,7 @@
 #include <linux/i2c-mux.h>
 #include <linux/i2c/pca954x.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 
@@ -186,6 +187,8 @@ static int pca954x_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
 	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct device_node *of_node = client->dev.of_node;
+	bool idle_disconnect;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;
@@ -217,6 +220,9 @@ static int pca954x_probe(struct i2c_client *client,
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
 
+	idle_disconnect = of_node &&
+		of_property_read_bool(of_node, "idle-disconnect");
+
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
 		force = 0;			  /* dynamic adap number */
@@ -234,8 +240,7 @@ static int pca954x_probe(struct i2c_client *client,
 		data->virt_adaps[num] =
 			i2c_add_mux_adapter(adap, &client->dev, client,
 				force, num, class, pca954x_select_chan,
-				(pdata && pdata->modes[num].deselect_on_exit)
-					? pca954x_deselect_mux : NULL);
+				idle_disconnect ? pca954x_deselect_mux : NULL);
 
 		if (data->virt_adaps[num] == NULL) {
 			ret = -ENODEV;
diff --git a/include/linux/i2c/pca954x.h b/include/linux/i2c/pca954x.h
index 1712677..66bf1fb 100644
--- a/include/linux/i2c/pca954x.h
+++ b/include/linux/i2c/pca954x.h
@@ -29,13 +29,10 @@
 
 /* Per channel initialisation data:
  * @adap_id: bus number for the adapter. 0 = don't care
- * @deselect_on_exit: set this entry to 1, if your H/W needs deselection
- *                    of this channel after transaction.
  *
  */
 struct pca954x_platform_mode {
 	int		adap_id;
-	unsigned int	deselect_on_exit:1;
 	unsigned int	class;
 };
 

Re: [PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[Wolfram Sang]

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

On Fri, Dec 19, 2014 at 06:00:10PM +0100, Alexander Sverdlin wrote:
> of: i2c: Add idle-disconnect DT property to PCA954x mux driver
> 
> Add idle-disconnect device tree property to PCA954x mux driver. The new property
> forces the multiplexer to disconnect child buses in idle state. This is used, for
> example, when there are several multiplexers on the same bus and the devices on
> the underlying buses might have same I2C addresses.

Basically OK. Question to DT maintainers: "idle-disconnect",
"i2c-mux-idle-disconnect", or is there another existing binding we could
use?

> At the same time old (and not used in the tree) platform data binding
> deselect_on_exit is removed to simplify the implementation. Old binding has
> different (per-channel) semantics and doesn't fit well in the new concept.

I'd prefer to keep it. It should be only one || more. It is not really
in the way IMO.

Thanks,

   Wolfram


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

Re: [PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[Alexander Sverdlin]

Hi Wolfram!

On 15/01/15 13:32, ext Wolfram Sang wrote:
> On Fri, Dec 19, 2014 at 06:00:10PM +0100, Alexander Sverdlin wrote:
>> of: i2c: Add idle-disconnect DT property to PCA954x mux driver
>>
>> Add idle-disconnect device tree property to PCA954x mux driver. The new property
>> forces the multiplexer to disconnect child buses in idle state. This is used, for
>> example, when there are several multiplexers on the same bus and the devices on
>> the underlying buses might have same I2C addresses.
> 
> Basically OK. Question to DT maintainers: "idle-disconnect",
> "i2c-mux-idle-disconnect", or is there another existing binding we could
> use?
> 
>> At the same time old (and not used in the tree) platform data binding
>> deselect_on_exit is removed to simplify the implementation. Old binding has
>> different (per-channel) semantics and doesn't fit well in the new concept.
> 
> I'd prefer to keep it. It should be only one || more. It is not really
> in the way IMO.

It complicates the implementation 3x times :) This is part of our discussion with Laurent:
----
>>
>> I would copy pdata->modes[chan].deselect_on_exit to data->idle_disconnect
>> in the probe function, so you could avoiding accessing pdata here.
>
> Unfortunately, this pdata has different (per-channel) semantics. I cannot
> really understand, why it was done this way, but anyway it's not possible
> to use one global bit to represent per-channel bits without changing the
> behavior.
>
> I'm not keen to brake out-of-tree code (if any), but may be it will be
> decided to drop this per-channel deselect_on_exit, because it's not used at
> least in the kernel tree...

I'd vote for removing deselect_on_exit from platform data, but I won't insist.
----


-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[Wolfram Sang]

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

On Thu, Jan 15, 2015 at 02:09:09PM +0100, Alexander Sverdlin wrote:
> Hi Wolfram!
> 
> On 15/01/15 13:32, ext Wolfram Sang wrote:
> > On Fri, Dec 19, 2014 at 06:00:10PM +0100, Alexander Sverdlin wrote:
> >> of: i2c: Add idle-disconnect DT property to PCA954x mux driver
> >>
> >> Add idle-disconnect device tree property to PCA954x mux driver. The new property
> >> forces the multiplexer to disconnect child buses in idle state. This is used, for
> >> example, when there are several multiplexers on the same bus and the devices on
> >> the underlying buses might have same I2C addresses.
> > 
> > Basically OK. Question to DT maintainers: "idle-disconnect",
> > "i2c-mux-idle-disconnect", or is there another existing binding we could
> > use?
> > 
> >> At the same time old (and not used in the tree) platform data binding
> >> deselect_on_exit is removed to simplify the implementation. Old binding has
> >> different (per-channel) semantics and doesn't fit well in the new concept.
> > 
> > I'd prefer to keep it. It should be only one || more. It is not really
> > in the way IMO.
> 
> It complicates the implementation 3x times :) This is part of our discussion with Laurent:

Does it? I don't want DT and platform_data to behave equally. I just
want to keep being backwards compatible. So, I'd suggest:

(pdata && pdata->modes[num].deselect_on_exit) || idle_disconnect ? pca954x_deselect_mux : NULL);

> > I'm not keen to brake out-of-tree code (if any), but may be it will be
> > decided to drop this per-channel deselect_on_exit, because it's not used at
> > least in the kernel tree...

I couldn't find a user of the platform_data, at all. But removing
platform_data support is a seperate patch, and deprecating platform_data
is a seperate and general issue IMO.


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

Re: [PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[Alexander Sverdlin]

Hi Wolfram,

On 15/01/15 14:19, ext Wolfram Sang wrote:
>>>> of: i2c: Add idle-disconnect DT property to PCA954x mux driver
>>>> > >>
>>>> > >> Add idle-disconnect device tree property to PCA954x mux driver. The new property
>>>> > >> forces the multiplexer to disconnect child buses in idle state. This is used, for
>>>> > >> example, when there are several multiplexers on the same bus and the devices on
>>>> > >> the underlying buses might have same I2C addresses.
>>> > > 
>>> > > Basically OK. Question to DT maintainers: "idle-disconnect",
>>> > > "i2c-mux-idle-disconnect", or is there another existing binding we could
>>> > > use?
>>> > > 
>>>> > >> At the same time old (and not used in the tree) platform data binding
>>>> > >> deselect_on_exit is removed to simplify the implementation. Old binding has
>>>> > >> different (per-channel) semantics and doesn't fit well in the new concept.
>>> > > 
>>> > > I'd prefer to keep it. It should be only one || more. It is not really
>>> > > in the way IMO.
>> > 
>> > It complicates the implementation 3x times :) This is part of our discussion with Laurent:
> Does it? I don't want DT and platform_data to behave equally. I just
> want to keep being backwards compatible. So, I'd suggest:
> 
> (pdata && pdata->modes[num].deselect_on_exit) || idle_disconnect ? pca954x_deselect_mux : NULL);

you are right, thanks for the hint :)
I'll prepare v3...

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[Laurent Pinchart]

Hi Wolfram,

On Thursday 15 January 2015 14:19:35 Wolfram Sang wrote:
> On Thu, Jan 15, 2015 at 02:09:09PM +0100, Alexander Sverdlin wrote:
> > On 15/01/15 13:32, ext Wolfram Sang wrote:
> >> On Fri, Dec 19, 2014 at 06:00:10PM +0100, Alexander Sverdlin wrote:
> >>> of: i2c: Add idle-disconnect DT property to PCA954x mux driver
> >>> 
> >>> Add idle-disconnect device tree property to PCA954x mux driver. The new
> >>> property forces the multiplexer to disconnect child buses in idle
> >>> state. This is used, for example, when there are several multiplexers
> >>> on the same bus and the devices on the underlying buses might have
> >>> same I2C addresses.
> >> 
> >> Basically OK. Question to DT maintainers: "idle-disconnect",
> >> "i2c-mux-idle-disconnect", or is there another existing binding we could
> >> use?
> >> 
> >>> At the same time old (and not used in the tree) platform data binding
> >>> deselect_on_exit is removed to simplify the implementation. Old binding
> >>> has different (per-channel) semantics and doesn't fit well in the new
> >>> concept.
> >> 
> >> I'd prefer to keep it. It should be only one || more. It is not really
> >> in the way IMO.
> > 
> > It complicates the implementation 3x times :) This is part of our
> > discussion with Laurent:
>
> Does it? I don't want DT and platform_data to behave equally. I just
> want to keep being backwards compatible. So, I'd suggest:
> 
> (pdata && pdata->modes[num].deselect_on_exit) || idle_disconnect ?
> pca954x_deselect_mux : NULL);
>
> >> I'm not keen to brake out-of-tree code (if any), but may be it will be
> >> decided to drop this per-channel deselect_on_exit, because it's not used
> >> at least in the kernel tree...
> 
> I couldn't find a user of the platform_data, at all. But removing
> platform_data support is a seperate patch, and deprecating platform_data
> is a seperate and general issue IMO.

Sure, but it could be a preliminary patch on top of which to add idle-
disconnect DT support :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] of: i2c: Add idle-disconnect DT property to PCA954x mux driver

[Rob Herring]

On Thu, Jan 15, 2015 at 6:32 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Fri, Dec 19, 2014 at 06:00:10PM +0100, Alexander Sverdlin wrote:
>> of: i2c: Add idle-disconnect DT property to PCA954x mux driver
>>
>> Add idle-disconnect device tree property to PCA954x mux driver. The new property
>> forces the multiplexer to disconnect child buses in idle state. This is used, for
>> example, when there are several multiplexers on the same bus and the devices on
>> the underlying buses might have same I2C addresses.
>
> Basically OK. Question to DT maintainers: "idle-disconnect",
> "i2c-mux-idle-disconnect", or is there another existing binding we could
> use?

Not that I'm aware of. If this is specific to i2c-mux's then I'd go
with the latter. Either way, this should go in a common i2c or i2c-mux
binding doc.

Alternatively, couldn't the kernel do this automatically when the
above conditions happen?

Rob

[PATCH v3] of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver

[Alexander Sverdlin]

of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver

Add i2c-mux-idle-disconnect device tree property to PCA954x mux driver. The new
property forces the multiplexer to disconnect child buses in idle state. This is
used, for example, when there are several multiplexers on the same bus and the
devices on the underlying buses might have same I2C addresses.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-OYasijW0DpE@public.gmane.org>
---

Changes in v3:
- preserve legacy platform data deselect_on_exit flag
- change property name to i2c-mux-idle-disconnect
Changes in v2:
- dropped idle-state binding
- headers in alphabetical order
- removed old platform data deselect_on_exit flag

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |    3 +++
 drivers/i2c/muxes/i2c-mux-pca954x.c                |   11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -16,6 +16,9 @@ Required Properties:
 Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the reset input.
+  - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
+    children in idle state. This is necessary for example, if there are several
+    multiplexers on the bus and the devices behind them use same I2C addresses.
 
 
 Example:
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,6 +41,7 @@
 #include <linux/i2c-mux.h>
 #include <linux/i2c/pca954x.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 
@@ -186,6 +187,9 @@ static int pca954x_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
 	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct device_node *np = client->dev.of_node;
+	struct device_node *of_node = client->dev.of_node;
+	bool idle_disconnect_dt;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;
@@ -217,10 +221,14 @@ static int pca954x_probe(struct i2c_client *client,
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
 
+	idle_disconnect_dt = of_node &&
+		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
+
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
 		force = 0;			  /* dynamic adap number */
 		class = 0;			  /* no class by default */
+		bool idle_disconnect_pd = false;
 		if (pdata) {
 			if (num < pdata->num_modes) {
 				/* force static number */
@@ -229,12 +237,13 @@ static int pca954x_probe(struct i2c_client *client,
 			} else
 				/* discard unconfigured channels */
 				break;
+			idle_disconnect_pd = pdata->modes[num].deselect_on_exit;
 		}
 
 		data->virt_adaps[num] =
 			i2c_add_mux_adapter(adap, &client->dev, client,
 				force, num, class, pca954x_select_chan,
-				(pdata && pdata->modes[num].deselect_on_exit)
+				(idle_disconnect_pd || idle_disconnect_dt)
 					? pca954x_deselect_mux : NULL);
 
 		if (data->virt_adaps[num] == NULL) {
--
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] of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver

[Wolfram Sang]

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


> -				(pdata && pdata->modes[num].deselect_on_exit)
> +				(idle_disconnect_pd || idle_disconnect_dt)

I like that. Very readable.

I don't like the buildlog, however:

drivers/i2c/muxes/i2c-mux-pca954x.c: In function 'pca954x_probe':
drivers/i2c/muxes/i2c-mux-pca954x.c:231:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
drivers/i2c/muxes/i2c-mux-pca954x.c:190:22: warning: unused variable 'np' [-Wunused-variable]


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

[PATCH v4] of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver

[Alexander Sverdlin]

of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver

Add i2c-mux-idle-disconnect device tree property to PCA954x mux driver. The new
property forces the multiplexer to disconnect child buses in idle state. This is
used, for example, when there are several multiplexers on the same bus and the
devices on the underlying buses might have same I2C addresses.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-OYasijW0DpE@public.gmane.org>
---

Changes in v4:
- fix compilation warnings
Changes in v3:
- preserve legacy platform data deselect_on_exit flag
- change property name to i2c-mux-idle-disconnect
Changes in v2:
- dropped idle-state binding
- headers in alphabetical order
- removed old platform data deselect_on_exit flag

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |    3 +++
 drivers/i2c/muxes/i2c-mux-pca954x.c                |   10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index 34a3fb6..cf53d5f 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -16,6 +16,9 @@ Required Properties:
 Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the reset input.
+  - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
+    children in idle state. This is necessary for example, if there are several
+    multiplexers on the bus and the devices behind them use same I2C addresses.
 
 
 Example:
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index ec11b40..b81f888 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,6 +41,7 @@
 #include <linux/i2c-mux.h>
 #include <linux/i2c/pca954x.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 
@@ -186,6 +187,8 @@ static int pca954x_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
 	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct device_node *of_node = client->dev.of_node;
+	bool idle_disconnect_dt;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;
@@ -217,8 +220,12 @@ static int pca954x_probe(struct i2c_client *client,
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
 
+	idle_disconnect_dt = of_node &&
+		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
+
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
+		bool idle_disconnect_pd = false;
 		force = 0;			  /* dynamic adap number */
 		class = 0;			  /* no class by default */
 		if (pdata) {
@@ -229,12 +236,13 @@ static int pca954x_probe(struct i2c_client *client,
 			} else
 				/* discard unconfigured channels */
 				break;
+			idle_disconnect_pd = pdata->modes[num].deselect_on_exit;
 		}
 
 		data->virt_adaps[num] =
 			i2c_add_mux_adapter(adap, &client->dev, client,
 				force, num, class, pca954x_select_chan,
-				(pdata && pdata->modes[num].deselect_on_exit)
+				(idle_disconnect_pd || idle_disconnect_dt)
 					? pca954x_deselect_mux : NULL);
 
 		if (data->virt_adaps[num] == NULL) {

Re: [PATCH v4] of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver

[Wolfram Sang]

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

On Fri, Jan 23, 2015 at 04:41:29PM +0100, Alexander Sverdlin wrote:
> of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver
> 
> Add i2c-mux-idle-disconnect device tree property to PCA954x mux driver. The new
> property forces the multiplexer to disconnect child buses in idle state. This is
> used, for example, when there are several multiplexers on the same bus and the
> devices on the underlying buses might have same I2C addresses.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-OYasijW0DpE@public.gmane.org>

Applied to for-next, thanks! Added a newline after declaring
idle_disconnect_pd.


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