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
