Date:   Sun, 9 Dec 2018 22:46:19 +0100
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Peter Rosin <peda@...ntia.se>,
        Luca Ceresoli <lucaceresoli77@...il.com>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Wolfram Sang <wsa@...-dreams.de>
Subject: Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm

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

Powered by blists - more mailing lists