Hi Peter, after some pondering, find below my reply. On 27/11/18 19:51, Peter Rosin wrote: > On 2018-11-18 12:13, Luca Ceresoli wrote: >> Hi Peter, >> >> On 10/10/18 17:48, Luca Ceresoli wrote: >>> Hi Peter, >>> >>> On 08/10/2018 23:43, Peter Rosin wrote: >>>> On 2018-10-03 17:19, Luca Ceresoli wrote: >>>>> From: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx> >>>>> >>>>> i2c-mux instantiates one i2c_algorithm for each downstream adapter. >>>>> However these algorithms are all identical, depending only on the >>>>> parent adapter. >>>>> >>>>> Avoid duplication by hoisting the i2c_algorithm from the adapters to >>>>> the i2c_mux_core object, and reuse it in all the adapters. >>>> >>>> Ouch, while I like the concept of having one i2c_algorithm per mux, >>>> this patch is not working. Various i2c-mux drivers set the >>>> muxc->mux_locked variable *after* the i2c_mux_alloc call, and this >>>> patch breaks such use. >> >> I finally had a look into this issue. Three drivers are setting >> mux_locked after i2c_mux_alloc: i2c-mux-gpmux, i2c-mux-gpio and >> i2c-mux-pinctrl. >> >> i2c-mux-gpmux is trivial to fix. >> >> The other two are not trivial because: >> >> 1. they compute mux_locked from other variables >> 2. those variables are stored in the drivers "private" data >> 3. their private data is stored inside struct i2c_mux_core >> (muxc->priv) which exists only after i2c_mux_alloc() >> >> In those cases computing mux_locked before i2c_mux_alloc() involves >> quite invasive changes. It took 3 non-trivial commits just for >> i2c-mux-gpio, and I still have to look into i2c-mux-pinctrl. >> >> So the question is: do we really want to do this? >> >> Using the private storage provided by i2c_mux_alloc() is a handy >> feature, at least for simpler drivers which know in advance the flags >> they need to set. OTOH I don't like individual drivers to manipulate >> mux_core flags that look very much like internal data. It makes any >> change to the i2c mux core harder, since every changed line could have >> side effects in some drivers, which is what's happening here. >> >> What's your opinion about this issue? > > I obviously don't like that drivers are poking around in struct > i2c_mux_core. > > But, your description sounds precisely how I remembered it. The > underlying problem is of course that i2c-mux-gpio and > i2c-mux-pinctrl do really nasty digs into internal parts of the > gpio and the pinctrl subsystems as they *try* to figure out if > they should be mux-locked or parent-locked. The result of that > digging is not completely reliable, but it solves the issue > without help from device-tree properties in at least one case > that I know about. However, for that case I also know that there > is no risk of regression since I control the distribution of > both kernel and .dtb for any upgrade. Anyway, it was done like > it was since I at the time did not dare to question the feedback > from the device-tree camp, and actually thought it was a good > thing, and thus did not push for a device-tree property when > Rob complained about the property not describing HW and instead > was just working around kernel issues [1]. The mux-locked vs. > parent-locked property has been added since. In retrospect, the > whole attempt to auto-detect mux-locked or parent-locked was a > mistake, and everything would have been so much easier if the > device-tree could always just state what the requirement is. At > least that's my current thoughts on the matter. Maybe we should > attempt to remove the ugly auto-detect code and see if anyone > complains? I'd love to remove that code, but this would change existing behavior, and the change would go unnoticed until something breaks. Scary. What about requiring that either "mux-locked" or "parent-locked" is specified in DT? If none is specified, the driver would fail loudly at probe time and users would stop and fix their DT. Then of course they need to _upgrade_ their DT in existing products. > But of course, another aspect is that not everything is DT, so > perhaps there is no clean solution? Well, platform code would have to be changed like DT, adding 2 flags: mux-locked and parent-locked, and exactly one must be set. This seems to me the safest path. Do you think it is a good idea, especially with respect to the device tree camp? Thanks, -- Luca
