Hello Cosmin, Tomi, On Tue, 3 Dec 2024 12:35:29 +0200 Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote: > On 12/3/24 11:39 AM, Luca Ceresoli wrote: > > Hello Tomi, > > > > On Fri, 29 Nov 2024 15:31:45 +0200 > > Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote: > > > >> On 29/11/2024 13:53, Luca Ceresoli wrote: > >> > >>>> So strictly speaking it's not an ATR, but this achieves the same. > >>> > >>> Thanks for the extensive and very useful explanation. I had completely > >>> missed the GMSL serder and their different I2C handling, apologies. > >>> > >>> So, the "parent ATR" is the GMSL deser, which is not an ATR but > >>> implementing it using i2c-atr makes the implementation cleaner. That > >>> makes sense. > >> > >> Right. > >> > >> But, honestly, I can't make my mind if I like the use of ATR here or not =). > > > > Hehe, indeed, hardware designers use a lot of fantasy in stretching the > > I2C standard to its limits, perhaps more than actually needed. > > > >> So it's not an ATR, but I'm not quite sure what it is. It's not just > >> that we need to change the addresses of the serializers, we need to do > >> that in particular way, enabling one port at a time to do the change. > >> > >> If we forget about the init time hurdles, and consider the situation > >> after the serializers are been set up and all ports have been enabled, > >> we have: > >> > >> There's the main i2c bus, on which we have the deserializer. The > >> deserializer acts as a i2c repeater (for any transaction that's not > >> directed to the deser), sending the messages to all serializers. The > >> serializers catch transactions directed at the ser, and everything else > >> goes through ATR and to the remote bus. > >> > >> Do we have something that represents such a "i2c repeater"? I guess we > >> could just have an i2c bus, created by the deser, and all the sers would > >> be on that bus. So we'd somehow do the initial address change first, > >> then set up the i2c bus, and the serializer i2c clients would be added > >> to that bus. > > > > So you think about another thing, like i2c-repeater, in addition to > > i2c-mux and i2c-atr? > > > > Since most of the functionality needed (besides allowing pass-through > transfers for unmapped I2C addresses) can be achieved already using I2C > ATR, I think we should make use of it. If it allows code reuse, then it makes sense. > > Well, I think it would make sense, as it would generalize a feature > > that might be used by other chips. However at the moment we do have a > > working driver for the GMSL deser, and so I don't see the benefit of > > extracting the i2c-repeater functionality to a separate file, unless > > there are drivers for other chips being implemented: this would motivate > > extracting common features to a shared file. IOW, I'd not generalize > > something with a single user. > > > > We have GMSL drivers for 6 new chips that make use of the I2C ATR, and > we want to send these upstream. Adding pass-through support for the I2C > ATR is one of the only things keeping us back, and it's the solution > that makes the most sense to me. I see, so I understand the aim of this patch. As it was presented initially, in a "fixes" series and without any mention to new GMSL drivers, it didn't make sense to me. I think it would be ideal to send it in the same series as the GMSL driver(s) using it, otherwise there is no reason to want this change. Also it should not be presented as a fix, because it is not: it is changing the ATR functionality in order to extend it to new chips. Finally, documenting the oddities (the deser being implemented using i2c-atr even though it is not an ATR, in the first place) wouldbe very useful in the commit message and/or cover letter. Best regards, Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
