Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [RFC,v2 0/6] TI camera serdes and I2C address translation
@ 2019-07-23 20:37 Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Luca Ceresoli @ 2019-07-23 20:37 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Luca Ceresoli, devicetree, linux-kernel, Mauro Carvalho Chehab,
	Rob Herring, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC,v2 1/6] i2c: core: let adapters be notified of client attach/detach
  2019-07-23 20:37 [RFC,v2 0/6] TI camera serdes and I2C address translation Luca Ceresoli
@ 2019-07-23 20:37 ` Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2019-07-23 20:37 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Luca Ceresoli, devicetree, linux-kernel, Mauro Carvalho Chehab,
	Rob Herring, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
  2019-07-23 20:37 [RFC,v2 0/6] TI camera serdes and I2C address translation Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
@ 2019-07-23 20:37 ` Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2019-07-23 20:37 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Luca Ceresoli, devicetree, linux-kernel, Mauro Carvalho Chehab,
	Rob Herring, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
  2019-07-23 20:37 [RFC,v2 0/6] TI camera serdes and I2C address translation Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
@ 2019-07-23 20:37 ` Luca Ceresoli
  2019-08-13 15:44   ` Rob Herring
  2019-07-23 20:37 ` [RFC,v2 4/6] media: dt-bindings: add DS90UB953-Q1 video serializer Luca Ceresoli
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Luca Ceresoli @ 2019-07-23 20:37 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Luca Ceresoli, devicetree, linux-kernel, Mauro Carvalho Chehab,
	Rob Herring, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC,v2 4/6] media: dt-bindings: add DS90UB953-Q1 video serializer
  2019-07-23 20:37 [RFC,v2 0/6] TI camera serdes and I2C address translation Luca Ceresoli
                   ` (2 preceding siblings ...)
  2019-07-23 20:37 ` [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
@ 2019-07-23 20:37 ` Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 5/6] media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer Luca Ceresoli
  5 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2019-07-23 20:37 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Luca Ceresoli, devicetree, linux-kernel, Mauro Carvalho Chehab,
	Rob Herring, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC,v2 5/6] media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer
  2019-07-23 20:37 [RFC,v2 0/6] TI camera serdes and I2C address translation Luca Ceresoli
                   ` (3 preceding siblings ...)
  2019-07-23 20:37 ` [RFC,v2 4/6] media: dt-bindings: add DS90UB953-Q1 video serializer Luca Ceresoli
@ 2019-07-23 20:37 ` Luca Ceresoli
  2019-07-23 20:37 ` [RFC,v2 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer Luca Ceresoli
  5 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2019-07-23 20:37 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Luca Ceresoli, devicetree, linux-kernel, Mauro Carvalho Chehab,
	Rob Herring, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC,v2 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer
  2019-07-23 20:37 [RFC,v2 0/6] TI camera serdes and I2C address translation Luca Ceresoli
                   ` (4 preceding siblings ...)
  2019-07-23 20:37 ` [RFC,v2 5/6] media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer Luca Ceresoli
@ 2019-07-23 20:37 ` Luca Ceresoli
  5 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2019-07-23 20:37 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Luca Ceresoli, devicetree, linux-kernel, Mauro Carvalho Chehab,
	Rob Herring, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
  2019-07-23 20:37 ` [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
@ 2019-08-13 15:44   ` Rob Herring
  2019-08-19 22:41     ` Luca Ceresoli
  0 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2019-08-13 15:44 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: linux-media, linux-i2c, devicetree, linux-kernel,
	Mauro Carvalho Chehab, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
  2019-08-13 15:44   ` Rob Herring
@ 2019-08-19 22:41     ` Luca Ceresoli
  2019-08-20 15:44       ` Rob Herring
  0 siblings, 1 reply; 11+ messages in thread
From: Luca Ceresoli @ 2019-08-19 22:41 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-media, linux-i2c, devicetree, linux-kernel,
	Mauro Carvalho Chehab, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
  2019-08-19 22:41     ` Luca Ceresoli
@ 2019-08-20 15:44       ` Rob Herring
  2019-08-21 21:50         ` Luca Ceresoli
  0 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2019-08-20 15:44 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Linux Media Mailing List, Linux I2C, devicetree, linux-kernel,
	Mauro Carvalho Chehab, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
  2019-08-20 15:44       ` Rob Herring
@ 2019-08-21 21:50         ` Luca Ceresoli
  0 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2019-08-21 21:50 UTC (permalink / raw)
  To: Rob Herring
  Cc: Linux Media Mailing List, Linux I2C, devicetree, linux-kernel,
	Mauro Carvalho Chehab, Mark Rutland, Wolfram Sang, Sakari Ailus,
	Hans Verkuil, Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy, Peter Rosin

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-23 20:37 [RFC,v2 0/6] TI camera serdes and I2C address translation Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
2019-08-13 15:44   ` Rob Herring
2019-08-19 22:41     ` Luca Ceresoli
2019-08-20 15:44       ` Rob Herring
2019-08-21 21:50         ` Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 4/6] media: dt-bindings: add DS90UB953-Q1 video serializer Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 5/6] media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer Luca Ceresoli

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox