[RFC,v2 0/6] TI camera serdes and I2C address translation

[RFC,v2 0/6] TI camera serdes and I2C address translation

[Luca Ceresoli]

Hi,

this is a second round of RFC patches to move forward on discussion about
proper kernel support for the TI DS90UB9xx serializer/deserializer chipsets
with I2C address translation.

RFCv2 is a major improvement over RFCv1, with several parts rewritten from
scratch. I2C address translationis now in its own file, there's decent
remote GPIO support, the deser driver is much closer to being complete, I
added a minimal serializer driver and, last but not least, forwarding one
video stream works.

My long-term goal is to be able to model different camera modules [or
display or other modules] similarly to beaglebone capes or rpi hats,
up to a model where:

 1. there can be different camera modules being designed over time
 2. there can be different base boards being designed over time
 3. there is a standard interconnection between them (mechanical,
    electrical, communication bus)
 4. camera modules and base boards are designed and sold independently
    (thanks to point 3)
 5. a camera module can be removed, and a different model connected, at
    runtime while the other modules are streaming

To allow the required flexibility I introduced the idea of an I2C alias
pool that must be defined in device tree. It is the responsibility of the
DT writer to fill the pool with addresses that are otherwise unused on the
local bus. The pool could not be filled automatically because there might
be conflicting chips on the local bus that are unknown to the software, or
that are just connected later.

Addresses from the pool are assigned to remote I2C slaves at runtime, when
they are added on the remote bus. The technical details of how address
translation works are documented in patch 2.

The big beast is hotplugging. Unfortunately this does not seem to be doable
"the right way" at the moment for at least two reasons. First, a v4l2 media
graph cannot be modified while the pipe is streaming, and AFAIK this will
not be possible soon. Second, because handling hotplug of devicetree-based
peripherals would require runtime DT overlay insertion and removal, which
is a slowly progressing work, but again not ready currently.

To overcome at least some of these limitations I found a compromise. The
model that I would consider "the correct one" looks like this:

 <-- base board -->        <------- remote camera module ------->
                           .---------------------.
 .-----.    .-----.        |         SER         |
 | CPU |----| DES |========|----------.----------|
 `-----'    `-----'  FPD   | GPIO ctl | I2C adap |----+----+----.
                    Link 3 `---------------------'    |    |    |
                    cable        ||||             remote I2C slaves
	                    remote GPIO pins	    

Here the deserializer (DES) is always present and connected, so it can be
probed vie DT during boot. The serializer (SER) is instantiated at runtime
when a link is established on the FPD-Link cable and the model
detected. An I2C adapter is created under the SER, and all the remote I2C
slaves are then instantiated under it.

But this would require to stop and modify the v4l2 pipe, including the
cameras still connected, just because one of them has been removed (or a
cable has gone faulty).

The comprimise I took looks like this:

            .------------------.        .-----.
 .-----.    |                  |========| SER |
 | CPU |----| DES   .----------|        `-----'
 `-----'    |       | I2C adap |----+----+----.
            `------------------'    |    |    |
                                remote I2C slaves

In this case the I2C adapter (representing the "remote" bus) is
instantiated under the DES, and is always present. This stil allows proper
hotplugging of the SER, and userspace can still add/remove remote I2C
slaves. But it makes it possible to instantiate a sensor and leave it
always instantiated, so that the v4l2 pipe is never modified and "believes"
the sensor is always there. Of course this opens other issues, and requires
non-standard wachanisms to start/stop the sensor and handle missing frames
when it is disconnected. My prototype design works thanks to the above
structure, a somewhat tweaked sensor driver and a bit of help from
userspace.

Finally, remote GPIOs.

            .------------------.        .-----.
 .-----.    |                  |========| SER |
 | CPU |----| DES   .----------|        `-----'
 `-----'    |       | GPIO ctl |          ||||
            `------------------'     remote GPIO pins


Similar to the I2C adapter, I chose to instantiate on the DES the GPIO
controllers to control the remote GPIOs, even if it looks like it should be
under the serializers. This decision allows to have the remote GPIOs
described in DT, and always available, so that e.g. the always-instantiated
sensor driver DT node can say 'reset-gpios = <&deser 1 2 0>'.

That's all, see the patches for the details.

References:

[0] Vladimir Zapolskiy proposal on other TI chips:
    https://www.spinics.net/lists/linux-gpio/msg33291.html
[1] Kieran Bingham's patches covering Maxim chips:
    https://www.spinics.net/lists/linux-media/msg142367.html

[RFCv1] https://lore.kernel.org/linux-media/20190108223953.9969-1-luca@lucaceresoli.net/


Luca Ceresoli (6):
  i2c: core: let adapters be notified of client attach/detach
  i2c: add I2C Address Translator (ATR) support
  media: dt-bindings: add DS90UB954-Q1 video deserializer
  media: dt-bindings: add DS90UB953-Q1 video serializer
  media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer
  media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer

 .../bindings/media/i2c/ti,ds90ub953-q1.txt    |   42 +
 .../bindings/media/i2c/ti,ds90ub954-q1.txt    |  194 ++
 drivers/i2c/Kconfig                           |    9 +
 drivers/i2c/Makefile                          |    1 +
 drivers/i2c/i2c-atr.c                         |  557 ++++++
 drivers/i2c/i2c-core-base.c                   |   16 +
 drivers/media/i2c/Kconfig                     |   24 +
 drivers/media/i2c/Makefile                    |    3 +
 drivers/media/i2c/ds90ub953.c                 |  354 ++++
 drivers/media/i2c/ds90ub954.c                 | 1575 +++++++++++++++++
 include/dt-bindings/media/ds90ub953.h         |   16 +
 include/linux/i2c-atr.h                       |   82 +
 include/linux/i2c.h                           |   16 +
 13 files changed, 2889 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
 create mode 100644 drivers/i2c/i2c-atr.c
 create mode 100644 drivers/media/i2c/ds90ub953.c
 create mode 100644 drivers/media/i2c/ds90ub954.c
 create mode 100644 include/dt-bindings/media/ds90ub953.h
 create mode 100644 include/linux/i2c-atr.h

-- 
2.17.1

[RFC,v2 1/6] i2c: core: let adapters be notified of client attach/detach

[Luca Ceresoli]

An adapter might need to know when a new device is about to be
added. This will soon bee needed to implement an "I2C address
translator" (ATR for short), a device that propagates I2C transactions
with a different slave address (an "alias" address). An ATR driver
needs to know when a slave is being added to find a suitable alias and
program the device translation map.

Add an attach/detach callback pair to allow adapter drivers to be
notified of clients being added and removed.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes RFCv1 -> RFCv2:

 - Document struct i2c_attach_operations
---
 drivers/i2c/i2c-core-base.c | 16 ++++++++++++++++
 include/linux/i2c.h         | 16 ++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index f26ed495d384..c08ca4bca9c1 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -776,6 +776,11 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 		}
 	}
 
+	if (adap->attach_ops &&
+	    adap->attach_ops->attach_client &&
+	    adap->attach_ops->attach_client(adap, info, client) != 0)
+		goto err_attach_client;
+
 	status = device_register(&client->dev);
 	if (status)
 		goto out_free_props;
@@ -786,6 +791,9 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 	return client;
 
 out_free_props:
+	if (adap->attach_ops && adap->attach_ops->detach_client)
+		adap->attach_ops->detach_client(adap, client);
+err_attach_client:
 	if (info->properties)
 		device_remove_properties(&client->dev);
 out_err_put_of_node:
@@ -832,9 +840,17 @@ EXPORT_SYMBOL_GPL(i2c_new_device);
  */
 void i2c_unregister_device(struct i2c_client *client)
 {
+	struct i2c_adapter *adap;
+
 	if (!client)
 		return;
 
+	adap = client->adapter;
+
+	if (adap->attach_ops &&
+	    adap->attach_ops->detach_client)
+		adap->attach_ops->detach_client(adap, client);
+
 	if (client->dev.of_node) {
 		of_node_clear_flag(client->dev.of_node, OF_POPULATED);
 		of_node_put(client->dev.of_node);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fa5552c2307b..ebc372a0e537 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -567,6 +567,21 @@ struct i2c_lock_operations {
 	void (*unlock_bus)(struct i2c_adapter *adapter, unsigned int flags);
 };
 
+/**
+ * struct i2c_attach_operations - callbacks to notify client attach/detach
+ * @attach_client: Notify of new client being attached
+ * @detach_client: Notify of new client being detached
+ *
+ * Both ops are optional.
+ */
+struct i2c_attach_operations {
+	int  (*attach_client)(struct i2c_adapter *adapter,
+			      const struct i2c_board_info *info,
+			      const struct i2c_client *client);
+	void (*detach_client)(struct i2c_adapter *adapter,
+			      const struct i2c_client *client);
+};
+
 /**
  * struct i2c_timings - I2C timing information
  * @bus_freq_hz: the bus frequency in Hz
@@ -690,6 +705,7 @@ struct i2c_adapter {
 
 	/* data fields that are valid for all devices	*/
 	const struct i2c_lock_operations *lock_ops;
+	const struct i2c_attach_operations *attach_ops;
 	struct rt_mutex bus_lock;
 	struct rt_mutex mux_lock;
 
-- 
2.17.1

[RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Luca Ceresoli]

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 is
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>

---

Changes RFCv1 -> RFCv2:

 RFCv1 was implemented inside i2c-mux.c and added yet more complexity
 there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR
 features are not in a MUX and vice versa, the overlapping is low. This was
 almost a complete rewrite, but for the records here are the main
 differences from the old implementation:

 - change bus description
 - remove I2C_ATR_ARBITRATOR and I2C_ATR_GATE support
 - select() optional
 - rename i2c_atr_alloc -> i2c_atr_new, add i2c_atr_delete, move to bottom
 - lock the ATR, not the adapter or the muxes on the adapter
 - remove parent-locked code
 - remove I2C_MUX_LOCKED flag, now unused
 - remove I2C_ATR_ATR flag (always true)
 - translation in i2c_atr_smbus_xfer too
 - i2c_atr_map_msgs: don't ignore mapping errors
 - always require the "i2c-atr" DT node, no magic
 - remove ACPI support
 - one algo in the atrc, not one per adapter
 - remove unneeded i2c_atr_root_adapter
 - ditch force_nr
 - don't allocate private user memory, just provide a plain userdata pointer
 - consolidate all ops in a single struct, simplify naming
 - remove adapters individually, allocate in atrc->adapter[chan_id]
---
 drivers/i2c/Kconfig     |   9 +
 drivers/i2c/Makefile    |   1 +
 drivers/i2c/i2c-atr.c   | 557 ++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-atr.h |  82 ++++++
 4 files changed, 649 insertions(+)
 create mode 100644 drivers/i2c/i2c-atr.c
 create mode 100644 include/linux/i2c-atr.h

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index abedd55a1264..5df088b1d9de 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -71,6 +71,15 @@ config I2C_MUX
 
 source "drivers/i2c/muxes/Kconfig"
 
+config I2C_ATR
+	tristate "I2C Address Translator (ATR) support"
+	help
+	  Enable support for I2C Address Translator (ATR) chips.
+
+	  An ATR allows accessing multiple I2C busses from a single
+	  physical bus via address translation instead of bus selection as
+	  i2c-muxes do.
+
 config I2C_HELPER_AUTO
 	bool "Autoselect pertinent helper modules"
 	default y
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index bed6ba63c983..81849ea393c7 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -13,6 +13,7 @@ i2c-core-$(CONFIG_OF) 		+= i2c-core-of.o
 obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
 obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
+obj-$(CONFIG_I2C_ATR)		+= i2c-atr.o
 obj-y				+= algos/ busses/ muxes/
 obj-$(CONFIG_I2C_STUB)		+= i2c-stub.o
 obj-$(CONFIG_I2C_SLAVE_EEPROM)	+= i2c-slave-eeprom.o
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
new file mode 100644
index 000000000000..2b61c10a8ff6
--- /dev/null
+++ b/drivers/i2c/i2c-atr.c
@@ -0,0 +1,557 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * drivers/i2c/i2c-atr.c -- I2C Address Translator
+ *
+ * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
+ *
+ * An I2C Address Translator (ATR) is a device with an I2C slave parent
+ * ("upstream") port and N I2C master child ("downstream") ports, and
+ * forwards transactions from upstream to the appropriate downstream port
+ * with a modified slave address. The address used on the parent bus is
+ * called the "alias" and is (potentially) different from the physical
+ * slave address of the child bus. Address translation is done by the
+ * hardware.
+ *
+ * An ATR looks similar to an i2c-mux except:
+ * - the address on the parent and child busses can be different
+ * - there is normally no need to select the child port; the alias used on
+ *   the parent bus implies it
+ *
+ * The ATR functionality can be provided by a chip with many other
+ * features. This file provides a helper to implement an ATR within your
+ * driver.
+ *
+ * The ATR creates a new I2C "child" adapter on each child bus. Adding
+ * devices on the child bus ends up in invoking the driver code to select
+ * an available alias. Maintaining an appropriate pool of available aliases
+ * and picking one for each new device is up to the driver implementer. The
+ * ATR maintains an table of currently assigned alias and uses it to modify
+ * 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:
+ *
+ *   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
+ *
+ * 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
+ *
+ * Originally based on i2c-mux.c
+ */
+
+#include <linux/i2c.h>
+#include <linux/i2c-atr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+
+/**
+ * struct i2c_atr_cli2alias_pair - Hold the alias assigned to a client.
+ * @node:   List node
+ * @client: Pointer to the client on the child bus
+ * @alias:  I2C alias address assigned by the driver.
+ *          This is the address that will be used to issue I2C transactions
+ *          on the parent (physical) bus.
+ */
+struct i2c_atr_cli2alias_pair {
+	struct list_head node;
+	const struct i2c_client *client;
+	u16 alias;
+};
+
+/*
+ * Data for each channel (child bus)
+ */
+struct i2c_atr_chan {
+	struct i2c_adapter adap;
+	struct i2c_atr *atr;
+	u32 chan_id;
+
+	struct list_head alias_list;
+
+	u16 *orig_addrs;
+	unsigned int orig_addrs_size;
+	struct mutex orig_addrs_lock; /* Lock orig_addrs during xfer */
+};
+
+static struct i2c_atr_cli2alias_pair *
+i2c_atr_find_mapping_by_client(struct list_head *list,
+			       struct i2c_client *client)
+{
+	struct i2c_atr_cli2alias_pair *c2a;
+
+	list_for_each_entry(c2a, list, node) {
+		if (c2a->client == client)
+			return c2a;
+	}
+
+	return NULL;
+}
+
+static struct i2c_atr_cli2alias_pair *
+i2c_atr_find_mapping_by_addr(struct list_head *list,
+			     u16 phys_addr)
+{
+	struct i2c_atr_cli2alias_pair *c2a;
+
+	list_for_each_entry(c2a, list, node) {
+		if (c2a->client->addr == phys_addr)
+			return c2a;
+	}
+
+	return NULL;
+}
+
+/*
+ * 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)) {
+		void *new_buf = kmalloc(num * sizeof(chan->orig_addrs[0]),
+					GFP_KERNEL);
+		if (new_buf == NULL)
+			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) {
+			msgs[i].addr = c2a->alias;
+		} else {
+			dev_err(atr->dev, "client 0x%02x not mapped!\n",
+				msgs[i].addr);
+			return -ENXIO;
+		}
+	}
+
+	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];
+}
+
+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 = 0;
+
+	/* Switch to the right atr port */
+	if (atr->ops->select) {
+		ret = atr->ops->select(atr, chan->chan_id);
+		if (ret < 0)
+			goto out;
+	}
+
+	/* Translate addresses */
+	mutex_lock(&chan->orig_addrs_lock);
+	ret = i2c_atr_map_msgs(chan, msgs, num);
+	if (ret < 0) {
+		mutex_unlock(&chan->orig_addrs_lock);
+		goto out;
+	}
+
+	/* Perform the transfer */
+	ret = i2c_transfer(parent, msgs, num);
+
+	/* Restore addresses */
+	i2c_atr_unmap_msgs(chan, msgs, num);
+	mutex_unlock(&chan->orig_addrs_lock);
+
+out:
+	if (atr->ops->deselect)
+		atr->ops->deselect(atr, chan->chan_id);
+
+	return ret;
+}
+
+static int i2c_atr_smbus_xfer(struct i2c_adapter *adap,
+			      u16 addr, unsigned short flags,
+			      char read_write, u8 command,
+			      int size, union i2c_smbus_data *data)
+{
+	struct i2c_atr_chan *chan = adap->algo_data;
+	struct i2c_atr *atr = chan->atr;
+	struct i2c_adapter *parent = atr->parent;
+	struct i2c_atr_cli2alias_pair *c2a;
+	int err = 0;
+
+	c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list, addr);
+	if (!c2a) {
+		dev_err(atr->dev, "client 0x%02x not mapped!\n", addr);
+		return -ENXIO;
+	}
+
+	if (atr->ops->select)
+		err = atr->ops->select(atr, chan->chan_id);
+	if (!err)
+		err = i2c_smbus_xfer(parent, c2a->alias, flags,
+				     read_write, command, size, data);
+	if (atr->ops->deselect)
+		atr->ops->deselect(atr, chan->chan_id);
+
+	return err;
+}
+
+static u32 i2c_atr_functionality(struct i2c_adapter *adap)
+{
+	struct i2c_atr_chan *chan = adap->algo_data;
+	struct i2c_adapter *parent = chan->atr->parent;
+
+	return parent->algo->functionality(parent);
+}
+
+static void i2c_atr_lock_bus(struct i2c_adapter *adapter, unsigned int flags)
+{
+	struct i2c_atr_chan *chan = adapter->algo_data;
+	struct i2c_atr *atr = chan->atr;
+
+	mutex_lock(&atr->lock);
+}
+
+static int i2c_atr_trylock_bus(struct i2c_adapter *adapter, unsigned int flags)
+{
+	struct i2c_atr_chan *chan = adapter->algo_data;
+	struct i2c_atr *atr = chan->atr;
+
+	return mutex_trylock(&atr->lock);
+}
+
+static void i2c_atr_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
+{
+	struct i2c_atr_chan *chan = adapter->algo_data;
+	struct i2c_atr *atr = chan->atr;
+
+	mutex_unlock(&atr->lock);
+}
+
+static const struct i2c_lock_operations i2c_atr_lock_ops = {
+	.lock_bus =    i2c_atr_lock_bus,
+	.trylock_bus = i2c_atr_trylock_bus,
+	.unlock_bus =  i2c_atr_unlock_bus,
+};
+
+static int i2c_atr_attach_client(struct i2c_adapter *adapter,
+				 const struct i2c_board_info *info,
+				 const struct i2c_client *client)
+{
+	struct i2c_atr_chan *chan = adapter->algo_data;
+	struct i2c_atr *atr = chan->atr;
+	struct i2c_atr_cli2alias_pair *c2a;
+	u16 alias_id = 0;
+	int err = 0;
+
+	c2a = kzalloc(sizeof(struct i2c_atr_cli2alias_pair), GFP_KERNEL);
+	if (!c2a) {
+		err = -ENOMEM;
+		goto err_alloc;
+	}
+
+	err = atr->ops->attach_client(atr, chan->chan_id, info, client,
+				      &alias_id);
+	if (err)
+		goto err_attach;
+	if (alias_id == 0) {
+		err = -EINVAL;
+		goto err_attach;
+	}
+
+	c2a->client = client;
+	c2a->alias = alias_id;
+	list_add(&c2a->node, &chan->alias_list);
+
+	return 0;
+
+err_attach:
+	kfree(c2a);
+err_alloc:
+	return err;
+}
+
+static void i2c_atr_detach_client(struct i2c_adapter *adapter,
+				  const struct i2c_client *client)
+{
+	struct i2c_atr_chan *chan = adapter->algo_data;
+	struct i2c_atr *atr = chan->atr;
+	struct i2c_atr_cli2alias_pair *c2a;
+
+	atr->ops->detach_client(atr, chan->chan_id, client);
+
+	c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client);
+	if (c2a != NULL) {
+		list_del(&c2a->node);
+		kfree(c2a);
+	}
+}
+
+static const struct i2c_attach_operations i2c_atr_attach_ops = {
+	.attach_client = i2c_atr_attach_client,
+	.detach_client = i2c_atr_detach_client,
+};
+
+/**
+ * i2c_atr_add_adapter - Create a child ("downstream") I2C bus.
+ * @atr:     The I2C ATR
+ * @chan_id: Index of the new adapter (0 .. max_adapters-1).  This value is
+ *           passed to the callbacks in `struct i2c_atr_ops`.
+ *
+ * After calling this function a new i2c bus will appear. Adding and
+ * removing devices on the downstream bus will result in calls to the
+ * `attach_client` and `detach_client` callbacks for the driver to assign
+ * an alias to the device.
+ *
+ * If there is a device tree node under "i2c-atr" whose "reg" property
+ * equals chan_id, the new adapter will receive that node and perhaps start
+ * adding devices under it. The callbacks for those additions will be made
+ * before i2c_atr_add_adapter() returns.
+ *
+ * Call i2c_atr_del_adapter() to remove the adapter.
+ *
+ * Return: 0 on success, a negative error code otherwise.
+ */
+int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id)
+{
+	struct i2c_adapter *parent = atr->parent;
+	struct device *dev = atr->dev;
+	struct i2c_atr_chan *chan;
+	char symlink_name[20];
+	int err;
+
+	if (chan_id >= atr->max_adapters)
+		return -EINVAL;
+
+	if (atr->adapter[chan_id]) {
+		dev_err(dev, "Adapter %d already present\n", chan_id);
+		return -EEXIST;
+	}
+
+	chan = kzalloc(sizeof(*chan), GFP_KERNEL);
+	if (!chan)
+		return -ENOMEM;
+
+	chan->atr = atr;
+	chan->chan_id = chan_id;
+	INIT_LIST_HEAD(&chan->alias_list);
+	mutex_init(&chan->orig_addrs_lock);
+
+	snprintf(chan->adap.name, sizeof(chan->adap.name),
+		 "i2c-%d-atr-%d", i2c_adapter_id(parent), chan_id);
+	chan->adap.owner = THIS_MODULE;
+	chan->adap.algo = &atr->algo;
+	chan->adap.algo_data = chan;
+	chan->adap.dev.parent = dev;
+	chan->adap.retries = parent->retries;
+	chan->adap.timeout = parent->timeout;
+	chan->adap.quirks = parent->quirks;
+	chan->adap.lock_ops = &i2c_atr_lock_ops;
+	chan->adap.attach_ops = &i2c_atr_attach_ops;
+
+	if (dev->of_node) {
+		struct device_node *atr_node;
+		struct device_node *child;
+		u32 reg;
+
+		atr_node = of_get_child_by_name(dev->of_node, "i2c-atr");
+
+		for_each_child_of_node(atr_node, child) {
+			err = of_property_read_u32(child, "reg", &reg);
+			if (err)
+				continue;
+			if (chan_id == reg)
+				break;
+		}
+
+		chan->adap.dev.of_node = child;
+		of_node_put(atr_node);
+	}
+
+	err = i2c_add_adapter(&chan->adap);
+	if (err) {
+		dev_err(dev, "failed to add atr-adapter %u (error=%d)\n",
+			chan_id, err);
+		goto err_add_adapter;
+	}
+
+	WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"),
+	     "can't create symlink to atr device\n");
+	snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id);
+	WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name),
+	     "can't create symlink for channel %u\n", chan_id);
+
+	dev_info(dev, "Added ATR child bus %d\n", i2c_adapter_id(&chan->adap));
+
+	atr->adapter[chan_id] = &chan->adap;
+	return 0;
+
+err_add_adapter:
+	mutex_destroy(&chan->orig_addrs_lock);
+	kfree(chan);
+	return err;
+}
+EXPORT_SYMBOL_GPL(i2c_atr_add_adapter);
+
+/**
+ * i2c_atr_del_adapter - Remove a child ("downstream") I2C bus added by
+ * i2c_atr_del_adapter().
+ * @atr:     The I2C ATR
+ * @chan_id: Index of the `adapter to be removed (0 .. max_adapters-1)
+ */
+void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id)
+{
+	char symlink_name[20];
+
+	struct i2c_adapter *adap = atr->adapter[chan_id];
+	struct i2c_atr_chan *chan = adap->algo_data;
+	struct device_node *np = adap->dev.of_node;
+	struct device *dev = atr->dev;
+
+	if (atr->adapter[chan_id] == NULL) {
+		dev_err(dev, "Adapter %d does not exist\n", chan_id);
+		return;
+	}
+
+	dev_info(dev, "Removing ATR child bus %d\n", i2c_adapter_id(adap));
+
+	atr->adapter[chan_id] = NULL;
+
+	snprintf(symlink_name, sizeof(symlink_name),
+		 "channel-%u", chan->chan_id);
+	sysfs_remove_link(&dev->kobj, symlink_name);
+	sysfs_remove_link(&chan->adap.dev.kobj, "atr_device");
+
+	i2c_del_adapter(adap);
+	of_node_put(np);
+	mutex_destroy(&chan->orig_addrs_lock);
+	kfree(chan);
+}
+EXPORT_SYMBOL_GPL(i2c_atr_del_adapter);
+
+/**
+ * i2c_atr_new() - Allocate and initialize an I2C ATR helper.
+ * @parent:       The parent (upstream) adapter
+ * @dev:          The device acting as an ATR
+ * @ops:          Driver-specific callbacks
+ * @max_adapters: Maximum number of child adapters
+ *
+ * The new ATR helper is connected to the parent adapter but has no child
+ * adapters. Call i2c_atr_add_adapter() to add some.
+ *
+ * Call i2c_atr_delete() to remove.
+ *
+ * Return: pointer to the new ATR helper object, or ERR_PTR
+ */
+struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
+			    const struct i2c_atr_ops *ops, int max_adapters)
+{
+	struct i2c_atr *atr;
+
+	if (!ops || !ops->attach_client || !ops->detach_client)
+		return ERR_PTR(-EINVAL);
+
+	atr = devm_kzalloc(dev, sizeof(*atr)
+			    + max_adapters * sizeof(atr->adapter[0]),
+			    GFP_KERNEL);
+	if (!atr)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&atr->lock);
+
+	atr->parent = parent;
+	atr->dev = dev;
+	atr->ops = ops;
+	atr->max_adapters = max_adapters;
+
+	if (parent->algo->master_xfer)
+		atr->algo.master_xfer = i2c_atr_master_xfer;
+	if (parent->algo->smbus_xfer)
+		atr->algo.smbus_xfer = i2c_atr_smbus_xfer;
+	atr->algo.functionality = i2c_atr_functionality;
+
+	return atr;
+}
+EXPORT_SYMBOL_GPL(i2c_atr_new);
+
+/**
+ * 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
+ * removed by calling i2c_atr_del_adapter().
+ */
+void i2c_atr_delete(struct i2c_atr *atr)
+{
+	mutex_destroy(&atr->lock);
+}
+EXPORT_SYMBOL_GPL(i2c_atr_delete);
+
+MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>");
+MODULE_DESCRIPTION("I2C Address Translator");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
new file mode 100644
index 000000000000..019816e5a50c
--- /dev/null
+++ b/include/linux/i2c-atr.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/**
+ * drivers/i2c/i2c-atr.h -- I2C Address Translator
+ *
+ * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
+ *
+ * Based on i2c-mux.h
+ */
+
+#ifndef _LINUX_I2C_ATR_H
+#define _LINUX_I2C_ATR_H
+
+#ifdef __KERNEL__
+
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+
+struct i2c_atr;
+
+/**
+ * struct i2c_atr_ops - Callbacks from ATR to the device driver.
+ * @select:        Ask the driver to select a child bus (optional)
+ * @deselect:      Ask the driver to deselect a child bus (optional)
+ * @attach_client: Notify the driver of a new device connected on a child
+ *                 bus. The driver must choose an I2C alias, configure the
+ *                 hardware to use it and return it in `alias_id`.
+ * @detach_client: Notify the driver of a device getting disconnected. The
+ *                 driver must configure the hardware to stop using the
+ *                 alias.
+ *
+ * All these functions return 0 on success, a negative error code otherwise.
+ */
+struct i2c_atr_ops {
+	int  (*select)(struct i2c_atr *atr, u32 chan_id);
+	int  (*deselect)(struct i2c_atr *atr, u32 chan_id);
+	int  (*attach_client)(struct i2c_atr *atr, u32 chan_id,
+			      const struct i2c_board_info *info,
+			      const struct i2c_client *client,
+			      u16 *alias_id);
+	void (*detach_client)(struct i2c_atr *atr, u32 chan_id,
+			      const struct i2c_client *client);
+};
+
+/**
+ * Helper to add I2C ATR features to a device driver.
+ */
+struct i2c_atr {
+	/* private: internal use only */
+
+	struct i2c_adapter *parent;
+	struct device *dev;
+	const struct i2c_atr_ops *ops;
+
+	void *priv;
+
+	struct i2c_algorithm algo;
+	struct mutex lock;
+	int max_adapters;
+
+	struct i2c_adapter *adapter[0];
+};
+
+struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
+			    const struct i2c_atr_ops *ops, int max_adapters);
+void i2c_atr_delete(struct i2c_atr *atr);
+
+static inline void i2c_atr_set_clientdata(struct i2c_atr *atr, void *data)
+{
+	atr->priv = data;
+}
+
+static inline void *i2c_atr_get_clientdata(struct i2c_atr *atr)
+{
+	return atr->priv;
+}
+
+int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id);
+void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id);
+
+#endif /* __KERNEL__ */
+
+#endif /* _LINUX_I2C_ATR_H */
-- 
2.17.1

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[jacopo mondi]

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

On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
> 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 is
> 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>
>
> ---
>
> Changes RFCv1 -> RFCv2:
>
>  RFCv1 was implemented inside i2c-mux.c and added yet more complexity
>  there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR
>  features are not in a MUX and vice versa, the overlapping is low. This was
>  almost a complete rewrite, but for the records here are the main
>  differences from the old implementation:

I'm not an i2c expert, but this looks very similar to me to an
augmented i2c-mux with the following additional features:
- automatic selection of the i2c address to use for the children
  behind the mux
- automatic translation of the addresses the logical aliases to
  the actual physical addresses of the slaves (with the help of the
  deserializer address translation feature in this case).

In the GMSL perspective we have similar needs but limited to the
selection of the i2c addresses to assign to the children behind our
mux. The maxims's chips work by reprogramming the remote devices and
do not support address translations on the deserializer side, unlike
what the TI chips do.

So I wonder if we could somehow split the here proposed solution in
two, one part to perform the address selection, the other one to
support address reprogramming.

One thing I have noticed is that it's up to the driver using the ATR
(the DS90UB954 deserializer in this case) picking into the alias pool
and assign aliases. I would have expected this to be a feature of the
ATR itself, if you aim to have the i2c-alias-pool as a standard
construct.

I understand the pool of free aliases 'belongs' to the base board
device, but the ATR could be provided with a pointer to it and the
routines to select and assign addresses generalized. Is there a reason
why you decided otherwise that I'm not seeing?

>
>  - change bus description
>  - remove I2C_ATR_ARBITRATOR and I2C_ATR_GATE support
>  - select() optional
>  - rename i2c_atr_alloc -> i2c_atr_new, add i2c_atr_delete, move to bottom
>  - lock the ATR, not the adapter or the muxes on the adapter
>  - remove parent-locked code
>  - remove I2C_MUX_LOCKED flag, now unused
>  - remove I2C_ATR_ATR flag (always true)
>  - translation in i2c_atr_smbus_xfer too
>  - i2c_atr_map_msgs: don't ignore mapping errors
>  - always require the "i2c-atr" DT node, no magic
>  - remove ACPI support
>  - one algo in the atrc, not one per adapter
>  - remove unneeded i2c_atr_root_adapter
>  - ditch force_nr
>  - don't allocate private user memory, just provide a plain userdata pointer
>  - consolidate all ops in a single struct, simplify naming
>  - remove adapters individually, allocate in atrc->adapter[chan_id]
> ---
>  drivers/i2c/Kconfig     |   9 +
>  drivers/i2c/Makefile    |   1 +
>  drivers/i2c/i2c-atr.c   | 557 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c-atr.h |  82 ++++++
>  4 files changed, 649 insertions(+)
>  create mode 100644 drivers/i2c/i2c-atr.c
>  create mode 100644 include/linux/i2c-atr.h
>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index abedd55a1264..5df088b1d9de 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -71,6 +71,15 @@ config I2C_MUX
>
>  source "drivers/i2c/muxes/Kconfig"
>
> +config I2C_ATR
> +	tristate "I2C Address Translator (ATR) support"
> +	help
> +	  Enable support for I2C Address Translator (ATR) chips.
> +
> +	  An ATR allows accessing multiple I2C busses from a single
> +	  physical bus via address translation instead of bus selection as
> +	  i2c-muxes do.
> +
>  config I2C_HELPER_AUTO
>  	bool "Autoselect pertinent helper modules"
>  	default y
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index bed6ba63c983..81849ea393c7 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -13,6 +13,7 @@ i2c-core-$(CONFIG_OF) 		+= i2c-core-of.o
>  obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
>  obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
>  obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
> +obj-$(CONFIG_I2C_ATR)		+= i2c-atr.o
>  obj-y				+= algos/ busses/ muxes/
>  obj-$(CONFIG_I2C_STUB)		+= i2c-stub.o
>  obj-$(CONFIG_I2C_SLAVE_EEPROM)	+= i2c-slave-eeprom.o
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> new file mode 100644
> index 000000000000..2b61c10a8ff6
> --- /dev/null
> +++ b/drivers/i2c/i2c-atr.c
> @@ -0,0 +1,557 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * drivers/i2c/i2c-atr.c -- I2C Address Translator
> + *
> + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
> + *
> + * An I2C Address Translator (ATR) is a device with an I2C slave parent
> + * ("upstream") port and N I2C master child ("downstream") ports, and
> + * forwards transactions from upstream to the appropriate downstream port
> + * with a modified slave address. The address used on the parent bus is
> + * called the "alias" and is (potentially) different from the physical
> + * slave address of the child bus. Address translation is done by the
> + * hardware.
> + *
> + * An ATR looks similar to an i2c-mux except:
> + * - the address on the parent and child busses can be different
> + * - there is normally no need to select the child port; the alias used on
> + *   the parent bus implies it
> + *
> + * The ATR functionality can be provided by a chip with many other
> + * features. This file provides a helper to implement an ATR within your
> + * driver.
> + *
> + * The ATR creates a new I2C "child" adapter on each child bus. Adding
> + * devices on the child bus ends up in invoking the driver code to select
> + * an available alias. Maintaining an appropriate pool of available aliases
> + * and picking one for each new device is up to the driver implementer. The
> + * ATR maintains an table of currently assigned alias and uses it
> to modify
> + * 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:
> + *
> + *   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
> + *

If I got you right this implements a three level translation, partly
performed by the ATR and partly performed by the deserializer chip.
The i2c alias helps the DESR to select the RX port (channel) where to
emit a transaction with the alias translated (with the deserializer's
own translation mechanism) to the physical address of the slave.

Quoting your here above example:

CPU --> 0x20 --> DESR Port0 --> 0x10 --> SLAVE
CPU --> 0x30 --> DESR Port1 --> 0x10 --> SLAVE

Did I get you right?

I'm following the DT bindings discussion with Rob and I'm glad your
last reply looks very similar to what we have in our proposed GMSL
bindings, so I hope the two could somehow converge.

Thanks
   j

> + * 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
> + *
> + * Originally based on i2c-mux.c
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/i2c-atr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +/**
> + * struct i2c_atr_cli2alias_pair - Hold the alias assigned to a client.
> + * @node:   List node
> + * @client: Pointer to the client on the child bus
> + * @alias:  I2C alias address assigned by the driver.
> + *          This is the address that will be used to issue I2C transactions
> + *          on the parent (physical) bus.
> + */
> +struct i2c_atr_cli2alias_pair {
> +	struct list_head node;
> +	const struct i2c_client *client;
> +	u16 alias;
> +};
> +
> +/*
> + * Data for each channel (child bus)
> + */
> +struct i2c_atr_chan {
> +	struct i2c_adapter adap;
> +	struct i2c_atr *atr;
> +	u32 chan_id;
> +
> +	struct list_head alias_list;
> +
> +	u16 *orig_addrs;
> +	unsigned int orig_addrs_size;
> +	struct mutex orig_addrs_lock; /* Lock orig_addrs during xfer */
> +};
> +
> +static struct i2c_atr_cli2alias_pair *
> +i2c_atr_find_mapping_by_client(struct list_head *list,
> +			       struct i2c_client *client)
> +{
> +	struct i2c_atr_cli2alias_pair *c2a;
> +
> +	list_for_each_entry(c2a, list, node) {
> +		if (c2a->client == client)
> +			return c2a;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct i2c_atr_cli2alias_pair *
> +i2c_atr_find_mapping_by_addr(struct list_head *list,
> +			     u16 phys_addr)
> +{
> +	struct i2c_atr_cli2alias_pair *c2a;
> +
> +	list_for_each_entry(c2a, list, node) {
> +		if (c2a->client->addr == phys_addr)
> +			return c2a;
> +	}
> +
> +	return NULL;
> +}
> +
> +/*
> + * 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)) {
> +		void *new_buf = kmalloc(num * sizeof(chan->orig_addrs[0]),
> +					GFP_KERNEL);
> +		if (new_buf == NULL)
> +			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) {
> +			msgs[i].addr = c2a->alias;
> +		} else {
> +			dev_err(atr->dev, "client 0x%02x not mapped!\n",
> +				msgs[i].addr);
> +			return -ENXIO;
> +		}
> +	}
> +
> +	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];
> +}
> +
> +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 = 0;
> +
> +	/* Switch to the right atr port */
> +	if (atr->ops->select) {
> +		ret = atr->ops->select(atr, chan->chan_id);
> +		if (ret < 0)
> +			goto out;
> +	}
> +
> +	/* Translate addresses */
> +	mutex_lock(&chan->orig_addrs_lock);
> +	ret = i2c_atr_map_msgs(chan, msgs, num);
> +	if (ret < 0) {
> +		mutex_unlock(&chan->orig_addrs_lock);
> +		goto out;
> +	}
> +
> +	/* Perform the transfer */
> +	ret = i2c_transfer(parent, msgs, num);
> +
> +	/* Restore addresses */
> +	i2c_atr_unmap_msgs(chan, msgs, num);
> +	mutex_unlock(&chan->orig_addrs_lock);
> +
> +out:
> +	if (atr->ops->deselect)
> +		atr->ops->deselect(atr, chan->chan_id);
> +
> +	return ret;
> +}
> +
> +static int i2c_atr_smbus_xfer(struct i2c_adapter *adap,
> +			      u16 addr, unsigned short flags,
> +			      char read_write, u8 command,
> +			      int size, union i2c_smbus_data *data)
> +{
> +	struct i2c_atr_chan *chan = adap->algo_data;
> +	struct i2c_atr *atr = chan->atr;
> +	struct i2c_adapter *parent = atr->parent;
> +	struct i2c_atr_cli2alias_pair *c2a;
> +	int err = 0;
> +
> +	c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list, addr);
> +	if (!c2a) {
> +		dev_err(atr->dev, "client 0x%02x not mapped!\n", addr);
> +		return -ENXIO;
> +	}
> +
> +	if (atr->ops->select)
> +		err = atr->ops->select(atr, chan->chan_id);
> +	if (!err)
> +		err = i2c_smbus_xfer(parent, c2a->alias, flags,
> +				     read_write, command, size, data);
> +	if (atr->ops->deselect)
> +		atr->ops->deselect(atr, chan->chan_id);
> +
> +	return err;
> +}
> +
> +static u32 i2c_atr_functionality(struct i2c_adapter *adap)
> +{
> +	struct i2c_atr_chan *chan = adap->algo_data;
> +	struct i2c_adapter *parent = chan->atr->parent;
> +
> +	return parent->algo->functionality(parent);
> +}
> +
> +static void i2c_atr_lock_bus(struct i2c_adapter *adapter, unsigned int flags)
> +{
> +	struct i2c_atr_chan *chan = adapter->algo_data;
> +	struct i2c_atr *atr = chan->atr;
> +
> +	mutex_lock(&atr->lock);
> +}
> +
> +static int i2c_atr_trylock_bus(struct i2c_adapter *adapter, unsigned int flags)
> +{
> +	struct i2c_atr_chan *chan = adapter->algo_data;
> +	struct i2c_atr *atr = chan->atr;
> +
> +	return mutex_trylock(&atr->lock);
> +}
> +
> +static void i2c_atr_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
> +{
> +	struct i2c_atr_chan *chan = adapter->algo_data;
> +	struct i2c_atr *atr = chan->atr;
> +
> +	mutex_unlock(&atr->lock);
> +}
> +
> +static const struct i2c_lock_operations i2c_atr_lock_ops = {
> +	.lock_bus =    i2c_atr_lock_bus,
> +	.trylock_bus = i2c_atr_trylock_bus,
> +	.unlock_bus =  i2c_atr_unlock_bus,
> +};
> +
> +static int i2c_atr_attach_client(struct i2c_adapter *adapter,
> +				 const struct i2c_board_info *info,
> +				 const struct i2c_client *client)
> +{
> +	struct i2c_atr_chan *chan = adapter->algo_data;
> +	struct i2c_atr *atr = chan->atr;
> +	struct i2c_atr_cli2alias_pair *c2a;
> +	u16 alias_id = 0;
> +	int err = 0;
> +
> +	c2a = kzalloc(sizeof(struct i2c_atr_cli2alias_pair), GFP_KERNEL);
> +	if (!c2a) {
> +		err = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	err = atr->ops->attach_client(atr, chan->chan_id, info, client,
> +				      &alias_id);
> +	if (err)
> +		goto err_attach;
> +	if (alias_id == 0) {
> +		err = -EINVAL;
> +		goto err_attach;
> +	}
> +
> +	c2a->client = client;
> +	c2a->alias = alias_id;
> +	list_add(&c2a->node, &chan->alias_list);
> +
> +	return 0;
> +
> +err_attach:
> +	kfree(c2a);
> +err_alloc:
> +	return err;
> +}
> +
> +static void i2c_atr_detach_client(struct i2c_adapter *adapter,
> +				  const struct i2c_client *client)
> +{
> +	struct i2c_atr_chan *chan = adapter->algo_data;
> +	struct i2c_atr *atr = chan->atr;
> +	struct i2c_atr_cli2alias_pair *c2a;
> +
> +	atr->ops->detach_client(atr, chan->chan_id, client);
> +
> +	c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client);
> +	if (c2a != NULL) {
> +		list_del(&c2a->node);
> +		kfree(c2a);
> +	}
> +}
> +
> +static const struct i2c_attach_operations i2c_atr_attach_ops = {
> +	.attach_client = i2c_atr_attach_client,
> +	.detach_client = i2c_atr_detach_client,
> +};
> +
> +/**
> + * i2c_atr_add_adapter - Create a child ("downstream") I2C bus.
> + * @atr:     The I2C ATR
> + * @chan_id: Index of the new adapter (0 .. max_adapters-1).  This value is
> + *           passed to the callbacks in `struct i2c_atr_ops`.
> + *
> + * After calling this function a new i2c bus will appear. Adding and
> + * removing devices on the downstream bus will result in calls to the
> + * `attach_client` and `detach_client` callbacks for the driver to assign
> + * an alias to the device.
> + *
> + * If there is a device tree node under "i2c-atr" whose "reg" property
> + * equals chan_id, the new adapter will receive that node and perhaps start
> + * adding devices under it. The callbacks for those additions will be made
> + * before i2c_atr_add_adapter() returns.
> + *
> + * Call i2c_atr_del_adapter() to remove the adapter.
> + *
> + * Return: 0 on success, a negative error code otherwise.
> + */
> +int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id)
> +{
> +	struct i2c_adapter *parent = atr->parent;
> +	struct device *dev = atr->dev;
> +	struct i2c_atr_chan *chan;
> +	char symlink_name[20];
> +	int err;
> +
> +	if (chan_id >= atr->max_adapters)
> +		return -EINVAL;
> +
> +	if (atr->adapter[chan_id]) {
> +		dev_err(dev, "Adapter %d already present\n", chan_id);
> +		return -EEXIST;
> +	}
> +
> +	chan = kzalloc(sizeof(*chan), GFP_KERNEL);
> +	if (!chan)
> +		return -ENOMEM;
> +
> +	chan->atr = atr;
> +	chan->chan_id = chan_id;
> +	INIT_LIST_HEAD(&chan->alias_list);
> +	mutex_init(&chan->orig_addrs_lock);
> +
> +	snprintf(chan->adap.name, sizeof(chan->adap.name),
> +		 "i2c-%d-atr-%d", i2c_adapter_id(parent), chan_id);
> +	chan->adap.owner = THIS_MODULE;
> +	chan->adap.algo = &atr->algo;
> +	chan->adap.algo_data = chan;
> +	chan->adap.dev.parent = dev;
> +	chan->adap.retries = parent->retries;
> +	chan->adap.timeout = parent->timeout;
> +	chan->adap.quirks = parent->quirks;
> +	chan->adap.lock_ops = &i2c_atr_lock_ops;
> +	chan->adap.attach_ops = &i2c_atr_attach_ops;
> +
> +	if (dev->of_node) {
> +		struct device_node *atr_node;
> +		struct device_node *child;
> +		u32 reg;
> +
> +		atr_node = of_get_child_by_name(dev->of_node, "i2c-atr");
> +
> +		for_each_child_of_node(atr_node, child) {
> +			err = of_property_read_u32(child, "reg", &reg);
> +			if (err)
> +				continue;
> +			if (chan_id == reg)
> +				break;
> +		}
> +
> +		chan->adap.dev.of_node = child;
> +		of_node_put(atr_node);
> +	}
> +
> +	err = i2c_add_adapter(&chan->adap);
> +	if (err) {
> +		dev_err(dev, "failed to add atr-adapter %u (error=%d)\n",
> +			chan_id, err);
> +		goto err_add_adapter;
> +	}
> +
> +	WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"),
> +	     "can't create symlink to atr device\n");
> +	snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id);
> +	WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name),
> +	     "can't create symlink for channel %u\n", chan_id);
> +
> +	dev_info(dev, "Added ATR child bus %d\n", i2c_adapter_id(&chan->adap));
> +
> +	atr->adapter[chan_id] = &chan->adap;
> +	return 0;
> +
> +err_add_adapter:
> +	mutex_destroy(&chan->orig_addrs_lock);
> +	kfree(chan);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(i2c_atr_add_adapter);
> +
> +/**
> + * i2c_atr_del_adapter - Remove a child ("downstream") I2C bus added by
> + * i2c_atr_del_adapter().
> + * @atr:     The I2C ATR
> + * @chan_id: Index of the `adapter to be removed (0 .. max_adapters-1)
> + */
> +void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id)
> +{
> +	char symlink_name[20];
> +
> +	struct i2c_adapter *adap = atr->adapter[chan_id];
> +	struct i2c_atr_chan *chan = adap->algo_data;
> +	struct device_node *np = adap->dev.of_node;
> +	struct device *dev = atr->dev;
> +
> +	if (atr->adapter[chan_id] == NULL) {
> +		dev_err(dev, "Adapter %d does not exist\n", chan_id);
> +		return;
> +	}
> +
> +	dev_info(dev, "Removing ATR child bus %d\n", i2c_adapter_id(adap));
> +
> +	atr->adapter[chan_id] = NULL;
> +
> +	snprintf(symlink_name, sizeof(symlink_name),
> +		 "channel-%u", chan->chan_id);
> +	sysfs_remove_link(&dev->kobj, symlink_name);
> +	sysfs_remove_link(&chan->adap.dev.kobj, "atr_device");
> +
> +	i2c_del_adapter(adap);
> +	of_node_put(np);
> +	mutex_destroy(&chan->orig_addrs_lock);
> +	kfree(chan);
> +}
> +EXPORT_SYMBOL_GPL(i2c_atr_del_adapter);
> +
> +/**
> + * i2c_atr_new() - Allocate and initialize an I2C ATR helper.
> + * @parent:       The parent (upstream) adapter
> + * @dev:          The device acting as an ATR
> + * @ops:          Driver-specific callbacks
> + * @max_adapters: Maximum number of child adapters
> + *
> + * The new ATR helper is connected to the parent adapter but has no child
> + * adapters. Call i2c_atr_add_adapter() to add some.
> + *
> + * Call i2c_atr_delete() to remove.
> + *
> + * Return: pointer to the new ATR helper object, or ERR_PTR
> + */
> +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> +			    const struct i2c_atr_ops *ops, int max_adapters)
> +{
> +	struct i2c_atr *atr;
> +
> +	if (!ops || !ops->attach_client || !ops->detach_client)
> +		return ERR_PTR(-EINVAL);
> +
> +	atr = devm_kzalloc(dev, sizeof(*atr)
> +			    + max_adapters * sizeof(atr->adapter[0]),
> +			    GFP_KERNEL);
> +	if (!atr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_init(&atr->lock);
> +
> +	atr->parent = parent;
> +	atr->dev = dev;
> +	atr->ops = ops;
> +	atr->max_adapters = max_adapters;
> +
> +	if (parent->algo->master_xfer)
> +		atr->algo.master_xfer = i2c_atr_master_xfer;
> +	if (parent->algo->smbus_xfer)
> +		atr->algo.smbus_xfer = i2c_atr_smbus_xfer;
> +	atr->algo.functionality = i2c_atr_functionality;
> +
> +	return atr;
> +}
> +EXPORT_SYMBOL_GPL(i2c_atr_new);
> +
> +/**
> + * 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
> + * removed by calling i2c_atr_del_adapter().
> + */
> +void i2c_atr_delete(struct i2c_atr *atr)
> +{
> +	mutex_destroy(&atr->lock);
> +}
> +EXPORT_SYMBOL_GPL(i2c_atr_delete);
> +
> +MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>");
> +MODULE_DESCRIPTION("I2C Address Translator");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
> new file mode 100644
> index 000000000000..019816e5a50c
> --- /dev/null
> +++ b/include/linux/i2c-atr.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/**
> + * drivers/i2c/i2c-atr.h -- I2C Address Translator
> + *
> + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
> + *
> + * Based on i2c-mux.h
> + */
> +
> +#ifndef _LINUX_I2C_ATR_H
> +#define _LINUX_I2C_ATR_H
> +
> +#ifdef __KERNEL__
> +
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +
> +struct i2c_atr;
> +
> +/**
> + * struct i2c_atr_ops - Callbacks from ATR to the device driver.
> + * @select:        Ask the driver to select a child bus (optional)
> + * @deselect:      Ask the driver to deselect a child bus (optional)
> + * @attach_client: Notify the driver of a new device connected on a child
> + *                 bus. The driver must choose an I2C alias, configure the
> + *                 hardware to use it and return it in `alias_id`.
> + * @detach_client: Notify the driver of a device getting disconnected. The
> + *                 driver must configure the hardware to stop using the
> + *                 alias.
> + *
> + * All these functions return 0 on success, a negative error code otherwise.
> + */
> +struct i2c_atr_ops {
> +	int  (*select)(struct i2c_atr *atr, u32 chan_id);
> +	int  (*deselect)(struct i2c_atr *atr, u32 chan_id);
> +	int  (*attach_client)(struct i2c_atr *atr, u32 chan_id,
> +			      const struct i2c_board_info *info,
> +			      const struct i2c_client *client,
> +			      u16 *alias_id);
> +	void (*detach_client)(struct i2c_atr *atr, u32 chan_id,
> +			      const struct i2c_client *client);
> +};
> +
> +/**
> + * Helper to add I2C ATR features to a device driver.
> + */
> +struct i2c_atr {
> +	/* private: internal use only */
> +
> +	struct i2c_adapter *parent;
> +	struct device *dev;
> +	const struct i2c_atr_ops *ops;
> +
> +	void *priv;
> +
> +	struct i2c_algorithm algo;
> +	struct mutex lock;
> +	int max_adapters;
> +
> +	struct i2c_adapter *adapter[0];
> +};
> +
> +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> +			    const struct i2c_atr_ops *ops, int max_adapters);
> +void i2c_atr_delete(struct i2c_atr *atr);
> +
> +static inline void i2c_atr_set_clientdata(struct i2c_atr *atr, void *data)
> +{
> +	atr->priv = data;
> +}
> +
> +static inline void *i2c_atr_get_clientdata(struct i2c_atr *atr)
> +{
> +	return atr->priv;
> +}
> +
> +int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id);
> +void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id);
> +
> +#endif /* __KERNEL__ */
> +
> +#endif /* _LINUX_I2C_ATR_H */
> --
> 2.17.1
>

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Luca Ceresoli]

Hi Jacopo,

thanks for your feedback.

On 01/09/19 16:31, 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.

Sure!

> In the meantime I would have some comments after having a read at the
> series and trying to apply its concept to GMSL
> 
> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>> 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 is
>> 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>
>>
>> ---
>>
>> Changes RFCv1 -> RFCv2:
>>
>>  RFCv1 was implemented inside i2c-mux.c and added yet more complexity
>>  there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR
>>  features are not in a MUX and vice versa, the overlapping is low. This was
>>  almost a complete rewrite, but for the records here are the main
>>  differences from the old implementation:
> 
> I'm not an i2c expert, but this looks very similar to me to an
> augmented i2c-mux with the following additional features:
> - automatic selection of the i2c address to use for the children
>   behind the mux
> - automatic translation of the addresses the logical aliases to
>   the actual physical addresses of the slaves (with the help of the
>   deserializer address translation feature in this case).

A notable difference in the hardware is that a mux needs an explicit
procedure to select a port. That's why the select() op is mandatory for
muxes. The ATR, at least in the DS90UB9xx case, selects the port
automatically based on the slave address. So I added an optional
select() op in the atr, but I suspect it's useless for "real" ATRs.

More differences derive from how Linux implements muxes. The i2c-mux
code has several flags for different locking schemes, and it has a
fairly complex DT parsing scheme. These are mostly a historical burden.
Adding the ATR features to i2c-mux.c was very tricky and error-prone due
to all of this code, that's why I have moved ATR to its own file in RFCv2.

> In the GMSL perspective we have similar needs but limited to the
> selection of the i2c addresses to assign to the children behind our
> mux. The maxims's chips work by reprogramming the remote devices and
> do not support address translations on the deserializer side, unlike
> what the TI chips do.

Indeed, the Maxim chips implement a simple mux, so it seems like the
only commonality with TI is the need for address pool management.

> So I wonder if we could somehow split the here proposed solution in
> two, one part to perform the address selection, the other one to
> support address reprogramming.
> 
> One thing I have noticed is that it's up to the driver using the ATR
> (the DS90UB954 deserializer in this case) picking into the alias pool
> and assign aliases. I would have expected this to be a feature of the
> ATR itself, if you aim to have the i2c-alias-pool as a standard
> construct.
> 
> I understand the pool of free aliases 'belongs' to the base board
> device, but the ATR could be provided with a pointer to it and the
> routines to select and assign addresses generalized. Is there a reason
> why you decided otherwise that I'm not seeing?

I was about to do it, but changed my mind for a non-strongly-technical
reason. The entire body of ds90_atr_attach_client() and
ds90_atr_detach_client() is guarded by a mutex to protect the alias pool
from concurrent access. I didn't feel comfortable in hoisting the mutex
into the ATR code and have all the attach and detach callbacks run with
a lock that might be unneeded in specific drivers. At least not with
only one driver using i2c-atr.

Also, I'm not even sure a mutex is really needed to protect the pool. Is
it possible that two client adding/removal operations happen
concurrently? SHould they be serialized at the i2c core level, the
mutexes would be unneeded. But I haven't dug in the core enough to
discover it. Wolfram?

However, even though the alias picking code is not that much, I'm OK in
moving it into the ATR core if it looks good.

Now, I see you thinking "why not moving the ATR code _and_ the alias
picking code in i2c-mux and benefit in the GMSL drivers?", and I'm a
little uncomfortable about this idea. Not because the idea is bad per
se, but because the intricacies of i2c-mux make this quite dreadful.

I have had some discussion with Peter Rosin about simplifying i2c-atr,
but they didn't show a clear path forward and were slowly abandoned.

>>  - change bus description
>>  - remove I2C_ATR_ARBITRATOR and I2C_ATR_GATE support
>>  - select() optional
>>  - rename i2c_atr_alloc -> i2c_atr_new, add i2c_atr_delete, move to bottom
>>  - lock the ATR, not the adapter or the muxes on the adapter
>>  - remove parent-locked code
>>  - remove I2C_MUX_LOCKED flag, now unused
>>  - remove I2C_ATR_ATR flag (always true)
>>  - translation in i2c_atr_smbus_xfer too
>>  - i2c_atr_map_msgs: don't ignore mapping errors
>>  - always require the "i2c-atr" DT node, no magic
>>  - remove ACPI support
>>  - one algo in the atrc, not one per adapter
>>  - remove unneeded i2c_atr_root_adapter
>>  - ditch force_nr
>>  - don't allocate private user memory, just provide a plain userdata pointer
>>  - consolidate all ops in a single struct, simplify naming
>>  - remove adapters individually, allocate in atrc->adapter[chan_id]
>> ---
>>  drivers/i2c/Kconfig     |   9 +
>>  drivers/i2c/Makefile    |   1 +
>>  drivers/i2c/i2c-atr.c   | 557 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/i2c-atr.h |  82 ++++++
>>  4 files changed, 649 insertions(+)
>>  create mode 100644 drivers/i2c/i2c-atr.c
>>  create mode 100644 include/linux/i2c-atr.h
>>
>> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
>> index abedd55a1264..5df088b1d9de 100644
>> --- a/drivers/i2c/Kconfig
>> +++ b/drivers/i2c/Kconfig
>> @@ -71,6 +71,15 @@ config I2C_MUX
>>
>>  source "drivers/i2c/muxes/Kconfig"
>>
>> +config I2C_ATR
>> +	tristate "I2C Address Translator (ATR) support"
>> +	help
>> +	  Enable support for I2C Address Translator (ATR) chips.
>> +
>> +	  An ATR allows accessing multiple I2C busses from a single
>> +	  physical bus via address translation instead of bus selection as
>> +	  i2c-muxes do.
>> +
>>  config I2C_HELPER_AUTO
>>  	bool "Autoselect pertinent helper modules"
>>  	default y
>> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
>> index bed6ba63c983..81849ea393c7 100644
>> --- a/drivers/i2c/Makefile
>> +++ b/drivers/i2c/Makefile
>> @@ -13,6 +13,7 @@ i2c-core-$(CONFIG_OF) 		+= i2c-core-of.o
>>  obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
>>  obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
>>  obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
>> +obj-$(CONFIG_I2C_ATR)		+= i2c-atr.o
>>  obj-y				+= algos/ busses/ muxes/
>>  obj-$(CONFIG_I2C_STUB)		+= i2c-stub.o
>>  obj-$(CONFIG_I2C_SLAVE_EEPROM)	+= i2c-slave-eeprom.o
>> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
>> new file mode 100644
>> index 000000000000..2b61c10a8ff6
>> --- /dev/null
>> +++ b/drivers/i2c/i2c-atr.c
>> @@ -0,0 +1,557 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
>> + * drivers/i2c/i2c-atr.c -- I2C Address Translator
>> + *
>> + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
>> + *
>> + * An I2C Address Translator (ATR) is a device with an I2C slave parent
>> + * ("upstream") port and N I2C master child ("downstream") ports, and
>> + * forwards transactions from upstream to the appropriate downstream port
>> + * with a modified slave address. The address used on the parent bus is
>> + * called the "alias" and is (potentially) different from the physical
>> + * slave address of the child bus. Address translation is done by the
>> + * hardware.
>> + *
>> + * An ATR looks similar to an i2c-mux except:
>> + * - the address on the parent and child busses can be different
>> + * - there is normally no need to select the child port; the alias used on
>> + *   the parent bus implies it
>> + *
>> + * The ATR functionality can be provided by a chip with many other
>> + * features. This file provides a helper to implement an ATR within your
>> + * driver.
>> + *
>> + * The ATR creates a new I2C "child" adapter on each child bus. Adding
>> + * devices on the child bus ends up in invoking the driver code to select
>> + * an available alias. Maintaining an appropriate pool of available aliases
>> + * and picking one for each new device is up to the driver implementer. The
>> + * ATR maintains an table of currently assigned alias and uses it
>> to modify
>> + * 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:
>> + *
>> + *   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
>> + *
> 
> If I got you right this implements a three level translation, partly
> performed by the ATR and partly performed by the deserializer chip.
> The i2c alias helps the DESR to select the RX port (channel) where to
> emit a transaction with the alias translated (with the deserializer's
> own translation mechanism) to the physical address of the slave.
> 
> Quoting your here above example:
> 
> CPU --> 0x20 --> DESR Port0 --> 0x10 --> SLAVE
> CPU --> 0x30 --> DESR Port1 --> 0x10 --> SLAVE
> 
> Did I get you right?

I would not call it a three level translation, but yes, it is how it works.

> I'm following the DT bindings discussion with Rob and I'm glad your
> last reply looks very similar to what we have in our proposed GMSL
> bindings, so I hope the two could somehow converge.

Good. After all Maxim and TI chips have the same topology, so it seems
natural that their DT description is similar.

-- 
Luca

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]


> Adding the ATR features to i2c-mux.c was very tricky and error-prone due
> to all of this code, that's why I have moved ATR to its own file in RFCv2.

I forgot to say that I like this.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Peter Rosin]

Hi!

[ Sorry about my absence. I've been meaning to comment on this series
  for a long time, but work and family keep interfering... ]

On 2019-09-03 09:31, Luca Ceresoli wrote:
> Hi Jacopo,
> 
> thanks for your feedback.
> 
> On 01/09/19 16:31, 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.
> 
> Sure!
> 
>> In the meantime I would have some comments after having a read at the
>> series and trying to apply its concept to GMSL
>>
>> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>>> 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 is
>>> 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>
>>>
>>> ---
>>>
>>> Changes RFCv1 -> RFCv2:
>>>
>>>  RFCv1 was implemented inside i2c-mux.c and added yet more complexity
>>>  there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR
>>>  features are not in a MUX and vice versa, the overlapping is low. This was
>>>  almost a complete rewrite, but for the records here are the main
>>>  differences from the old implementation:
>>
>> I'm not an i2c expert, but this looks very similar to me to an
>> augmented i2c-mux with the following additional features:
>> - automatic selection of the i2c address to use for the children
>>   behind the mux
>> - automatic translation of the addresses the logical aliases to
>>   the actual physical addresses of the slaves (with the help of the
>>   deserializer address translation feature in this case).
> 
> A notable difference in the hardware is that a mux needs an explicit
> procedure to select a port. That's why the select() op is mandatory for
> muxes. The ATR, at least in the DS90UB9xx case, selects the port
> automatically based on the slave address. So I added an optional
> select() op in the atr, but I suspect it's useless for "real" ATRs.
> 
> More differences derive from how Linux implements muxes. The i2c-mux
> code has several flags for different locking schemes, and it has a

It's two locking schemes if you count them carefully, so several is
a bit of an exaggeration. But agreed, two is more than I prefer.

> fairly complex DT parsing scheme. These are mostly a historical burden.
> Adding the ATR features to i2c-mux.c was very tricky and error-prone due
> to all of this code, that's why I have moved ATR to its own file in RFCv2.

Anyway, the locking in i2c-mux may be complex, but it does solve real
problems. The way I read this series, these problems are not dealt with
and therefore the ATR code will not function in all surroundings.

Some things to think about:
- What happens when you put a mux behind an ATR?
- What happens when you put an ATR behind a mux?
- What happens when you put an ATR between two muxes?
- Does it make a difference if the mux is parent-locked or mux-locked?
- What happens if client drivers lock the adapter in order to silence the
  bus for some reason or to keep two xfers together or something, and
  then do unlocked xfers?
- Can you put an ATR behind another ATR?
etc

I'm not saying that these things must be handled, and maybe they are
handled already, and maybe some of the combinations are not valid at
all. But the possibilities and limitations should be understood. And
preferably documented.

My gut feeling (without spending much time on it) is that ATR as
implemented in this series behave approximately like mux-locked muxes,
meaning that it is not obviously safe to put a parent-locked mux behind
an ATR and making it impossible for client devices behind an ATR to force
xfers to happen back-to-back from the root adapter.

And finally, I feel that it is not wise to introduce a third locking
scheme in the I2C topology. It's bad enough with two. Why not make the
ATR locking exactly match the mux-locked scheme to reduce the number
of cases one needs to consider? But maybe that's just me?

Cheers,
Peter

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Luca Ceresoli]

Hi Peter,

On 04/09/19 10:09, Peter Rosin wrote:> Hi!
>
> [ Sorry about my absence. I've been meaning to comment on this series
>   for a long time, but work and family keep interfering... ]

No problem, thanks for your comments. See below my reply.

>
> On 2019-09-03 09:31, Luca Ceresoli wrote:
>> Hi Jacopo,
>>
>> thanks for your feedback.
>>
>> On 01/09/19 16:31, 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.
>>
>> Sure!
>>
>>> In the meantime I would have some comments after having a read at the
>>> series and trying to apply its concept to GMSL
>>>
>>> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>>>> 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 is
>>>> 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>
>>>>
>>>> ---
>>>>
>>>> Changes RFCv1 -> RFCv2:
>>>>
>>>>  RFCv1 was implemented inside i2c-mux.c and added yet more complexity
>>>>  there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR
>>>>  features are not in a MUX and vice versa, the overlapping is low.
This was
>>>>  almost a complete rewrite, but for the records here are the main
>>>>  differences from the old implementation:
>>>
>>> I'm not an i2c expert, but this looks very similar to me to an
>>> augmented i2c-mux with the following additional features:
>>> - automatic selection of the i2c address to use for the children
>>>   behind the mux
>>> - automatic translation of the addresses the logical aliases to
>>>   the actual physical addresses of the slaves (with the help of the
>>>   deserializer address translation feature in this case).
>>
>> A notable difference in the hardware is that a mux needs an explicit
>> procedure to select a port. That's why the select() op is mandatory for
>> muxes. The ATR, at least in the DS90UB9xx case, selects the port
>> automatically based on the slave address. So I added an optional
>> select() op in the atr, but I suspect it's useless for "real" ATRs.
>>
>> More differences derive from how Linux implements muxes. The i2c-mux
>> code has several flags for different locking schemes, and it has a
>
> It's two locking schemes if you count them carefully, so several is
> a bit of an exaggeration. But agreed, two is more than I prefer.

Right, sorry for the exaggeration. Actually the most annoying part of
the mux code to me was not quite the two locking schemes, but rather the
DT parsing, which had some backward-compatibility to maintain.

>> fairly complex DT parsing scheme. These are mostly a historical burden.
>> Adding the ATR features to i2c-mux.c was very tricky and error-prone due
>> to all of this code, that's why I have moved ATR to its own file in
RFCv2.
>
> Anyway, the locking in i2c-mux may be complex, but it does solve real
> problems. The way I read this series, these problems are not dealt with
> and therefore the ATR code will not function in all surroundings.
>
> Some things to think about:
> - What happens when you put a mux behind an ATR?
> - What happens when you put an ATR behind a mux?
> - What happens when you put an ATR between two muxes?
> - Does it make a difference if the mux is parent-locked or mux-locked?
> - What happens if client drivers lock the adapter in order to silence the
>   bus for some reason or to keep two xfers together or something, and
>   then do unlocked xfers?
> - Can you put an ATR behind another ATR?
> etc

I still have to examine in depth all of the problems in the i2c-mux
documented in Documentation/i2c/i2c-topology (thanks for having written
those docs!), but at first sight it looks like the ATR is not going to
introduce big problems because of how it works.

An I2C mux works by electrically connecting the parent bus to one of the
child busses. But this implies any transactions happening on the parent
bus can leak through the mux and cause the problems you documented. The
Maxim GMSL serdes chips work de facto in the same way, even though there
is no simple electrical connection between the busses.

An ATR, at least the one in the TI serdes chips, uses a store-and-forward
technique:

 1. when a transaction starts on the parent bus, it waits for the first
    8 bits
 2. it checks if the slave address matches one of the aliases assigned
    to the remote chips; if it doesn't, it does not drive the child bus
    wires at all and ignores the rest of the transaction
 3. if there's a match it stretches the parent bus SCL and issues a
    {start + chip address + R/W} on the child bus, but replacing the
    alias with the physical chip address
 4. it then waits for an the ack on the child bus, then issues an ack on
    the parent bus
 5. for write transactions, it repeats until a stop condition or an
    error is seen: wait for an entire byte on parent bus, clock
    stretching on the parent bus, send the byte on the child bus, wait
    for ack on the child bus, send ack on the parent bus
 6. for read transactions it does the same as point 5 but with "parent
    bus" and "child bus" swapped

This technique does not leak any transaction to a child bus unless it's
directed to it. So it should have many less problems than the muxes, if any.

> I'm not saying that these things must be handled, and maybe they are
> handled already, and maybe some of the combinations are not valid at
> all. But the possibilities and limitations should be understood. And
> preferably documented.
>
> My gut feeling (without spending much time on it) is that ATR as
> implemented in this series behave approximately like mux-locked muxes,
> meaning that it is not obviously safe to put a parent-locked mux behind
> an ATR and making it impossible for client devices behind an ATR to force
> xfers to happen back-to-back from the root adapter.

For what I described above I don't expect the ATR + parent-locked mux
would have the same problems as the mux-locked mux + parent-locked mux.
The ATR will not let transaction on the root adapter leak through and
reach the mux, unless they are targeted at the child bus where the mux is.

Do you think this is correct? As I said, I still have to analyse all the
possible combinations.

-- 
Luca

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]


> I still have to examine in depth all of the problems in the i2c-mux
> documented in Documentation/i2c/i2c-topology (thanks for having written
> those docs!), but at first sight it looks like the ATR is not going to
> introduce big problems because of how it works.

Assuming we are using the previously discussed NEEDS_ATR flag for the adapter
instead of the attach/detach callbacks:

Can't we then simply understand an ATR as a generic 1:1 mapping device
which can be setup when registering an adapter?

When we add an adapter using i2c_add_adapter, we have:


              .-----.  Slave X @ 0x10
  .-----.     |     |   |
  | CPU |--A--| ATR |---+---- B
  `-----'     |     |
              `-----'

When we use i2c_add_mux_adapter, we have:


                                Slave X @ 0x10
              .-----.   .-----.   |
  .-----.     |     |---| ATR |---+---- B
  | CPU |--A--| MUX |   '-----'
  `-----'     |     |   .-----.
              |     |---| ATR |---+---- C
              `-----'   '-----'   |
                                 Slave Y @ 0x10


That way we could keep the topology handling solely to the mux-core.

Am I overlooking something?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Vladimir Zapolskiy]

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.

The undeniable fact is that the device tree bindings in my I2C bridge
implementation can be improved further, thanks to Luca for the comments.

> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>> 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 is
>> 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>
>>

--
Best wishes,
Vladimir

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Vladimir Zapolskiy]

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.

> The undeniable fact is that the device tree bindings in my I2C bridge
> implementation can be improved further, thanks to Luca for the comments.
> 
>> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>>> 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 is
>>> 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>
>>>
> 

--
Best wishes,
Vladimir

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Luca Ceresoli]

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

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

Hi Vladimir,

> I won't attend the LPC, however I would appreciate if you book some

A pity. I would have liked to have you in the room. Let's see if we can
get enough input from you via mail here.

> time to review my original / alternative implementation of the TI
> DS90Ux9xx I2C bridge device driver.

We have only 45 minutes, this will not allow to review specific
implementations. I want to give an overview of existing implementations
with pros/cons...

> 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.

... and I'd appreciate support here from you, like your summary of the
back then discussion (from where I can dive deeper if needed).

Also, with Luca's new series we discussed some scenarios which can
happen WRT to I2C address conflicts. Maybe you could comment on them,
too? As I read the old thread, you have a hardcoded aliases using
"ti,i2c-bridge-maps". This means you can't have own DTSI files for e.g.
add-on modules, do I get this correctly?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Vladimir Zapolskiy]

Hi Wolfram,

On 09/09/2019 10:22 AM, Wolfram Sang wrote:
> Hi Vladimir,
> 
>> I won't attend the LPC, however I would appreciate if you book some
> 
> A pity. I would have liked to have you in the room. Let's see if we can
> get enough input from you via mail here.
> 

if it might help, I'll attend the Embedded Recipes and ELCE conferences
this year.

>> time to review my original / alternative implementation of the TI
>> DS90Ux9xx I2C bridge device driver.
> 
> We have only 45 minutes, this will not allow to review specific
> implementations. I want to give an overview of existing implementations
> with pros/cons...
> 

Sure! Any shared summary/opinions are greatly welcome.

>> 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.
> 
> ... and I'd appreciate support here from you, like your summary of the
> back then discussion (from where I can dive deeper if needed).
> 
> Also, with Luca's new series we discussed some scenarios which can
> happen WRT to I2C address conflicts. Maybe you could comment on them,
> too?

I do remember I've commented on the Luca's suggestion of using dynamic
I2C addresses from a pool (the introduced "i2c-alias-pool" property).

I have to scrutinize it in Luca's v2, but then it might happen that the
userspace won't know to which IC on the remote side it communicates to.

> As I read the old thread, you have a hardcoded aliases using
> "ti,i2c-bridge-maps". This means you can't have own DTSI files for e.g.
> add-on modules, do I get this correctly?
> 

Basically hardcoding of aliases completely resolves the highlighted
above problem. Still it is possible to have own DTSI files for the FPD
link detachable PCBs, and this is an exceptionally important scenario,
however some limitations shall be applied:
* dt overlays for "local" derializer/deserializer ICs, it's a generic
  and universal solution, it is successfully used in the field,
* only "compatible" PCBs are supposed to be connected (same set of I2C
  devices/addresses on every such PCB), this is imperfect for sure,
* "ti,i2c-bridge-maps" list is excessive to cover "almost compatible"
  (in the sense from above) PCBs, some of the missed alias matches
  just won't instantiate a driver, this is of course also imperfect.

In general nothing critical should happen, if some I2C device on the
remote side is simply not probed in runtime, in opposite you can imagine
that writing for instance to another EEPROM of two on the remote side
could be harmful.

Any technically better solution to the two given approaches (from Luca
and from me) is more than appreciated. For non-dynamic/fixed local and
remote PCBs the fixed mapping is better, the dynamic case is covered
by the dt overlays, why not?

As a side note please do remember that the I2C bridging on Maxim GMSL
and TI DS90Ux9xx are different, the former one is a real mux, and the
latter one is not, I'm uncertain if it's reasonable to think of a
generalized solution which covers both IC series, so likely we
have to review the developed solutions for them separately instead
of trying to work out a combined one.

--
Best wishes,
Vladimir

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Luca Ceresoli]

Hi Vladimir,

On 09/09/19 16:10, Vladimir Zapolskiy wrote:
> Hi Wolfram,
> 
> On 09/09/2019 10:22 AM, Wolfram Sang wrote:
>> Hi Vladimir,
>>
>>> I won't attend the LPC, however I would appreciate if you book some
>>
>> A pity. I would have liked to have you in the room. Let's see if we can
>> get enough input from you via mail here.
>>
> 
> if it might help, I'll attend the Embedded Recipes and ELCE conferences
> this year.
> 
>>> time to review my original / alternative implementation of the TI
>>> DS90Ux9xx I2C bridge device driver.
>>
>> We have only 45 minutes, this will not allow to review specific
>> implementations. I want to give an overview of existing implementations
>> with pros/cons...
>>
> 
> Sure! Any shared summary/opinions are greatly welcome.
> 
>>> 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.
>>
>> ... and I'd appreciate support here from you, like your summary of the
>> back then discussion (from where I can dive deeper if needed).
>>
>> Also, with Luca's new series we discussed some scenarios which can
>> happen WRT to I2C address conflicts. Maybe you could comment on them,
>> too?
> 
> I do remember I've commented on the Luca's suggestion of using dynamic
> I2C addresses from a pool (the introduced "i2c-alias-pool" property).
> 
> I have to scrutinize it in Luca's v2, but then it might happen that the
> userspace won't know to which IC on the remote side it communicates to.

I think you suspect this because the assigned alias is
non-deterministic, so you might end up in writing to the wrong alias.
Well, this is not possible because the user is not supposed to know the
alias at all. Let's say you have two identical eeproms on remote port 0,
with physical addresses 0x50 and 0x51. Then, with my RFCv2 code, you'll
access them with physical numbers, not the alias:

# i2cdetect -l
i2c-0	i2c   	amba:camera-i2c@0      I2C adapter
i2c-4	i2c   	i2c-0-atr-0            I2C adapter
i2c-5	i2c   	i2c-0-atr-1            I2C adapter
# hexdump /sys/bus/i2c/devices/4-0050/eeprom
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0000100
# hexdump /sys/bus/i2c/devices/4-0051/eeprom
0000000 0001 0203 0405 0607 0809 0a0b 0c0d 0e0f
*
0000100
#

Here i2c-0 is the "local" bus, i2c-4 and i2c-5 are the remote busses on
ports 0 and 1. As you can see the eeproms are accessed using a name like
"4-0050", meaning physical slave address 0x50 on bus 4. No alias is needed.

Should you want to know the alias, perhaps for debugging (it's the
address you'll see on your logic analyzer), they are shown in the kernel
log.

Does this reply to your concern?

>> As I read the old thread, you have a hardcoded aliases using
>> "ti,i2c-bridge-maps". This means you can't have own DTSI files for e.g.
>> add-on modules, do I get this correctly?
>>
> 
> Basically hardcoding of aliases completely resolves the highlighted
> above problem. Still it is possible to have own DTSI files for the FPD
> link detachable PCBs, and this is an exceptionally important scenario,
> however some limitations shall be applied:
> * dt overlays for "local" derializer/deserializer ICs, it's a generic
>   and universal solution, it is successfully used in the field,
> * only "compatible" PCBs are supposed to be connected (same set of I2C
>   devices/addresses on every such PCB), this is imperfect for sure,
> * "ti,i2c-bridge-maps" list is excessive to cover "almost compatible"
>   (in the sense from above) PCBs, some of the missed alias matches
>   just won't instantiate a driver, this is of course also imperfect.
> 
> In general nothing critical should happen, if some I2C device on the
> remote side is simply not probed in runtime, in opposite you can imagine
> that writing for instance to another EEPROM of two on the remote side
> could be harmful.
> 
> Any technically better solution to the two given approaches (from Luca
> and from me) is more than appreciated. For non-dynamic/fixed local and
> remote PCBs the fixed mapping is better, the dynamic case is covered
> by the dt overlays, why not?
> 
> As a side note please do remember that the I2C bridging on Maxim GMSL
> and TI DS90Ux9xx are different, the former one is a real mux, and the
> latter one is not, I'm uncertain if it's reasonable to think of a
> generalized solution which covers both IC series, so likely we
> have to review the developed solutions for them separately instead
> of trying to work out a combined one.
> 
> --
> Best wishes,
> Vladimir
> 


-- 
Luca

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]


> Here i2c-0 is the "local" bus, i2c-4 and i2c-5 are the remote busses on
> ports 0 and 1. As you can see the eeproms are accessed using a name like
> "4-0050", meaning physical slave address 0x50 on bus 4. No alias is needed.
> 
> Should you want to know the alias, perhaps for debugging (it's the
> address you'll see on your logic analyzer), they are shown in the kernel
> log.

And to add to that: The aliases on i2c-0 will be marked busy and show up
as used if you run i2cdetect. So, you'd need a force-flag if you'd want
to access them from userspace.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

Hi Luca,

> + * Topology:
> + *
> + *                       Slave X @ 0x10
> + *               .-----.   |
> + *   .-----.     |     |---+---- B
> + *   | CPU |--A--| ATR |
> + *   `-----'     |     |---+---- C
> + *               `-----'   |
> + *                       Slave Y @ 0x10
> + *
> + * Alias table:
> + *
> + *   Client  Alias
> + *   -------------
> + *      X    0x20
> + *      Y    0x30

Great that you already provided docs for this driver!

One huge drawback for me is the attach/detach callbacks. One year ago, I
removed a similar callback from the I2C core ("[PATCH 0/2] i2c: remove
deprecated attach_adapter callback") because some drivers did a lot of
crazy things there. It took years to remove all that.

What I could imagine here: the adapter (B and C each in the picture
above) gets a flag like NEEDS_ATR before registering to the core. The
flag means all clients on that bus will have their address translated.
The core will figure out a free alias when a device is registered. We
can then have an ATR specific callback with the original and translated
address as arguments, so one can setup the HW as needed.

Do you think that would work? Jacopo, does that meet GMSL as well?

Regards,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Luca Ceresoli]

Hi Wolfram,

On 02/09/19 22:42, Wolfram Sang wrote:
> Hi Luca,
> 
>> + * Topology:
>> + *
>> + *                       Slave X @ 0x10
>> + *               .-----.   |
>> + *   .-----.     |     |---+---- B
>> + *   | CPU |--A--| ATR |
>> + *   `-----'     |     |---+---- C
>> + *               `-----'   |
>> + *                       Slave Y @ 0x10
>> + *
>> + * Alias table:
>> + *
>> + *   Client  Alias
>> + *   -------------
>> + *      X    0x20
>> + *      Y    0x30
> 
> Great that you already provided docs for this driver!
> 
> One huge drawback for me is the attach/detach callbacks. One year ago, I
> removed a similar callback from the I2C core ("[PATCH 0/2] i2c: remove
> deprecated attach_adapter callback") because some drivers did a lot of
> crazy things there. It took years to remove all that.

Oh dear, I was completely unaware, apologies! :-)

> What I could imagine here: the adapter (B and C each in the picture
> above) gets a flag like NEEDS_ATR before registering to the core. The
> flag means all clients on that bus will have their address translated.
> The core will figure out a free alias when a device is registered. We
> can then have an ATR specific callback with the original and translated
> address as arguments, so one can setup the HW as needed.

Do you mean moving the alias selection code from i2c-atr.c to the i2c
core? And the rest of the ATR core too?

> Do you think that would work?

Yes.

-- 
Luca

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]


> > One huge drawback for me is the attach/detach callbacks. One year ago, I
> > removed a similar callback from the I2C core ("[PATCH 0/2] i2c: remove
> > deprecated attach_adapter callback") because some drivers did a lot of
> > crazy things there. It took years to remove all that.
> 
> Oh dear, I was completely unaware, apologies! :-)

Oh, no need to apologize. You don't have to research the whole I2C history
before implementing something. Keeping the big picture is what I happily
provide.

> > What I could imagine here: the adapter (B and C each in the picture
> > above) gets a flag like NEEDS_ATR before registering to the core. The
> > flag means all clients on that bus will have their address translated.
> > The core will figure out a free alias when a device is registered. We
> > can then have an ATR specific callback with the original and translated
> > address as arguments, so one can setup the HW as needed.
> 
> Do you mean moving the alias selection code from i2c-atr.c to the i2c
> core? And the rest of the ATR core too?

I hope for something like this in the I2C core (simplified, naming needs
to be improved etc.) in i2c_new_device:

	if (client->adapter->flag & NEEDS_ATR) {
		i2c_atr_get_alias_address();
		/* probably a wrapper around a callback */
		i2c_atr_setup_hw();
	}

with all the i2c_atr_* functions in a seperate file. It would be great
if that file could be a completely independent module, but if it turns
out that we need some simple helpers in the core, I am probably OK with
that, too.

> > Do you think that would work?
> 
> Yes.

Cool!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Luca Ceresoli]

Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
with I2C Address Translator and remote GPIOs.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes RFCv1 -> RFCv2:

 - add explicit aliases for the FPD-link RX ports (optional)
 - add proper remote GPIO description
---
 .../bindings/media/i2c/ti,ds90ub954-q1.txt    | 194 ++++++++++++++++++
 1 file changed, 194 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
new file mode 100644
index 000000000000..73ce21ecc3b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
@@ -0,0 +1,194 @@
+Texas Instruments DS90UB954-Q1 dual video deserializer
+======================================================
+
+The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video
+streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It
+also allows access to remote I2C and GPIO.
+
+Required properties:
+
+ - compatible: must be "ti,ds90ub954-q1"
+
+ - reg: main I2C slave address; optionally aliases for RX port registers
+   and remote serializers. The main address is mandatory and must be the
+   first, others are optional and fall back to defaults if not
+   specified. See "reg-names".
+
+ - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip)
+ - i2c-alias-pool: list of I2C addresses that are known to be available on the
+                   "local" (SoC-to-deser) I2C bus; they will be picked at
+		   runtime and used as aliases to reach remove I2C chips
+ - gpio-controller
+ - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags
+
+Optional properties:
+
+ - reg-names: names of I2C address used to communicate with the chip, must
+              match the "reg" values; mandatory if there are 2 or more
+              addresses
+    - "main": the main I2C address, used to access shared registers
+    - "rxport0", "rxport1": I2C alias to access FPD-link RX port specific
+      registers; must not be used by other slaves on the same bus
+    - "ser0", "ser1": I2C alias to access the remote serializer connected
+      on each FPD-link RX port; must not be used by other slaves on the
+      same bus
+ - interrupts: interrupt pin from the chip
+
+Required subnodes:
+
+ - ports: A ports node with one port child node per device input and output
+          port, in accordance with the video interface bindings defined in
+          Documentation/devicetree/bindings/media/video-interfaces.txt. The
+          port nodes are numbered as follows:
+
+          Port Description
+          ------------------------------------
+          0    Input from FPD-Link 3 RX port 0
+          1    Input from FPD-Link 3 RX port 1
+          2    CSI-2 output
+
+          Each port must have a "remote-chip" subnode that defines the remote
+	  chip (serializer) with at least a "compatible" property
+
+ - i2c-atr: contains one child per RX port, each describes the I2C bus on
+            the remote side
+
+	    Required properties:
+	    - #address-cells = <1>;
+	    - #size-cells = <0>;
+
+	    Subnodes: one per each FPD-link RX port, each having:
+
+	    Required properties for "i2c-atr" child bus nodes:
+	    - reg: The number of the port where the remove chip is connected
+	    - #address-cells = <1>;
+	    - #size-cells = <0>;
+
+	    Optional properties for "i2c-atr" child bus nodes:
+	    - Other properties specific to the remote hardware
+	    - Child nodes conforming to i2c bus binding
+
+
+Device node example
+-------------------
+
+&i2c0 {
+	deser: deser@3d {
+		compatible = "ti,ds90ub954-q1";
+		reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
+		reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
+		clocks = <&clk_25M>;
+		interrupt-parent = <&gic>;
+		interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
+		reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
+
+		i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
+
+		gpio-controller;
+		#gpio-cells = <3>; /* rxport, remote gpio num, flags */
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				ds90ub954_fpd3_in0: endpoint {
+					remote-endpoint = <&sensor_0_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				ds90ub954_fpd3_in1: endpoint {
+					remote-endpoint = <&sensor_1_out>;
+				};
+			};
+
+			port@2 {
+				reg = <2>;
+				ds90ub954_mipi_out0: endpoint {
+					data-lanes = <1 2 3 4>;
+					/* Actually a REFCLK multiplier */
+					data-rate = <1600000000>;
+					remote-endpoint = <&csirx_0_in>;
+				};
+			};
+		};
+
+		i2c-atr {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			remote_i2c0: i2c@0 {
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			remote_i2c1: i2c@1 {
+				reg = <1>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+	};
+};
+
+&ds90ub954_fpd3_in0 {
+	remote-chip {
+		compatible = "ti,ds90ub953-q1";
+		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED>;
+	};
+};
+
+&ds90ub954_fpd3_in1 {
+	remote-chip {
+		compatible = "ti,ds90ub953-q1";
+		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED>;
+	};
+};
+
+&remote_i2c0 {
+	sensor_0@3c {
+		compatible = "sony,imx274";
+		reg = <0x3c>;
+
+		reset-gpios = <&deser 0 0 GPIO_ACTIVE_LOW>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			sensor_0_out: endpoint {
+				remote-endpoint = <&ds90ub954_fpd3_in0>;
+			};
+		};
+	};
+};
+
+&remote_i2c1 {
+	sensor_0@3c {
+		compatible = "sony,imx274";
+		reg = <0x3c>;
+
+		reset-gpios = <&deser 1 0 GPIO_ACTIVE_LOW>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			sensor_1_out: endpoint {
+				remote-endpoint = <&ds90ub954_fpd3_in1>;
+			};
+		};
+	};
+};
-- 
2.17.1

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Rob Herring]

On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote:
> Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
> with I2C Address Translator and remote GPIOs.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes RFCv1 -> RFCv2:
> 
>  - add explicit aliases for the FPD-link RX ports (optional)
>  - add proper remote GPIO description
> ---
>  .../bindings/media/i2c/ti,ds90ub954-q1.txt    | 194 ++++++++++++++++++
>  1 file changed, 194 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> new file mode 100644
> index 000000000000..73ce21ecc3b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> @@ -0,0 +1,194 @@
> +Texas Instruments DS90UB954-Q1 dual video deserializer
> +======================================================
> +
> +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video
> +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It
> +also allows access to remote I2C and GPIO.
> +
> +Required properties:
> +
> + - compatible: must be "ti,ds90ub954-q1"
> +
> + - reg: main I2C slave address; optionally aliases for RX port registers
> +   and remote serializers. The main address is mandatory and must be the
> +   first, others are optional and fall back to defaults if not
> +   specified. See "reg-names".
> +
> + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip)
> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
> +		   runtime and used as aliases to reach remove I2C chips

s/remove/remote/

Needs a vendor prefix.

> + - gpio-controller
> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags

We're pretty standardized on 2 cells for GPIO. Perhaps combine the port 
and gpio number to 1 cell.

> +
> +Optional properties:
> +
> + - reg-names: names of I2C address used to communicate with the chip, must
> +              match the "reg" values; mandatory if there are 2 or more
> +              addresses
> +    - "main": the main I2C address, used to access shared registers
> +    - "rxport0", "rxport1": I2C alias to access FPD-link RX port specific
> +      registers; must not be used by other slaves on the same bus
> +    - "ser0", "ser1": I2C alias to access the remote serializer connected
> +      on each FPD-link RX port; must not be used by other slaves on the
> +      same bus
> + - interrupts: interrupt pin from the chip
> +
> +Required subnodes:
> +
> + - ports: A ports node with one port child node per device input and output
> +          port, in accordance with the video interface bindings defined in
> +          Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +          port nodes are numbered as follows:
> +
> +          Port Description
> +          ------------------------------------
> +          0    Input from FPD-Link 3 RX port 0
> +          1    Input from FPD-Link 3 RX port 1
> +          2    CSI-2 output
> +
> +          Each port must have a "remote-chip" subnode that defines the remote
> +	  chip (serializer) with at least a "compatible" property

We don't allow other nodes within graph nodes. I'm not really clear what 
you are trying to do here.

> +
> + - i2c-atr: contains one child per RX port, each describes the I2C bus on
> +            the remote side
> +
> +	    Required properties:
> +	    - #address-cells = <1>;
> +	    - #size-cells = <0>;
> +
> +	    Subnodes: one per each FPD-link RX port, each having:
> +
> +	    Required properties for "i2c-atr" child bus nodes:
> +	    - reg: The number of the port where the remove chip is connected

s/remove/remote/

> +	    - #address-cells = <1>;
> +	    - #size-cells = <0>;
> +
> +	    Optional properties for "i2c-atr" child bus nodes:
> +	    - Other properties specific to the remote hardware

Such as?

> +	    - Child nodes conforming to i2c bus binding
> +
> +
> +Device node example
> +-------------------
> +
> +&i2c0 {
> +	deser: deser@3d {
> +		compatible = "ti,ds90ub954-q1";
> +		reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
> +		reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
> +		clocks = <&clk_25M>;
> +		interrupt-parent = <&gic>;
> +		interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
> +		reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
> +
> +		i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
> +
> +		gpio-controller;
> +		#gpio-cells = <3>; /* rxport, remote gpio num, flags */
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				ds90ub954_fpd3_in0: endpoint {
> +					remote-endpoint = <&sensor_0_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				ds90ub954_fpd3_in1: endpoint {
> +					remote-endpoint = <&sensor_1_out>;
> +				};
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				ds90ub954_mipi_out0: endpoint {
> +					data-lanes = <1 2 3 4>;
> +					/* Actually a REFCLK multiplier */
> +					data-rate = <1600000000>;
> +					remote-endpoint = <&csirx_0_in>;
> +				};
> +			};
> +		};
> +
> +		i2c-atr {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			remote_i2c0: i2c@0 {
> +				reg = <0>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;

Presumably, there are child I2C devices here. Please show that in the 
example.

> +			};
> +
> +			remote_i2c1: i2c@1 {
> +				reg = <1>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +	};
> +};
> +
> +&ds90ub954_fpd3_in0 {
> +	remote-chip {
> +		compatible = "ti,ds90ub953-q1";
> +		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE

Not documented.

> +				  DS90_GPIO_FUNC_UNUSED
> +				  DS90_GPIO_FUNC_UNUSED
> +				  DS90_GPIO_FUNC_UNUSED>;
> +	};
> +};
> +
> +&ds90ub954_fpd3_in1 {
> +	remote-chip {
> +		compatible = "ti,ds90ub953-q1";
> +		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
> +				  DS90_GPIO_FUNC_UNUSED
> +				  DS90_GPIO_FUNC_UNUSED
> +				  DS90_GPIO_FUNC_UNUSED>;
> +	};
> +};
> +
> +&remote_i2c0 {
> +	sensor_0@3c {
> +		compatible = "sony,imx274";
> +		reg = <0x3c>;
> +
> +		reset-gpios = <&deser 0 0 GPIO_ACTIVE_LOW>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +			sensor_0_out: endpoint {
> +				remote-endpoint = <&ds90ub954_fpd3_in0>;
> +			};
> +		};
> +	};
> +};
> +
> +&remote_i2c1 {
> +	sensor_0@3c {
> +		compatible = "sony,imx274";
> +		reg = <0x3c>;
> +
> +		reset-gpios = <&deser 1 0 GPIO_ACTIVE_LOW>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +			sensor_1_out: endpoint {
> +				remote-endpoint = <&ds90ub954_fpd3_in1>;
> +			};
> +		};
> +	};
> +};
> -- 
> 2.17.1
> 

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Luca Ceresoli]

Hi Rob,

thanks for your review.

On 13/08/19 17:44, Rob Herring wrote:
> On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote:
>> Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
>> with I2C Address Translator and remote GPIOs.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> ---
>>
>> Changes RFCv1 -> RFCv2:
>>
>>  - add explicit aliases for the FPD-link RX ports (optional)
>>  - add proper remote GPIO description
>> ---
>>  .../bindings/media/i2c/ti,ds90ub954-q1.txt    | 194 ++++++++++++++++++
>>  1 file changed, 194 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
>> new file mode 100644
>> index 000000000000..73ce21ecc3b6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
>> @@ -0,0 +1,194 @@
>> +Texas Instruments DS90UB954-Q1 dual video deserializer
>> +======================================================
>> +
>> +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video
>> +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It
>> +also allows access to remote I2C and GPIO.
>> +
>> +Required properties:
>> +
>> + - compatible: must be "ti,ds90ub954-q1"
>> +
>> + - reg: main I2C slave address; optionally aliases for RX port registers
>> +   and remote serializers. The main address is mandatory and must be the
>> +   first, others are optional and fall back to defaults if not
>> +   specified. See "reg-names".
>> +
>> + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip)
>> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
>> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
>> +		   runtime and used as aliases to reach remove I2C chips
> 
> s/remove/remote/

Will fix.

> Needs a vendor prefix.

The ultimate goal here is to define a standard property that all chips
able to to I2C forwarding (video serdes or, potentially, other chips)
can use. That's why the GMSL (Maxim deser) developers are in Cc: they
are also facing a similar need.

However I'm OK to change this to "ti,i2c-alias-pool" just in case there
are reasons to not use a common name [yet]. However, following this
argument, shouldn't a prefix be needed also for other nonstandard
strings, such as "i2c-atr" below?

>> + - gpio-controller
>> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags
> 
> We're pretty standardized on 2 cells for GPIO. Perhaps combine the port 
> and gpio number to 1 cell.

Oh dear. I dislike implementing software that does not model the
physical reality. I know it will bite me back sooner or later. Here we
_really_ have N physically separate GPIO controllers, and the number of
GPIOs they have depends on the model of the chip that is connected remotely.

This is how things look in the case of 2 ports:

 <-- base board -->         <------- remote camera module 1 ----->
                            .---------------------.
 .-----.    .------.        |         SER 1       |
 | CPU |----|port 1|========|----------.          |
 `-----'    |      |  FPD   | GPIO ctl |          |
            |      | Link 3 `---------------------'
            |      | cable        ||||
	    | DES  |         remote GPIO pins
            |      |
            |      |        <------- remote camera module 2 ----->
            |      |        .---------------------.
            |port 2|        |         SER 2       |
            |      |========|----------.          |
            `------'  FPD   | GPIO ctl |          |
                     Link 3 `---------------------'
                     cable    ||||||||
	                     remote GPIO pins

Perhaps we should have N separate gpiochips, one per port? Under which
node? Not under "ports" ("We don't allow other nodes within graph
nodes"), and "i2c-atr" does not seem like a suitable name. Under a new
"remote-gpiochips" node?

>> +Required subnodes:
>> +
>> + - ports: A ports node with one port child node per device input and output
>> +          port, in accordance with the video interface bindings defined in
>> +          Documentation/devicetree/bindings/media/video-interfaces.txt. The
>> +          port nodes are numbered as follows:
>> +
>> +          Port Description
>> +          ------------------------------------
>> +          0    Input from FPD-Link 3 RX port 0
>> +          1    Input from FPD-Link 3 RX port 1
>> +          2    CSI-2 output
>> +
>> +          Each port must have a "remote-chip" subnode that defines the remote
>> +	  chip (serializer) with at least a "compatible" property
> 
> We don't allow other nodes within graph nodes. I'm not really clear what 
> you are trying to do here.

Each of the deser ports (2 ports in this chip) creates a physical
point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL"
in the Maxim chips. One "remote chip" serializer can be connected to
each bus. The remote chip has, at least, a model (e.g. DS90UB953) and
some properties. So I need a place where model and properties can be
described. The port node looked like a good place, but as you point out
it is not.

Adding to the above discussion about 3 gpio-cells, I can think of a
different, tentative DT layout:

deser: deser@3d {
	compatible = "ti,ds90ub954-q1";
	reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
	reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
	clocks = <&clk_25M>;
	interrupt-parent = <&gic>;
	interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
	reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;

	i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			endpoint {
				remote-endpoint = <&sensor_0_out>;
			};
		};

		port@1 {
			reg = <1>;
			endpoint {
				remote-endpoint = <&sensor_1_out>;
			};
		};

		port@2 {
			reg = <2>;
			endpoint {
				data-lanes = <1 2 3 4>;
				/* Actually a REFCLK multiplier */
				data-rate = <1600000000>;
				remote-endpoint = <&csirx_0_in>;
			};
		};
	};

	remote-chips {
		#address-cells = <1>;
		#size-cells = <0>;

		remote-chip@0 {
			reg = <0>;
			chip {
				compatible = "ti,ds90ub953-q1";
				gpio-functions = <...>;
			};

			remote_i2c0: i2c@0 {
				reg = <0>;
				#address-cells = <1>;
				#size-cells = <0>;
			};

			gpio-controller;
			/* 2 cells for _each_ gpiochip */
			#gpio-cells = <2>; /* remote gpio num, flags */
		};

		remote-chip@1 {
			reg = <1>;
			/* similar to remote chip 0 */
		};
	};
};

Here there are two subnodes:

 - "remote-chips" with a subnode per remote port, each describing
   the remote chip model and properties, remote i2c bus, gpio controller
   and others (other chips have I2S, SPI, UART...)

 - "ports" with only the standard v4l2 port properties, no subnodes
   under the endpoint nodes

Does this look better from the device tree point of view?

>> + - i2c-atr: contains one child per RX port, each describes the I2C bus on
>> +            the remote side
>> +
>> +	    Required properties:
>> +	    - #address-cells = <1>;
>> +	    - #size-cells = <0>;
>> +
>> +	    Subnodes: one per each FPD-link RX port, each having:
>> +
>> +	    Required properties for "i2c-atr" child bus nodes:
>> +	    - reg: The number of the port where the remove chip is connected
> 
> s/remove/remote/

Ok.

>> +	    - #address-cells = <1>;
>> +	    - #size-cells = <0>;
>> +
>> +	    Optional properties for "i2c-atr" child bus nodes:
>> +	    - Other properties specific to the remote hardware
> 
> Such as?

"clock-frequency" at least. The remote chip is an I2C master, thus any
property that applies to an I2C master might apply as well.

"clock-frequency" is only half-implemented in these RFC patches:

 * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK
   value of the deser to the remote chip (serializer): this value is
   the physical reference clock the remote chip receives, all of its
   timings are based on it

 * the remote chip, given refclk, computes the register values that best
   approximate "clock-frequency" [not implemented in this version, would
   be in patch 6]

I plan to implement and document the whole clock-frequency feature for
the next patch iteration. But I'd love to receive comments about how
reflck is passed from the deser to the serializer via platform_data.

>> +Device node example
>> +-------------------
>> +
>> +&i2c0 {
>> +	deser: deser@3d {
>> +		compatible = "ti,ds90ub954-q1";
>> +		reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
>> +		reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
>> +		clocks = <&clk_25M>;
>> +		interrupt-parent = <&gic>;
>> +		interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
>> +		reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
>> +
>> +		i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
>> +
>> +		gpio-controller;
>> +		#gpio-cells = <3>; /* rxport, remote gpio num, flags */
>> +
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +				ds90ub954_fpd3_in0: endpoint {
>> +					remote-endpoint = <&sensor_0_out>;
>> +				};
>> +			};
>> +
>> +			port@1 {
>> +				reg = <1>;
>> +				ds90ub954_fpd3_in1: endpoint {
>> +					remote-endpoint = <&sensor_1_out>;
>> +				};
>> +			};
>> +
>> +			port@2 {
>> +				reg = <2>;
>> +				ds90ub954_mipi_out0: endpoint {
>> +					data-lanes = <1 2 3 4>;
>> +					/* Actually a REFCLK multiplier */
>> +					data-rate = <1600000000>;
>> +					remote-endpoint = <&csirx_0_in>;
>> +				};
>> +			};
>> +		};
>> +
>> +		i2c-atr {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			remote_i2c0: i2c@0 {
>> +				reg = <0>;
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
> 
> Presumably, there are child I2C devices here. Please show that in the 
> example.

They are shown below to avoid excessive nesting, look for "&remote_i2c0"
(1).

>> +&ds90ub954_fpd3_in0 {
>> +	remote-chip {
>> +		compatible = "ti,ds90ub953-q1";
>> +		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
> 
> Not documented.

That's because this node describes the remote chip. The remote chip is a
ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to
showing some child I2C devices under the I2C master, as you suggested
just above, except it's not an I2C bus but an FPD-Link 3 "bus".

Is it OK if I replace the whole gpio-functions node with the comment
"/* properties specific to the ti,ds90ub953-q1 chip */"?

>> +&remote_i2c0 {
>> +	sensor_0@3c {
>> +		compatible = "sony,imx274";
>> +		reg = <0x3c>;

(1) here are the child I2C device mentioned above

-- 
Luca

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Rob Herring]

On Mon, Aug 19, 2019 at 5:41 PM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>
> Hi Rob,
>
> thanks for your review.
>
> On 13/08/19 17:44, Rob Herring wrote:
> > On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote:
> >> Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
> >> with I2C Address Translator and remote GPIOs.
> >>
> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> >>
> >> ---
> >>
> >> Changes RFCv1 -> RFCv2:
> >>
> >>  - add explicit aliases for the FPD-link RX ports (optional)
> >>  - add proper remote GPIO description
> >> ---
> >>  .../bindings/media/i2c/ti,ds90ub954-q1.txt    | 194 ++++++++++++++++++
> >>  1 file changed, 194 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> >> new file mode 100644
> >> index 000000000000..73ce21ecc3b6
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> >> @@ -0,0 +1,194 @@
> >> +Texas Instruments DS90UB954-Q1 dual video deserializer
> >> +======================================================
> >> +
> >> +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video
> >> +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It
> >> +also allows access to remote I2C and GPIO.
> >> +
> >> +Required properties:
> >> +
> >> + - compatible: must be "ti,ds90ub954-q1"
> >> +
> >> + - reg: main I2C slave address; optionally aliases for RX port registers
> >> +   and remote serializers. The main address is mandatory and must be the
> >> +   first, others are optional and fall back to defaults if not
> >> +   specified. See "reg-names".
> >> +
> >> + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip)
> >> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
> >> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
> >> +               runtime and used as aliases to reach remove I2C chips
> >
> > s/remove/remote/
>
> Will fix.
>
> > Needs a vendor prefix.
>
> The ultimate goal here is to define a standard property that all chips
> able to to I2C forwarding (video serdes or, potentially, other chips)
> can use. That's why the GMSL (Maxim deser) developers are in Cc: they
> are also facing a similar need.
>
> However I'm OK to change this to "ti,i2c-alias-pool" just in case there
> are reasons to not use a common name [yet]. However, following this
> argument, shouldn't a prefix be needed also for other nonstandard
> strings, such as "i2c-atr" below?

Okay, no vendor prefix is fine.

'i2c-atr' is a node name, not a property so no vendor prefix.

>
> >> + - gpio-controller
> >> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags
> >
> > We're pretty standardized on 2 cells for GPIO. Perhaps combine the port
> > and gpio number to 1 cell.
>
> Oh dear. I dislike implementing software that does not model the
> physical reality. I know it will bite me back sooner or later. Here we
> _really_ have N physically separate GPIO controllers, and the number of
> GPIOs they have depends on the model of the chip that is connected remotely.
>
> This is how things look in the case of 2 ports:
>
>  <-- base board -->         <------- remote camera module 1 ----->
>                             .---------------------.
>  .-----.    .------.        |         SER 1       |
>  | CPU |----|port 1|========|----------.          |
>  `-----'    |      |  FPD   | GPIO ctl |          |
>             |      | Link 3 `---------------------'
>             |      | cable        ||||
>             | DES  |         remote GPIO pins
>             |      |
>             |      |        <------- remote camera module 2 ----->
>             |      |        .---------------------.
>             |port 2|        |         SER 2       |
>             |      |========|----------.          |
>             `------'  FPD   | GPIO ctl |          |
>                      Link 3 `---------------------'
>                      cable    ||||||||
>                              remote GPIO pins
>
> Perhaps we should have N separate gpiochips, one per port?

Yes, seems like it.

> Under which
> node? Not under "ports" ("We don't allow other nodes within graph
> nodes"), and "i2c-atr" does not seem like a suitable name. Under a new
> "remote-gpiochips" node?
>
> >> +Required subnodes:
> >> +
> >> + - ports: A ports node with one port child node per device input and output
> >> +          port, in accordance with the video interface bindings defined in
> >> +          Documentation/devicetree/bindings/media/video-interfaces.txt. The
> >> +          port nodes are numbered as follows:
> >> +
> >> +          Port Description
> >> +          ------------------------------------
> >> +          0    Input from FPD-Link 3 RX port 0
> >> +          1    Input from FPD-Link 3 RX port 1
> >> +          2    CSI-2 output
> >> +
> >> +          Each port must have a "remote-chip" subnode that defines the remote
> >> +      chip (serializer) with at least a "compatible" property
> >
> > We don't allow other nodes within graph nodes. I'm not really clear what
> > you are trying to do here.
>
> Each of the deser ports (2 ports in this chip) creates a physical
> point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL"
> in the Maxim chips. One "remote chip" serializer can be connected to
> each bus. The remote chip has, at least, a model (e.g. DS90UB953) and
> some properties. So I need a place where model and properties can be
> described. The port node looked like a good place, but as you point out
> it is not.
>
> Adding to the above discussion about 3 gpio-cells, I can think of a
> different, tentative DT layout:
>
> deser: deser@3d {
>         compatible = "ti,ds90ub954-q1";
>         reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
>         reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
>         clocks = <&clk_25M>;
>         interrupt-parent = <&gic>;
>         interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
>         reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
>
>         i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
>
>         ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@0 {
>                         reg = <0>;
>                         endpoint {
>                                 remote-endpoint = <&sensor_0_out>;
>                         };
>                 };
>
>                 port@1 {
>                         reg = <1>;
>                         endpoint {
>                                 remote-endpoint = <&sensor_1_out>;
>                         };
>                 };
>
>                 port@2 {
>                         reg = <2>;
>                         endpoint {
>                                 data-lanes = <1 2 3 4>;
>                                 /* Actually a REFCLK multiplier */
>                                 data-rate = <1600000000>;
>                                 remote-endpoint = <&csirx_0_in>;
>                         };
>                 };
>         };
>
>         remote-chips {

I don't have a better suggestion for the location of this.

>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 remote-chip@0 {
>                         reg = <0>;
>                         chip {
>                                 compatible = "ti,ds90ub953-q1";

Seems like this should be in the parent? It's the ds90ub953
implementing the I2C bus, GPIO controller, etc.?

>                                 gpio-functions = <...>;
>                         };
>
>                         remote_i2c0: i2c@0 {

What does '0' mean here? At a given level, you can only have 1 number
space of addresses.

>                                 reg = <0>;
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>                         };
>
>                         gpio-controller;
>                         /* 2 cells for _each_ gpiochip */
>                         #gpio-cells = <2>; /* remote gpio num, flags */
>                 };
>
>                 remote-chip@1 {
>                         reg = <1>;
>                         /* similar to remote chip 0 */
>                 };
>         };
> };
>
> Here there are two subnodes:
>
>  - "remote-chips" with a subnode per remote port, each describing
>    the remote chip model and properties, remote i2c bus, gpio controller
>    and others (other chips have I2S, SPI, UART...)
>
>  - "ports" with only the standard v4l2 port properties, no subnodes
>    under the endpoint nodes
>
> Does this look better from the device tree point of view?

Yes.

> >> +        - #address-cells = <1>;
> >> +        - #size-cells = <0>;
> >> +
> >> +        Optional properties for "i2c-atr" child bus nodes:
> >> +        - Other properties specific to the remote hardware
> >
> > Such as?
>
> "clock-frequency" at least. The remote chip is an I2C master, thus any
> property that applies to an I2C master might apply as well.
>
> "clock-frequency" is only half-implemented in these RFC patches:
>
>  * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK
>    value of the deser to the remote chip (serializer): this value is
>    the physical reference clock the remote chip receives, all of its
>    timings are based on it
>
>  * the remote chip, given refclk, computes the register values that best
>    approximate "clock-frequency" [not implemented in this version, would
>    be in patch 6]
>
> I plan to implement and document the whole clock-frequency feature for
> the next patch iteration. But I'd love to receive comments about how
> reflck is passed from the deser to the serializer via platform_data.

Okay. Please try to make bindings as complete as possible even if
there's not yet driver support.

> >> +            i2c-atr {
> >> +                    #address-cells = <1>;
> >> +                    #size-cells = <0>;
> >> +
> >> +                    remote_i2c0: i2c@0 {
> >> +                            reg = <0>;
> >> +                            #address-cells = <1>;
> >> +                            #size-cells = <0>;
> >
> > Presumably, there are child I2C devices here. Please show that in the
> > example.
>
> They are shown below to avoid excessive nesting, look for "&remote_i2c0"
> (1).

I'd rather have longish lines than have things split up.

>
> >> +&ds90ub954_fpd3_in0 {
> >> +    remote-chip {
> >> +            compatible = "ti,ds90ub953-q1";
> >> +            gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
> >
> > Not documented.
>
> That's because this node describes the remote chip. The remote chip is a
> ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to
> showing some child I2C devices under the I2C master, as you suggested
> just above, except it's not an I2C bus but an FPD-Link 3 "bus".

I think I realized this after sending this...

>
> Is it OK if I replace the whole gpio-functions node with the comment
> "/* properties specific to the ti,ds90ub953-q1 chip */"?

Not necessary. gpio-functions does need a vendor prefix though.

Rob

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Luca Ceresoli]

Hi Rob,

On 20/08/19 17:44, Rob Herring wrote:
>>>> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
>>>> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
>>>> +               runtime and used as aliases to reach remove I2C chips
>>>
>>> s/remove/remote/
>>
>> Will fix.
>>
>>> Needs a vendor prefix.
>>
>> The ultimate goal here is to define a standard property that all chips
>> able to to I2C forwarding (video serdes or, potentially, other chips)
>> can use. That's why the GMSL (Maxim deser) developers are in Cc: they
>> are also facing a similar need.
>>
>> However I'm OK to change this to "ti,i2c-alias-pool" just in case there
>> are reasons to not use a common name [yet]. However, following this
>> argument, shouldn't a prefix be needed also for other nonstandard
>> strings, such as "i2c-atr" below?
> 
> Okay, no vendor prefix is fine.
> 
> 'i2c-atr' is a node name, not a property so no vendor prefix.

I see, thanks.

>>>> + - gpio-controller
>>>> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags
>>>
>>> We're pretty standardized on 2 cells for GPIO. Perhaps combine the port
>>> and gpio number to 1 cell.
>>
>> Oh dear. I dislike implementing software that does not model the
>> physical reality. I know it will bite me back sooner or later. Here we
>> _really_ have N physically separate GPIO controllers, and the number of
>> GPIOs they have depends on the model of the chip that is connected remotely.
>>
>> This is how things look in the case of 2 ports:
>>
>>  <-- base board -->         <------- remote camera module 1 ----->
>>                             .---------------------.
>>  .-----.    .------.        |         SER 1       |
>>  | CPU |----|port 1|========|----------.          |
>>  `-----'    |      |  FPD   | GPIO ctl |          |
>>             |      | Link 3 `---------------------'
>>             |      | cable        ||||
>>             | DES  |         remote GPIO pins
>>             |      |
>>             |      |        <------- remote camera module 2 ----->
>>             |      |        .---------------------.
>>             |port 2|        |         SER 2       |
>>             |      |========|----------.          |
>>             `------'  FPD   | GPIO ctl |          |
>>                      Link 3 `---------------------'
>>                      cable    ||||||||
>>                              remote GPIO pins
>>
>> Perhaps we should have N separate gpiochips, one per port?
> 
> Yes, seems like it.

I'm more and more convinced of that.

>>>> +Required subnodes:
>>>> +
>>>> + - ports: A ports node with one port child node per device input and output
>>>> +          port, in accordance with the video interface bindings defined in
>>>> +          Documentation/devicetree/bindings/media/video-interfaces.txt. The
>>>> +          port nodes are numbered as follows:
>>>> +
>>>> +          Port Description
>>>> +          ------------------------------------
>>>> +          0    Input from FPD-Link 3 RX port 0
>>>> +          1    Input from FPD-Link 3 RX port 1
>>>> +          2    CSI-2 output
>>>> +
>>>> +          Each port must have a "remote-chip" subnode that defines the remote
>>>> +      chip (serializer) with at least a "compatible" property
>>>
>>> We don't allow other nodes within graph nodes. I'm not really clear what
>>> you are trying to do here.
>>
>> Each of the deser ports (2 ports in this chip) creates a physical
>> point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL"
>> in the Maxim chips. One "remote chip" serializer can be connected to
>> each bus. The remote chip has, at least, a model (e.g. DS90UB953) and
>> some properties. So I need a place where model and properties can be
>> described. The port node looked like a good place, but as you point out
>> it is not.
>>
>> Adding to the above discussion about 3 gpio-cells, I can think of a
>> different, tentative DT layout:
>>
>> deser: deser@3d {
>>         compatible = "ti,ds90ub954-q1";
>>         reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
>>         reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
>>         clocks = <&clk_25M>;
>>         interrupt-parent = <&gic>;
>>         interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
>>         reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
>>
>>         i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
>>
>>         ports {
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>
>>                 port@0 {
>>                         reg = <0>;
>>                         endpoint {
>>                                 remote-endpoint = <&sensor_0_out>;
>>                         };
>>                 };
>>
>>                 port@1 {
>>                         reg = <1>;
>>                         endpoint {
>>                                 remote-endpoint = <&sensor_1_out>;
>>                         };
>>                 };
>>
>>                 port@2 {
>>                         reg = <2>;
>>                         endpoint {
>>                                 data-lanes = <1 2 3 4>;
>>                                 /* Actually a REFCLK multiplier */
>>                                 data-rate = <1600000000>;
>>                                 remote-endpoint = <&csirx_0_in>;
>>                         };
>>                 };
>>         };
>>
>>         remote-chips {
> 
> I don't have a better suggestion for the location of this.
> 
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>
>>                 remote-chip@0 {
>>                         reg = <0>;
>>                         chip {
>>                                 compatible = "ti,ds90ub953-q1";
> 
> Seems like this should be in the parent? It's the ds90ub953
> implementing the I2C bus, GPIO controller, etc.?

You're right: it's the ds90ub953 implementing i2c, gpio etc. The
(undoubtedly weird) DT structure originates from the hotplugging issue
that I explained in the cover letter. The compromise I found to handle
it is to describe the remote I2C adapters and GPIO controllers as if
they were part of the "local" domain (the ds90ub954). This is why they
are represented in DT outside the ds90ub953 node: the "chip" node would
be added by an overlay, the i2c and gpio nodes are always there.

But this corresponds to how I implemented it in the code. Let's see if
at least DT can have a better layout:

deser: deser@3d {
	compatible = "ti,ds90ub954-q1";
	reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
	reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
	clocks = <&clk_25M>;
	interrupt-parent = <&gic>;
	interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
	reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;

	i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			endpoint {
				remote-endpoint = <&sensor_0_out>;
			};
		};

		port@1 {
			reg = <1>;
			endpoint {
				remote-endpoint = <&sensor_1_out>;
			};
		};

		port@2 {
			reg = <2>;
			endpoint {
				data-lanes = <1 2 3 4>;
				/* Actually a REFCLK multiplier */
				data-rate = <1600000000>;
				remote-endpoint = <&csirx_0_in>;
			};
		};
	};

	remote-chips {
		#address-cells = <1>;
		#size-cells = <0>;

		remote-chip@0 {
			reg = <0>;

			/* dynamic */
			compatible = "ti,ds90ub953-q1";
			gpio-functions = <...>;

			remote_i2c0: i2c {
				reg = <0>;
				#address-cells = <1>;
				#size-cells = <0>;
			};

			gpio-controller;
			/* 2 cells for _each_ gpiochip: */
			/* remote gpio num, flags */
			#gpio-cells = <2>;
		};

		remote-chip@1 {
			reg = <1>;
			/* similar to remote chip 0 */
		};
	};
};

Now things _look_ correct: I just removed the "chip" subnode and hoisted
its properties up, where the i2c and gpio nodes are. However this would
not correspond a currently doable implementation. The gpio- and
i2c-related nodes under remote-chip@<N> would be always instantiated in
the base board DT, while the "compatible" property would be
added/removed by overlays on hotplug. This would require the deser chip
driver to probe the remote serializer chip not when the device node
appears but when the compatible property appears inside it. This would
look weird on the device driver side, even though I understand very well
that a DT needs to be future-proof much more than a driver.

I'll have a more detailed look into this.

Also, in case this were not clear, no hotplugging is actually
implemented currently, mostly due to the lack of a complete
implementation of runtime DT overlay insertion and removal. I think work
is in progress on that side, but it doesn't look like it is done yet.
However I'm trying to be ready for it when it will be available.

>>                         remote_i2c0: i2c@0 {
> 
> What does '0' mean here? At a given level, you can only have 1 number
> space of addresses.

Copy-paste leftover, will fix.

>>>> +        - #address-cells = <1>;
>>>> +        - #size-cells = <0>;
>>>> +
>>>> +        Optional properties for "i2c-atr" child bus nodes:
>>>> +        - Other properties specific to the remote hardware
>>>
>>> Such as?
>>
>> "clock-frequency" at least. The remote chip is an I2C master, thus any
>> property that applies to an I2C master might apply as well.
>>
>> "clock-frequency" is only half-implemented in these RFC patches:
>>
>>  * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK
>>    value of the deser to the remote chip (serializer): this value is
>>    the physical reference clock the remote chip receives, all of its
>>    timings are based on it
>>
>>  * the remote chip, given refclk, computes the register values that best
>>    approximate "clock-frequency" [not implemented in this version, would
>>    be in patch 6]
>>
>> I plan to implement and document the whole clock-frequency feature for
>> the next patch iteration. But I'd love to receive comments about how
>> reflck is passed from the deser to the serializer via platform_data.
> 
> Okay. Please try to make bindings as complete as possible even if
> there's not yet driver support.
> 
>>>> +            i2c-atr {
>>>> +                    #address-cells = <1>;
>>>> +                    #size-cells = <0>;
>>>> +
>>>> +                    remote_i2c0: i2c@0 {
>>>> +                            reg = <0>;
>>>> +                            #address-cells = <1>;
>>>> +                            #size-cells = <0>;
>>>
>>> Presumably, there are child I2C devices here. Please show that in the
>>> example.
>>
>> They are shown below to avoid excessive nesting, look for "&remote_i2c0"
>> (1).
> 
> I'd rather have longish lines than have things split up.

Ok, but do we have 80-chars limit in the bindings too?

>>>> +&ds90ub954_fpd3_in0 {
>>>> +    remote-chip {
>>>> +            compatible = "ti,ds90ub953-q1";
>>>> +            gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
>>>
>>> Not documented.
>>
>> That's because this node describes the remote chip. The remote chip is a
>> ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to
>> showing some child I2C devices under the I2C master, as you suggested
>> just above, except it's not an I2C bus but an FPD-Link 3 "bus".
> 
> I think I realized this after sending this...
> 
>>
>> Is it OK if I replace the whole gpio-functions node with the comment
>> "/* properties specific to the ti,ds90ub953-q1 chip */"?
> 
> Not necessary. gpio-functions does need a vendor prefix though.

Fair enough. I think other models/brands of serdes chips will need
something similar, but they could accept different values, wo I'll add a
"ti," prefix.

-- 
Luca

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]


> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
> +		   runtime and used as aliases to reach remove I2C chips

After some internal discussion, I have been kinda convinced that it may
be better to assume all non-described addresses are free to use and
enter the pool.

The problem with the binding above is that you may run out of aliases
depending on how many aliases one to-be-attached module needs or how
many modules will be attached. And another add-on module with
non-repogrammable devices may occupy addresses from the defined pool
above.

I am not perfectly happy with the assumption that all undescribed
addresses are automatically free. That also might need DTS updates to
describe all clients properly. But this change only needs to be done
once, and it will improve the description of the hardware.

So, I could agree this is the better option of the two.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Luca Ceresoli]

Hi Wolfram,

On 02/09/19 22:48, Wolfram Sang wrote:
> 
>> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
>> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
>> +		   runtime and used as aliases to reach remove I2C chips
> 
> After some internal discussion, I have been kinda convinced that it may
> be better to assume all non-described addresses are free to use and
> enter the pool.
> 
> The problem with the binding above is that you may run out of aliases
> depending on how many aliases one to-be-attached module needs or how
> many modules will be attached.

Not if you define enough addresses in the pool. E.g. the DS90UB954
hardware can have 8 aliases per port, so if you have (n_ports * 8)
addresses in the pool the problem is solved.

> And another add-on module with
> non-repogrammable devices may occupy addresses from the defined pool
> above.

You mean a new device on the local (SoC-to-ATR) bus? Well, it could as
well occupy a non-described address that the ATR has already picked as
an alias.

> I am not perfectly happy with the assumption that all undescribed
> addresses are automatically free. That also might need DTS updates to
> describe all clients properly. But this change only needs to be done
> once, and it will improve the description of the hardware.

Right, but I still suspect some users won't do their homework and
discover address conflicts at runtime, maybe months later, in a painful
way. Also a chip might be undocumented on a given board, so they could
do their homework and still have problems.

Despite my comments, I'm not strongly against your proposal. To me it
doesn't seem to solve any problem, while it does introduce some degree
of risk. Could you elaborate more on but what benefit it introduces?

Thanks,
-- 
Luca

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]


> Not if you define enough addresses in the pool. E.g. the DS90UB954
> hardware can have 8 aliases per port, so if you have (n_ports * 8)
> addresses in the pool the problem is solved.

And then you plug-in somewhere another board with another need for ATR
and you are out of addresses.

> > And another add-on module with
> > non-repogrammable devices may occupy addresses from the defined pool
> > above.
> 
> You mean a new device on the local (SoC-to-ATR) bus? Well, it could as
> well occupy a non-described address that the ATR has already picked as
> an alias.

Nope, I mean a seperate add-on which has a hardcoded I2C address on the
bus of the ATR parent. Then this hardcoded address needs to be removed
from the pool if it is in the wrong range.

> > I am not perfectly happy with the assumption that all undescribed
> > addresses are automatically free. That also might need DTS updates to
> > describe all clients properly. But this change only needs to be done
> > once, and it will improve the description of the hardware.
> 
> Right, but I still suspect some users won't do their homework and
> discover address conflicts at runtime, maybe months later, in a painful
> way. Also a chip might be undocumented on a given board, so they could
> do their homework and still have problems.

Yes, we probably need a binding to mark an address as used even though
we don't know the device or don't have a driver for it.

Don't get me wrong, I know what you mean. One of my boards has a client
soldered in a way so that it is still in debug mode. That means it
listens to addresses 0x03-0x07 to provide debug information. Took me a
while to find out what is happening there.

But still, 'i2cdetect' showed all of these.

> Despite my comments, I'm not strongly against your proposal. To me it
> doesn't seem to solve any problem, while it does introduce some degree
> of risk. Could you elaborate more on but what benefit it introduces?

I'd think the risk of running out of defined addresses is somewhere
equal to running into (after a while) an unexpectedly used address.
I like the fix for the latter better because describing what is on the
bus is more helpful and generic than updating the pool-property every
time you need it. Plus, as mentioned above, other add-on hardware may
disturb your pool allocation.

I expect this topic to be one of the discussion points of the BoF.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Luca Ceresoli]

Hi Wolfram,

On 03/09/19 11:34, Wolfram Sang wrote:
> 
>> Not if you define enough addresses in the pool. E.g. the DS90UB954
>> hardware can have 8 aliases per port, so if you have (n_ports * 8)
>> addresses in the pool the problem is solved.
> 
> And then you plug-in somewhere another board with another need for ATR
> and you are out of addresses.

Now I got what you mean: the aliases in a pool are reserved to an ATR
and not available to another ATR. What about hoisting the pool one level
up in DT, to the adapter where the ATRs are connected?

>>> And another add-on module with
>>> non-repogrammable devices may occupy addresses from the defined pool
>>> above.
>>
>> You mean a new device on the local (SoC-to-ATR) bus? Well, it could as
>> well occupy a non-described address that the ATR has already picked as
>> an alias.
> 
> Nope, I mean a seperate add-on which has a hardcoded I2C address on the
> bus of the ATR parent. Then this hardcoded address needs to be removed
> from the pool if it is in the wrong range.

As I understand it, we are referring to the same use case:

.---------------.       .-----.   .-------------------.
| adapter (SoC) |---+---| ATR |---| remote slave 0x10 |
'---------------'   |   '-----'   '-------------------'
                    |
              .----------.
              | device X |
              '----------'

Here device X with hard-coded address 0x20 is plugged in at runtime.

As you say it, if 0x20 is in the ATR pool we have a problem.

But even without an explicit pool, initially 0x20 is unused and the ATR
can use it for remote slave 0x10. Then at some point device X is
connected, and suddenly 0x20 conflicts.

To a limited extend the explicit pool could help if the list of possible
addons is known: one can put in the pool only address that will never
appear in addons.

Hey, wait, the no-pool solution could have a way to handle this
conflict. When device X is connected, the adapter can look for a new
alias (0x21), call the i2c_atr_deconfigure_hw() and i2c_atr_setup_hw()
callbacks to stop using 0x20 and start using 0x21. Doesn't look very
lovely, but may be worth considering.

>>> I am not perfectly happy with the assumption that all undescribed
>>> addresses are automatically free. That also might need DTS updates to
>>> describe all clients properly. But this change only needs to be done
>>> once, and it will improve the description of the hardware.
>>
>> Right, but I still suspect some users won't do their homework and
>> discover address conflicts at runtime, maybe months later, in a painful
>> way. Also a chip might be undocumented on a given board, so they could
>> do their homework and still have problems.
> 
> Yes, we probably need a binding to mark an address as used even though
> we don't know the device or don't have a driver for it.
> 
> Don't get me wrong, I know what you mean. One of my boards has a client
> soldered in a way so that it is still in debug mode. That means it
> listens to addresses 0x03-0x07 to provide debug information. Took me a
> while to find out what is happening there.
> 
> But still, 'i2cdetect' showed all of these.
> 
>> Despite my comments, I'm not strongly against your proposal. To me it
>> doesn't seem to solve any problem, while it does introduce some degree
>> of risk. Could you elaborate more on but what benefit it introduces?
> 
> I'd think the risk of running out of defined addresses is somewhere
> equal to running into (after a while) an unexpectedly used address.
> I like the fix for the latter better because describing what is on the
> bus is more helpful and generic than updating the pool-property every
> time you need it. Plus, as mentioned above, other add-on hardware may
> disturb your pool allocation.
> 
> I expect this topic to be one of the discussion points of the BoF.

Definitely.  And having a list of use cases would help a lot IMO. I had
never considered the use case described above, for example.

Thanks,
-- 
Luca

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

Hi Luca,

> Now I got what you mean: the aliases in a pool are reserved to an ATR
> and not available to another ATR. What about hoisting the pool one level
> up in DT, to the adapter where the ATRs are connected?

Frankly, it sounds error prone to keep them in sync.

> As I understand it, we are referring to the same use case:
> 
> .---------------.       .-----.   .-------------------.
> | adapter (SoC) |---+---| ATR |---| remote slave 0x10 |
> '---------------'   |   '-----'   '-------------------'
>                     |
>               .----------.
>               | device X |
>               '----------'
> 
> Here device X with hard-coded address 0x20 is plugged in at runtime.

Yes. For the generic case, it doesn't even need to be plugged at
runtime. A baseboard can have various additional modules with various
requirements. Hot-plugging (not a favourite topic for I2C) adds to it.

> As you say it, if 0x20 is in the ATR pool we have a problem.

Right.

> But even without an explicit pool, initially 0x20 is unused and the ATR
> can use it for remote slave 0x10. Then at some point device X is
> connected, and suddenly 0x20 conflicts.

I haven't considered hot-plugging so far, but you are right here.

> To a limited extend the explicit pool could help if the list of possible
> addons is known: one can put in the pool only address that will never
> appear in addons.

And this is why I kinda accepted the all-unoccupied-addresses-are-free
approach. For a generic solution, the list of possible add-ons is
unknown and we should expect "the worst".

> Hey, wait, the no-pool solution could have a way to handle this
> conflict. When device X is connected, the adapter can look for a new
> alias (0x21), call the i2c_atr_deconfigure_hw() and i2c_atr_setup_hw()
> callbacks to stop using 0x20 and start using 0x21. Doesn't look very
> lovely, but may be worth considering.

Thanks for also thinking about the other approach! Everything combining
I2C and hot-plugging is likely to not be lovely, so this may be OK given
it really works in practice.

Another note: I was hoping we could keep the number of callbacks to one.
E.g. if i2c_atr_setup_hw() has the same values for 'address' and 'alias'
(or one of them is zero), then we unregister an alias, otherwise we
register. Not sure if this will work out in the end, but only one
callback is what I'd like to have. (It could be put into the
i2c_algorithm struct maybe)


> Definitely.  And having a list of use cases would help a lot IMO. I had
> never considered the use case described above, for example.

Yes. I hope to see some new faces in the room giving us their cases.

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Sakari Ailus]

Hi Luca,

On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote:

...

> +Device node example
> +-------------------
> +
> +&i2c0 {
> +	deser: deser@3d {
> +		compatible = "ti,ds90ub954-q1";
> +		reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
> +		reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
> +		clocks = <&clk_25M>;
> +		interrupt-parent = <&gic>;
> +		interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
> +		reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
> +
> +		i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
> +
> +		gpio-controller;
> +		#gpio-cells = <3>; /* rxport, remote gpio num, flags */
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				ds90ub954_fpd3_in0: endpoint {
> +					remote-endpoint = <&sensor_0_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				ds90ub954_fpd3_in1: endpoint {
> +					remote-endpoint = <&sensor_1_out>;
> +				};
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				ds90ub954_mipi_out0: endpoint {
> +					data-lanes = <1 2 3 4>;
> +					/* Actually a REFCLK multiplier */
> +					data-rate = <1600000000>;

What is data-rate used for? Is it documented somewhere? Could you use
link-frequencies property instead? It's defined in video-interfaces.txt.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer

[Luca Ceresoli]

Hi Sakari,

On 10/09/19 10:43, Sakari Ailus wrote:
> Hi Luca,
> 
> On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote:
> 
> ...
> 
>> +Device node example
>> +-------------------
>> +
>> +&i2c0 {
>> +	deser: deser@3d {
>> +		compatible = "ti,ds90ub954-q1";
>> +		reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
>> +		reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
>> +		clocks = <&clk_25M>;
>> +		interrupt-parent = <&gic>;
>> +		interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
>> +		reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
>> +
>> +		i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
>> +
>> +		gpio-controller;
>> +		#gpio-cells = <3>; /* rxport, remote gpio num, flags */
>> +
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +				ds90ub954_fpd3_in0: endpoint {
>> +					remote-endpoint = <&sensor_0_out>;
>> +				};
>> +			};
>> +
>> +			port@1 {
>> +				reg = <1>;
>> +				ds90ub954_fpd3_in1: endpoint {
>> +					remote-endpoint = <&sensor_1_out>;
>> +				};
>> +			};
>> +
>> +			port@2 {
>> +				reg = <2>;
>> +				ds90ub954_mipi_out0: endpoint {
>> +					data-lanes = <1 2 3 4>;
>> +					/* Actually a REFCLK multiplier */
>> +					data-rate = <1600000000>;
> 
> What is data-rate used for? Is it documented somewhere? Could you use
> link-frequencies property instead? It's defined in video-interfaces.txt.

Right, it should be link-frequencies. Thanks for pointing out.

-- 
Luca

[RFC,v2 4/6] media: dt-bindings: add DS90UB953-Q1 video serializer

[Luca Ceresoli]

Describe the Texas Instruments DS90UB953-Q1, a video serializer with remote
access to I2C and GPIOs.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes RFCv1 -> RFCv2: none, this patch is new in RFCv2
---
 .../bindings/media/i2c/ti,ds90ub953-q1.txt    | 42 +++++++++++++++++++
 include/dt-bindings/media/ds90ub953.h         | 16 +++++++
 2 files changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.txt
 create mode 100644 include/dt-bindings/media/ds90ub953.h

diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.txt
new file mode 100644
index 000000000000..ba24f887b607
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.txt
@@ -0,0 +1,42 @@
+Texas Instruments DS90UB953-Q1 video serializer
+===============================================
+
+The TI DS90UB953-Q1 is a MIPI CSI-2 video serializer that forwards a MIPI
+CSI-2 input video stream over an FPD Link 3 connection to a remote
+deserializer. It also allows access to I2C and GPIO from the deserializer.
+
+The DT definitions can be found in include/dt-bindings/media/ds90ub953.h
+
+When used as a the remote counterpart of a deserializer (e.g. the
+DS90UB954-Q1), the serializer is described in the
+"deserializer/ports/port@<N>/endpoint/remote-chip" node.
+
+Required properties:
+
+ - compatible: must be "ti,ds90ub953-q1"
+
+Optional properties:
+
+ - gpio-functions: a list of 4 values defining how the 4 GPIO pins are
+   connected in hardware; possible values are:
+    - DS90_GPIO_FUNC_UNUSED (0): the GPIO is not connected
+    - DS90_GPIO_FUNC_INPUT (1): the GPIO is an input to the ds90ub953
+    - DS90_GPIO_FUNC_OUTPUT_REMOTE (2): the GPIO is an output from the
+      ds90ub953, to be driven from the remote chip (deserializer)
+
+ - ti,ds90ub953-q1-clk-inv-pol-quirk: the MIPI CSI-2 input clock lane has
+                                      inverted polarity
+
+
+Device node example
+-------------------
+
+&ds90ub954_fpd3_in0 {
+	remote-chip {
+		compatible = "ti,ds90ub953-q1";
+		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
+				  DS90_GPIO_FUNC_OUTPUT_REMOTE
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED>;
+	};
+};
diff --git a/include/dt-bindings/media/ds90ub953.h b/include/dt-bindings/media/ds90ub953.h
new file mode 100644
index 000000000000..5359432968e9
--- /dev/null
+++ b/include/dt-bindings/media/ds90ub953.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/**
+ * Definitions for the Texas Instruments DS90UB953-Q1 video serializer
+ *
+ * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
+ */
+
+#ifndef _DS90UB953_H
+#define _DS90UB953_H
+
+#define DS90_GPIO_FUNC_UNUSED             0
+#define DS90_GPIO_FUNC_INPUT              1
+#define DS90_GPIO_FUNC_OUTPUT_REMOTE      2
+#define DS90_GPIO_N_FUNCS                 3
+
+#endif /* _DS90UB953_H */
-- 
2.17.1

[RFC,v2 5/6] media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer

[Luca Ceresoli]

Add a driver for the TI DS90UB954-Q1, a MIPI CSI-2 video deserializer that
forwards video streams from up to two FPD-Link 3 connections to a MIPI
CSI-2 output. It also allows access to remote I2C and GPIO.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes RFCv1 -> RFCv2:

 - i2c
   - add one i2c client per FPD-link RX port; this avoids the need to
     select ports before accessing the paged registers, and the locking
     that goes with it
   - don't use regmap: we have many clients now, having a regmap each was
     more burden than usefulness here as we use very few regmap features
   - switch from i2c-mux to i2c-atr, where the ATR functionality is now
   - add remote I2C adapters during probe, not when linked (simplifies a
     lot)
 - v4l2
   - v4l2: implement start/stop stream (!)
   - v4l2: remove unimplemented functions
 - device tree
   - get the remote serializer alias from DT, or fallback to a default
   - ditch the 'rxports' DT node, init rxports from 'ports'
   - get node to the remote chip from DT
   - get REFCLK from DT and expose it to the remote serializer
 - add remote GPIO support in the form of a gpiochip for each rxport to
   control GPIOs on the remote chip (input only for now)
 - enable IRQ (but keep polling loop as fallback)
 - add minimal CSI TX port management (DT + enable)
 - sysfs: notify 'locked' change (for poll(2) usage)
 - add test pattern generation
 - add some documentation
 - make log messages more uniform
 - many, many, many minor changes, fixes and cleanups

LIMITATIONS / TODO:

 - Implementation of V4L2 features is minimal; works in 1920x1080 YUV422
   only
 - Tested only with one of the two inputs at a time; no Virtual Channel ID
   mapping (routing) implemented
 - Do we really need ds90->alias_table_lock to protect the ATR table? Or
   are attach operations serialized?
 - The 'status' sysfs file is not sysfs compliant (contains multiple
   values). It was initially used to test the code and it should be
   rewritten differently.
 - Well tested on real hardware, but only on a 4.14 kernel
---
 drivers/media/i2c/Kconfig     |   14 +
 drivers/media/i2c/Makefile    |    2 +
 drivers/media/i2c/ds90ub954.c | 1575 +++++++++++++++++++++++++++++++++
 3 files changed, 1591 insertions(+)
 create mode 100644 drivers/media/i2c/ds90ub954.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 79ce9ec6fc1b..e7088ccfd280 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -555,6 +555,20 @@ config VIDEO_THS8200
 	  To compile this driver as a module, choose M here: the
 	  module will be called ths8200.
 
+comment "Video serializers and deserializers"
+
+config VIDEO_DS90UB954
+	tristate "TI DS90UB954-Q1 deserializer"
+	depends on OF_GPIO
+	select I2C_ATR
+	help
+	  Device driver for the Texas Instruments "DS90UB954-Q1 Dual
+	  4.16 Gbps FPD-Link III Deserializer Hub With MIPI CSI-2
+	  Outputs for 2MP/60fps Cameras and RADAR".
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ds90ub954.
+
 comment "Camera sensor devices"
 
 config VIDEO_APTINA_PLL
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index fd4ea86dedd5..66e52ededa4e 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -115,4 +115,6 @@ obj-$(CONFIG_VIDEO_IMX319)	+= imx319.o
 obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
 obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
 
+obj-$(CONFIG_VIDEO_DS90UB954)	+= ds90ub954.o
+
 obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/ds90ub954.c b/drivers/media/i2c/ds90ub954.c
new file mode 100644
index 000000000000..e5893948b3bf
--- /dev/null
+++ b/drivers/media/i2c/ds90ub954.c
@@ -0,0 +1,1575 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Driver for the Texas Instruments DS90UB954-Q1 video deserializer
+ *
+ * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/i2c-atr.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/slab.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-ctrls.h>
+
+/* TODO increase DS90_FPD_RX_NPORTS to 2, test, fix */
+#define DS90_FPD_RX_NPORTS	1  /* Physical FPD-link RX ports */
+#define DS90_CSI_TX_NPORTS	1  /* Physical CSI-2 TX ports */
+#define DS90_NPORTS		(DS90_FPD_RX_NPORTS + DS90_CSI_TX_NPORTS)
+
+#define DS90_NUM_GPIOS		7  /* Physical GPIO pins */
+#define DS90_NUM_BC_GPIOS	4  /* Max GPIOs on Back Channel */
+
+#define DS90_NUM_SLAVE_ALIASES	8
+#define DS90_MAX_POOL_ALIASES	(DS90_FPD_RX_NPORTS * DS90_NUM_SLAVE_ALIASES)
+
+/*
+ * Register map
+ *
+ * 0x00-0x32   Shared
+ * 0x33-0x3A   CSI-2 TX (per-port paged on DS90UB960, shared on 954)
+ * 0x4C        Shared
+ * 0x4D-0x7F   FPD-Link RX, per-port paged
+ * 0xB0-0xBF   Shared
+ * 0xD0-0xDF   FPD-Link RX, per-port paged
+ * 0xF0-0xF5   Shared
+ * 0xF8-0xFB   Shared
+ * All others  Reserved
+ *
+ * Register defines prefixes:
+ * DS90_SR_* = Shared register
+ * DS90_RR_* = FPD-Link RX, per-port paged register
+ * DS90_TR_* = CSI-2 TX, per-port paged register
+ * DS90_XR_* = Reserved register
+ * DS90_IR_* = Indirect register
+ */
+
+#define DS90_SR_I2C_DEV_ID		0x00
+#define DS90_SR_RESET			0x01
+#define DS90_SR_GEN_CONFIG		0x02
+#define DS90_SR_REV_MASK		0x03
+#define DS90_SR_DEVICE_STS		0x04
+#define DS90_SR_PAR_ERR_THOLD_HI	0x05
+#define DS90_SR_PAR_ERR_THOLD_LO	0x06
+#define DS90_SR_BCC_WDOG_CTL		0x07
+#define DS90_SR_I2C_CTL1		0x08
+#define DS90_SR_I2C_CTL2		0x09
+#define DS90_SR_SCL_HIGH_TIME		0x0A
+#define DS90_SR_SCL_LOW_TIME		0x0B
+#define DS90_SR_RX_PORT_CTL		0x0C
+#define DS90_SR_IO_CTL			0x0D
+#define DS90_SR_GPIO_PIN_STS		0x0E
+#define DS90_SR_GPIO_INPUT_CTL		0x0F
+#define DS90_SR_GPIO_PIN_CTL(n)		(0x10 + (n)) /* n < DS90_NUM_GPIOS */
+#define DS90_SR_FS_CTL			0x18
+#define DS90_SR_FS_HIGH_TIME_1		0x19
+#define DS90_SR_FS_HIGH_TIME_0		0x1A
+#define DS90_SR_FS_LOW_TIME_1		0x1B
+#define DS90_SR_FS_LOW_TIME_0		0x1C
+#define DS90_SR_MAX_FRM_HI		0x1D
+#define DS90_SR_MAX_FRM_LO		0x1E
+#define DS90_SR_CSI_PLL_CTL		0x1F
+
+#define DS90_SR_FWD_CTL1		0x20
+#define DS90_SR_FWD_CTL1_PORT_DIS(n)		BIT((n)+4)
+
+#define DS90_SR_FWD_CTL2		0x21
+#define DS90_SR_FWD_STS			0x22
+
+#define DS90_SR_INTERRUPT_CTL		0x23
+#define DS90_SR_INTERRUPT_CTL_INT_EN		BIT(7)
+#define DS90_SR_INTERRUPT_CTL_IE_CSI_TX0	BIT(4)
+#define DS90_SR_INTERRUPT_CTL_IE_RX(n)		BIT((n)) /* rxport[n] IRQ */
+#define DS90_SR_INTERRUPT_CTL_ALL		0x83 // TODO 0x93 to enable CSI
+
+#define DS90_SR_INTERRUPT_STS		0x24
+#define DS90_SR_INTERRUPT_STS_INT		BIT(7)
+#define DS90_SR_INTERRUPT_STS_IS_CSI_TX0	BIT(4)
+#define DS90_SR_INTERRUPT_STS_IS_RX(n)		BIT((n)) /* rxport[n] IRQ */
+
+#define DS90_SR_TS_CONFIG		0x25
+#define DS90_SR_TS_CONTROL		0x26
+#define DS90_SR_TS_LINE_HI		0x27
+#define DS90_SR_TS_LINE_LO		0x28
+#define DS90_SR_TS_STATUS		0x29
+#define DS90_SR_TIMESTAMP_P0_HI		0x2A
+#define DS90_SR_TIMESTAMP_P0_LO		0x2B
+#define DS90_SR_TIMESTAMP_P1_HI		0x2C
+#define DS90_SR_TIMESTAMP_P1_LO		0x2D
+
+#define DS90_TR_CSI_CTL			0x33
+#define DS90_TR_CSI_CTL_CSI_CAL_EN		BIT(6)
+#define DS90_TR_CSI_CTL_CSI_ENABLE		BIT(0)
+
+#define DS90_TR_CSI_CTL2		0x34
+#define DS90_TR_CSI_STS			0x35
+#define DS90_TR_CSI_TX_ICR		0x36
+
+#define DS90_TR_CSI_TX_ISR		0x37
+#define DS90_TR_CSI_TX_ISR_IS_CSI_SYNC_ERROR	BIT(3)
+#define DS90_TR_CSI_TX_ISR_IS_CSI_PASS_ERROR	BIT(1)
+
+#define DS90_TR_CSI_TEST_CTL		0x38
+#define DS90_TR_CSI_TEST_PATT_HI	0x39
+#define DS90_TR_CSI_TEST_PATT_LO	0x3A
+
+#define DS90_XR_AEQ_CTL1		0x42
+#define DS90_XR_AEQ_ERR_THOLD		0x43
+#define DS90_XR_FPD3_CAP		0x4A
+#define DS90_XR_RAW_EMBED_DTYPE	0x4B
+
+#define DS90_SR_FPD3_PORT_SEL		0x4C
+
+#define DS90_RR_RX_PORT_STS1		0x4D
+#define DS90_RR_RX_PORT_STS1_BCC_CRC_ERROR	BIT(5)
+#define DS90_RR_RX_PORT_STS1_LOCK_STS_CHG	BIT(4)
+#define DS90_RR_RX_PORT_STS1_BCC_SEQ_ERROR	BIT(3)
+#define DS90_RR_RX_PORT_STS1_PARITY_ERROR	BIT(2)
+#define DS90_RR_RX_PORT_STS1_PORT_PASS		BIT(1)
+#define DS90_RR_RX_PORT_STS1_LOCK_STS		BIT(0)
+
+#define DS90_RR_RX_PORT_STS2		0x4E
+#define DS90_RR_RX_PORT_STS2_LINE_LEN_UNSTABLE	BIT(7)
+#define DS90_RR_RX_PORT_STS2_LINE_LEN_CHG	BIT(6)
+#define DS90_RR_RX_PORT_STS2_FPD3_ENCODE_ERROR	BIT(5)
+#define DS90_RR_RX_PORT_STS2_BUFFER_ERROR	BIT(4)
+#define DS90_RR_RX_PORT_STS2_CSI_ERROR		BIT(3)
+#define DS90_RR_RX_PORT_STS2_FREQ_STABLE	BIT(2)
+#define DS90_RR_RX_PORT_STS2_CABLE_FAULT	BIT(1)
+#define DS90_RR_RX_PORT_STS2_LINE_CNT_CHG	BIT(0)
+
+#define DS90_RR_RX_FREQ_HIGH		0x4F
+#define DS90_RR_RX_FREQ_LOW		0x50
+#define DS90_RR_SENSOR_STS_0		0x51
+#define DS90_RR_SENSOR_STS_1		0x52
+#define DS90_RR_SENSOR_STS_2		0x53
+#define DS90_RR_SENSOR_STS_3		0x54
+#define DS90_RR_RX_PAR_ERR_HI		0x55
+#define DS90_RR_RX_PAR_ERR_LO		0x56
+#define DS90_RR_BIST_ERR_COUNT		0x57
+
+#define DS90_RR_BCC_CONFIG		0x58
+#define DS90_RR_BCC_CONFIG_I2C_PASS_THROUGH	BIT(6)
+
+#define DS90_RR_DATAPATH_CTL1		0x59
+#define DS90_RR_DATAPATH_CTL2		0x5A
+#define DS90_RR_SER_ID			0x5B
+#define DS90_RR_SER_ALIAS_ID		0x5C
+
+/* For these two register sets: n < DS90_NUM_SLAVE_ALIASES */
+#define DS90_RR_SLAVE_ID(n)		(0x5D + (n))
+#define DS90_RR_SLAVE_ALIAS(n)		(0x65 + (n))
+
+#define DS90_RR_PORT_CONFIG		0x6D
+#define DS90_RR_BC_GPIO_CTL(n)		(0x6E + (n)) /* n < 2 */
+#define DS90_RR_RAW10_ID		0x70
+#define DS90_RR_RAW12_ID		0x71
+#define DS90_RR_CSI_VC_MAP		0x72
+#define DS90_RR_LINE_COUNT_HI		0x73
+#define DS90_RR_LINE_COUNT_LO		0x74
+#define DS90_RR_LINE_LEN_1		0x75
+#define DS90_RR_LINE_LEN_0		0x76
+#define DS90_RR_FREQ_DET_CTL		0x77
+#define DS90_RR_MAILBOX_1		0x78
+#define DS90_RR_MAILBOX_2		0x79
+
+#define DS90_RR_CSI_RX_STS		0x7A
+#define DS90_RR_CSI_RX_STS_LENGTH_ERR		BIT(3)
+#define DS90_RR_CSI_RX_STS_CKSUM_ERR		BIT(2)
+#define DS90_RR_CSI_RX_STS_ECC2_ERR		BIT(1)
+#define DS90_RR_CSI_RX_STS_ECC1_ERR		BIT(0)
+
+#define DS90_RR_CSI_ERR_COUNTER	0x7B
+#define DS90_RR_PORT_CONFIG2		0x7C
+#define DS90_RR_PORT_PASS_CTL		0x7D
+#define DS90_RR_SEN_INT_RISE_CTL	0x7E
+#define DS90_RR_SEN_INT_FALL_CTL	0x7F
+
+#define DS90_XR_REFCLK_FREQ		0xA5
+
+#define DS90_SR_IND_ACC_CTL		0xB0
+#define DS90_SR_IND_ACC_CTL_IA_AUTO_INC		BIT(1)
+
+#define DS90_SR_IND_ACC_ADDR		0xB1
+#define DS90_SR_IND_ACC_DATA		0xB2
+#define DS90_SR_BIST_CONTROL		0xB3
+#define DS90_SR_MODE_IDX_STS		0xB8
+#define DS90_SR_LINK_ERROR_COUNT	0xB9
+#define DS90_SR_FPD3_ENC_CTL		0xBA
+#define DS90_SR_FV_MIN_TIME		0xBC
+#define DS90_SR_GPIO_PD_CTL		0xBE
+
+#define DS90_RR_PORT_DEBUG		0xD0
+#define DS90_RR_AEQ_CTL2		0xD2
+#define DS90_RR_AEQ_STATUS		0xD3
+#define DS90_RR_AEQ_BYPASS		0xD4
+#define DS90_RR_AEQ_MIN_MAX		0xD5
+#define DS90_RR_PORT_ICR_HI		0xD8
+#define DS90_RR_PORT_ICR_LO		0xD9
+#define DS90_RR_PORT_ISR_HI		0xDA
+#define DS90_RR_PORT_ISR_LO		0xDB
+#define DS90_RR_FC_GPIO_STS		0xDC
+#define DS90_RR_FC_GPIO_ICR		0xDD
+#define DS90_RR_SEN_INT_RISE_STS	0xDE
+#define DS90_RR_SEN_INT_FALL_STS	0xDF
+
+#define DS90_SR_FPD3_RX_ID0		0xF0
+#define DS90_SR_FPD3_RX_ID1		0xF1
+#define DS90_SR_FPD3_RX_ID2		0xF2
+#define DS90_SR_FPD3_RX_ID3		0xF3
+#define DS90_SR_FPD3_RX_ID4		0xF4
+#define DS90_SR_FPD3_RX_ID5		0xF5
+#define DS90_SR_I2C_RX_ID(n)		(0xF8 + (n)) /* < DS90_FPD_RX_NPORTS */
+
+/* DS90_IR_PGEN_*: Indirect Registers for Test Pattern Generator */
+
+#define DS90_IR_PGEN_CTL		0x01
+#define DS90_IR_PGEN_CTL_PGEN_ENABLE		BIT(0)
+
+#define DS90_IR_PGEN_CFG		0x02
+#define DS90_IR_PGEN_CSI_DI		0x03
+#define DS90_IR_PGEN_LINE_SIZE1		0x04
+#define DS90_IR_PGEN_LINE_SIZE0		0x05
+#define DS90_IR_PGEN_BAR_SIZE1		0x06
+#define DS90_IR_PGEN_BAR_SIZE0		0x07
+#define DS90_IR_PGEN_ACT_LPF1		0x08
+#define DS90_IR_PGEN_ACT_LPF0		0x09
+#define DS90_IR_PGEN_TOT_LPF1		0x0A
+#define DS90_IR_PGEN_TOT_LPF0		0x0B
+#define DS90_IR_PGEN_LINE_PD1		0x0C
+#define DS90_IR_PGEN_LINE_PD0		0x0D
+#define DS90_IR_PGEN_VBP		0x0E
+#define DS90_IR_PGEN_VFP		0x0F
+#define DS90_IRT_PGEN_COLOR(n)		(0x10 + (n)) /* n < 15 */
+
+/**
+ * struct ds90_rxport_info - Info for instantiating rxports from device tree
+ * local_name:       DT name of the RX port
+ * remote_name:      DT name of the remote serializer
+ * local_def_alias:  Fallback I2C alias for the RX port if not found in DT
+ * remote_def_alias: Fallback I2C alias for the remote deserializer if not
+ *                   found in DT
+ */
+struct ds90_rxport_info {
+	const char *local_name;
+	const char *remote_name;
+	u8 local_def_alias;
+	u8 remote_def_alias;
+};
+
+struct ds90_rxport {
+	/* Errors and anomalies counters */
+	u64 bcc_crc_error_count;
+	u64 bcc_seq_error_count;
+	u64 line_len_unstable_count;
+	u64 line_len_chg_count;
+	u64 fpd3_encode_error_count;
+	u64 buffer_error_count;
+	u64 line_cnt_chg_count;
+	u64 csi_rx_sts_length_err_count;
+	u64 csi_rx_sts_cksum_err_count;
+	u64 csi_rx_sts_ecc2_err_count;
+	u64 csi_rx_sts_ecc1_err_count;
+
+	struct gpio_chip        gpio_chip;
+	char                    gpio_chip_name[64];
+
+	struct i2c_client      *reg_client; /* for per-port local registers */
+
+	struct device_node     *remote_of_node; /* "remote-chip" OF node */
+	struct i2c_client      *ser_client; /* remote serializer */
+	unsigned short          ser_alias; /* ser i2c alias (lower 7 bits) */
+	bool                    locked;
+
+	struct ds90_data *ds90;
+	unsigned short nport; /* RX port number, and index in ds90->rxport[] */
+};
+
+struct ds90_csitxport {
+	u32                     data_rate; /* Nominal data rate (Gb/s) */
+};
+
+struct ds90_data {
+	struct i2c_client      *client;  /* for shared local registers */
+	struct gpio_desc       *reset_gpio;
+	struct task_struct     *kthread;
+	struct i2c_atr         *atr;
+	struct ds90_rxport     *rxport[DS90_FPD_RX_NPORTS];
+	struct ds90_csitxport   csitxport;
+
+	struct v4l2_subdev          sd;
+	struct media_pad            pads[DS90_NPORTS];
+	struct v4l2_mbus_framefmt   fmt[DS90_NPORTS];
+	struct v4l2_ctrl_handler    ctrl_handler;
+
+	unsigned long               refclk;
+
+	/* Address Translator alias-to-slave map table */
+	size_t       atr_alias_num; /* Number of aliases configured */
+	u16          atr_alias_id[DS90_MAX_POOL_ALIASES]; /* 0 = no alias */
+	u16          atr_slave_id[DS90_MAX_POOL_ALIASES]; /* 0 = not in use */
+	struct mutex alias_table_lock;
+};
+
+#define sd_to_ds90(_sd) container_of(_sd, struct ds90_data, sd)
+
+enum {
+	TEST_PATTERN_DISABLED = 0,
+	TEST_PATTERN_V_COLOR_BARS_1,
+	TEST_PATTERN_V_COLOR_BARS_2,
+	TEST_PATTERN_V_COLOR_BARS_4,
+	TEST_PATTERN_V_COLOR_BARS_8,
+};
+
+static const char * const ds90_tpg_qmenu[] = {
+	"Disabled",
+	"1 vertical color bar",
+	"2 vertical color bars",
+	"4 vertical color bars",
+	"8 vertical color bars",
+};
+
+/* -----------------------------------------------------------------------------
+ * Basic device access
+ */
+
+static int ds90_read(const struct ds90_data *ds90,
+		     const struct i2c_client *client,
+		     u8 reg, u8 *val)
+{
+	int ret = i2c_smbus_read_byte_data(client, reg);
+
+	if (ret < 0) {
+		dev_err(&ds90->client->dev,
+			"%s[0x%02x]: cannot read register 0x%02x (%d)!\n",
+			__func__, client->addr, reg, ret);
+	} else {
+		*val = ret;
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static int ds90_read_shared(const struct ds90_data *ds90, u8 reg, u8 *val)
+{
+	return ds90_read(ds90, ds90->client, reg, val);
+}
+
+static int ds90_read_rxport(const struct ds90_data *ds90, int nport,
+			    u8 reg, u8 *val)
+{
+	return ds90_read(ds90, ds90->rxport[nport]->reg_client, reg, val);
+}
+
+static int ds90_write(const struct ds90_data *ds90,
+		      const struct i2c_client *client,
+		      u8 reg, u8 val)
+{
+	int ret = i2c_smbus_write_byte_data(client, reg, val);
+
+	if (ret < 0)
+		dev_err(&ds90->client->dev,
+			"%s[0x%02x]: cannot write register 0x%02x (%d)!\n",
+			__func__, client->addr, reg, ret);
+	else
+		ret = 0;
+
+	return ret;
+}
+
+static int ds90_write_shared(const struct ds90_data *ds90, u8 reg, u8 val)
+{
+	return ds90_write(ds90, ds90->client, reg, val);
+}
+
+static int ds90_write_rxport(const struct ds90_data *ds90, int nport,
+			     u8 reg, u8 val)
+{
+	return ds90_write(ds90, ds90->rxport[nport]->reg_client, reg, val);
+}
+
+static int ds90_write_ind8(const struct ds90_data *ds90, u8 reg, u8 val)
+{
+	int err;
+
+	err = ds90_write_shared(ds90, DS90_SR_IND_ACC_ADDR, reg);
+	if (!err)
+		err = ds90_write_shared(ds90, DS90_SR_IND_ACC_DATA, val);
+	return err;
+}
+
+/* Assumes IA_AUTO_INC is set in DS90_SR_IND_ACC_CTL */
+static int ds90_write_ind16(const struct ds90_data *ds90, u8 reg, u16 val)
+{
+	int err;
+
+	err = ds90_write_shared(ds90, DS90_SR_IND_ACC_ADDR, reg);
+	if (!err)
+		err = ds90_write_shared(ds90, DS90_SR_IND_ACC_DATA, val >> 8);
+	if (!err)
+		err = ds90_write_shared(ds90, DS90_SR_IND_ACC_DATA, val & 0xff);
+	return err;
+}
+
+static int ds90_update_bits(const struct ds90_data *ds90,
+			    const struct i2c_client *client,
+			    u8 reg, u8 mask, u8 val)
+{
+	int ret = i2c_smbus_read_byte_data(client, reg);
+
+	if (ret < 0) {
+		dev_err(&ds90->client->dev,
+			"%s[0x%02x]: cannot read register 0x%02x (%d)!\n",
+			__func__, client->addr, reg, ret);
+		return ret;
+	}
+
+	ret = i2c_smbus_write_byte_data(client, reg,
+					(ret & ~mask) | (val & mask));
+	if (ret < 0) {
+		dev_err(&ds90->client->dev,
+			"%s[0x%02x]: cannot write register 0x%02x (%d)!\n",
+			__func__, client->addr, reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ds90_update_bits_shared(const struct ds90_data *ds90,
+				   u8 reg, u8 mask, u8 val)
+{
+	return ds90_update_bits(ds90, ds90->client,
+				reg, mask, val);
+}
+
+static int ds90_update_bits_rxport(const struct ds90_data *ds90, int nport,
+				   u8 reg, u8 mask, u8 val)
+{
+	return ds90_update_bits(ds90, ds90->rxport[nport]->reg_client,
+				reg, mask, val);
+}
+
+static void ds90_reset(const struct ds90_data *ds90, bool keep_reset)
+{
+	gpiod_set_value_cansleep(ds90->reset_gpio, 1);
+	usleep_range(3000, 6000); /* min 2 ms */
+
+	if (!keep_reset) {
+		gpiod_set_value_cansleep(ds90->reset_gpio, 0);
+		usleep_range(2000, 4000); /* min 1 ms */
+	}
+}
+
+/* -----------------------------------------------------------------------------
+ * I2C-ATR (address translator)
+ */
+
+static int ds90_atr_attach_client(struct i2c_atr *atr, u32 chan_id,
+				  const struct i2c_board_info *info,
+				  const struct i2c_client *client,
+				  u16 *alias_id)
+{
+	struct ds90_data *ds90 = i2c_atr_get_clientdata(atr);
+	struct ds90_rxport *rxport = ds90->rxport[chan_id];
+	struct device *dev = &ds90->client->dev;
+	u16 alias = 0;
+	int reg_idx;
+	int pool_idx;
+	int err = 0;
+
+	dev_dbg(dev, "rx%d: %s\n", chan_id, __func__);
+
+	mutex_lock(&ds90->alias_table_lock);
+
+	/* Find unused alias in table */
+
+	for (pool_idx = 0; pool_idx < ds90->atr_alias_num; pool_idx++)
+		if (ds90->atr_slave_id[pool_idx] == 0)
+			break;
+
+	if (pool_idx == ds90->atr_alias_num) {
+		dev_warn(dev, "rx%d: alias pool exhausted\n", rxport->nport);
+		err = -EADDRNOTAVAIL;
+		goto out;
+	}
+
+	alias = ds90->atr_alias_id[pool_idx];
+
+	/* Find first unused alias register */
+
+	for (reg_idx = 0; reg_idx < DS90_NUM_SLAVE_ALIASES; reg_idx++) {
+		u8 regval;
+
+		err = ds90_read_rxport(ds90, chan_id,
+				       DS90_RR_SLAVE_ALIAS(reg_idx), &regval);
+		if (!err && regval == 0)
+			break;
+	}
+
+	if (reg_idx == DS90_NUM_SLAVE_ALIASES) {
+		dev_warn(dev, "rx%d: all aliases in use\n", rxport->nport);
+		err = -EADDRNOTAVAIL;
+		goto out;
+	}
+
+	/* Map alias to slave */
+
+	ds90_write_rxport(ds90, chan_id,
+			  DS90_RR_SLAVE_ID(reg_idx), client->addr << 1);
+	ds90_write_rxport(ds90, chan_id,
+			  DS90_RR_SLAVE_ALIAS(reg_idx), alias << 1);
+
+	ds90->atr_slave_id[pool_idx] = client->addr;
+
+	*alias_id = alias; /* tell the atr which alias we chose */
+
+	dev_info(dev, "rx%d: client 0x%02x mapped at alias 0x%02x (%s)\n",
+		 rxport->nport, client->addr, alias, client->name);
+
+out:
+	mutex_unlock(&ds90->alias_table_lock);
+	return err;
+}
+
+static void ds90_atr_detach_client(struct i2c_atr *atr, u32 chan_id,
+				   const struct i2c_client *client)
+{
+	struct ds90_data *ds90 = i2c_atr_get_clientdata(atr);
+	struct ds90_rxport *rxport = ds90->rxport[chan_id];
+	struct device *dev = &ds90->client->dev;
+	u16 alias = 0;
+	int reg_idx;
+	int pool_idx;
+
+	mutex_lock(&ds90->alias_table_lock);
+
+	/* Find alias mapped to this client */
+
+	for (pool_idx = 0; pool_idx < ds90->atr_alias_num; pool_idx++)
+		if (ds90->atr_slave_id[pool_idx] == client->addr)
+			break;
+
+	if (pool_idx == ds90->atr_alias_num) {
+		dev_err(dev, "rx%d: client 0x%02x is not mapped!\n",
+			rxport->nport, client->addr);
+		goto out;
+	}
+
+	alias = ds90->atr_alias_id[pool_idx];
+
+	/* Find alias register used for this client */
+
+	for (reg_idx = 0; reg_idx < DS90_NUM_SLAVE_ALIASES; reg_idx++) {
+		u8 regval;
+		int err;
+
+		err = ds90_read_rxport(ds90, chan_id,
+				       DS90_RR_SLAVE_ALIAS(reg_idx), &regval);
+		if (!err && regval == (alias << 1))
+			break;
+	}
+
+	if (reg_idx == DS90_NUM_SLAVE_ALIASES) {
+		dev_err(dev,
+			"rx%d: cannot find alias 0x%02x reg (client 0x%02x)!\n",
+			rxport->nport, alias, client->addr);
+		goto out;
+	}
+
+	/* Unmap */
+
+	ds90_write_rxport(ds90, chan_id, DS90_RR_SLAVE_ALIAS(reg_idx), 0);
+	ds90->atr_slave_id[pool_idx] = 0;
+
+	dev_info(dev, "rx%d: client 0x%02x unmapped from alias 0x%02x (%s)\n",
+		 rxport->nport, client->addr, alias, client->name);
+
+out:
+	mutex_unlock(&ds90->alias_table_lock);
+}
+
+static const struct i2c_atr_ops ds90_atr_ops = {
+	.attach_client = ds90_atr_attach_client,
+	.detach_client = ds90_atr_detach_client,
+};
+
+/* -----------------------------------------------------------------------------
+ * CSI ports
+ */
+
+static int ds90_csiport_probe_one(struct ds90_data *ds90,
+				  const struct device_node *np)
+{
+	struct device *dev = &ds90->client->dev;
+	struct ds90_csitxport *csitxport = &ds90->csitxport;
+
+	if (of_property_read_u32(np, "data-rate", &csitxport->data_rate) != 0) {
+		dev_err(dev, "OF: %s: missing \"data-rate\" node\n",
+			of_node_full_name(np));
+		return -EINVAL;
+	}
+
+	if (csitxport->data_rate != 1600000000 &&
+	    csitxport->data_rate !=  800000000 &&
+	    csitxport->data_rate !=  400000000) {
+		dev_err(dev, "OF: %s: invalid \"data-rate\" node\n",
+			of_node_full_name(np));
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "Nominal data rate: %u", csitxport->data_rate);
+
+	return 0;
+}
+
+static void ds90_csi_handle_events(struct ds90_data *ds90)
+{
+	struct device *dev = &ds90->client->dev;
+	u8 csi_tx_isr;
+	int err;
+
+	err = ds90_read_shared(ds90, DS90_TR_CSI_TX_ISR, &csi_tx_isr);
+
+	if (!err) {
+		if (csi_tx_isr & DS90_TR_CSI_TX_ISR_IS_CSI_SYNC_ERROR)
+			dev_warn(dev, "CSI_SYNC_ERROR\n");
+
+		if (csi_tx_isr & DS90_TR_CSI_TX_ISR_IS_CSI_PASS_ERROR)
+			dev_warn(dev, "CSI_PASS_ERROR\n");
+	}
+}
+
+/* -----------------------------------------------------------------------------
+ * GPIO CHIP: control GPIOs on the remote side
+ */
+
+static int ds90_gpio_direction_out(struct gpio_chip *chip,
+				   unsigned int offset, int value)
+{
+	struct ds90_rxport *rxport = gpiochip_get_data(chip);
+	unsigned int reg_addr = DS90_RR_BC_GPIO_CTL(offset / 2);
+	unsigned int reg_shift = (offset % 2) * 4;
+
+	ds90_update_bits_rxport(rxport->ds90, rxport->nport, reg_addr,
+				0xf << reg_shift,
+				(0x8 + !!value) << reg_shift);
+	return 0;
+}
+
+static void ds90_gpio_set(struct gpio_chip *chip,
+			  unsigned int offset, int value)
+{
+	ds90_gpio_direction_out(chip, offset, value);
+}
+
+static int ds90_gpio_of_xlate(struct gpio_chip *chip,
+			      const struct of_phandle_args *gpiospec,
+			      u32 *flags)
+{
+	struct ds90_rxport *rxport = gpiochip_get_data(chip);
+	u32 bank = gpiospec->args[0];
+
+	if (bank != rxport->nport)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[2];
+
+	return gpiospec->args[1];
+}
+
+static int ds90_gpiochip_probe(struct ds90_data *ds90, int nport)
+{
+	struct ds90_rxport *rxport = ds90->rxport[nport];
+	struct gpio_chip *gc = &rxport->gpio_chip;
+	struct device *dev = &ds90->client->dev;
+	int err;
+
+	scnprintf(rxport->gpio_chip_name, sizeof(rxport->gpio_chip_name),
+		  "%s:rx%d", dev_name(dev), nport);
+
+	gc->label               = rxport->gpio_chip_name;
+	gc->parent              = dev;
+	gc->owner               = THIS_MODULE;
+	gc->base                = -1;
+	gc->can_sleep           = 1;
+	gc->ngpio               = DS90_NUM_BC_GPIOS;
+	gc->direction_output    = ds90_gpio_direction_out;
+	gc->set                 = ds90_gpio_set;
+	gc->of_xlate            = ds90_gpio_of_xlate;
+	gc->of_node             = ds90->client->dev.of_node;
+	gc->of_gpio_n_cells     = 3;
+
+	err = gpiochip_add_data(gc, rxport);
+	if (err) {
+		dev_err(dev, "rx%d: Failed to add GPIOs: %d\n", nport, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static void ds90_gpiochip_remove(struct ds90_data *ds90, int nport)
+{
+	struct ds90_rxport *rxport = ds90->rxport[nport];
+	struct gpio_chip *gc = &rxport->gpio_chip;
+
+	gpiochip_remove(gc);
+}
+
+/* -----------------------------------------------------------------------------
+ * RX ports
+ */
+
+static ssize_t locked_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf);
+static ssize_t status_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf);
+
+static struct device_attribute dev_attr_locked[] = {
+	__ATTR_RO(locked),
+	__ATTR_RO(locked),
+};
+
+static struct device_attribute dev_attr_status[] = {
+	__ATTR_RO(status),
+	__ATTR_RO(status),
+};
+
+static struct attribute *ds90_rxport0_attrs[] = {
+	&dev_attr_locked[0].attr,
+	&dev_attr_status[0].attr,
+	NULL
+};
+
+static struct attribute *ds90_rxport1_attrs[] = {
+	&dev_attr_locked[1].attr,
+	&dev_attr_status[1].attr,
+	NULL
+};
+
+static ssize_t locked_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	int nport = (attr - dev_attr_locked);
+	const struct ds90_data *ds90 = dev_get_drvdata(dev);
+	const struct ds90_rxport *rxport = ds90->rxport[nport];
+
+	return scnprintf(buf, PAGE_SIZE, "%d", rxport->locked);
+}
+
+static ssize_t status_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	int nport = (attr - dev_attr_status);
+	const struct ds90_data *ds90 = dev_get_drvdata(dev);
+	const struct ds90_rxport *rxport = ds90->rxport[nport];
+
+	return scnprintf(buf, PAGE_SIZE,
+			 "bcc_crc_error_count = %llu\n"
+			 "bcc_seq_error_count = %llu\n"
+			 "line_len_unstable_count = %llu\n"
+			 "line_len_chg_count = %llu\n"
+			 "fpd3_encode_error_count = %llu\n"
+			 "buffer_error_count = %llu\n"
+			 "line_cnt_chg_count = %llu\n"
+			 "csi_rx_sts_length_err_count = %llu\n"
+			 "csi_rx_sts_cksum_err_count = %llu\n"
+			 "csi_rx_sts_ecc2_err_count = %llu\n"
+			 "csi_rx_sts_ecc1_err_count = %llu\n",
+			 rxport->bcc_crc_error_count,
+			 rxport->bcc_seq_error_count,
+			 rxport->line_len_unstable_count,
+			 rxport->line_len_chg_count,
+			 rxport->fpd3_encode_error_count,
+			 rxport->buffer_error_count,
+			 rxport->line_cnt_chg_count,
+			 rxport->csi_rx_sts_length_err_count,
+			 rxport->csi_rx_sts_cksum_err_count,
+			 rxport->csi_rx_sts_ecc2_err_count,
+			 rxport->csi_rx_sts_ecc1_err_count);
+}
+
+struct attribute_group ds90_rxport_attr_group[] = {
+	{ .name = "rx0", .attrs = ds90_rxport0_attrs },
+	{ .name = "rx1", .attrs = ds90_rxport1_attrs },
+};
+
+/*
+ * Instantiate serializer and i2c adapter for the just-locked remote
+ * end.
+ *
+ * @note Must be called with ds90->alias_table_lock not held! The added i2c
+ * adapter will probe new slaves, which can request i2c transfers, ending
+ * up in calling ds90_atr_attach_client() where the lock is taken.
+ */
+static int ds90_rxport_add_serializer(struct ds90_data *ds90, int nport)
+{
+	struct ds90_rxport *rxport = ds90->rxport[nport];
+	struct device *dev = &ds90->client->dev;
+	struct i2c_board_info ser_info = { .type = "ds90ub953-q1",
+					   .of_node = rxport->remote_of_node,
+					   // TODO is this OK?
+					   .platform_data = &ds90->refclk };
+	int err;
+
+	/*
+	 * Adding the serializer under rxport->adap would be cleaner,
+	 * but it would need tweaks to bypass the alias table. Adding
+	 * to the upstream adapter is way simpler.
+	 */
+	ser_info.addr = rxport->ser_alias;
+	rxport->ser_client = i2c_new_device(ds90->client->adapter, &ser_info);
+	if (!rxport->ser_client) {
+		dev_err(dev, "rx%d: cannot add %s i2c device",
+			nport, ser_info.type);
+		return -EIO;
+	}
+
+	dev_info(dev, "rx%d: remote serializer at alias 0x%02x\n",
+		 nport, rxport->ser_client->addr);
+
+	return err;
+}
+
+static void ds90_rxport_remove_serializer(struct ds90_data *ds90, int nport)
+{
+	struct ds90_rxport *rxport = ds90->rxport[nport];
+
+	if (rxport->ser_client) {
+		i2c_unregister_device(rxport->ser_client);
+		rxport->ser_client = NULL;
+	}
+}
+
+/*
+ * Return the local alias for a given remote serializer.
+ * Get it from devicetree, if absent fallback to the default.
+ */
+static unsigned short
+ds90_rxport_get_remote_alias(struct ds90_data *ds90,
+			     const struct ds90_rxport_info *info)
+{
+	struct device_node *np = ds90->client->dev.of_node;
+	u32 alias = info->remote_def_alias;
+	int i;
+
+	if (np) {
+		i = of_property_match_string(np, "reg-names",
+					     info->remote_name);
+		if (i >= 0)
+			of_property_read_u32_index(np, "reg", i, &alias);
+	}
+
+	return alias;
+}
+
+static int ds90_rxport_probe_one(struct ds90_data *ds90,
+				 const struct device_node *np,
+				 unsigned int nport)
+{
+	struct device *dev = &ds90->client->dev;
+	const struct ds90_rxport_info *info;
+	struct ds90_rxport *rxport;
+	int err;
+
+	static const struct ds90_rxport_info rxport_info[DS90_FPD_RX_NPORTS] = {
+		{ "rxport0", "ser0", 0x40, 0x50 },
+		{ "rxport1", "ser1", 0x41, 0x51 },
+	};
+
+	if (ds90->rxport[nport]) {
+		dev_err(dev, "OF: %s: reg value %d is duplicated\n",
+			of_node_full_name(np), nport);
+		return -EADDRINUSE;
+	}
+
+	rxport = kzalloc(sizeof(*rxport), GFP_KERNEL);
+	if (!rxport)
+		return -ENOMEM;
+
+	ds90->rxport[nport] = rxport;
+
+	info = &rxport_info[nport];
+
+	rxport->nport     = nport;
+	rxport->ds90      = ds90;
+	rxport->ser_alias = ds90_rxport_get_remote_alias(ds90, info);
+
+	rxport->remote_of_node = of_get_child_by_name(np, "remote-chip");
+	if (!rxport->remote_of_node) {
+		dev_err(dev, "OF: %s: missing remote-chip child\n",
+			of_node_full_name(np));
+		err = -EINVAL;
+		goto err_remote_chip;
+	}
+
+	/* Initialize access to local registers */
+	rxport->reg_client = i2c_new_secondary_device(ds90->client,
+						      info->local_name,
+						      info->local_def_alias);
+	if (!rxport->reg_client) {
+		err = -ENOMEM;
+		goto err_new_secondary_device;
+	}
+	ds90_write_shared(ds90, DS90_SR_I2C_RX_ID(nport),
+			  rxport->reg_client->addr << 1);
+
+	err = ds90_gpiochip_probe(ds90, nport);
+	if (err)
+		goto err_gpiochip_probe;
+
+	/* Enable all interrupt sources from this port */
+	ds90_write_rxport(ds90, nport, DS90_RR_PORT_ICR_HI, 0x07);
+	ds90_write_rxport(ds90, nport, DS90_RR_PORT_ICR_LO, 0x7f);
+
+	/* Set pass-through, but preserve BC_FREQ_SELECT strapping options */
+	ds90_update_bits_rxport(ds90, nport, DS90_RR_BCC_CONFIG,
+				DS90_RR_BCC_CONFIG_I2C_PASS_THROUGH, ~0);
+
+	/* Enable I2C communication to the serializer via the alias addr */
+	ds90_write_rxport(ds90, nport,
+			  DS90_RR_SER_ALIAS_ID, rxport->ser_alias << 1);
+
+	err = sysfs_create_group(&dev->kobj, &ds90_rxport_attr_group[nport]);
+	if (err) {
+		dev_err(dev, "rx%d: failed creating sysfs group", nport);
+		goto err_sysfs;
+	}
+
+	err = i2c_atr_add_adapter(ds90->atr, nport);
+	if (err) {
+		dev_err(dev, "rx%d: cannot add adapter", nport);
+		goto err_add_adapter;
+	}
+
+	dev_info(dev, "rx%d: at alias 0x%02x\n",
+		 nport, rxport->reg_client->addr);
+
+	return 0;
+
+err_add_adapter:
+	sysfs_remove_group(&dev->kobj, &ds90_rxport_attr_group[nport]);
+err_sysfs:
+	ds90_gpiochip_remove(ds90, nport);
+err_gpiochip_probe:
+	i2c_unregister_device(rxport->reg_client);
+err_new_secondary_device:
+	of_node_put(rxport->remote_of_node);
+err_remote_chip:
+	ds90->rxport[nport] = NULL;
+	kfree(rxport);
+	return err;
+}
+
+static void ds90_rxport_remove_one(struct ds90_data *ds90, int nport)
+{
+	struct ds90_rxport *rxport = ds90->rxport[nport];
+	struct device *dev = &ds90->client->dev;
+
+	i2c_atr_del_adapter(ds90->atr, nport);
+	ds90_rxport_remove_serializer(ds90, nport);
+	ds90_gpiochip_remove(ds90, nport);
+	i2c_unregister_device(rxport->reg_client);
+	sysfs_remove_group(&dev->kobj, &ds90_rxport_attr_group[nport]);
+	of_node_put(rxport->remote_of_node);
+	kfree(rxport);
+}
+
+static int ds90_atr_probe(struct ds90_data *ds90)
+{
+	struct i2c_adapter *parent_adap = ds90->client->adapter;
+	struct device *dev = &ds90->client->dev;
+
+	ds90->atr = i2c_atr_new(parent_adap, dev, &ds90_atr_ops,
+				DS90_FPD_RX_NPORTS);
+	if (IS_ERR(ds90->atr))
+		return PTR_ERR(ds90->atr);
+
+	i2c_atr_set_clientdata(ds90->atr, ds90);
+
+	return 0;
+}
+
+static void ds90_atr_remove(struct ds90_data *ds90)
+{
+	i2c_atr_delete(ds90->atr);
+	ds90->atr = NULL;
+}
+
+static void ds90_rxport_handle_events(struct ds90_data *ds90, int nport)
+{
+	struct ds90_rxport *rxport = ds90->rxport[nport];
+	struct device *dev = &ds90->client->dev;
+	u8 rx_port_sts1;
+	u8 rx_port_sts2;
+	u8 csi_rx_sts;
+	bool locked;
+	int err = 0;
+
+	/* Read interrupts (also clears most of them) */
+	if (!err)
+		err = ds90_read_rxport(ds90, nport,
+				       DS90_RR_RX_PORT_STS1, &rx_port_sts1);
+	if (!err)
+		err = ds90_read_rxport(ds90, nport,
+				       DS90_RR_RX_PORT_STS2, &rx_port_sts2);
+	if (!err)
+		err = ds90_read_rxport(ds90, nport,
+				       DS90_RR_CSI_RX_STS, &csi_rx_sts);
+
+	if (err)
+		return;
+
+	if (rx_port_sts1 & DS90_RR_RX_PORT_STS1_BCC_CRC_ERROR)
+		rxport->bcc_crc_error_count++;
+
+	if (rx_port_sts1 & DS90_RR_RX_PORT_STS1_BCC_SEQ_ERROR)
+		rxport->bcc_seq_error_count++;
+
+	if (rx_port_sts2 & DS90_RR_RX_PORT_STS2_LINE_LEN_UNSTABLE)
+		rxport->line_len_unstable_count++;
+
+	if (rx_port_sts2 & DS90_RR_RX_PORT_STS2_LINE_LEN_CHG)
+		rxport->line_len_chg_count++;
+
+	if (rx_port_sts2 & DS90_RR_RX_PORT_STS2_FPD3_ENCODE_ERROR)
+		rxport->fpd3_encode_error_count++;
+
+	if (rx_port_sts2 & DS90_RR_RX_PORT_STS2_BUFFER_ERROR)
+		rxport->buffer_error_count++;
+
+	if (rx_port_sts2 & DS90_RR_RX_PORT_STS2_LINE_CNT_CHG)
+		rxport->line_cnt_chg_count++;
+
+	if (csi_rx_sts & DS90_RR_CSI_RX_STS_LENGTH_ERR)
+		rxport->csi_rx_sts_length_err_count++;
+
+	if (csi_rx_sts & DS90_RR_CSI_RX_STS_CKSUM_ERR)
+		rxport->csi_rx_sts_cksum_err_count++;
+
+	if (csi_rx_sts & DS90_RR_CSI_RX_STS_ECC2_ERR)
+		rxport->csi_rx_sts_ecc2_err_count++;
+
+	if (csi_rx_sts & DS90_RR_CSI_RX_STS_ECC1_ERR)
+		rxport->csi_rx_sts_ecc1_err_count++;
+
+	/* Update locked status */
+	locked = rx_port_sts1 & DS90_RR_RX_PORT_STS1_LOCK_STS;
+	if (locked && !rxport->locked) {
+		dev_info(dev, "rx%d: LOCKED\n", nport);
+		/* See note about locking in ds90_rxport_add_serializer()! */
+		ds90_rxport_add_serializer(ds90, nport);
+		sysfs_notify(&dev->kobj,
+			     ds90_rxport_attr_group[nport].name, "locked");
+	} else if (!locked && rxport->locked) {
+		dev_info(dev, "rx%d: NOT LOCKED\n", nport);
+		ds90_rxport_remove_serializer(ds90, nport);
+		sysfs_notify(&dev->kobj,
+			     ds90_rxport_attr_group[nport].name, "locked");
+	}
+	rxport->locked = locked;
+}
+
+/* -----------------------------------------------------------------------------
+ * V4L2
+ */
+
+static void ds90_set_tpg(struct ds90_data *ds90, int tpg_num)
+{
+	/*
+	 * Note: no need to write DS90_REG_IND_ACC_CTL: the only indirect
+	 * registers target we use is "CSI-2 Pattern Generator & Timing
+	 * Registers", which is the default
+	 */
+
+	if (tpg_num == 0) {
+		/* TPG off, enable forwarding from FPD-3 RX ports */
+		ds90_write_shared(ds90, DS90_SR_FWD_CTL1, 0x00);
+
+		ds90_write_ind8(ds90, DS90_IR_PGEN_CTL, 0x00);
+		return;
+	} else {
+		/* TPG on */
+
+		u8 vbp = 33;
+		u8 vfp = 10;
+		u16 blank_lines = vbp + vfp + 2; /* total blanking lines */
+
+		u16 width  = ds90->fmt[DS90_NPORTS - 1].width;
+		u16 height = ds90->fmt[DS90_NPORTS - 1].height;
+		u16 bytespp = 2; /* For MEDIA_BUS_FMT_UYVY8_1X16 */
+		u8 cbars_idx = tpg_num - TEST_PATTERN_V_COLOR_BARS_1;
+		u8 num_cbars = 1 << cbars_idx;
+
+		u16 line_size = width * bytespp;      /* Line size [bytes] */
+		u16 bar_size = line_size / num_cbars; /* cbar size [bytes] */
+		u16 act_lpf = height;                 /* active lines/frame */
+		u16 tot_lpf = act_lpf + blank_lines;  /* tot lines/frame */
+		/* Line period in 10-ns units */
+		u16 line_pd = 100000000 / 60 / tot_lpf;
+
+		/* Disable forwarding from FPD-3 RX ports */
+		ds90_write_shared(ds90,
+				  DS90_SR_FWD_CTL1,
+				  DS90_SR_FWD_CTL1_PORT_DIS(0) |
+				  DS90_SR_FWD_CTL1_PORT_DIS(1));
+
+		/* Access Indirect Pattern Gen */
+		ds90_write_shared(ds90,
+				  DS90_SR_IND_ACC_CTL,
+				  DS90_SR_IND_ACC_CTL_IA_AUTO_INC | 0);
+
+		ds90_write_ind8(ds90,
+				DS90_IR_PGEN_CTL,
+				DS90_IR_PGEN_CTL_PGEN_ENABLE);
+
+		/* YUV422 8bit: 2 bytes/block, CSI-2 data type 0x1e */
+		ds90_write_ind8(ds90, DS90_IR_PGEN_CFG, cbars_idx << 4 | 0x2);
+		ds90_write_ind8(ds90, DS90_IR_PGEN_CSI_DI, 0x1e);
+
+		ds90_write_ind16(ds90, DS90_IR_PGEN_LINE_SIZE1, line_size);
+		ds90_write_ind16(ds90, DS90_IR_PGEN_BAR_SIZE1,  bar_size);
+		ds90_write_ind16(ds90, DS90_IR_PGEN_ACT_LPF1,   act_lpf);
+		ds90_write_ind16(ds90, DS90_IR_PGEN_TOT_LPF1,   tot_lpf);
+		ds90_write_ind16(ds90, DS90_IR_PGEN_LINE_PD1,   line_pd);
+		ds90_write_ind8(ds90,  DS90_IR_PGEN_VBP,        vbp);
+		ds90_write_ind8(ds90,  DS90_IR_PGEN_VFP,        vfp);
+	}
+}
+
+static int ds90_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct ds90_data *ds90 = container_of(ctrl->handler, struct ds90_data,
+					      ctrl_handler);
+
+	switch (ctrl->id) {
+	case V4L2_CID_TEST_PATTERN:
+		ds90_set_tpg(ds90, ctrl->val);
+		break;
+	}
+	return 0;
+}
+
+static int ds90_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct ds90_data *ds90 = sd_to_ds90(sd);
+	unsigned int csi_ctl = DS90_TR_CSI_CTL_CSI_ENABLE;
+	unsigned int speed_select = 3;
+
+	if (enable) {
+		switch (ds90->csitxport.data_rate) {
+		case 1600000000:
+			speed_select = 0;
+			break;
+		case  800000000:
+			speed_select = 2;
+			break;
+		case  400000000:
+			speed_select = 3;
+			break;
+		}
+
+		/*
+		 * From the datasheet: "initial CSI Skew-Calibration
+		 * sequence [...] should be set when operating at 1.6 Gbps"
+		 */
+		if (speed_select == 0)
+			csi_ctl |= DS90_TR_CSI_CTL_CSI_CAL_EN;
+
+		ds90_write_shared(ds90, DS90_SR_CSI_PLL_CTL, speed_select);
+		ds90_write_shared(ds90, DS90_TR_CSI_CTL, csi_ctl);
+	} else {
+		ds90_write_shared(ds90, DS90_TR_CSI_CTL, 0);
+	}
+
+	return 0;
+}
+
+static struct v4l2_mbus_framefmt *
+ds90_get_pad_format(struct ds90_data *ds90,
+		    struct v4l2_subdev_pad_config *cfg,
+		    unsigned int pad, u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(&ds90->sd, cfg, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &ds90->fmt[pad];
+	default:
+		return NULL;
+	}
+}
+
+static int ds90_get_fmt(struct v4l2_subdev *sd,
+			struct v4l2_subdev_pad_config *cfg,
+			struct v4l2_subdev_format *format)
+{
+	struct ds90_data *ds90 = sd_to_ds90(sd);
+	struct v4l2_mbus_framefmt *cfg_fmt;
+
+	if (format->pad >= DS90_NPORTS)
+		return -EINVAL;
+
+	cfg_fmt = ds90_get_pad_format(ds90, cfg, format->pad, format->which);
+	if (!cfg_fmt)
+		return -EINVAL;
+
+	format->format = *cfg_fmt;
+
+	return 0;
+}
+
+static void ds90_init_format(struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->width		= 1920;
+	fmt->height		= 1080;
+	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
+	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
+	fmt->field		= V4L2_FIELD_NONE;
+}
+
+static const struct v4l2_ctrl_ops ds90_ctrl_ops = {
+	.s_ctrl		= ds90_s_ctrl,
+};
+
+static const struct v4l2_subdev_video_ops ds90_video_ops = {
+	.s_stream	= ds90_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops ds90_pad_ops = {
+	.get_fmt	= ds90_get_fmt,
+};
+
+static const struct v4l2_subdev_ops ds90_subdev_ops = {
+	.video		= &ds90_video_ops,
+	.pad		= &ds90_pad_ops,
+};
+
+/* -----------------------------------------------------------------------------
+ * Core
+ */
+
+static irqreturn_t ds90_handle_events(int irq, void *arg)
+{
+	struct ds90_data *ds90 = arg;
+	u8 int_sts;
+	int err;
+	int i;
+
+	err = ds90_read_shared(ds90, DS90_SR_INTERRUPT_STS, &int_sts);
+
+	if (!err && int_sts) {
+		if (int_sts & DS90_SR_INTERRUPT_STS_IS_CSI_TX0)
+			ds90_csi_handle_events(ds90);
+
+		for (i = 0; i < DS90_FPD_RX_NPORTS; i++)
+			if (int_sts & DS90_SR_INTERRUPT_STS_IS_RX(i) &&
+			    ds90->rxport[i])
+				ds90_rxport_handle_events(ds90, i);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int ds90_run(void *arg)
+{
+	struct ds90_data *ds90 = arg;
+
+	while (1) {
+		if (kthread_should_stop())
+			break;
+
+		ds90_handle_events(0, ds90);
+
+		msleep(1000);
+	}
+
+	return 0;
+}
+
+static void ds90_remove_ports(struct ds90_data *ds90)
+{
+	int i;
+
+	for (i = 0; i < DS90_FPD_RX_NPORTS; i++)
+		if (ds90->rxport[i])
+			ds90_rxport_remove_one(ds90, i);
+
+	/* CSI ports have no _remove_one(). No rollback needed. */
+}
+
+static int ds90_parse_dt(struct ds90_data *ds90)
+{
+	struct device_node *np = ds90->client->dev.of_node;
+	struct device *dev = &ds90->client->dev;
+	struct device_node *ep_np = NULL;
+	int err = 0;
+	int n;
+
+	if (!np) {
+		dev_err(dev, "OF: no device tree node!\n");
+		return -ENOENT;
+	}
+
+	n = of_property_read_variable_u16_array(np, "i2c-alias-pool",
+						ds90->atr_alias_id,
+						2, DS90_MAX_POOL_ALIASES);
+	if (n < 0)
+		dev_warn(dev,
+			 "OF: no i2c-alias-pool, can't access remote I2C slaves");
+
+	ds90->atr_alias_num = n;
+
+	dev_dbg(dev, "i2c-alias-pool has %zu aliases", ds90->atr_alias_num);
+
+	for_each_endpoint_of_node(np, ep_np) {
+		struct of_endpoint ep;
+
+		of_graph_parse_endpoint(ep_np, &ep);
+
+		if (ep.port >= DS90_NPORTS) {
+			dev_err(dev,
+				"OF: %s: missing or invalid reg property\n",
+				of_node_full_name(np));
+			return -EINVAL;
+		}
+
+		if (ep.port < DS90_FPD_RX_NPORTS)
+			err = ds90_rxport_probe_one(ds90, ep_np, ep.port);
+		else
+			err = ds90_csiport_probe_one(ds90, ep_np);
+
+		if (err) {
+			of_node_put(ep_np);
+			break;
+		}
+	}
+
+	if (err)
+		ds90_remove_ports(ds90);
+
+	return err;
+}
+
+static int ds90_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct ds90_data *ds90;
+	struct clk *clk;
+	u8 rev_mask;
+	int err;
+	int i;
+
+	ds90 = devm_kzalloc(dev, sizeof(*ds90), GFP_KERNEL);
+	if (!ds90)
+		return -ENOMEM;
+
+	ds90->client = client;
+	i2c_set_clientdata(client, ds90);
+	mutex_init(&ds90->alias_table_lock);
+
+	/* get reset pin from DT */
+	ds90->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ds90->reset_gpio)) {
+		err = PTR_ERR(ds90->reset_gpio);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "Cannot get reset GPIO (%d)", err);
+		return err;
+	}
+
+	ds90_reset(ds90, false);
+
+	clk = clk_get(dev, NULL);
+	if (IS_ERR(clk)) {
+		err = PTR_ERR(clk);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "Cannot get REFCLK (%d)", err);
+		return err;
+	}
+	ds90->refclk = clk_get_rate(clk);
+	clk_put(clk);
+	dev_dbg(dev, "REFCLK %lu", ds90->refclk);
+
+	/* Runtime check register accessibility */
+	err = ds90_read_shared(ds90, DS90_SR_REV_MASK, &rev_mask);
+	if (err) {
+		dev_err(dev, "Cannot read first register (%d), abort\n", err);
+		goto err_reg_read;
+	}
+
+	err = ds90_atr_probe(ds90);
+	if (err)
+		goto err_atr_probe;
+
+	err = ds90_parse_dt(ds90);
+	if (err)
+		goto err_parse_dt;
+
+	/* V4L2 */
+
+	for (i = 0; i < DS90_NPORTS; i++)
+		ds90_init_format(&ds90->fmt[i]);
+
+	v4l2_i2c_subdev_init(&ds90->sd, client, &ds90_subdev_ops);
+	v4l2_ctrl_handler_init(&ds90->ctrl_handler,
+			       ARRAY_SIZE(ds90_tpg_qmenu) - 1);
+	ds90->sd.ctrl_handler = &ds90->ctrl_handler;
+
+	v4l2_ctrl_new_std_menu_items(&ds90->ctrl_handler, &ds90_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(ds90_tpg_qmenu) - 1, 0, 0,
+				     ds90_tpg_qmenu);
+
+	if (ds90->ctrl_handler.error) {
+		err = ds90->ctrl_handler.error;
+		goto err_add_ctrls;
+	}
+
+	/* Let both the I2C client and the subdev point to us */
+	i2c_set_clientdata(client, ds90); /* v4l2_i2c_subdev_init writes it */
+	v4l2_set_subdevdata(&ds90->sd, ds90);
+
+	ds90->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ds90->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+
+	for (i = 0; i < DS90_NPORTS; i++)
+		ds90->pads[i].flags = (i < DS90_FPD_RX_NPORTS) ?
+			MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
+
+	err = media_entity_pads_init(&ds90->sd.entity, DS90_NPORTS, ds90->pads);
+	if (err)
+		goto err_pads_init;
+
+	err = v4l2_async_register_subdev(&ds90->sd);
+	if (err) {
+		dev_err(dev, "v4l2_async_register_subdev error %d\n", err);
+		goto err_register_subdev;
+	}
+
+	/* Kick off */
+
+	if (client->irq) {
+		dev_info(dev, "using IRQ %d\n", client->irq);
+
+		err = devm_request_threaded_irq(dev, client->irq,
+						NULL, ds90_handle_events,
+						IRQF_ONESHOT, client->name,
+						ds90);
+		if (err) {
+			dev_err(dev, "Cannot enable IRQ (%d)\n", err);
+			goto err_irq;
+		}
+
+		/* Disable GPIO3 as input */
+		ds90_update_bits_shared(ds90, DS90_SR_GPIO_INPUT_CTL,
+					BIT(3), 0);
+		/* Enable GPIO3 as output, active low interrupt */
+		ds90_write_shared(ds90, DS90_SR_GPIO_PIN_CTL(3), 0xd1);
+
+		ds90_write_shared(ds90, DS90_SR_INTERRUPT_CTL,
+				  DS90_SR_INTERRUPT_CTL_ALL);
+	} else {
+		/* No IRQ, fallback to polling */
+
+		ds90->kthread = kthread_run(ds90_run, ds90, dev_name(dev));
+		if (IS_ERR(ds90->kthread)) {
+			err = PTR_ERR(ds90->kthread);
+			dev_err(dev, "Cannot create kthread (%d)\n", err);
+			goto err_kthread;
+		}
+		dev_info(dev, "using polling mode\n");
+	}
+
+	/* By default enable forwarding from both ports */
+	ds90_write_shared(ds90, DS90_SR_FWD_CTL1, 0x00);
+
+	dev_info(dev, "Successfully probed (rev/mask %02x)\n", rev_mask);
+
+	return 0;
+
+err_kthread:
+err_irq:
+	v4l2_async_unregister_subdev(&ds90->sd);
+err_register_subdev:
+	media_entity_cleanup(&ds90->sd.entity);
+err_pads_init:
+err_add_ctrls:
+	v4l2_ctrl_handler_free(&ds90->ctrl_handler);
+	ds90_remove_ports(ds90);
+err_parse_dt:
+	ds90_atr_remove(ds90);
+err_atr_probe:
+err_reg_read:
+	ds90_reset(ds90, true);
+	mutex_destroy(&ds90->alias_table_lock);
+	return err;
+}
+
+static int ds90_remove(struct i2c_client *client)
+{
+	struct ds90_data *ds90 = i2c_get_clientdata(client);
+
+	dev_info(&client->dev, "Removing\n");
+
+	if (ds90->kthread)
+		kthread_stop(ds90->kthread);
+	v4l2_async_unregister_subdev(&ds90->sd);
+	media_entity_cleanup(&ds90->sd.entity);
+	v4l2_ctrl_handler_free(&ds90->ctrl_handler);
+	ds90_remove_ports(ds90);
+	ds90_atr_remove(ds90);
+	ds90_reset(ds90, true);
+	mutex_destroy(&ds90->alias_table_lock);
+
+	return 0;
+}
+
+static const struct i2c_device_id ds90_id[] = {
+	{ "ds90ub954-q1", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ds90_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id ds90_dt_ids[] = {
+	{ .compatible = "ti,ds90ub954-q1", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ds90_dt_ids);
+#endif
+
+static struct i2c_driver ds90ub954_driver = {
+	.probe_new	= ds90_probe,
+	.remove		= ds90_remove,
+	.id_table	= ds90_id,
+	.driver = {
+		.name	= "ds90ub954",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(ds90_dt_ids),
+	},
+};
+
+module_i2c_driver(ds90ub954_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Texas Instruments DS90UB954-Q1 CSI-2 dual deserializer driver");
+MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>");
-- 
2.17.1

[RFC,v2 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer

[Luca Ceresoli]

Add a driver for the TI DS90UB953-Q1, a MIPI CSI-2 video serializer that
forwards a MIPI CSI-2 input video stream over an FPD Link 3 connection to a
remote deserializer. It also allows access to I2C and GPIO from the
deserializer.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes RFCv1 -> RFCv2: none, this patch is new in RFCv2
---
 drivers/media/i2c/Kconfig     |  10 +
 drivers/media/i2c/Makefile    |   1 +
 drivers/media/i2c/ds90ub953.c | 354 ++++++++++++++++++++++++++++++++++
 3 files changed, 365 insertions(+)
 create mode 100644 drivers/media/i2c/ds90ub953.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index e7088ccfd280..9db84ebdc060 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -569,6 +569,16 @@ config VIDEO_DS90UB954
 	  To compile this driver as a module, choose M here: the
 	  module will be called ds90ub954.
 
+config VIDEO_DS90UB953
+	tristate "TI DS90UB953-Q1 serializer"
+	help
+	  Device driver for the Texas Instruments "DS90UB953-Q1
+	  FPD-Link III 4.16-Gbps Serializer With CSI-2 Interface for
+	  2.3MP/60fps Cameras, RADAR, and Other Sensors".
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ds90ub953.
+
 comment "Camera sensor devices"
 
 config VIDEO_APTINA_PLL
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 66e52ededa4e..98c124c47be2 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -116,5 +116,6 @@ obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
 obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
 
 obj-$(CONFIG_VIDEO_DS90UB954)	+= ds90ub954.o
+obj-$(CONFIG_VIDEO_DS90UB953)	+= ds90ub953.o
 
 obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
new file mode 100644
index 000000000000..d88366f81a8d
--- /dev/null
+++ b/drivers/media/i2c/ds90ub953.c
@@ -0,0 +1,354 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Driver for the Texas Instruments DS90UB953-Q1 video serializer
+ *
+ * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/delay.h>
+#include <dt-bindings/media/ds90ub953.h>
+
+#define DS90_NUM_GPIOS			4  /* Physical GPIO pins */
+
+
+#define DS90_REG_DEVICE_ID		0x00
+
+#define DS90_REG_RESET_CTL		0x01
+#define DS90_REG_RESET_CTL_RESTART_AUTOLOAD	BIT(2)
+#define DS90_REG_RESET_CTL_DIGITAL_RESET_1	BIT(1)
+#define DS90_REG_RESET_CTL_DIGITAL_RESET_0	BIT(0)
+
+#define DS90_REG_GENERAL_CFG		0x02
+#define DS90_REG_MODE_SEL		0x03
+#define DS90_REG_BC_MODE_SELECT		0x04
+#define DS90_REG_PLLCLK_CTRL		0x05
+#define DS90_REG_CLKOUT_CTRL0		0x06
+#define DS90_REG_CLKOUT_CTRL1		0x07
+#define DS90_REG_BCC_WATCHDOG		0x08
+#define DS90_REG_I2C_CONTROL1		0x09
+#define DS90_REG_I2C_CONTROL2		0x0A
+#define DS90_REG_SCL_HIGH_TIME		0x0B
+#define DS90_REG_SCL_LOW_TIME		0x0C
+
+#define DS90_REG_LOCAL_GPIO_DATA	0x0D
+#define DS90_REG_LOCAL_GPIO_DATA_RMTEN(n)	BIT((n) + 4)
+#define DS90_REG_LOCAL_GPIO_DATA_OUT_SRC(n)	BIT((n) + 4)
+
+#define DS90_REG_GPIO_INPUT_CTRL	0x0E
+#define DS90_REG_GPIO_INPUT_CTRL_INPUT_EN(n)	BIT((n))
+#define DS90_REG_GPIO_INPUT_CTRL_OUT_EN(n)	BIT((n) + 4)
+
+#define DS90_REG_DVP_CFG		0x10
+#define DS90_REG_DVP_DT			0x11
+#define DS90_REG_FORCE_BIST_ERR		0x13
+#define DS90_REG_REMOTE_BIST_CTRL	0x14
+#define DS90_REG_SENSOR_VGAIN		0x15
+#define DS90_REG_SENSOR_CTRL0		0x17
+#define DS90_REG_SENSOR_CTRL1		0x18
+#define DS90_REG_SENSOR_V0_THRESH	0x19
+#define DS90_REG_SENSOR_V1_THRESH	0x1A
+#define DS90_REG_SENSOR_T_THRESH	0x1B
+#define DS90_REG_SENSOR_T_THRESH	0x1B
+#define DS90_REG_ALARM_CSI_EN		0x1C
+#define DS90_REG_ALARM_SENSE_EN		0x1D
+#define DS90_REG_ALARM_BC_EN		0x1E
+
+#define DS90_REG_CSI_POL_SEL		0x20
+#define DS90_REG_CSI_POL_SEL_POLARITY_CLK0	BIT(4)
+
+#define DS90_REG_CSI_LP_POLARITY	0x21
+#define DS90_REG_CSI_LP_POLARITY_POL_LP_CLK0	BIT(4)
+
+#define DS90_REG_CSI_EN_HSRX		0x22
+#define DS90_REG_CSI_EN_LPRX		0x23
+#define DS90_REG_CSI_EN_RXTERM		0x24
+#define DS90_REG_CSI_PKT_HDR_TINIT_CTRL 0x31
+#define DS90_REG_BCC_CONFIG		0x32
+#define DS90_REG_DATAPATH_CTL1		0x33
+#define DS90_REG_REMOTE_PAR_CAP1	0x35
+#define DS90_REG_DES_ID			0x37
+#define DS90_REG_SLAVE_ID_0		0x39
+#define DS90_REG_SLAVE_ID_1		0x3A
+#define DS90_REG_SLAVE_ID_2		0x3B
+#define DS90_REG_SLAVE_ID_3		0x3C
+#define DS90_REG_SLAVE_ID_4		0x3D
+#define DS90_REG_SLAVE_ID_5		0x3E
+#define DS90_REG_SLAVE_ID_6		0x3F
+#define DS90_REG_SLAVE_ID_7		0x40
+#define DS90_REG_SLAVE_ID_ALIAS_0	0x41
+#define DS90_REG_SLAVE_ID_ALIAS_0	0x41
+#define DS90_REG_SLAVE_ID_ALIAS_1	0x42
+#define DS90_REG_SLAVE_ID_ALIAS_2	0x43
+#define DS90_REG_SLAVE_ID_ALIAS_3	0x44
+#define DS90_REG_SLAVE_ID_ALIAS_4	0x45
+#define DS90_REG_SLAVE_ID_ALIAS_5	0x46
+#define DS90_REG_SLAVE_ID_ALIAS_6	0x47
+#define DS90_REG_SLAVE_ID_ALIAS_7	0x48
+#define DS90_REG_BC_CTRL		0x49
+#define DS90_REG_REV_MASK_ID		0x50
+
+#define DS90_REG_DEVICE_STS		0x51
+#define DS90_REG_DEVICE_STS_CFG_INIT_DONE	BIT(6)
+
+#define DS90_REG_GENERAL_STATUS		0x52
+#define DS90_REG_GPIO_PIN_STS		0x53
+#define DS90_REG_BIST_ERR_CNT		0x54
+#define DS90_REG_CRC_ERR_CNT1		0x55
+#define DS90_REG_CRC_ERR_CNT2		0x56
+#define DS90_REG_SENSOR_STATUS		0x57
+#define DS90_REG_SENSOR_V0		0x58
+#define DS90_REG_SENSOR_V1		0x59
+#define DS90_REG_SENSOR_T		0x5A
+#define DS90_REG_SENSOR_T		0x5A
+#define DS90_REG_CSI_ERR_CNT		0x5C
+#define DS90_REG_CSI_ERR_STATUS		0x5D
+#define DS90_REG_CSI_ERR_DLANE01	0x5E
+#define DS90_REG_CSI_ERR_DLANE23	0x5F
+#define DS90_REG_CSI_ERR_CLK_LANE	0x60
+#define DS90_REG_CSI_PKT_HDR_VC_ID	0x61
+#define DS90_REG_PKT_HDR_WC_LSB		0x62
+#define DS90_REG_PKT_HDR_WC_MSB		0x63
+#define DS90_REG_CSI_ECC		0x64
+#define DS90_REG_IND_ACC_CTL		0xB0
+#define DS90_REG_IND_ACC_ADDR		0xB1
+#define DS90_REG_IND_ACC_DATA		0xB2
+#define DS90_REG_FPD3_RX_ID0		0xF0
+#define DS90_REG_FPD3_RX_ID1		0xF1
+#define DS90_REG_FPD3_RX_ID2		0xF2
+#define DS90_REG_FPD3_RX_ID3		0xF3
+#define DS90_REG_FPD3_RX_ID4		0xF4
+#define DS90_REG_FPD3_RX_ID5		0xF5
+
+struct ds90_data {
+	struct i2c_client      *client;
+
+	u32 gpio_func[DS90_NUM_GPIOS];
+};
+
+/* -----------------------------------------------------------------------------
+ * Basic device access
+ */
+static s32 ds90_read(const struct ds90_data *ds90, u8 reg)
+{
+	s32 ret;
+
+	ret = i2c_smbus_read_byte_data(ds90->client, reg);
+	if (ret < 0)
+		dev_err(&ds90->client->dev, "Cannot read register 0x%02x!\n",
+			reg);
+
+	return ret;
+}
+
+static s32 ds90_write(const struct ds90_data *ds90, u8 reg, u8 val)
+{
+	s32 ret;
+
+	ret = i2c_smbus_write_byte_data(ds90->client, reg, val);
+	if (ret < 0)
+		dev_err(&ds90->client->dev, "Cannot write register 0x%02x!\n",
+			reg);
+
+	return ret;
+}
+
+/* -----------------------------------------------------------------------------
+ * GPIOs
+ */
+
+static int ds90_configure_gpios(struct ds90_data *ds90)
+{
+	struct device *dev = &ds90->client->dev;
+	u8 gpio_input_ctrl = 0;
+	u8 local_gpio_data = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ds90->gpio_func); i++) {
+
+		if (ds90->gpio_func[i] >= DS90_GPIO_N_FUNCS) {
+			dev_err(dev,
+				"Unknown gpio-functions value %u, GPIO%d will be unused",
+				ds90->gpio_func[i], i);
+			continue;
+		}
+
+		switch (ds90->gpio_func[i]) {
+		case DS90_GPIO_FUNC_INPUT:
+			gpio_input_ctrl |= DS90_REG_GPIO_INPUT_CTRL_INPUT_EN(i);
+			break;
+		case DS90_GPIO_FUNC_OUTPUT_REMOTE:
+			gpio_input_ctrl |= DS90_REG_GPIO_INPUT_CTRL_OUT_EN(i);
+			local_gpio_data |= DS90_REG_LOCAL_GPIO_DATA_RMTEN(i);
+			break;
+		}
+	}
+
+	ds90_write(ds90, DS90_REG_LOCAL_GPIO_DATA, local_gpio_data);
+	ds90_write(ds90, DS90_REG_GPIO_INPUT_CTRL, gpio_input_ctrl);
+	/* TODO setting DATAPATH_CTL1 is needed for inputs? */
+
+	return 0;
+}
+
+/* -----------------------------------------------------------------------------
+ * Core
+ */
+
+/*
+ * Reset via registers (useful from remote).
+ * Note: the procedure is undocumented, but this one seems to work.
+ */
+static void ds90_soft_reset(struct ds90_data *ds90)
+{
+	int retries = 10;
+	s32 ret;
+
+	while (retries-- > 0) {
+		ret = ds90_write(ds90, DS90_REG_RESET_CTL,
+				 DS90_REG_RESET_CTL_DIGITAL_RESET_1);
+		if (ret >= 0)
+			break;
+		usleep_range(1000, 3000);
+	}
+
+	retries = 10;
+	while (retries-- > 0) {
+		ret = ds90_read(ds90, DS90_REG_DEVICE_STS);
+		if (ret >= 0 && (ret & DS90_REG_DEVICE_STS_CFG_INIT_DONE))
+			break;
+		usleep_range(1000, 3000);
+	}
+}
+
+static int ds90_parse_dt(struct ds90_data *ds90)
+{
+	struct device_node *np = ds90->client->dev.of_node;
+	struct device *dev = &ds90->client->dev;
+	int err;
+
+	if (!np) {
+		dev_err(dev, "OF: no device tree node!\n");
+		return -ENOENT;
+	}
+
+	/* optional, if absent all GPIO pins are unused */
+	err = of_property_read_u32_array(np, "gpio-functions", ds90->gpio_func,
+					ARRAY_SIZE(ds90->gpio_func));
+	if (err && err != -EINVAL)
+		dev_err(dev, "DT: invalid gpio-functions property (%d)", err);
+
+	if (of_property_read_bool(np, "ti,ds90ub953-q1-clk-inv-pol-quirk")) {
+		ds90_write(ds90,
+			   DS90_REG_CSI_POL_SEL,
+			   DS90_REG_CSI_POL_SEL_POLARITY_CLK0);
+		ds90_write(ds90,
+			   DS90_REG_CSI_LP_POLARITY,
+			   DS90_REG_CSI_LP_POLARITY_POL_LP_CLK0);
+	}
+
+	return 0;
+}
+
+static int ds90_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct ds90_data *ds90;
+	s32 rev_mask;
+	int err;
+
+	dev_dbg(dev, "probing, addr 0x%02x\n", client->addr);
+
+	ds90 = devm_kzalloc(dev, sizeof(*ds90), GFP_KERNEL);
+	if (!ds90)
+		return -ENOMEM;
+
+	ds90->client = client;
+	i2c_set_clientdata(client, ds90);
+
+	ds90_soft_reset(ds90);
+
+	rev_mask = ds90_read(ds90, DS90_REG_REV_MASK_ID);
+	if (rev_mask < 0) {
+		err = rev_mask;
+		dev_err(dev,
+			"Cannot read first register (%d), abort\n", err);
+		goto err_reg_read;
+	}
+
+	err = ds90_parse_dt(ds90);
+	if (err)
+		goto err_parse_dt;
+
+	/* I2C fast mode 400 kHz */
+	/* TODO compute values from REFCLK */
+	ds90_write(ds90, DS90_REG_SCL_HIGH_TIME, 0x13);
+	ds90_write(ds90, DS90_REG_SCL_LOW_TIME,  0x26);
+
+	/*
+	 * TODO compute these hard-coded values
+	 *
+	 * SET CLK_OUT:
+	 * MODE    = 0 = CSI-2 sync (strap, reg 0x03) -> refclk from deser
+	 * REFCLK  = 23..26 MHz (REFCLK pin @ remote deserializer)
+	 * FC      = fwd channel data rate = 160 x REFCLK
+	 * CLK_OUT = FC * M / (HS_CLK_DIV * N)
+	 *         = FC * 1 / (4 * 20) = 2 * REFCLK
+	 */
+	ds90_write(ds90, DS90_REG_CLKOUT_CTRL0, 0x41); /* div by 4, M=1 */
+	ds90_write(ds90, DS90_REG_CLKOUT_CTRL1,   20); /* N=20 */
+
+	err = ds90_configure_gpios(ds90);
+	if (err)
+		goto err_configure_gpios;
+
+	dev_info(dev, "Successfully probed (rev/mask %02x)\n", rev_mask);
+
+	return 0;
+
+err_configure_gpios:
+err_parse_dt:
+err_reg_read:
+	return err;
+}
+
+static int ds90_remove(struct i2c_client *client)
+{
+	dev_info(&client->dev, "Removing\n");
+	return 0;
+}
+
+static const struct i2c_device_id ds90_id[] = {
+	{ "ds90ub953-q1", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ds90_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id ds90_dt_ids[] = {
+	{ .compatible = "ti,ds90ub953-q1", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ds90_dt_ids);
+#endif
+
+static struct i2c_driver ds90ub953_driver = {
+	.probe_new	= ds90_probe,
+	.remove		= ds90_remove,
+	.id_table	= ds90_id,
+	.driver = {
+		.name	= "ds90ub953",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(ds90_dt_ids),
+	},
+};
+
+module_i2c_driver(ds90ub953_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Texas Instruments DS90UB953-Q1 CSI-2 serializer driver");
+MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>");
-- 
2.17.1