Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Luca Ceresoli]

Hi Vladimir,

On 09/09/19 05:56, Vladimir Zapolskiy wrote:
> Hi Luca, Jacopo, Wolfram, Peter,
> 
> On 09/08/2019 11:45 PM, Vladimir Zapolskiy wrote:
>> Hi Luca, Jacopo, Wolfram, Peter,
>>
>> On 09/01/2019 05:31 PM, jacopo mondi wrote:
>>> Hi Luca,
>>>    thanks for keep pushing this series! I hope we can use part of this
>>> for the (long time) on-going GMSL work...
>>>
>>> I hope you will be patient enough to provide (another :) overview
>>> of this work during the BoF Wolfram has organized at LPC for the next
>>> week.
>>>
>>> In the meantime I would have some comments after having a read at the
>>> series and trying to apply its concept to GMSL
>>>
>>
>> I won't attend the LPC, however I would appreciate if you book some
>> time to review my original / alternative implementation of the TI
>> DS90Ux9xx I2C bridge device driver.
>>
>> For your convenience the links to the driver are given below:
>> * dt bindings: https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#mead5ea226550b
>> * driver code: https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m2fe3664c5f884
>> * usage example: https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m56c146f5decdc
>>
>> The reasons why my driver is better/more flexible/more functional are
>> discussed earlier, please let me know, if you expect anything else
>> from me to add, also I would be happy to get a summary of your offline
>> discussion.
> 
> I forgot to repeat my main objection against Luca's approach, the TI
> DS90Ux9xx I2C bridge driver does not require to call i2c_add_adapter()
> or register a new mux/bus and then do run select/deselect in runtime to
> overcome the created handicap.

Whether the ser/deser drivers should or not instantiate an adapter
depends on whether the child I2C bus is a separate bus or it's "the same
bus" as the parent bus. Which in turn is not obvious, and depends on how
you look at it.

On one hand, the child bus looks like it is the same as the parent bus
because transactions on the child bus also happen on the parent bus (bus
not the other way around).

On the other hand the child bus also looks like a separate bus because
it is electrically separated from the parent bus, it can have a
different speed, and devices on one child bus cannot talk with devices
on another child bus under the same ser/deser.

The way you modeled it has the advantage of not requiring a runtime
rewriting of slave addresses. Thas's what I implemented in
i2c_atr_map_msgs(), I don't love the fact that it happens at every
iteration, but its runtime cost is negligible compared to I2C speeds. On
the other hand I'm not sure how your approach would work if you have an
i2c bridge behind another i2c bridge (see the "ATR behind another ATR"
case mentioned by Peter).

By contrast, adding an adapter for each child bus has its advantages. I
didn't have to write code to instantiate the devices, letting the i2c
core do it. Also, as child busses show up as real busses it's possible
to instantiate devices from userspace just like any standard i2c bus with:

  echo eeprom 0x50 > /sys/bus/i2c/devices/i2c-4/new_device

and devices will be assigned an alias and be usable normally.

Finally, there is no need to call select()/deselect() in runtime. Those
callbacks are optional, and I'm probably removing them in a future
iteration as I'm more and more convinced they are not needed at all.

-- 
Luca