Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support

Hi Tomi, Wolfram,

On Wed, 22 Feb 2023 15:29:00 +0200
Tomi Valkeinen [off-list ref] wrote:

From: Luca Ceresoli <luca@lucaceresoli.net>

An ATR is a device that looks similar to an i2c-mux: it has an I2C
slave "upstream" port and N master "downstream" ports, and forwards
transactions from upstream to the appropriate downstream port. But it
is different in that the forwarded transaction has a different slave
address. The address used on the upstream bus is called the "alias"
and is (potentially) different from the physical slave address of the
downstream chip.

Add a helper file (just like i2c-mux.c for a mux or switch) to allow
implementing ATR features in a device driver. The helper takes care or
adapter creation/destruction and translates addresses at each transaction.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Wolfram, I think Tomi improved this work as much as currently possible
and this patch now looks extremely good to me. I wish we had this in
mainline soon. Does it make sense for me to send a Reviewed-by tag,
given I already have a S-o-b one?

I have a few _extremely_ minor notes below, but I hope they won't
slow down merging this work. They can definitely be addressed as a
follow-up patch after merging this.

Thank you a lot Tomi for having persisted in improving the ATR code!

s/an table/a table/

+all I2C transactions directed to devices on the child buses.
+
+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

s/sends/requests/ is possibly better to clarify there is still no
electrical transaction yet at this step, as we are still in software.

+ - 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
+
+Usage:
+
+ 1. In your driver (typically in the probe function) add an ATR by
+    calling i2c_atr_new() passing your attach/detach callbacks
+ 2. When the attach callback is called pick an appropriate alias,
+    configure it in your chip and return the chosen alias in the
+    alias_id parameter
+ 3. When the detach callback is called, deconfigure the alias from
+    your chip and put it back in the pool for later usage
+
+I2C ATR functions and data structures
+-------------------------------------
+
+.. kernel-doc:: include/linux/i2c-atr.h

...

Not quite anymore I think... should this line be removed?

+/**
+ * struct i2c_atr - The I2C ATR instance
+ * @parent:    The parent &struct i2c_adapter
+ * @dev:       The device that owns the I2C ATR instance
+ * @ops:       &struct i2c_atr_ops
+ * @priv:      Private driver data, set with i2c_atr_set_driver_data()
+ * @algo:      The &struct i2c_algorithm for adapters
+ * @lock:      Lock for the I2C bus segment (see &struct i2c_lock_operations)
+ * @max_adapters: Maximum number of adapters this I2C ATR can have
+ * @adapter:   Array of adapters
+ */
+struct i2c_atr {
+	struct i2c_adapter *parent;
+	struct device *dev;
+	const struct i2c_atr_ops *ops;
+
+	void *priv;
+
+	struct i2c_algorithm algo;
+	/* lock for the I2C bus segment (see struct i2c_lock_operations) */

This comment is identical to the one in the kerneldoc comments just
above, I'd just remove it.

+	struct mutex lock;
+	int max_adapters;
+
+	struct notifier_block i2c_nb;

Undocumented?

...

+void i2c_atr_delete(struct i2c_atr *atr)
+{

Maybe here we could iterate over atr->adapter[] and if any is != NULL
just call BUG_ON() or WARN()?

+	bus_unregister_notifier(&i2c_bus_type, &atr->i2c_nb);
+	mutex_destroy(&atr->lock);
+	kfree(atr);
+}
+EXPORT_SYMBOL_NS_GPL(i2c_atr_delete, I2C_ATR);

...

As above, this does not apply very much anymore as it did in v1.

...

+/**
+ * i2c_atr_delete - Delete an I2C ATR helper.
+ * @atr: I2C ATR helper to be deleted.
+ *
+ * Precondition: all the adapters added with i2c_atr_add_adapter() mumst be

s/mumst/must/

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com