of: i2c: Add DT bindings for idle states to PCA954x mux driver

of: i2c: Add DT bindings for idle states to PCA954x mux driver

Introduce two new device tree bindings to specify idle state of PCA954x family of I2C multiplexors:
- idle-state: specifies particular child bus to be selected in idle;
- idle-disconnect: signals that mux should disconnect all child buses in
idle;

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

---
 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |    3 +
 drivers/i2c/muxes/i2c-mux-pca954x.c                |   51 +++++++++++++++--
 2 files changed, 48 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index 34a3fb6..1fbe287 100644

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

@@ -16,6 +16,9 @@ Required Properties:
 Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the reset input.
+  - idle-state: Child bus connected in idle state (specified by its "reg"

value) + - idle-disconnect: Boolean; if defined, forces mux to disconnect
all children + in idle state

Example:

(quoted diff) +0 -0

b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644

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

 #define PCA954X_MAX_NCHANS 8
 

@@ -62,6 +63,8 @@ struct pca954x {
 	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
 
 	u8 last_chan;		/* last register value */
+	bool idle_disconnect;
+	s8 idle_chan;		/* valid if not negative */
 };
 
 struct chip_desc {

drivers/i2c/muxes/i2c-mux-pca954x.c +37 -6

+		/* Deselect active channel */
+		data->last_chan = 0;
+		return pca954x_reg_write(adap, client, data->last_chan);
+	}
 
-	/* Deselect active channel */
-	data->last_chan = 0;
-	return pca954x_reg_write(adap, client, data->last_chan);
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(adap, client, data->idle_chan);
+
+	return 0;
 }
 
 /*

@@ -186,6 +199,7 @@ 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;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;

@@ -216,6 +230,27 @@ static int pca954x_probe(struct i2c_client *client,
 
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
+	data->idle_chan = -1;		   /* no forced idle state */
+
+	if (of_node) {
+		u32 ch;
+
+		if (of_property_read_bool(of_node, "idle-disconnect"))
+			data->idle_disconnect = true;
+
+		if (!of_property_read_u32_index(of_node, "idle-state", 0, &ch)) {
+			if (ch < PCA954X_MAX_NCHANS) {
+				data->idle_chan = ch;
+				/* Force idle state from the beginning */
+				ret = pca954x_select_chan(adap, client, ch);
+				if (ret)
+					return ret;
+			} else {
+				dev_warn(&client->dev,
+				         "Invalid idle-state property\n");
+			}
+		}
+	}
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {

@@ -234,8 +269,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);
+				pca954x_deselect_mux);
 
 		if (data->virt_adaps[num] == NULL) {
 			ret = -ENODEV;

@@ -281,7 +315,12 @@ static int pca954x_resume(struct device *dev)
 	struct pca954x *data = i2c_get_clientdata(client);
 
 	data->last_chan = 0;
-	return i2c_smbus_write_byte(client, 0);
+	/* Restore idle state on resume */
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(to_i2c_adapter(client->dev.parent),
+		                           client, data->idle_chan);
+	else
+		return i2c_smbus_write_byte(client, 0);
 }
 #endif

Hi Alexander,

Thank you for the patch.

Quoted 2 lines · On Monday 15 December 2014 02:25:18 Alexander Sverdlin wrote:

of: i2c: Add DT bindings for idle states to PCA954x mux driver

Quoted 5 lines

Introduce two new device tree bindings to specify idle state of PCA954x family of I2C multiplexors:
- idle-state: specifies particular child bus to be selected in idle;
- idle-disconnect: signals that mux should disconnect all child buses in
idle;

Could you please briefly explain your use case(s) for those two properties ?

Quoted 7 lines

of: i2c: Add DT bindings for idle states to PCA954x mux driver

Introduce two new device tree bindings to specify idle state of PCA954x family of I2C multiplexors:
- idle-state: specifies particular child bus to be selected in idle;
- idle-disconnect: signals that mux should disconnect all child buses in
idle;

Could you please briefly explain your use case(s) for those two properties ?

Quoted 20 lines

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

---
 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |    3 +
 drivers/i2c/muxes/i2c-mux-pca954x.c                |   51 +++++++++++++++--
 2 files changed, 48 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index 34a3fb6..1fbe287 100644

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

@@ -16,6 +16,9 @@ Required Properties:
 Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the reset input.
+  - idle-state: Child bus connected in idle state (specified by its "reg"

value) + - idle-disconnect: Boolean; if defined, forces mux to disconnect
all children + in idle state

drivers/i2c/muxes/i2c-mux-pca954x.c line 46

Quoted 10 lines

Example:

(quoted diff) +0 -0

b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644

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

@@ -43,6 +43,7 @@
 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
+#include <linux/of.h>

Could you please keep headers sorted alphabetically ?

Quoted 10 lines

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

 #define PCA954X_MAX_NCHANS 8
 

@@ -62,6 +63,8 @@ struct pca954x {
 	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
 
 	u8 last_chan;		/* last register value */
+	bool idle_disconnect;
+	s8 idle_chan;		/* valid if not negative */
 };
 

drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_deselect_mux(struct i2c_adapter · line 182

Quoted 10 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +5 -0

 struct chip_desc {

@@ -172,10 +175,20 @@ static int pca954x_deselect_mux(struct i2c_adapter
 *adap, void *client, u32 chan)
 {
 	struct pca954x *data = i2c_get_clientdata(client);
+	struct pca954x_platform_data *pdata =
+		dev_get_platdata(&((struct i2c_client *)client)->dev);
+
+	if ((pdata && pdata->modes[chan].deselect_on_exit) ||
+	    data->idle_disconnect) {

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 74 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +37 -6

+		/* Deselect active channel */
+		data->last_chan = 0;
+		return pca954x_reg_write(adap, client, data->last_chan);
+	}
 
-	/* Deselect active channel */
-	data->last_chan = 0;
-	return pca954x_reg_write(adap, client, data->last_chan);
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(adap, client, data->idle_chan);
+
+	return 0;
 }
 
 /*

@@ -186,6 +199,7 @@ 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;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;

@@ -216,6 +230,27 @@ static int pca954x_probe(struct i2c_client *client,
 
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
+	data->idle_chan = -1;		   /* no forced idle state */
+
+	if (of_node) {
+		u32 ch;
+
+		if (of_property_read_bool(of_node, "idle-disconnect"))
+			data->idle_disconnect = true;
+
+		if (!of_property_read_u32_index(of_node, "idle-state", 0, &ch)) {
+			if (ch < PCA954X_MAX_NCHANS) {
+				data->idle_chan = ch;
+				/* Force idle state from the beginning */
+				ret = pca954x_select_chan(adap, client, ch);
+				if (ret)
+					return ret;
+			} else {
+				dev_warn(&client->dev,
+				         "Invalid idle-state property\n");
+			}
+		}
+	}
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {

@@ -234,8 +269,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);
+				pca954x_deselect_mux);
 
 		if (data->virt_adaps[num] == NULL) {
 			ret = -ENODEV;

@@ -281,7 +315,12 @@ static int pca954x_resume(struct device *dev)
 	struct pca954x *data = i2c_get_clientdata(client);
 
 	data->last_chan = 0;
-	return i2c_smbus_write_byte(client, 0);
+	/* Restore idle state on resume */
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(to_i2c_adapter(client->dev.parent),
+		                           client, data->idle_chan);
+	else
+		return i2c_smbus_write_byte(client, 0);
 }
 #endif

Hello Laurent,

Quoted 11 lines · ext Laurent Pinchart

Quoted 8 lines

of: i2c: Add DT bindings for idle states to PCA954x mux driver

Introduce two new device tree bindings to specify idle state of PCA954x

family of I2C multiplexors:
- idle-state: specifies particular child bus to be selected in idle;
- idle-disconnect: signals that mux should disconnect all child buses in idle;

Could you please briefly explain your use case(s) for those two properties ?

idle-state specifies which bus will be connected when there is no data transfer. i2c-mux-gpio also has "idle-state" (though, it can replace both connect and disconnect cases in my patch). i2c-mux-pinctrl has special "idle" convention, specifying idle state as last in the list of states. Again, it can serve for both my properties.

idle-disconnect is what we actually use for some security reasons (so that I2C bus of the external SFP cage is not connected to the rest of I2C bus in idle).

Quoted 23 lines

Quoted 23 lines

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

---

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |    3 +
 drivers/i2c/muxes/i2c-mux-pca954x.c                |   51 ++++++++++++--
 2 files changed, 48 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index 34a3fb6..1fbe287 100644

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

@@ -16,6 +16,9 @@ Required Properties:
 Optional Properties:
   - reset-gpios: Reference to the GPIO connected to the reset input.
 
+  - idle-state: Child bus connected in idle state (specified by its
 "reg" value)
+  - idle-disconnect: Boolean; if defined, forces mux to disconnect all

children
+ in idle state

Quoted 12 lines

drivers/i2c/muxes/i2c-mux-pca954x.c line 46

Quoted 10 lines

Example:

(quoted diff) +0 -0

b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644

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

@@ -43,6 +43,7 @@
 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
+#include <linux/of.h>

Could you please keep headers sorted alphabetically ?

Yes...

Quoted 22 lines

Quoted 22 lines

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

 #define PCA954X_MAX_NCHANS 8
 

@@ -62,6 +63,8 @@ struct pca954x {
 
 	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
 
 	u8 last_chan;		/* last register value */
 
+	bool idle_disconnect;
+	s8 idle_chan;		/* valid if not negative */
 

};

struct chip_desc {

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

@@ -172,10 +175,20 @@ static int pca954x_deselect_mux(struct i2c_adapter
 *adap, void *client, u32 chan)
 
 {
 
 	struct pca954x *data = i2c_get_clientdata(client);
 

Quoted 8 lines

drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_deselect_mux(struct i2c_adapter *adap, · line 182

Quoted 5 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +5 -0

+	struct pca954x_platform_data *pdata =
+		dev_get_platdata(&((struct i2c_client *)client)->dev);
+
+	if ((pdata && pdata->modes[chan].deselect_on_exit) ||
+	    data->idle_disconnect) {

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

Quoted 76 lines

Quoted 76 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +8 -3

+		/* Deselect active channel */
+		data->last_chan = 0;
+		return pca954x_reg_write(adap, client, data->last_chan);
+	}
 
-	/* Deselect active channel */
-	data->last_chan = 0;
-	return pca954x_reg_write(adap, client, data->last_chan);
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(adap, client, data->idle_chan);
+
+	return 0;

}

/*

drivers/i2c/muxes/i2c-mux-pca954x.c +29 -3

@@ -186,6 +199,7 @@ 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;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;

@@ -216,6 +230,27 @@ static int pca954x_probe(struct i2c_client *client,
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
 
+	data->idle_chan = -1;		   /* no forced idle state */
+
+	if (of_node) {
+		u32 ch;
+
+		if (of_property_read_bool(of_node, "idle-disconnect"))
+			data->idle_disconnect = true;
+
+		if (!of_property_read_u32_index(of_node, "idle-state", 0, &ch)) {
+			if (ch < PCA954X_MAX_NCHANS) {
+				data->idle_chan = ch;
+				/* Force idle state from the beginning */
+				ret = pca954x_select_chan(adap, client, ch);
+				if (ret)
+					return ret;
+			} else {
+				dev_warn(&client->dev,
+				         "Invalid idle-state property\n");
+			}
+		}
+	}
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {

@@ -234,8 +269,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);
+				pca954x_deselect_mux);
 
 		if (data->virt_adaps[num] == NULL) {
 			ret = -ENODEV;

@@ -281,7 +315,12 @@ static int pca954x_resume(struct device *dev)
 	struct pca954x *data = i2c_get_clientdata(client);
 
 	data->last_chan = 0;
 
-	return i2c_smbus_write_byte(client, 0);
+	/* Restore idle state on resume */
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(to_i2c_adapter(client->dev.parent),
+		                           client, data->idle_chan);
+	else
+		return i2c_smbus_write_byte(client, 0);
 
 }

#endif

Quoted 18 lines

Quoted 11 lines

Quoted 8 lines

of: i2c: Add DT bindings for idle states to PCA954x mux driver

Introduce two new device tree bindings to specify idle state of PCA954x

family of I2C multiplexors:
- idle-state: specifies particular child bus to be selected in idle;
- idle-disconnect: signals that mux should disconnect all child buses in idle;

Could you please briefly explain your use case(s) for those two properties ?

idle-state specifies which bus will be connected when there is no data transfer. i2c-mux-gpio also has "idle-state" (though, it can replace both connect and disconnect cases in my patch). i2c-mux-pinctrl has special "idle" convention, specifying idle state as last in the list of states. Again, it can serve for both my properties.

Quoted 3 lines

idle-disconnect is what we actually use for some security reasons (so that I2C bus of the external SFP cage is not connected to the rest of I2C bus in idle).

Thank you for the explanation. I would add it to the DT bindings documentation, or at least to the commit message.

I understand the use cases of the idle-disconnect property. What do you use idle-state for, when the idle state is different from disconnecting the bus ?

Quoted 74 lines

Quoted 23 lines

Quoted 23 lines

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

---

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |    3 +
 drivers/i2c/muxes/i2c-mux-pca954x.c                |   51 ++++++++++++--
 2 files changed, 48 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index 34a3fb6..1fbe287 100644

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

@@ -16,6 +16,9 @@ Required Properties:
 Optional Properties:
   - reset-gpios: Reference to the GPIO connected to the reset input.
 
+  - idle-state: Child bus connected in idle state (specified by its
 "reg" value)
+  - idle-disconnect: Boolean; if defined, forces mux to disconnect all

children
+ in idle state

Quoted 12 lines

drivers/i2c/muxes/i2c-mux-pca954x.c line 46

Quoted 10 lines

Example:

(quoted diff) +0 -0

b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644

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

@@ -43,6 +43,7 @@
 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
+#include <linux/of.h>

Could you please keep headers sorted alphabetically ?

Yes...

Quoted 22 lines

Quoted 22 lines

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

 #define PCA954X_MAX_NCHANS 8
 

@@ -62,6 +63,8 @@ struct pca954x {
 
 	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
 
 	u8 last_chan;		/* last register value */
 
+	bool idle_disconnect;
+	s8 idle_chan;		/* valid if not negative */
 

};

struct chip_desc {

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

@@ -172,10 +175,20 @@ static int pca954x_deselect_mux(struct i2c_adapter
 *adap, void *client, u32 chan)
 
 {
 
 	struct pca954x *data = i2c_get_clientdata(client);
 

Quoted 8 lines

drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_deselect_mux(struct i2c_adapter *adap, · line 182

Quoted 5 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +5 -0

+	struct pca954x_platform_data *pdata =
+		dev_get_platdata(&((struct i2c_client *)client)->dev);
+
+	if ((pdata && pdata->modes[chan].deselect_on_exit) ||
+	    data->idle_disconnect) {

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.

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'd vote for removing deselect_on_exit from platform data, but I won't insist.

Quoted 76 lines

Quoted 76 lines

Quoted 76 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +8 -3

+		/* Deselect active channel */
+		data->last_chan = 0;
+		return pca954x_reg_write(adap, client, data->last_chan);
+	}
 
-	/* Deselect active channel */
-	data->last_chan = 0;
-	return pca954x_reg_write(adap, client, data->last_chan);
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(adap, client, data->idle_chan);
+
+	return 0;

}

/*

drivers/i2c/muxes/i2c-mux-pca954x.c +29 -3

@@ -186,6 +199,7 @@ 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;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;

@@ -216,6 +230,27 @@ static int pca954x_probe(struct i2c_client *client,
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
 
+	data->idle_chan = -1;		   /* no forced idle state */
+
+	if (of_node) {
+		u32 ch;
+
+		if (of_property_read_bool(of_node, "idle-disconnect"))
+			data->idle_disconnect = true;
+
+		if (!of_property_read_u32_index(of_node, "idle-state", 0, &ch)) {
+			if (ch < PCA954X_MAX_NCHANS) {
+				data->idle_chan = ch;
+				/* Force idle state from the beginning */
+				ret = pca954x_select_chan(adap, client, ch);
+				if (ret)
+					return ret;
+			} else {
+				dev_warn(&client->dev,
+				         "Invalid idle-state property\n");
+			}
+		}
+	}
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {

@@ -234,8 +269,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);
+				pca954x_deselect_mux);
 
 		if (data->virt_adaps[num] == NULL) {
 			ret = -ENODEV;

@@ -281,7 +315,12 @@ static int pca954x_resume(struct device *dev)
 	struct pca954x *data = i2c_get_clientdata(client);
 
 	data->last_chan = 0;
 
-	return i2c_smbus_write_byte(client, 0);
+	/* Restore idle state on resume */
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(to_i2c_adapter(client->dev.parent),
+		                           client, data->idle_chan);
+	else
+		return i2c_smbus_write_byte(client, 0);
 
 }

#endif

Quoted 18 lines · ext Laurent Pinchart

Quoted 18 lines

Quoted 11 lines

Quoted 8 lines

of: i2c: Add DT bindings for idle states to PCA954x mux driver

Introduce two new device tree bindings to specify idle state of PCA954x

family of I2C multiplexors:
- idle-state: specifies particular child bus to be selected in idle;
- idle-disconnect: signals that mux should disconnect all child buses in idle;

Could you please briefly explain your use case(s) for those two properties ?

idle-state specifies which bus will be connected when there is no data transfer. i2c-mux-gpio also has "idle-state" (though, it can replace both connect and disconnect cases in my patch). i2c-mux-pinctrl has special "idle" convention, specifying idle state as last in the list of states. Again, it can serve for both my properties.

Quoted 6 lines

Quoted 3 lines

idle-disconnect is what we actually use for some security reasons (so that I2C bus of the external SFP cage is not connected to the rest of I2C bus in idle).

Thank you for the explanation. I would add it to the DT bindings documentation, or at least to the commit message.

Ok, I'll send v2 with all your comments addressed.

Quoted 3 lines

I understand the use cases of the idle-disconnect property. What do you use idle-state for, when the idle state is different from disconnecting the bus ?

We do not have a use case for that. It was done only for the sake of completeness. Because other MUXes actually offer user this possibility. I was initially thinking about providing this on i2c-mux level for all of them, but unfortunately they all use deselect callback in different way and pinctrl is the worst case, as idle state cannot be represented with an int, but should be a pointer.

Anyway, if you think there is no use-case for this, I can drop this part.

Quoted 79 lines

Quoted 79 lines

Quoted 31 lines

Quoted 31 lines

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

---

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |    3 +
 drivers/i2c/muxes/i2c-mux-pca954x.c                |   51
 ++++++++++++--
 2 files changed, 48 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index 34a3fb6..1fbe287 100644

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

@@ -16,6 +16,9 @@ Required Properties:
 Optional Properties:
   - reset-gpios: Reference to the GPIO connected to the reset input.
 
+  - idle-state: Child bus connected in idle state (specified by its
 "reg" value)
+  - idle-disconnect: Boolean; if defined, forces mux to disconnect all

children

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

+    in idle state
 
 Example:

(quoted diff) +0 -0

b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644

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

@@ -43,6 +43,7 @@
 

Quoted 7 lines

drivers/i2c/muxes/i2c-mux-pca954x.c line 48

Quoted 5 lines

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

 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 
+#include <linux/of.h>

Could you please keep headers sorted alphabetically ?

Yes...

Quoted 23 lines

Quoted 23 lines

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

 #define PCA954X_MAX_NCHANS 8

@@ -62,6 +63,8 @@ struct pca954x {
 
 	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
 
 	u8 last_chan;		/* last register value */
 
+	bool idle_disconnect;
+	s8 idle_chan;		/* valid if not negative */
 

};

struct chip_desc {

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

@@ -172,10 +175,20 @@ static int pca954x_deselect_mux(struct

i2c_adapter
*adap, void *client, u32 chan)

{

struct pca954x *data = i2c_get_clientdata(client);

Quoted 9 lines

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

Quoted 5 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +5 -0

+	struct pca954x_platform_data *pdata =
+		dev_get_platdata(&((struct i2c_client *)client)->dev);
+
+	if ((pdata && pdata->modes[chan].deselect_on_exit) ||
+	    data->idle_disconnect) {

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.

Quoted 6 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'd vote for removing deselect_on_exit from platform data, but I won't insist.

Then it should be a separate patch, probably... I can send a series removing pdata pits and adding a DT binding.

Quoted 93 lines

Quoted 93 lines

Quoted 93 lines

Quoted 93 lines

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

+		/* Deselect active channel */
+		data->last_chan = 0;
+		return pca954x_reg_write(adap, client, data->last_chan);
+	}
 
-	/* Deselect active channel */
-	data->last_chan = 0;
-	return pca954x_reg_write(adap, client, data->last_chan);
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(adap, client, data->idle_chan);
+
+	return 0;
 
 }
 
 /*

@@ -186,6 +199,7 @@ 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;

drivers/i2c/muxes/i2c-mux-pca954x.c +21 -0

 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;

@@ -216,6 +230,27 @@ static int pca954x_probe(struct i2c_client
 *client,
 
 	data->type = id->driver_data;
 	data->last_chan = 0;		   /* force the first selection */
 
+	data->idle_chan = -1;		   /* no forced idle state */
+
+	if (of_node) {
+		u32 ch;
+
+		if (of_property_read_bool(of_node, "idle-disconnect"))
+			data->idle_disconnect = true;
+
+		if (!of_property_read_u32_index(of_node, "idle-state", 0, &ch)) {
+			if (ch < PCA954X_MAX_NCHANS) {
+				data->idle_chan = ch;
+				/* Force idle state from the beginning */
+				ret = pca954x_select_chan(adap, client, ch);
+				if (ret)
+					return ret;
+			} else {
+				dev_warn(&client->dev,
+				         "Invalid idle-state property\n");
+			}
+		}
+	}
 

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

 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {

@@ -234,8 +269,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);
+				pca954x_deselect_mux);
 
 		if (data->virt_adaps[num] == NULL) {
 
 			ret = -ENODEV;

@@ -281,7 +315,12 @@ static int pca954x_resume(struct device *dev)
 
 	struct pca954x *data = i2c_get_clientdata(client);
 
 	data->last_chan = 0;
 
-	return i2c_smbus_write_byte(client, 0);
+	/* Restore idle state on resume */
+	if (data->idle_chan >= 0)
+		return pca954x_select_chan(to_i2c_adapter(client->dev.parent),
+		                           client, data->idle_chan);
+	else
+		return i2c_smbus_write_byte(client, 0);
 

}
#endif

Quoted 11 lines

Quoted 11 lines

I understand the use cases of the idle-disconnect property. What do you

Quoted 2 lines

Quoted 2 lines

use idle-state for, when the idle state is different from disconnecting the bus ?

We do not have a use case for that. It was done only for the sake of completeness. Because other MUXes actually offer user this possibility. I was initially thinking about providing this on i2c-mux level for all of them, but unfortunately they all use deselect callback in different way and pinctrl is the worst case, as idle state cannot be represented with an int, but should be a pointer.

Quoted 1 line

Quoted 1 line

Anyway, if you think there is no use-case for this, I can drop this part.

Adding a DT property without a clear use case usually makes me a bit wary. I would thus prefer going for either idle-disconnect alone, or for an idle-state property that would allow selecting disconnection as one of the possible values.