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.shear...@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.shear...@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.shear...@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);
>
