On 2019-01-25 18:43, Robert Shearman wrote:
> From: Robert Shearman <robert.shearman@xxxxxxx>
>
> 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 the platform data or 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 device deselect mask using
> sysfs as a complement to the method of instantiating the device via
> sysfs.
Hi Robert,
Right, so I was still contemplating if I preferred sysfs or a module
parameter. I think a module param will get messy if you want
different behavior for different instances? But you can avoid locking
issues if you know up front what rules are in effect. I guess sysfs is
the more flexible approach.
Anyway, this new sysfs interface must be documented in
Documentation/ABI/...
However, I have some issues with your proposed interface. You are
exposing the raw deselect mask, which is an implementation detail
that is not directly accessible with the previous configuration
methods. I see no reason to provide a more flexible interface than
a boolean 'idle_disconnect' flag. I would very much prefer if this
new interface can be reused by other muxes in the future, should
the need arise, and idle_disconnect is present as a configuration
interface for other muxes so that seems like an okay interface to
me.
I also wonder what sane semantics are if someone tries to change
this idle_disconnect flag while a transaction is in progress? My gut
reaction is that such attempts should result in -EBUSY. You need
to add locking to handle that.
> Signed-off-by: Robert Shearman <robert.shearman@xxxxxxx>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 41 +++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index bfabf985e830..a425223c5c87 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -263,6 +263,40 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
> return pca954x_reg_write(muxc->parent, client, data->last_chan);
> }
>
> +static ssize_t deselect_mask_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, "0x%x\n", data->deselect);
> +}
> +
> +static ssize_t deselect_mask_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);
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 0, &val);
> + if (ret < 0)
> + return ret;
> +
> + if (val >= 1 << data->chip->nchans)
> + return -EINVAL;
> +
> + data->deselect = val;
I think the coding standard prescribes an empty line before the return.
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(deselect_mask);
> +
> static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
> {
> struct pca954x *data = dev_id;
> @@ -329,8 +363,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_deselect_mask);
> +
> if (data->irq) {
> for (c = 0; c < data->chip->nchans; c++) {
> irq = irq_find_mapping(data->irq, c);
> @@ -453,6 +490,10 @@ static int pca954x_probe(struct i2c_client *client,
> goto fail_cleanup;
> }
>
> + ret = device_create_file(dev, &dev_attr_deselect_mask);
> + if (ret)
> + goto fail_cleanup;
> +
Is it worth failing altogether if this fails? I don't think the attr
is going to be needed in most cases. Not that I expect failure, but...
Cheers,
Peter
> dev_info(dev, "registered %d multiplexed busses for I2C %s %s\n",
> num, data->chip->muxtype == pca954x_ismux
> ? "mux" : "switch", client->name);
>
