i2c: mux: pca954x: allow management of device deselect mask via sysfs

Message ID 20190125174340.15058-1-robertshearman@gmail.com
State Superseded
Headers show
Series
  • i2c: mux: pca954x: allow management of device deselect mask via sysfs
Related show

Commit Message

Robert Shearman Jan. 25, 2019, 5:43 p.m.
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 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.

Signed-off-by: Robert Shearman <robert.shearman@att.com>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 41 +++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

Comments

Peter Rosin Jan. 25, 2019, 6:56 p.m. | #1
On 2019-01-25 18: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 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@att.com>
> ---
>  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);
>
Robert Shearman Jan. 28, 2019, 12:03 p.m. | #2
On 25/01/2019 18:56, Peter Rosin wrote:
> On 2019-01-25 18: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 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@att.com>
>> ---
>>   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);
>>
>
Peter Rosin Jan. 28, 2019, 5:23 p.m. | #3
On 2019-01-28 13:03, Robert Shearman wrote:
> On 25/01/2019 18:56, Peter Rosin wrote:
>> On 2019-01-25 18: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 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.

Right, I missed this. Now that I think again, it might be even nicer
with an interface that specifies how the mux should behave when idle. I
see three valid settings:

- disconnect the mux (the behavior you desire)
- leave the mux as is (the default behavior)
- set the mux to some predetermined channel

The last one might be useful if there is one channel that is used almost
always, and you want to reduce the latency for normal stuff after a rare
transactions on other channels (or if the mux cannot disconnect and some
channel is preferable for some reason; this mux *can* disconnect so this
reason is of course not applicable here).

The mux subsystem uses -1 for "as-is", -2 for "disconnect" and
non-negative numbers are used to identify a specific idle channel.

Cheers,
Peter

Patch

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;
+	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;
+
 	dev_info(dev, "registered %d multiplexed busses for I2C %s %s\n",
 		 num, data->chip->muxtype == pca954x_ismux
 				? "mux" : "switch", client->name);