| Message ID | 20190125174340.15058-1-robertshearman@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: mux: pca954x: allow management of device deselect mask via sysfs | expand |
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); >
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); >> >
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
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);