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@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. 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
