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

PATCH Superseded Alexander Sverdlin 4 participants 11 messages Dec 19, 2014 – Jan 23, 2015 5 messages not in the archive

Subsystems (3)

Device tree bindings I2C subsystem host drivers I2C muxes

lore

Share & tools

lore

Alexander Sverdlin

author

2014-12-19 17:00 UTC #0 lore raw

PATCH v2 1/1 of: i2c: Add idle-disconnect DT property to PCA954x mux driver

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 <[email protected]>

---

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(-)

Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt +3 -0

index 34a3fb6..0272463 100644
@@ -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:

drivers/i2c/muxes/i2c-mux-pca954x.c +7 -2

index ec11b40..3fe966f 100644
@@ -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;

include/linux/i2c/pca954x.h +0 -3

index 1712677..66bf1fb 100644
@@ -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;
 };

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

2015-01-15 12:32 UTC #2 lore raw

Quoted 6 lines · 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.

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

Quoted 3 lines

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

Alexander Sverdlin

author

2015-01-15 13:09 UTC #3 lore raw

Hi Wolfram!

Quoted 12 lines · ext Wolfram Sang

Quoted 6 lines · 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.
Basically OK. Question to DT maintainers: "idle-disconnect", "i2c-mux-idle-disconnect", or is there another existing binding we could use?

Quoted 6 lines

Quoted 3 lines
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:
----

Quoted 4 lines

Quoted 3 lines
I would copy pdata->modes[chan].deselect_on_exit to data->idle_disconnect in the probe function, so you could avoiding accessing pdata here.

Quoted 8 lines

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.
----

Signature · 2 lines

Best regards,
Alexander Sverdlin.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

2015-01-15 13:19 UTC #5 lore raw

Quoted 15 lines · Alexander Sverdlin

Hi Wolfram!
Quoted 12 lines · ext Wolfram Sang
Quoted 6 lines · 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.
Basically OK. Question to DT maintainers: "idle-disconnect", "i2c-mux-idle-disconnect", or is there another existing binding we could use?

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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);

Quoted 3 lines

Quoted 3 lines
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.

Alexander Sverdlin

author

2015-01-15 13:35 UTC #6 lore raw

Hi Wolfram,

Quoted 15 lines · ext Wolfram Sang

Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
of: i2c: Add idle-disconnect DT property to PCA954x mux driver
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
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?
Quoted 3 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
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...

Signature · 2 lines

Best regards,
Alexander Sverdlin.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Alexander Sverdlin

author

2015-01-19 13:29 UTC #8 lore raw

PATCH v3 1/1 of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver

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 <[email protected]>

---

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(-)

Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt +3 -0

@@ -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:

drivers/i2c/muxes/i2c-mux-pca954x.c +10 -1

@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

2015-01-22 14:56 UTC #10 lore raw

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

drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_probe(struct i2c_client *client, · line 246

Quoted 3 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +1 -1

-				(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]

Alexander Sverdlin

author

2015-01-23 15:41 UTC #11 lore raw

PATCH v4 1/1 of: i2c: Add i2c-mux-idle-disconnect DT property to PCA954x mux driver

In reply to a message not in the archive

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

Signed-off-by Alexander Sverdlin <[email protected]>

---

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(-)

Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt +3 -0

index 34a3fb6..cf53d5f 100644
@@ -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:

drivers/i2c/muxes/i2c-mux-pca954x.c +9 -1

index ec11b40..b81f888 100644
@@ -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) {

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

Applied

2015-01-23 16:13 UTC #13 lore raw

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

Quoted 8 lines · 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 <[email protected]>

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

Laurent Pinchart

2015-01-15 14:40 UTC #14 lore raw

In reply to Wolfram Sang

Hi Wolfram,

Quoted 32 lines · On Thursday 15 January 2015 14:19:35 Wolfram Sang wrote:

Quoted 19 lines · Alexander Sverdlin
Quoted 18 lines · ext Wolfram Sang
Quoted 7 lines · 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.
Basically OK. Question to DT maintainers: "idle-disconnect", "i2c-mux-idle-disconnect", or is there another existing binding we could use?
Quoted 4 lines
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.
Quoted 5 lines
Quoted 2 lines
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);

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
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 :-)

Signature · 3 lines

Regards,

Laurent Pinchart

Rob Herring

2015-01-15 15:48 UTC #15 lore raw

In reply to Wolfram Sang

Quoted 11 lines · Wolfram Sang

Quoted 6 lines · 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.
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