| Message ID | 20190225155105.19378-4-robertshearman@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: mux: pca954x: allow management of device idle state via sysfs | expand |
On 2019-02-25 16:51, 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 pca954x 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 +++++ Should not ABI/... patches go to someone? Who? Did you use get_maintainer.pl? I think you should at least include the main kernel list? Maybe someone filters messages looking for ABI changes? > drivers/i2c/muxes/i2c-mux-pca954x.c | 84 +++++++++++++++++-- > 2 files changed, 97 insertions(+), 7 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..809545a5f9e4 > --- /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 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 pca954x mux on the same parent Nit: It doesn't need to be a PCA954x mux, it could be any mux. Cheers, Peter
> > .../ABI/testing/sysfs-bus-i2c-devices-pca954x | 20 +++++ > > Should not ABI/... patches go to someone? Who? Did you use > get_maintainer.pl? I think you should at least include the main kernel > list? Maybe someone filters messages looking for ABI changes? CCing Greg is a good idea here. The hunk doesn't really to go via his tree AFAIK but an ack from him might be good. > > +What: /sys/bus/i2c/.../idle_state > > +Date: January 2019 > > +KernelVersion: 5.2 > > +Contact: Robert Shearman <robert.shearman@att.com> > > +Description: > > + Value 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 pca954x mux on the same parent > > Nit: It doesn't need to be a PCA954x mux, it could be any mux. And maybe say that it only exists for mux devices?
On 26/02/2019 11:54, Peter Rosin wrote: > On 2019-02-25 16:51, 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 pca954x 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 +++++ > > Should not ABI/... patches go to someone? Who? Did you use > get_maintainer.pl? I think you should at least include the main kernel > list? Maybe someone filters messages looking for ABI changes? get_maintainer.pl doesn't recommend anything more than usual with the presence of the ABI changes, but I can certainly copy LKML on the next version of the series. > >> drivers/i2c/muxes/i2c-mux-pca954x.c | 84 +++++++++++++++++-- >> 2 files changed, 97 insertions(+), 7 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..809545a5f9e4 >> --- /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 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 pca954x mux on the same parent > > Nit: It doesn't need to be a PCA954x mux, it could be any mux. Ok, will fix. I'll remove the reference to mux here since it could be any device and needn't be behind a mux. Thanks, Rob
On 26/02/2019 13:16, Wolfram Sang wrote: > >>> .../ABI/testing/sysfs-bus-i2c-devices-pca954x | 20 +++++ >> >> Should not ABI/... patches go to someone? Who? Did you use >> get_maintainer.pl? I think you should at least include the main kernel >> list? Maybe someone filters messages looking for ABI changes? > > CCing Greg is a good idea here. The hunk doesn't really to go via his > tree AFAIK but an ack from him might be good. Ok, will do. > >>> +What: /sys/bus/i2c/.../idle_state >>> +Date: January 2019 >>> +KernelVersion: 5.2 >>> +Contact: Robert Shearman <robert.shearman@att.com> >>> +Description: >>> + Value 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 pca954x mux on the same parent >> >> Nit: It doesn't need to be a PCA954x mux, it could be any mux. > > And maybe say that it only exists for mux devices? > Maybe I'm misunderstanding you, but does the filename of sysfs-bus-i2c-devices-pca954x not already convey that information? Thanks, Rob
> > > > +What: /sys/bus/i2c/.../idle_state > > > > +Date: January 2019 > > > > +KernelVersion: 5.2 > > > > +Contact: Robert Shearman <robert.shearman@att.com> > > > > +Description: > > > > + Value 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 pca954x mux on the same parent > > > > > > Nit: It doesn't need to be a PCA954x mux, it could be any mux. > > > > And maybe say that it only exists for mux devices? > > > > Maybe I'm misunderstanding you, but does the filename of > sysfs-bus-i2c-devices-pca954x not already convey that information? Unless I am overlooking something, the "What:" line above has no indication that the idle_state file only exists for muxes. So, it could be assumed it exists for every I2C device. My suggestion was to make that super clear in the "Description" that this is not the case. Does that make sense?
On 2019-02-27 18:55, Robert Shearman wrote: > On 26/02/2019 11:54, Peter Rosin wrote: >> On 2019-02-25 16:51, 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 pca954x 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 +++++ >> >> Should not ABI/... patches go to someone? Who? Did you use >> get_maintainer.pl? I think you should at least include the main kernel >> list? Maybe someone filters messages looking for ABI changes? > > get_maintainer.pl doesn't recommend anything more than usual with the > presence of the ABI changes, but I can certainly copy LKML on the next > version of the series. Please also Cc Greg as hinted by Wolfram, that seems like a good choice. >> >>> drivers/i2c/muxes/i2c-mux-pca954x.c | 84 +++++++++++++++++-- >>> 2 files changed, 97 insertions(+), 7 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..809545a5f9e4 >>> --- /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 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 pca954x mux on the same parent >> >> Nit: It doesn't need to be a PCA954x mux, it could be any mux. > > Ok, will fix. I'll remove the reference to mux here since it could be > any device and needn't be behind a mux. Oh, but some kind of mux is needed for the situation to occur. If there are no muxes forming an adapter tree, the I2C core will prevent address clashes. So, you need to have the same I2C address on two different branches of the tree, and they cannot be branches on the same mux. In summary, the minimum tree displaying the problem has two parallel muxes. That is, if IIUC... Cheers, Peter
On 27/02/2019 18:07, Wolfram Sang wrote: > >>>>> +What: /sys/bus/i2c/.../idle_state >>>>> +Date: January 2019 >>>>> +KernelVersion: 5.2 >>>>> +Contact: Robert Shearman <robert.shearman@att.com> >>>>> +Description: >>>>> + Value 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 pca954x mux on the same parent >>>> >>>> Nit: It doesn't need to be a PCA954x mux, it could be any mux. >>> >>> And maybe say that it only exists for mux devices? >>> >> >> Maybe I'm misunderstanding you, but does the filename of >> sysfs-bus-i2c-devices-pca954x not already convey that information? > > Unless I am overlooking something, the "What:" line above has no > indication that the idle_state file only exists for muxes. So, it could > be assumed it exists for every I2C device. My suggestion was to make > that super clear in the "Description" that this is not the case. > > Does that make sense? Yes, that makes sense. Thanks for explaining the reasoning, Rob
On 27/02/2019 18:34, Peter Rosin wrote: > On 2019-02-27 18:55, Robert Shearman wrote: >> On 26/02/2019 11:54, Peter Rosin wrote: >>> On 2019-02-25 16:51, 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 pca954x 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 +++++ >>> >>> Should not ABI/... patches go to someone? Who? Did you use >>> get_maintainer.pl? I think you should at least include the main kernel >>> list? Maybe someone filters messages looking for ABI changes? >> >> get_maintainer.pl doesn't recommend anything more than usual with the >> presence of the ABI changes, but I can certainly copy LKML on the next >> version of the series. > > Please also Cc Greg as hinted by Wolfram, that seems like a good choice. > >>> >>>> drivers/i2c/muxes/i2c-mux-pca954x.c | 84 +++++++++++++++++-- >>>> 2 files changed, 97 insertions(+), 7 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..809545a5f9e4 >>>> --- /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 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 pca954x mux on the same parent >>> >>> Nit: It doesn't need to be a PCA954x mux, it could be any mux. >> >> Ok, will fix. I'll remove the reference to mux here since it could be >> any device and needn't be behind a mux. > > Oh, but some kind of mux is needed for the situation to occur. If there > are no muxes forming an adapter tree, the I2C core will prevent address > clashes. So, you need to have the same I2C address on two different > branches of the tree, and they cannot be branches on the same mux. > In summary, the minimum tree displaying the problem has two parallel > muxes. Yes, good point - you made this point previously and I made the mistake again! I'll retain the reference to the mux, but remove the PCA954x specifics. Thanks, Rob
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..809545a5f9e4 --- /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 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 pca954x 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 5005bba8c72c..28d50b3aca12 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) @@ -422,7 +487,6 @@ static int pca954x_probe(struct i2c_client *client, for (num = 0; num < data->chip->nchans; num++) { force = 0; /* dynamic adap number */ class = 0; /* no class by default */ - data->deselect |= idle_disconnect_dt << num; ret = i2c_mux_add_adapter(muxc, force, num, class); if (ret) @@ -438,6 +502,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);