| Message ID | 548E387E.2020502@nsn.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
Hi Alexander, Thank you for the patch. On Monday 15 December 2014 02:25:18 Alexander Sverdlin wrote: > 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 ? > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> > --- > .../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 + > drivers/i2c/muxes/i2c-mux-pca954x.c | 51 +++++++++++++++-- > 2 files changed, 48 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index > 34a3fb6..1fbe287 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-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: > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c > b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > @@ -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 ? > #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 { > @@ -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. > + /* 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 Laurent, Am 15.12.2014 um 09:40 schrieb Laurent Pinchart: > Hi Alexander, > > Thank you for the patch. > > On Monday 15 December 2014 02:25:18 Alexander Sverdlin wrote: >> 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 ? Rationale (which should be added) is: If there are two or more multiplexers/switches connected to same bus unused ones need to be set to neutral/unconnected state, otherwise transactions to devices behind current multiplexer get visible to all buses lately addressed on the other multiplexers. This will lead to address conflicts for all devices with same address. Usage of new bindings is xor. KR Michael -- 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
Hello Laurent, On 15/12/14 09:40, ext Laurent Pinchart wrote: >> 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). >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> >> --- >> .../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 + >> drivers/i2c/muxes/i2c-mux-pca954x.c | 51 +++++++++++++++-- >> 2 files changed, 48 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt >> b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index >> 34a3fb6..1fbe287 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-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: >> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c >> b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644 >> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c >> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c >> @@ -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... >> #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 { >> @@ -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. 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... >> + /* 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, On Tuesday 16 December 2014 09:09:22 Alexander Sverdlin wrote: > Hello Laurent, > > On 15/12/14 09:40, ext Laurent Pinchart wrote: > >> 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). 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 ? > >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> > >> --- > >> > >> .../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 + > >> drivers/i2c/muxes/i2c-mux-pca954x.c | 51 ++++++++++++-- > >> 2 files changed, 48 insertions(+), 6 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > >> b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index > >> 34a3fb6..1fbe287 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-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: > >> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c > >> b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644 > >> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > >> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > >> @@ -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... > > >> #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 { > >> > >> @@ -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. > > 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. > >> + /* 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, On 16/12/14 12:07, ext Laurent Pinchart wrote: >>>> 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). > > 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. > 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. >>>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> >>>> --- >>>> >>>> .../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 + >>>> drivers/i2c/muxes/i2c-mux-pca954x.c | 51 ++++++++++++-- >>>> 2 files changed, 48 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt >>>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index >>>> 34a3fb6..1fbe287 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-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: >>>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c >>>> b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644 >>>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c >>>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c >>>> @@ -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... >> >>>> #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 { >>>> >>>> @@ -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. >> >> 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. Then it should be a separate patch, probably... I can send a series removing pdata pits and adding a DT binding. >>>> + /* 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, On Tuesday 16 December 2014 13:49:13 Alexander Sverdlin wrote: > On 16/12/14 12:07, ext Laurent Pinchart wrote: > >>>> 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). > > > > 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. Thank you. > > 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. 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. > >>>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com> > >>>> --- > >>>> > >>>> .../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 + > >>>> drivers/i2c/muxes/i2c-mux-pca954x.c | 51 > >>>> ++++++++++++-- > >>>> 2 files changed, 48 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > >>>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index > >>>> 34a3fb6..1fbe287 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-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: > >>>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c > >>>> b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644 > >>>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > >>>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > >>>> @@ -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... > >> > >>>> #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 { > >>>> > >>>> @@ -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. > >> > >> 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. > > Then it should be a separate patch, probably... I can send a series removing > pdata pits and adding a DT binding. > > >>>> + /* 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! On 16/12/14 14:38, ext Laurent Pinchart wrote: >>> 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. > 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. I thought about this, but this would require some magic value for idle-state property, which would not be obvious for the readers of .dts file without consulting the documentation. So, I would prefer to drop the idle channel selection completely...
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index 34a3fb6..1fbe287 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-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: diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c index ec11b40..69cf603 100644 --- a/drivers/i2c/muxes/i2c-mux-pca954x.c +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c @@ -43,6 +43,7 @@ #include <linux/module.h> #include <linux/pm.h> #include <linux/slab.h> +#include <linux/of.h> #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 { @@ -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) { + /* 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
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 <alexander.sverdlin@nsn.com> --- .../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 + drivers/i2c/muxes/i2c-mux-pca954x.c | 51 +++++++++++++++++-- 2 files changed, 48 insertions(+), 6 deletions(-)