diff mbox

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

Message ID 5494599A.8050803@nsn.com
State Needs Review / ACK, archived
Headers show

Checks

Context Check Description
robh/checkpatch warning total: 1 errors, 0 warnings, 0 lines checked
robh/patch-applied success

Commit Message

Alexander Sverdlin Dec. 19, 2014, 5 p.m. UTC 
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@nsn.com>
---

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



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

Comments

Wolfram Sang Jan. 15, 2015, 12:32 p.m. UTC | #1 
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
Alexander Sverdlin Jan. 15, 2015, 1:09 p.m. UTC | #2 
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.
----
Wolfram Sang Jan. 15, 2015, 1:19 p.m. UTC | #3 
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.
Alexander Sverdlin Jan. 15, 2015, 1:35 p.m. UTC | #4 
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...
Laurent Pinchart Jan. 15, 2015, 2:40 p.m. UTC | #5 
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 :-)
Rob Herring Jan. 15, 2015, 3:48 p.m. UTC | #6 
On Thu, Jan 15, 2015 at 6:32 AM, Wolfram Sang <wsa@the-dreams.de> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 };