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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Some inline comments below.

Regards
Matthias

Am 22.02.23 um 14:29 schrieb Tomi Valkeinen:
From: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxx>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
  Documentation/i2c/index.rst         |   1 +
  Documentation/i2c/muxes/i2c-atr.rst |  97 +++++
  MAINTAINERS                         |   8 +
  drivers/i2c/Kconfig                 |   9 +
  drivers/i2c/Makefile                |   1 +
  drivers/i2c/i2c-atr.c               | 548 ++++++++++++++++++++++++++++
  include/linux/i2c-atr.h             | 116 ++++++
  7 files changed, 780 insertions(+)
  create mode 100644 Documentation/i2c/muxes/i2c-atr.rst
  create mode 100644 drivers/i2c/i2c-atr.c
  create mode 100644 include/linux/i2c-atr.h

[...]
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
new file mode 100644
index 000000000000..5ab890b83670
--- /dev/null
+++ b/drivers/i2c/i2c-atr.c
@@ -0,0 +1,548 @@
[...]
+
+/*
+ * Replace all message addresses with their aliases, saving the original
+ * addresses.
+ *
+ * This function is internal for use in i2c_atr_master_xfer(). It must be
+ * followed by i2c_atr_unmap_msgs() to restore the original addresses.
+ */
+static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
+			    int num)
+{
+	struct i2c_atr *atr = chan->atr;
+	static struct i2c_atr_cli2alias_pair *c2a;
+	int i;
+
+	/* Ensure we have enough room to save the original addresses */
+	if (unlikely(chan->orig_addrs_size < num)) {
+		u16 *new_buf;
+
+		/* We don't care about old data, hence no realloc() */
+		new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL);
+		if (!new_buf)
+			return -ENOMEM;
+
+		kfree(chan->orig_addrs);
+		chan->orig_addrs = new_buf;
+		chan->orig_addrs_size = num;
+	}
+
+	for (i = 0; i < num; i++) {
+		chan->orig_addrs[i] = msgs[i].addr;
+
+		c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list,
+						   msgs[i].addr);
+		if (!c2a) {
+			dev_err(atr->dev, "client 0x%02x not mapped!\n",
+				msgs[i].addr);
+			return -ENXIO;
I miss the roll-back of previously modified msgs[].addr values.

+		}
+
+		msgs[i].addr = c2a->alias;
+	}
+
+	return 0;
+}
+
+/*
+ * Restore all message address aliases with the original addresses. This
+ * function is internal for use in i2c_atr_master_xfer().
+ *
+ * @see i2c_atr_map_msgs()
+ */
+static void i2c_atr_unmap_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
+			       int num)
+{
+	int i;
+
+	for (i = 0; i < num; i++)
+		msgs[i].addr = chan->orig_addrs[i];
Does this code needs null and size checks for orig_addrs/orig_addrs_size to protect from oopses? This cannot happen now as i2c_atr_master_xfer returns early when i2c_atr_map_msgs fails.

+}
+
+static int i2c_atr_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			       int num)
+{
+	struct i2c_atr_chan *chan = adap->algo_data;
+	struct i2c_atr *atr = chan->atr;
+	struct i2c_adapter *parent = atr->parent;
+	int ret;
+
+	/* Translate addresses */
+	mutex_lock(&chan->orig_addrs_lock);
+
+	ret = i2c_atr_map_msgs(chan, msgs, num);
+	if (ret < 0)
+		goto err_unlock;
+
+	/* Perform the transfer */
+	ret = i2c_transfer(parent, msgs, num);
+
+	/* Restore addresses */
+	i2c_atr_unmap_msgs(chan, msgs, num);
+
+err_unlock:
+	mutex_unlock(&chan->orig_addrs_lock);
+
+	return ret;
+}
+
[...]





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux