| Message ID | 20190228114343.36443-4-robertshearman@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Peter Rosin |
| Headers | show |
| Series | i2c: mux: pca954x: allow management of device idle state via sysfs | expand |
Hi Greg, I was wondering if you think the below ABI addition looks sane to you? Or, should I ask someone else for a tag? Cheers, Peter On 2019-02-28 12:43, Robert Shearman wrote: > From: Robert Shearman <robert.shearman@att.com> > > The behaviour, by default, to not deselect after each transfer is > unsafe when there is a device with an address that conflicts with > another device on another mux on the same parent bus, and it > may not be convenient to use devicetree to set the deselect mux, > e.g. when running on x86_64 when ACPI is used to discover most of the > device hierarchy. > > Therefore, provide the ability to set the idle state behaviour using a > new sysfs file, idle_state as a complement to the method of > instantiating the device via sysfs. The possible behaviours are > disconnect, i.e. to deselect all channels from the mux, as-is (the > default), i.e. leave the last channel selected, and set a > predetermined channel. > > The current behaviour of leaving the channel as-is after each > transaction is preserved. > > Signed-off-by: Robert Shearman <robert.shearman@att.com> > --- > .../ABI/testing/sysfs-bus-i2c-devices-pca954x | 20 +++++ > drivers/i2c/muxes/i2c-mux-pca954x.c | 85 +++++++++++++++++-- > 2 files changed, 97 insertions(+), 8 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x > > diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x b/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x > new file mode 100644 > index 000000000000..0b0de8cd0d13 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x > @@ -0,0 +1,20 @@ > +What: /sys/bus/i2c/.../idle_state > +Date: January 2019 > +KernelVersion: 5.2 > +Contact: Robert Shearman <robert.shearman@att.com> > +Description: > + Value that exists only for mux devices that can be > + written to control the behaviour of the multiplexer on > + idle. Possible values: > + -2 - disconnect on idle, i.e. deselect the last used > + channel, which is useful when there is a device > + with an address that conflicts with another > + device on another mux on the same parent bus. > + -1 - leave the mux as-is, which is the most optimal > + setting in terms of I2C operations and is the > + default mode. > + 0..<nchans> - set the mux to a predetermined channel, > + which is useful if there is one channel that is > + used almost always, and you want to reduce the > + latency for normal operations after rare > + transactions on other channels > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c > index e32fef560684..923aa3a5a3dc 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > @@ -49,6 +49,7 @@ > #include <linux/pm.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > +#include <dt-bindings/mux/mux.h> > > #define PCA954X_MAX_NCHANS 8 > > @@ -84,7 +85,9 @@ struct pca954x { > const struct chip_desc *chip; > > u8 last_chan; /* last register value */ > - u8 deselect; > + /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */ > + s8 idle_state; > + > struct i2c_client *client; > > struct irq_domain *irq; > @@ -253,15 +256,71 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan) > { > struct pca954x *data = i2c_mux_priv(muxc); > struct i2c_client *client = data->client; > + s8 idle_state; > + > + idle_state = READ_ONCE(data->idle_state); > + if (idle_state >= 0) > + /* Set the mux back to a predetermined channel */ > + return pca954x_select_chan(muxc, idle_state); > + > + if (idle_state == MUX_IDLE_DISCONNECT) { > + /* Deselect active channel */ > + data->last_chan = 0; > + return pca954x_reg_write(muxc->parent, client, > + data->last_chan); > + } > > - if (!(data->deselect & (1 << chan))) > - return 0; > + /* otherwise leave as-is */ > > - /* Deselect active channel */ > - data->last_chan = 0; > - return pca954x_reg_write(muxc->parent, client, data->last_chan); > + return 0; > +} > + > +static ssize_t idle_state_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + struct pca954x *data = i2c_mux_priv(muxc); > + > + return sprintf(buf, "%d\n", READ_ONCE(data->idle_state)); > +} > + > +static ssize_t idle_state_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + struct pca954x *data = i2c_mux_priv(muxc); > + int val; > + int ret; > + > + ret = kstrtoint(buf, 0, &val); > + if (ret < 0) > + return ret; > + > + if (val != MUX_IDLE_AS_IS && val != MUX_IDLE_DISCONNECT && > + (val < 0 || val >= data->chip->nchans)) > + return -EINVAL; > + > + i2c_lock_bus(muxc->parent, I2C_LOCK_SEGMENT); > + > + WRITE_ONCE(data->idle_state, val); > + /* > + * Set the mux into a state consistent with the new > + * idle_state. > + */ > + if (data->last_chan || val != MUX_IDLE_DISCONNECT) > + ret = pca954x_deselect_mux(muxc, 0); > + > + i2c_unlock_bus(muxc->parent, I2C_LOCK_SEGMENT); > + > + return ret < 0 ? ret : count; > } > > +static DEVICE_ATTR_RW(idle_state); > + > static irqreturn_t pca954x_irq_handler(int irq, void *dev_id) > { > struct pca954x *data = dev_id; > @@ -328,8 +387,11 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc) > static void pca954x_cleanup(struct i2c_mux_core *muxc) > { > struct pca954x *data = i2c_mux_priv(muxc); > + struct i2c_client *client = data->client; > int c, irq; > > + device_remove_file(&client->dev, &dev_attr_idle_state); > + > if (data->irq) { > for (c = 0; c < data->chip->nchans; c++) { > irq = irq_find_mapping(data->irq, c); > @@ -410,9 +472,12 @@ static int pca954x_probe(struct i2c_client *client, > } > > data->last_chan = 0; /* force the first selection */ > + data->idle_state = MUX_IDLE_AS_IS; > > idle_disconnect_dt = np && > of_property_read_bool(np, "i2c-mux-idle-disconnect"); > + if (idle_disconnect_dt) > + data->idle_state = MUX_IDLE_DISCONNECT; > > ret = pca954x_irq_setup(muxc); > if (ret) > @@ -420,8 +485,6 @@ static int pca954x_probe(struct i2c_client *client, > > /* Now create an adapter for each channel */ > for (num = 0; num < data->chip->nchans; num++) { > - data->deselect |= idle_disconnect_dt << num; > - > ret = i2c_mux_add_adapter(muxc, 0, num, 0); > if (ret) > goto fail_cleanup; > @@ -436,6 +499,12 @@ static int pca954x_probe(struct i2c_client *client, > goto fail_cleanup; > } > > + /* > + * The attr probably isn't going to be needed in most cases, > + * so don't fail completely on error. > + */ > + device_create_file(dev, &dev_attr_idle_state); > + > dev_info(dev, "registered %d multiplexed busses for I2C %s %s\n", > num, data->chip->muxtype == pca954x_ismux > ? "mux" : "switch", client->name); >
On Wed, Mar 13, 2019 at 07:04:06AM +0000, Peter Rosin wrote: > Hi Greg, > > I was wondering if you think the below ABI addition looks sane to you? I am not the i2c maintainer :) > Or, should I ask someone else for a tag? It's the middle of the merge window. None of us can do much other than focus on that until 5.1-rc1 is out. thanks, greg k-h
Hi Greg, > > I was wondering if you think the below ABI addition looks sane to you? > I am not the i2c maintainer :) Peter is the i2c-mux maintainer, so I trust him very much on the I2C side of things. We just wondered if you'd notice a general flaw when exposing such an interface to userspace. > > Or, should I ask someone else for a tag? > > It's the middle of the merge window. None of us can do much other than > focus on that until 5.1-rc1 is out. Sure, thanks anyway for the response. Regards, Wolfram
On Wed, Mar 13, 2019 at 05:36:10PM +0100, Wolfram Sang wrote: > Hi Greg, > > > > I was wondering if you think the below ABI addition looks sane to you? > > I am not the i2c maintainer :) > > Peter is the i2c-mux maintainer, so I trust him very much on the I2C > side of things. We just wondered if you'd notice a general flaw when > exposing such an interface to userspace. Ah, I see. No, looks fine to me, it's a single value, well documented, should be just fine. thanks, greg k-h
diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x b/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x new file mode 100644 index 000000000000..0b0de8cd0d13 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-pca954x @@ -0,0 +1,20 @@ +What: /sys/bus/i2c/.../idle_state +Date: January 2019 +KernelVersion: 5.2 +Contact: Robert Shearman <robert.shearman@att.com> +Description: + Value that exists only for mux devices that can be + written to control the behaviour of the multiplexer on + idle. Possible values: + -2 - disconnect on idle, i.e. deselect the last used + channel, which is useful when there is a device + with an address that conflicts with another + device on another mux on the same parent bus. + -1 - leave the mux as-is, which is the most optimal + setting in terms of I2C operations and is the + default mode. + 0..<nchans> - set the mux to a predetermined channel, + which is useful if there is one channel that is + used almost always, and you want to reduce the + latency for normal operations after rare + transactions on other channels diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c index e32fef560684..923aa3a5a3dc 100644 --- a/drivers/i2c/muxes/i2c-mux-pca954x.c +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c @@ -49,6 +49,7 @@ #include <linux/pm.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <dt-bindings/mux/mux.h> #define PCA954X_MAX_NCHANS 8 @@ -84,7 +85,9 @@ struct pca954x { const struct chip_desc *chip; u8 last_chan; /* last register value */ - u8 deselect; + /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */ + s8 idle_state; + struct i2c_client *client; struct irq_domain *irq; @@ -253,15 +256,71 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan) { struct pca954x *data = i2c_mux_priv(muxc); struct i2c_client *client = data->client; + s8 idle_state; + + idle_state = READ_ONCE(data->idle_state); + if (idle_state >= 0) + /* Set the mux back to a predetermined channel */ + return pca954x_select_chan(muxc, idle_state); + + if (idle_state == MUX_IDLE_DISCONNECT) { + /* Deselect active channel */ + data->last_chan = 0; + return pca954x_reg_write(muxc->parent, client, + data->last_chan); + } - if (!(data->deselect & (1 << chan))) - return 0; + /* otherwise leave as-is */ - /* Deselect active channel */ - data->last_chan = 0; - return pca954x_reg_write(muxc->parent, client, data->last_chan); + return 0; +} + +static ssize_t idle_state_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct i2c_client *client = to_i2c_client(dev); + struct i2c_mux_core *muxc = i2c_get_clientdata(client); + struct pca954x *data = i2c_mux_priv(muxc); + + return sprintf(buf, "%d\n", READ_ONCE(data->idle_state)); +} + +static ssize_t idle_state_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev); + struct i2c_mux_core *muxc = i2c_get_clientdata(client); + struct pca954x *data = i2c_mux_priv(muxc); + int val; + int ret; + + ret = kstrtoint(buf, 0, &val); + if (ret < 0) + return ret; + + if (val != MUX_IDLE_AS_IS && val != MUX_IDLE_DISCONNECT && + (val < 0 || val >= data->chip->nchans)) + return -EINVAL; + + i2c_lock_bus(muxc->parent, I2C_LOCK_SEGMENT); + + WRITE_ONCE(data->idle_state, val); + /* + * Set the mux into a state consistent with the new + * idle_state. + */ + if (data->last_chan || val != MUX_IDLE_DISCONNECT) + ret = pca954x_deselect_mux(muxc, 0); + + i2c_unlock_bus(muxc->parent, I2C_LOCK_SEGMENT); + + return ret < 0 ? ret : count; } +static DEVICE_ATTR_RW(idle_state); + static irqreturn_t pca954x_irq_handler(int irq, void *dev_id) { struct pca954x *data = dev_id; @@ -328,8 +387,11 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc) static void pca954x_cleanup(struct i2c_mux_core *muxc) { struct pca954x *data = i2c_mux_priv(muxc); + struct i2c_client *client = data->client; int c, irq; + device_remove_file(&client->dev, &dev_attr_idle_state); + if (data->irq) { for (c = 0; c < data->chip->nchans; c++) { irq = irq_find_mapping(data->irq, c); @@ -410,9 +472,12 @@ static int pca954x_probe(struct i2c_client *client, } data->last_chan = 0; /* force the first selection */ + data->idle_state = MUX_IDLE_AS_IS; idle_disconnect_dt = np && of_property_read_bool(np, "i2c-mux-idle-disconnect"); + if (idle_disconnect_dt) + data->idle_state = MUX_IDLE_DISCONNECT; ret = pca954x_irq_setup(muxc); if (ret) @@ -420,8 +485,6 @@ static int pca954x_probe(struct i2c_client *client, /* Now create an adapter for each channel */ for (num = 0; num < data->chip->nchans; num++) { - data->deselect |= idle_disconnect_dt << num; - ret = i2c_mux_add_adapter(muxc, 0, num, 0); if (ret) goto fail_cleanup; @@ -436,6 +499,12 @@ static int pca954x_probe(struct i2c_client *client, goto fail_cleanup; } + /* + * The attr probably isn't going to be needed in most cases, + * so don't fail completely on error. + */ + device_create_file(dev, &dev_attr_idle_state); + dev_info(dev, "registered %d multiplexed busses for I2C %s %s\n", num, data->chip->muxtype == pca954x_ismux ? "mux" : "switch", client->name);