Hi Andy, On Wed, 18 Jan 2023 19:39:46 +0200 Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote: > On Wed, Jan 18, 2023 at 06:17:53PM +0100, Luca Ceresoli wrote: > > On Wed, 18 Jan 2023 16:23:53 +0200 > > Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote: > > ... > > > > > +A typical example follows. > > > > + > > > > +Topology:: > > > > + > > > > + Slave X @ 0x10 > > > > + .-----. | > > > > + .-----. | |---+---- B > > > > + | CPU |--A--| ATR | > > > > + `-----' | |---+---- C > > > > + `-----' | > > > > + Slave Y @ 0x10 > > > > + > > > > +Alias table: > > > > + > > > > +.. table:: > > > > + > > > > + ====== ===== > > > > + Client Alias > > > > + ====== ===== > > > > + X 0x20 > > > > + Y 0x30 > > > > + ====== ===== > > > > + > > > > +Transaction: > > > > + > > > > + - Slave X driver sends a transaction (on adapter B), slave address 0x10 > > > > + - ATR driver rewrites messages with address 0x20, forwards to adapter A > > > > + - Physical I2C transaction on bus A, slave address 0x20 > > > > + - ATR chip propagates transaction on bus B with address translated to 0x10 > > > > + - Slave X chip replies on bus B > > > > + - ATR chip forwards reply on bus A > > > > + - ATR driver rewrites messages with address 0x10 > > > > + - Slave X driver gets back the msgs[], with reply and address 0x10 > > > > > > I'm not sure I got the real / virtual status of the adapters. Are the B and C > > > virtual ones, while A is the real? > > > > Let me reply, as I wrote these docs back at the times and thus I feel > > guilty in case that's unclear. :) > > > > I don't like the word "virtual" in this situation. A, B and C are all > > physical busses, made of copper and run by electrons on PCBs. B and C > > are the "remote" or "downstream" busses (w.r.t. the CPU), where the i2c > > devices are and where transactions happen using the address that the > > chip responds to. A is the "local" or "upstream" bus that is driven > > directly by the CPU (*) and where address aliases are used. Using > > aliases there is necessary because using address 0x10 would be > > ambiguous as there are two 0x10 chips out there. > > > > (*) There could be more layers of course, but still A is "closer to the > > CPU than B and C", for the sake of completeness. > > Can the diagram and/or text be updated to elaborate this? Let's see whether the text below is better. I haven't changed the image, I don't think we can do much more in ASCII, but maybe we can replace it with an SVG [0]? [0] https://github.com/lucaceresoli/docs/blob/master/video-serdes-linux/images/i2c-ti.svg A typical example follows. Topology:: Slave X @ 0x10 .-----. | .-----. | |---+---- B | CPU |--A--| ATR | `-----' | |---+---- C `-----' | Slave Y @ 0x10 Alias table: A, B and C are three physical I2C busses, electrically independent from each other. The ATR receives the transactions initiated on bus A and propagates them on bus B or bus C or none depending on the device address in the transaction and based on the alias table. Alias table: .. table:: =============== ===== Client Alias =============== ===== X (bus B, 0x10) 0x20 Y (bus C, 0x10) 0x30 =============== ===== Transaction: - Slave X driver sends a transaction (on adapter B), slave address 0x10 - ATR driver finds slave X is on bus B and has alias 0x20, rewrites messages with address 0x20, forwards to adapter A - Physical I2C transaction on bus A, slave address 0x20 - ATR chip detects transaction on address 0x20, finds it in table, propagates transaction on bus B with address translated to 0x10, keeps clock streched on bus A waiting for reply - Slave X chip (on bus B) detects transaction at its own physical address 0x10 and replies normally - ATR chip stops clock stretching and forwards reply on bus A, with address translated back to 0x20 - ATR driver receives the reply, rewrites messages with address 0x10 as they were initially - Slave X driver gets back the msgs[], with reply and address 0x10 Let me know whether this sounds better. And perhaps Tomi can further improve it. > > > > +void i2c_atr_set_driver_data(struct i2c_atr *atr, void *data) > > > > +{ > > > > + atr->priv = data; > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(i2c_atr_set_driver_data, I2C_ATR); > > > > + > > > > +void *i2c_atr_get_driver_data(struct i2c_atr *atr) > > > > +{ > > > > + return atr->priv; > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(i2c_atr_get_driver_data, I2C_ATR); > > > > > > Just to be sure: Is it really _driver_ data and not _device instance_ data? > > > > It is device instance data indeed. I don't remember why this got > > changed, but in v3 it was i2c_atr_set_clientdata(). > > It's me who was and is against calling it clientdata due to possible > confusion with i2c_set/get_clientdata() that is about *driver data*. > I missed that time the fact that this is about device instance data. > I dunno which name would be better in this case, i2c_atr_set/get_client_priv() ? Not sure I'm following you here. The i2c_atr_set_clientdata() name was given for similarity with i2c_set_clientdata(). The latter wraps dev_set_drvdata(), which sets `struct device`->driver_data. There is one driver_data per each `struct device` instance, not per each driver. The same goes for i2c_atr_set_driver_data(): there is one priv pointer per each `struct i2c_atr` instance. -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
