On 25/01/2019 18:56, Peter Rosin wrote:
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 Peter,
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/...
Ok, will include that for v2.
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.
Good suggestion, will do that in v2.
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.
I don't think locking is necessary just for setting data->deselect since
the value is only used during the deselect and the only possible
outcomes are that the deselect happens or it doesn't, regardless of
interleavings of instructions of the two operations. This is of course
assuming that data->deselect is read/written atomically, which I
probably should guarantee a little better.
Having said that, it might be nicer semantics to ensure that the mux
isn't left selected after setting idle_disconnect = 1, which implies an
I2C transaction and thus a lock. If you agree, then I'll go down that route.
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.
Indeed, will address in v2.
+ 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...
Maybe not, will remove the error checking in v2 then.
Thanks,
Rob
Cheers,
Peter
dev_info(dev, "registered %d multiplexed busses for I2C %s %s\n",
num, data->chip->muxtype == pca954x_ismux
? "mux" : "switch", client->name);
