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? But of course, another aspect is that not everything is DT, so perhaps there is no clean solution? Cheers, Peter [1] https://lkml.org/lkml/2016/1/6/437
