* [RFCv3 0/6] Hi,
@ 2022-02-06 11:59 Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
` (7 more replies)
0 siblings, 8 replies; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-06 11:59 UTC (permalink / raw)
To: linux-media, linux-i2c
Cc: Luca Ceresoli, devicetree, linux-kernel, Rob Herring,
Mark Rutland, Wolfram Sang, Sakari Ailus, Hans Verkuil,
Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
Vladimir Zapolskiy, Peter Rosin, Mauro Carvalho Chehab,
Tomi Valkeinen, matti.vaittinen
this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
DS90UB9xx serializer/deserializer chipsets with I2C address translation.
I sent RFCv2 back in 2019 (!). After that I have applied most of the
improvements proposed during code review, most notably device tree
representation and proper use of kernel abstractions for clocks and GPIO. I
have also done many improvements all over the drivers code.
However I still don't consider these drivers "ready", hence the RFC status.
One reason is that, while the I2C ATR idea has been considered good by
Wolfram, its implementation requires I2C core changes that have been tried
but never made it to mainline. I think that discussion needs to be reopened
and work has to be done on that side. Thus for the time being this code
still has the alias pool: it is an interim solution until I2C core is
ready.
Also be aware that the only hardware where I sould test this code runs a
v4.19 kernel. I cannot guarantee it will work perfectly on mainline.
And since my hardware has only one camera connected to each deserializer I
dropped support. However I wrote the code to be able to easily add support
for 2 and 4 camera inputs as well as 2 CSI-2 outputs (DS90UB960).
Finally, I dropped all attempts at supporting hotplug. The goals I had 2+
years ago are not reasonably doable even with current kernels. Luckily all
the users that I talked with are happy without hotplug. For this reason I
simplified the serializer management in the DS90UB954 driver by keeping the
serializer always instantiated.
Even with the above limitations I felt I'd send this v3 anyway since
several people have contacted me since v2 asking whether this
implementation has made progress towards mainline. Some even improved on
top of my code it their own forks. As I cannot afford to work on this topic
in the near future, here is the latest and greatest version I can produce,
with all the improvements I made so far.
That's all, enjoy the code!
References:
[RFCv2] https://lore.kernel.org/lkml/20190723203723.11730-1-luca@lucaceresoli.net/
[RFCv1] https://lore.kernel.org/linux-media/20190108223953.9969-1-luca@lucaceresoli.net/
Best regards.
Luca
Luca Ceresoli (6):
i2c: core: let adapters be notified of client attach/detach
i2c: add I2C Address Translator (ATR) support
media: dt-bindings: add DS90UB953-Q1 video serializer
media: dt-bindings: add DS90UB954-Q1 video deserializer
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.yaml | 96 +
.../bindings/media/i2c/ti,ds90ub954-q1.yaml | 235 +++
MAINTAINERS | 22 +
drivers/i2c/Kconfig | 9 +
drivers/i2c/Makefile | 1 +
drivers/i2c/i2c-atr.c | 557 ++++++
drivers/i2c/i2c-core-base.c | 18 +-
drivers/media/i2c/Kconfig | 22 +
drivers/media/i2c/Makefile | 3 +
drivers/media/i2c/ds90ub953.c | 560 ++++++
drivers/media/i2c/ds90ub954.c | 1648 +++++++++++++++++
include/dt-bindings/media/ds90ub953.h | 16 +
include/linux/i2c-atr.h | 82 +
include/linux/i2c.h | 16 +
14 files changed, 3284 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
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.25.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFCv3 1/6] i2c: core: let adapters be notified of client attach/detach
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
@ 2022-02-06 11:59 ` Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
` (6 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-06 11:59 UTC (permalink / raw)
To: linux-media, linux-i2c
Cc: Luca Ceresoli, devicetree, linux-kernel, Rob Herring,
Mark Rutland, Wolfram Sang, Sakari Ailus, Hans Verkuil,
Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
Vladimir Zapolskiy, Peter Rosin, Mauro Carvalho Chehab,
Tomi Valkeinen, matti.vaittinen
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 RFCv2 -> RFCv3:
- rebase on current master
Changes RFCv1 -> RFCv2:
- Document struct i2c_attach_operations
---
drivers/i2c/i2c-core-base.c | 18 +++++++++++++++++-
include/linux/i2c.h | 16 ++++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 2c59dd748a49..aea1e1b552b0 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -966,15 +966,23 @@ 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 out_remove_swnode;
+
status = device_register(&client->dev);
if (status)
- goto out_remove_swnode;
+ goto out_detach_client;
dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
client->name, dev_name(&client->dev));
return client;
+out_detach_client:
+ if (adap->attach_ops && adap->attach_ops->detach_client)
+ adap->attach_ops->detach_client(adap, client);
out_remove_swnode:
device_remove_software_node(&client->dev);
out_err_put_of_node:
@@ -996,9 +1004,17 @@ EXPORT_SYMBOL_GPL(i2c_new_client_device);
*/
void i2c_unregister_device(struct i2c_client *client)
{
+ struct i2c_adapter *adap;
+
if (IS_ERR_OR_NULL(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 7d4f52ceb7b5..aadd71e0533c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -587,6 +587,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
@@ -729,6 +744,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.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
@ 2022-02-06 11:59 ` Luca Ceresoli
2022-02-08 11:16 ` Andy Shevchenko
2022-03-16 14:11 ` Vaittinen, Matti
2022-02-06 11:59 ` [RFCv3 3/6] media: dt-bindings: add DS90UB953-Q1 video serializer Luca Ceresoli
` (5 subsequent siblings)
7 siblings, 2 replies; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-06 11:59 UTC (permalink / raw)
To: linux-media, linux-i2c
Cc: Luca Ceresoli, devicetree, linux-kernel, Rob Herring,
Mark Rutland, Wolfram Sang, Sakari Ailus, Hans Verkuil,
Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
Vladimir Zapolskiy, Peter Rosin, Mauro Carvalho Chehab,
Tomi Valkeinen, matti.vaittinen
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 RFCv2 -> v3:
- fix const-related warnings
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]
---
MAINTAINERS | 7 +
drivers/i2c/Kconfig | 9 +
drivers/i2c/Makefile | 1 +
drivers/i2c/i2c-atr.c | 557 ++++++++++++++++++++++++++++++++++++++++
include/linux/i2c-atr.h | 82 ++++++
5 files changed, 656 insertions(+)
create mode 100644 drivers/i2c/i2c-atr.c
create mode 100644 include/linux/i2c-atr.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 69a2935daf6c..7383aec87e4a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8961,6 +8961,13 @@ L: linux-acpi@vger.kernel.org
S: Maintained
F: drivers/i2c/i2c-core-acpi.c
+I2C ADDRESS TRANSLATOR (ATR)
+M: Luca Ceresoli <luca@lucaceresoli.net>
+L: linux-i2c@vger.kernel.org
+S: Maintained
+F: drivers/i2c/i2c-atr.c
+F: include/linux/i2c-atr.h
+
I2C CONTROLLER DRIVER FOR NVIDIA GPU
M: Ajay Gupta <ajayg@nvidia.com>
L: linux-i2c@vger.kernel.org
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index 438905e2a1d0..c6d1a345ea6d 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 c1d493dc9bac..3f71ce4711e3 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..f44a5696f7d8
--- /dev/null
+++ b/drivers/i2c/i2c-atr.c
@@ -0,0 +1,557 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * 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(const struct list_head *list,
+ const 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(const 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", ®);
+ 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.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFCv3 3/6] media: dt-bindings: add DS90UB953-Q1 video serializer
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
@ 2022-02-06 11:59 ` Luca Ceresoli
2022-02-07 21:48 ` Rob Herring
2022-02-06 11:59 ` [RFCv3 4/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
` (4 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-06 11:59 UTC (permalink / raw)
To: linux-media, linux-i2c
Cc: Luca Ceresoli, devicetree, linux-kernel, Rob Herring,
Mark Rutland, Wolfram Sang, Sakari Ailus, Hans Verkuil,
Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
Vladimir Zapolskiy, Peter Rosin, Mauro Carvalho Chehab,
Tomi Valkeinen, matti.vaittinen
Describe the Texas Instruments DS90UB953-Q1, a MIPI CSI-2 video serializer
with I2C Address Translator and remote GPIOs.
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
Changes RFCv2 -> RFCv3:
- rewrite in yaml
Changes RFCv1 -> RFCv2: none, this patch is new in RFCv2
---
.../bindings/media/i2c/ti,ds90ub953-q1.yaml | 96 +++++++++++++++++++
MAINTAINERS | 7 ++
include/dt-bindings/media/ds90ub953.h | 16 ++++
3 files changed, 119 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
create mode 100644 include/dt-bindings/media/ds90ub953.h
diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
new file mode 100644
index 000000000000..2a836a3e65e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2019 Renesas Electronics Corp.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub953-q1.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments DS90UB953-Q1 video serializer
+
+maintainers:
+ - Luca Ceresoli <luca@lucaceresoli.net>
+
+description: |
+ 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/remote-chips/remote-chip@[01]" node.
+
+properties:
+ compatible:
+ const: ti,ds90ub953-q1
+
+ reg:
+ description: |
+ Index of the remote (serializer) RX port that this serializer is
+ connected to.
+ maxItems: 1
+
+ clocks:
+ description: FPD-Link line rate (provided by the deserializer)
+ maxItems: 1
+
+ gpio-controller: true
+
+ '#gpio-cells':
+ const: 2
+
+ '#clock-cells':
+ const: 0
+
+ ti,gpio-functions:
+ description: |
+ 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 unused
+ - DS90_GPIO_FUNC_INPUT (1): the GPIO is an input to the ds90ub953,
+ the remote chip (deserializer) can read its value
+ - DS90_GPIO_FUNC_OUTPUT_REMOTE (2): the GPIO is an output from the
+ ds90ub953, the remote chip (deserializer) can set its value
+ For unspecified values the GPIO is assumed to be unused.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ maxItems: 4
+
+patternProperties:
+ '^ti,ds90ub953-q1-(clk|d[0-3])-inv-pol-quirk$':
+ description: |
+ The MIPI CSI-2 input clock lane or any of the data lanes has inverted
+ polarity in hardware
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - gpio-controller
+ - '#gpio-cells'
+ - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/media/ds90ub953.h>
+ remote-chips {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ remote-chip@0 {
+ reg = <0>;
+ compatible = "ti,ds90ub953-q1";
+ clocks = <&deser>;
+ ti,gpio-functions =
+ <DS90_GPIO_FUNC_UNUSED
+ DS90_GPIO_FUNC_OUTPUT_REMOTE
+ DS90_GPIO_FUNC_UNUSED
+ DS90_GPIO_FUNC_UNUSED>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ #clock-cells = <0>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 7383aec87e4a..4429ce035496 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19090,6 +19090,13 @@ F: include/linux/dma/k3-udma-glue.h
F: include/linux/dma/ti-cppi5.h
F: include/linux/dma/k3-psil.h
+TEXAS INSTRUMENTS DS90UB953 VIDEO SERIALIZER DRIVER
+M: Luca Ceresoli <luca@lucaceresoli.net>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
+F: include/dt-bindings/media/ds90ub953.h
+
TEXAS INSTRUMENTS' SYSTEM CONTROL INTERFACE (TISCI) PROTOCOL DRIVER
M: Nishanth Menon <nm@ti.com>
M: Tero Kristo <kristo@kernel.org>
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.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFCv3 4/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
` (2 preceding siblings ...)
2022-02-06 11:59 ` [RFCv3 3/6] media: dt-bindings: add DS90UB953-Q1 video serializer Luca Ceresoli
@ 2022-02-06 11:59 ` Luca Ceresoli
2022-02-06 18:46 ` Rob Herring
2022-02-07 19:39 ` Rob Herring
2022-02-06 11:59 ` [RFCv3 5/6] media: ds90ub954: new driver for TI " Luca Ceresoli
` (3 subsequent siblings)
7 siblings, 2 replies; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-06 11:59 UTC (permalink / raw)
To: linux-media, linux-i2c
Cc: Luca Ceresoli, devicetree, linux-kernel, Rob Herring,
Mark Rutland, Wolfram Sang, Sakari Ailus, Hans Verkuil,
Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
Vladimir Zapolskiy, Peter Rosin, Mauro Carvalho Chehab,
Tomi Valkeinen, matti.vaittinen
Describe the Texas Instruments DS90UB954-Q1, a 2-input MIPI CSI-2 video
deserializer with I2C Address Translator and remote GPIOs.
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
Changes RFCv2 -> RFCv3:
- rewrite in yaml
- use new layout based on remote-chips under the main deser node
- new clock configuration based on common clock framework
Changes RFCv1 -> RFCv2:
- add explicit aliases for the FPD-link RX ports (optional)
- add proper remote GPIO description
---
.../bindings/media/i2c/ti,ds90ub954-q1.yaml | 235 ++++++++++++++++++
MAINTAINERS | 6 +
2 files changed, 241 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
new file mode 100644
index 000000000000..95dc3d22f5d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
@@ -0,0 +1,235 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2019 Renesas Electronics Corp.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub954-q1.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments DS90UB954-Q1 dual video deserializer
+
+maintainers:
+ - Luca Ceresoli <luca@lucaceresoli.net>
+
+description: |
+ 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.
+
+properties:
+ compatible:
+ const: ti,ds90ub954-q1
+
+ reg:
+ description: |
+ 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".
+
+ reg-names:
+ description: |
+ Names of I2C address used to communicate with the chip, must match
+ the "reg" values; mandatory if there are 2 or more addresses.
+ "main" is the main I2C address, used to access shared registers.
+ "rxport0" and "rxport1" are the I2C alias to access FPD-link RX
+ port specific registers; must not be used by other slaves on the
+ same bus. "ser0" and "ser1" are the 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.
+ minItems: 1
+ maxItems: 5
+ items:
+ - const: main
+ - const: rxport0
+ - const: rxport1
+ - const: ser0
+ - const: ser1
+
+ clocks:
+ description: provider of the clock on the XIN/REFCLK pin
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ description: chip reset GPIO connected to PDB pin (active low)
+
+ i2c-alias-pool:
+ description: |
+ 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 remote I2C chips
+
+ '#clock-cells':
+ description: |
+ the DS90UB954 provides the FPD line rate clock to the serializer
+ const: 0
+
+ ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+
+ patternProperties:
+ '^port@[01]$':
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ description: FPD-Link RX port 0 (RIN0+/RIN0- pins)
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ '^port@2$':
+ $ref: /schemas/graph.yaml#/properties/port
+ description: MIPI-CSI2 TX port
+
+ remote-chips:
+ type: object
+
+ properties:
+ '#address-cells':
+ const: 1
+ '#size-cells':
+ const: 0
+
+ patternProperties:
+ '^remote-chip@([01]+)$':
+ type: object
+ $ref: /schemas/media/i2c/ti,ds90ub953-q1.yaml#
+
+ required:
+ - '#address-cells'
+ - '#size-cells'
+
+ additionalProperties: false
+
+ i2c-atr:
+ description: |
+ Each child describes the I2C bus on the remote side of an RX port
+ type: object
+
+ properties:
+ '#address-cells':
+ const: 1
+ '#size-cells':
+ const: 0
+
+ patternProperties:
+ '^i2c@([01]+)$':
+ type: object
+
+ properties:
+ reg:
+ maxItems: 1
+ '#address-cells':
+ const: 1
+ '#size-cells':
+ const: 0
+ clock-frequency:
+ minimum: 1
+ maximum: 1000000
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - ports
+ - remote-chips
+ - i2c-atr
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/media/ds90ub953.h>
+
+ i2c@0 {
+ reg = <0x0 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ 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_LOW>;
+ reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
+
+ #clock-cells = <0>;
+
+ i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ ds90ub954_fpd3_in0: endpoint {
+ remote-endpoint = <&sensor_0_out>;
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+ ds90ub954_mipi_out0: endpoint {
+ data-lanes = <1 2 3 4>;
+ link-frequencies = /bits/ 64 <400000000>;
+ remote-endpoint = <&csirx_0_in>;
+ };
+ };
+ };
+
+ remote-chips {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ des0_ser0: remote-chip@0 {
+ reg = <0>;
+ compatible = "ti,ds90ub953-q1";
+ clocks = <&deser>;
+ ti,gpio-functions =
+ <DS90_GPIO_FUNC_UNUSED
+ DS90_GPIO_FUNC_OUTPUT_REMOTE
+ DS90_GPIO_FUNC_UNUSED
+ DS90_GPIO_FUNC_UNUSED>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ #clock-cells = <0>;
+ };
+ };
+
+ i2c-atr {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ remote_i2c0: i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor_0@1a {
+ compatible = "sony,imx274";
+ reg = <0x1a>;
+
+ reset-gpios = <&des0_ser0 1 GPIO_ACTIVE_LOW>;
+
+ port {
+ sensor_0_out: endpoint {
+ remote-endpoint = <&ds90ub954_fpd3_in0>;
+ };
+ };
+ };
+ };
+ };
+ };
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 4429ce035496..f0156062f788 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19097,6 +19097,12 @@ S: Maintained
F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
F: include/dt-bindings/media/ds90ub953.h
+TEXAS INSTRUMENTS DS90UB954 VIDEO DESERIALIZER DRIVER
+M: Luca Ceresoli <luca@lucaceresoli.net>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
+
TEXAS INSTRUMENTS' SYSTEM CONTROL INTERFACE (TISCI) PROTOCOL DRIVER
M: Nishanth Menon <nm@ti.com>
M: Tero Kristo <kristo@kernel.org>
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFCv3 5/6] media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
` (3 preceding siblings ...)
2022-02-06 11:59 ` [RFCv3 4/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
@ 2022-02-06 11:59 ` Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer Luca Ceresoli
` (2 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-06 11:59 UTC (permalink / raw)
To: linux-media, linux-i2c
Cc: Luca Ceresoli, devicetree, linux-kernel, Rob Herring,
Mark Rutland, Wolfram Sang, Sakari Ailus, Hans Verkuil,
Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
Vladimir Zapolskiy, Peter Rosin, Mauro Carvalho Chehab,
Tomi Valkeinen, matti.vaittinen
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 RFCv2 -> RFCv3:
- update to new internal I2C APIs:
err_new_secondary_device -> err_new_ancillary_device
i2c_new_device -> i2c_new_client_device
- update to new internal V4L2 APIs (get_fmt)
- handle link parity errors and fix IRQ flood
- add debug messages while parsing ports
- drop code related to 2nd rx port, we support one port only
- keep the serializer always instantiated
- add link to serializer under each rx port sysfs dir
- implement clocks using common clock framework, expose the FPD line rate
as a clock
- allocate CSI tx port dynamically (coherently with FPD RX ports, useful
for future ds90ub960 support)
- make lock/unlock dectection more robust
- new DT layout: add remote-chips for serializer description
- instantiate a remote GPIO controller per each port
- improve logging and error reporting
- switch to link-frequencies, compute best speed select based on REFCLK
- fix kernel-doc syntax errors
- misc improvements
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
---
MAINTAINERS | 1 +
drivers/media/i2c/Kconfig | 12 +
drivers/media/i2c/Makefile | 2 +
drivers/media/i2c/ds90ub954.c | 1648 +++++++++++++++++++++++++++++++++
4 files changed, 1663 insertions(+)
create mode 100644 drivers/media/i2c/ds90ub954.c
diff --git a/MAINTAINERS b/MAINTAINERS
index f0156062f788..5aeb557ab5da 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19102,6 +19102,7 @@ M: Luca Ceresoli <luca@lucaceresoli.net>
L: linux-media@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
+F: drivers/media/i2c/ds90ub954.c
TEXAS INSTRUMENTS' SYSTEM CONTROL INTERFACE (TISCI) PROTOCOL DRIVER
M: Nishanth Menon <nm@ti.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 69c56e24a612..9a02444bd647 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -480,6 +480,18 @@ config VIDEO_MAX9286
To compile this driver as a module, choose M here: the
module will be called max9286.
+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 "Video and audio decoders"
config VIDEO_SAA717X
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index b01f6cd05ee8..84e9ad00236a 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -135,5 +135,7 @@ obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
obj-$(CONFIG_VIDEO_MAX9271_LIB) += max9271.o
obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
+obj-$(CONFIG_VIDEO_DS90UB954) += ds90ub954.o
obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.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..649480b8861c
--- /dev/null
+++ b/drivers/media/i2c/ds90ub954.c
@@ -0,0 +1,1648 @@
+// 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/clk-provider.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>
+#include <media/v4l2-fwnode.h>
+
+/* TODO add support for 2nd FPD-link RX port */
+#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;
+ u64 fpd3_parity_errors;
+
+ struct i2c_client *reg_client; /* for per-port local registers */
+ struct i2c_client *ser_client; /* remote serializer */
+ unsigned short ser_alias; /* ser i2c alias (lower 7 bits) */
+ bool locked;
+
+ struct gpio_chip gpio_chip;
+ char gpio_chip_name[64];
+
+ struct ds90_data *ds90;
+ unsigned short nport; /* RX port number, and index in ds90->rxport[] */
+};
+
+struct ds90_csitxport {
+ unsigned int speed_select; /* CSI_TX_SPEED */
+};
+
+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[DS90_CSI_TX_NPORTS];
+
+ struct v4l2_subdev sd;
+ struct media_pad pads[DS90_NPORTS];
+ struct v4l2_mbus_framefmt fmt[DS90_NPORTS];
+ struct v4l2_ctrl_handler ctrl_handler;
+
+ struct clk *refclk;
+ struct clk_hw *line_clk_hw;
+
+ /* 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), ®val);
+ 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), ®val);
+ 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,
+ unsigned int nport,
+ struct device_node *np)
+{
+ struct device *dev = &ds90->client->dev;
+ struct ds90_csitxport *csitxport;
+ struct v4l2_fwnode_endpoint vep;
+ unsigned long f_parent = clk_get_rate(ds90->refclk);
+ unsigned long wanted_bpsl, actual_bpsl, mul; // bpsl = bit/second/lane
+ unsigned int speed_select;
+ int err;
+
+ csitxport = devm_kzalloc(dev, sizeof(*csitxport), GFP_KERNEL);
+ if (!csitxport)
+ return -ENOMEM;
+
+ err = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(np), &vep);
+ if (err)
+ return err;
+
+ if (vep.nr_of_link_frequencies != 1) {
+ v4l2_fwnode_endpoint_free(&vep);
+ return dev_err_probe(dev, -EINVAL,
+ "OF: %pOF: missing or too many link-frequencies\n", np);
+ }
+
+ /* CSI-2 is DDR */
+ wanted_bpsl = vep.link_frequencies[0] * 2;
+
+ // CSI-2 tx bps/lane = REFCLK * {16, 32 or 64} (see CSI_TX_SPEED in datasheet).
+ // Find the multiplier that gets the closest bps/lane.
+ mul = wanted_bpsl / f_parent;
+ if (mul < 24) {
+ mul = 16;
+ speed_select = 3;
+ } else if (mul < 48) {
+ mul = 32;
+ speed_select = 2;
+ } else {
+ mul = 64;
+ speed_select = 0;
+ }
+
+ actual_bpsl = f_parent * mul;
+
+ if (actual_bpsl != wanted_bpsl)
+ dev_warn(dev, "CSI-2 data rate: %lu bps/lane (wanted %lu)",
+ actual_bpsl, wanted_bpsl);
+
+ csitxport->speed_select = speed_select;
+ ds90->csitxport[nport] = csitxport;
+
+ v4l2_fwnode_endpoint_free(&vep);
+
+ 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);
+ int nport = rxport->nport;
+
+ unsigned int reg_addr = DS90_RR_BC_GPIO_CTL(offset / 2);
+ unsigned int reg_shift = (offset % 2) * 4;
+
+ ds90_update_bits_rxport(rxport->ds90, 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_gpiochip_probe(struct ds90_data *ds90, int nport,
+ struct device_node *rchip_np)
+{
+ 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:%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_node = rchip_np;
+ gc->of_gpio_n_cells = 2;
+
+ err = gpiochip_add_data(gc, rxport);
+ if (err)
+ return dev_err_probe(dev, err, "Failed adding gpiochip\n");
+
+ return 0;
+}
+
+static void ds90_gpiochip_remove(struct ds90_data *ds90, int nport)
+{
+ struct gpio_chip *gc = &ds90->rxport[nport]->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"
+ "fpd3_parity_errors = %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,
+ rxport->fpd3_parity_errors);
+}
+
+struct attribute_group ds90_rxport_attr_group[] = {
+ { .name = "rx0", .attrs = ds90_rxport0_attrs },
+ { .name = "rx1", .attrs = ds90_rxport1_attrs },
+};
+
+/*
+ * Instantiate remote serializer.
+ *
+ * @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 device_node *rchip_np)
+{
+ struct ds90_rxport *rxport = ds90->rxport[nport];
+ struct device *dev = &ds90->client->dev;
+ struct i2c_board_info ser_info = { .type = "ds90ub953-q1",
+ .addr = rxport->ser_alias,
+ .of_node = rchip_np };
+
+ /*
+ * 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.
+ */
+ rxport->ser_client = i2c_new_client_device(ds90->client->adapter, &ser_info);
+ if (!i2c_client_has_driver(rxport->ser_client)) {
+ rxport->ser_client = NULL;
+ return dev_err_probe(dev, PTR_ERR(rxport->ser_client),
+ "rx%d: cannot add %s i2c device",
+ nport, ser_info.type);
+ }
+
+ dev_info(dev, "rx%d: remote serializer at alias 0x%02x\n",
+ nport, rxport->ser_client->addr);
+
+ WARN(sysfs_add_link_to_group(&dev->kobj, ds90_rxport_attr_group[nport].name,
+ &rxport->ser_client->dev.kobj, "serializer"),
+ "rx%d: can't create ser symlink\n", nport);
+
+ return 0;
+}
+
+static void ds90_rxport_remove_serializer(struct ds90_data *ds90, int nport)
+{
+ struct device *dev = &ds90->client->dev;
+ struct ds90_rxport *rxport = ds90->rxport[nport];
+
+ if (rxport->ser_client) {
+ sysfs_remove_link_from_group(&dev->kobj,
+ ds90_rxport_attr_group[nport].name,
+ "serializer");
+ 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,
+ unsigned int nport,
+ struct device_node *endpoint_np,
+ struct device_node *rchip_np)
+{
+ static const struct ds90_rxport_info rxport_info[DS90_FPD_RX_NPORTS] = {
+ { "rxport0", "ser0", 0x40, 0x50 },
+ };
+
+ struct device *dev = &ds90->client->dev;
+ struct ds90_rxport *rxport;
+ int err;
+ const struct ds90_rxport_info *info = &rxport_info[nport];
+
+ if (ds90->rxport[nport])
+ return dev_err_probe(dev, -EADDRINUSE,
+ "OF: %pOF: duplicated reg value %d\n",
+ endpoint_np, nport);
+
+ rxport = devm_kzalloc(dev, sizeof(*rxport), GFP_KERNEL);
+ if (!rxport)
+ return -ENOMEM;
+
+ ds90->rxport[nport] = rxport;
+
+ rxport->nport = nport;
+ rxport->ds90 = ds90;
+ rxport->ser_alias = ds90_rxport_get_remote_alias(ds90, info);
+
+ /* Initialize access to local registers */
+ rxport->reg_client = i2c_new_ancillary_device(ds90->client,
+ info->local_name,
+ info->local_def_alias);
+ if (IS_ERR(rxport->reg_client)) {
+ err = PTR_ERR(rxport->reg_client);
+ goto err_new_ancillary_device;
+ }
+ ds90_write_shared(ds90, DS90_SR_I2C_RX_ID(nport),
+ rxport->reg_client->addr << 1);
+
+ /* 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 I2C 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 = ds90_rxport_add_serializer(ds90, nport, rchip_np);
+ if (err)
+ goto err_add_serializer;
+
+ err = ds90_gpiochip_probe(ds90, nport, rchip_np);
+ if (err)
+ goto err_gpiochip_probe;
+
+ 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:
+ ds90_gpiochip_remove(ds90, nport);
+err_gpiochip_probe:
+ ds90_rxport_remove_serializer(ds90, nport);
+err_add_serializer:
+ sysfs_remove_group(&dev->kobj, &ds90_rxport_attr_group[nport]);
+err_sysfs:
+ i2c_unregister_device(rxport->reg_client);
+err_new_ancillary_device:
+ 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_gpiochip_remove(ds90, nport);
+ ds90_rxport_remove_serializer(ds90, nport);
+ i2c_unregister_device(rxport->reg_client);
+ sysfs_remove_group(&dev->kobj, &ds90_rxport_attr_group[nport]);
+}
+
+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++;
+
+ if (rx_port_sts1 & DS90_RR_RX_PORT_STS1_PARITY_ERROR) {
+ u8 cnt_hi;
+ u8 cnt_lo;
+ int err_hi;
+ int err_lo;
+
+ err_hi = ds90_read_rxport(ds90, nport,
+ DS90_RR_RX_PAR_ERR_HI, &cnt_hi);
+ err_lo = ds90_read_rxport(ds90, nport,
+ DS90_RR_RX_PAR_ERR_LO, &cnt_lo);
+
+ if (!err_hi && !err_lo)
+ rxport->fpd3_parity_errors += (cnt_hi << 8) + cnt_lo;
+ }
+
+ /* Update locked status */
+ locked = rx_port_sts1 & DS90_RR_RX_PORT_STS1_LOCK_STS;
+ if (rx_port_sts1 & DS90_RR_RX_PORT_STS1_LOCK_STS_CHG ||
+ locked != rxport->locked) {
+ dev_info(dev, "rx%d: %sLOCKED\n", nport, locked ? "" : "NOT ");
+ rxport->locked = locked;
+ sysfs_notify(&dev->kobj, ds90_rxport_attr_group[nport].name, "locked");
+ }
+}
+
+/* -----------------------------------------------------------------------------
+ * V4L2
+ */
+
+static void ds90_set_tpg(struct ds90_data *ds90, int tpg_num)
+{
+ 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);
+ } 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);
+ 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;
+
+ if (enable) {
+ /*
+ * From the datasheet: "initial CSI Skew-Calibration
+ * sequence [...] should be set when operating at 1.6 Gbps"
+ */
+ if (ds90->csitxport[0]->speed_select == 0) // Only port 0 implemented currently
+ csi_ctl |= DS90_TR_CSI_CTL_CSI_CAL_EN;
+
+ ds90_write_shared(ds90, DS90_SR_CSI_PLL_CTL, ds90->csitxport[0]->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_state *state,
+ unsigned int pad, u32 which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_format(&ds90->sd, state, 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_state *state,
+ 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, state, 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. */
+}
+
+/*
+ * Return node of remote chip with a given reg.
+ *
+ * Returns a node pointer if found, with refcount incremented, use
+ * of_node_put() on it when done. Returns NULL if node is not found.
+ */
+static struct device_node *ds90_get_rchip_node(struct ds90_data *ds90, int nport)
+{
+ struct device *dev = &ds90->client->dev;
+ struct device_node *rchips;
+ struct device_node *child;
+ u32 reg;
+ int err;
+
+ rchips = of_get_child_by_name(dev->of_node, "remote-chips");
+
+ for_each_child_of_node(rchips, child) {
+ err = of_property_read_u32(child, "reg", ®);
+ if (err)
+ continue;
+ if (nport == reg)
+ break;
+ }
+
+ of_node_put(rchips);
+ return child;
+}
+
+static int ds90_register_clocks(struct ds90_data *ds90)
+{
+ struct device *dev = &ds90->client->dev;
+ const char *name;
+ int err;
+
+ /* Get our input clock (REFCLK, 23..26 MHz) */
+
+ ds90->refclk = devm_clk_get(dev, NULL);
+ if (IS_ERR(ds90->refclk))
+ return dev_err_probe(dev, PTR_ERR(ds90->refclk), "Cannot get REFCLK");
+
+ dev_dbg(dev, "REFCLK: %10lu Hz\n", clk_get_rate(ds90->refclk));
+
+ /* Provide FPD-Link III line rate (160 * REFCLK in Synchronous mode) */
+
+ name = kasprintf(GFP_KERNEL, "%s.fpd_line_rate", dev_name(dev));
+ ds90->line_clk_hw =
+ devm_clk_hw_register_fixed_factor(dev, name,
+ __clk_get_name(ds90->refclk),
+ 0, 160, 1);
+ kfree(name);
+ if (IS_ERR(ds90->line_clk_hw))
+ return dev_err_probe(dev, PTR_ERR(ds90->line_clk_hw),
+ "Cannot register clock HW\n");
+
+ dev_dbg(dev, "line rate: %10lu Hz\n", clk_hw_get_rate(ds90->line_clk_hw));
+
+ /* Expose the line rate to OF */
+
+ err = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, ds90->line_clk_hw);
+ if (err)
+ return dev_err_probe(dev, err, "Cannot add OF clock provider\n");
+
+ return 0;
+}
+
+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)
+ return dev_err_probe(dev, -ENOENT, "OF: no device tree node!\n");
+
+ 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);
+
+ dev_dbg(dev, "parsing port %d ep %d: %pOF",
+ ep.port, ep.id, ep.local_node);
+
+ if (ep.port >= DS90_NPORTS || ep.id != 0)
+ return dev_err_probe(dev, -EINVAL,
+ "OF: %pOF: missing or invalid port/endpoint number\n",
+ np);
+
+ if (ep.port < DS90_FPD_RX_NPORTS) {
+ struct device_node *rc_np = ds90_get_rchip_node(ds90, ep.port);
+
+ if (!rc_np) {
+ dev_err(dev, "OF: rx%d: missing remote chip", ep.port);
+ of_node_put(rc_np);
+ err = -EINVAL;
+ break;
+ }
+ err = ds90_rxport_probe_one(ds90, ep.port, ep_np, rc_np);
+ of_node_put(rc_np);
+ } else {
+ err = ds90_csiport_probe_one(ds90, ep.port - DS90_FPD_RX_NPORTS, 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;
+ 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))
+ return dev_err_probe(dev, PTR_ERR(ds90->reset_gpio),
+ "Cannot get reset GPIO");
+
+ ds90_reset(ds90, false);
+
+ err = ds90_register_clocks(ds90);
+ if (err)
+ goto err_register_clocks;
+
+ err = clk_prepare_enable(ds90->refclk);
+ if (err) {
+ dev_err(dev, "Cannot prepare and enable REFCLK (%d)", err);
+ goto err_clk_prepare_enable;
+ }
+
+ /* 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:
+err_clk_prepare_enable:
+err_register_clocks:
+ 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.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFCv3 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
` (4 preceding siblings ...)
2022-02-06 11:59 ` [RFCv3 5/6] media: ds90ub954: new driver for TI " Luca Ceresoli
@ 2022-02-06 11:59 ` Luca Ceresoli
2022-02-06 12:05 ` [RFCv3 0/6] TI camera serdes and I2C address translation Luca Ceresoli
2022-02-07 12:06 ` [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,) Tomi Valkeinen
7 siblings, 0 replies; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-06 11:59 UTC (permalink / raw)
To: linux-media, linux-i2c
Cc: Luca Ceresoli, devicetree, linux-kernel, Rob Herring,
Mark Rutland, Wolfram Sang, Sakari Ailus, Hans Verkuil,
Laurent Pinchart, Kieran Bingham, Jacopo Mondi,
Vladimir Zapolskiy, Peter Rosin, Mauro Carvalho Chehab,
Tomi Valkeinen, matti.vaittinen
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 RFCv2 -> v3:
- expose error counters in sysfs
- avoid spurions "Cannot read register" during reset
- add sysfs attribute for error status
- add vendor prefix to DT property gpio-functions
- use common clock framework, expose CLK_OUT as a clock
- code reorganization and cleanups
Changes RFCv1 -> RFCv2: none, this patch is new in RFCv2
---
MAINTAINERS | 1 +
drivers/media/i2c/Kconfig | 10 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/ds90ub953.c | 560 ++++++++++++++++++++++++++++++++++
4 files changed, 572 insertions(+)
create mode 100644 drivers/media/i2c/ds90ub953.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 5aeb557ab5da..85fc43ff22d9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19095,6 +19095,7 @@ M: Luca Ceresoli <luca@lucaceresoli.net>
L: linux-media@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
+F: drivers/media/i2c/ds90ub953.c
F: include/dt-bindings/media/ds90ub953.h
TEXAS INSTRUMENTS DS90UB954 VIDEO DESERIALIZER DRIVER
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9a02444bd647..d84e7679d0ed 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -492,6 +492,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 "Video and audio decoders"
config VIDEO_SAA717X
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 84e9ad00236a..178de6ad736c 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -136,6 +136,7 @@ obj-$(CONFIG_VIDEO_MAX9271_LIB) += max9271.o
obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
obj-$(CONFIG_VIDEO_DS90UB954) += ds90ub954.o
+obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o
obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.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..d4f4621d66eb
--- /dev/null
+++ b/drivers/media/i2c/ds90ub953.c
@@ -0,0 +1,560 @@
+// 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/clk.h>
+#include <linux/clk-provider.h>
+#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 <linux/rational.h>
+#include <linux/slab.h>
+#include <linux/sysfs.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;
+ struct clk *line_rate_clk;
+
+ struct clk_hw clk_out_hw;
+
+ u32 gpio_func[DS90_NUM_GPIOS];
+ bool inv_clock_pol;
+
+ u64 csi_err_cnt;
+
+ u8 clkout_mul;
+ u8 clkout_div;
+ u8 clkout_ctrl0;
+ u8 clkout_ctrl1;
+};
+
+/* -----------------------------------------------------------------------------
+ * 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;
+}
+
+/*
+ * 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) {
+ usleep_range(1000, 3000);
+ ret = ds90_read(ds90, DS90_REG_DEVICE_STS);
+ if (ret >= 0 && (ret & DS90_REG_DEVICE_STS_CFG_INIT_DONE))
+ break;
+ }
+}
+
+/* -----------------------------------------------------------------------------
+ * sysfs
+ */
+
+static ssize_t bc_crc_err_cnt_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ds90_data *ds90 = dev_get_drvdata(dev);
+ s32 lsb = ds90_read(ds90, DS90_REG_CRC_ERR_CNT1);
+ s32 msb = ds90_read(ds90, DS90_REG_CRC_ERR_CNT2);
+ int val;
+
+ if (lsb < 0)
+ return lsb;
+ if (msb < 0)
+ return msb;
+
+ val = (msb << 8) | lsb;
+
+ return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t csi_err_cnt_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ds90_data *ds90 = dev_get_drvdata(dev);
+ s32 val = ds90_read(ds90, DS90_REG_CSI_ERR_CNT);
+
+ if (val > 0)
+ ds90->csi_err_cnt += val;
+
+ return sprintf(buf, "%llu\n", ds90->csi_err_cnt);
+}
+
+static ssize_t csi_err_status_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ds90_data *ds90 = dev_get_drvdata(dev);
+ s32 val = ds90_read(ds90, DS90_REG_CSI_ERR_STATUS);
+
+ return sprintf(buf, "0x%02x\n", val);
+}
+
+static DEVICE_ATTR_RO(bc_crc_err_cnt);
+static DEVICE_ATTR_RO(csi_err_cnt);
+static DEVICE_ATTR_RO(csi_err_status);
+
+static struct attribute *ds90_attributes[] = {
+ &dev_attr_bc_crc_err_cnt.attr,
+ &dev_attr_csi_err_cnt.attr,
+ &dev_attr_csi_err_status.attr,
+ NULL
+};
+
+static const struct attribute_group ds90_attr_group = {
+ .attrs = ds90_attributes,
+};
+
+/* -----------------------------------------------------------------------------
+ * Clock output
+ */
+
+/*
+ * Assume mode 0 "CSI-2 Synchronous mode" (strap, reg 0x03) is always
+ * used. In this mode all clocks are derived from the deserializer. Other
+ * modes are not implemented.
+ */
+
+/*
+ * We always use 4 as a pre-divider (HS_CLK_DIV = 2).
+ *
+ * According to the datasheet:
+ * - "HS_CLK_DIV typically should be set to either 16, 8, or 4 (default)."
+ * - "if it is not possible to have an integer ratio of N/M, it is best to
+ * select a smaller value for HS_CLK_DIV.
+ *
+ * For above reasons the default HS_CLK_DIV seems the best in the average
+ * case. Use always that value to keep the code simple.
+ */
+static const unsigned long hs_clk_div = 2;
+static const unsigned long prediv = (1 << hs_clk_div);
+
+static unsigned long ds90_clkout_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ds90_data *ds90 = container_of(hw, struct ds90_data, clk_out_hw);
+ s32 ctrl0 = ds90_read(ds90, DS90_REG_CLKOUT_CTRL0);
+ s32 ctrl1 = ds90_read(ds90, DS90_REG_CLKOUT_CTRL1);
+ unsigned long mul, div, ret;
+
+ if (ctrl0 < 0 || ctrl1 < 0) {
+ // Perhaps link down, use cached values
+ ctrl0 = ds90->clkout_ctrl0;
+ ctrl1 = ds90->clkout_ctrl1;
+ }
+
+ mul = ctrl0 & 0x1f;
+ div = ctrl1 & 0xff;
+
+ if (div == 0)
+ return 0;
+
+ ret = parent_rate / prediv * mul / div;
+
+ return ret;
+}
+
+static long ds90_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ struct ds90_data *ds90 = container_of(hw, struct ds90_data, clk_out_hw);
+ struct device *dev = &ds90->client->dev;
+ unsigned long mul, div, res;
+
+ rational_best_approximation(rate, *parent_rate / prediv,
+ (1 << 5) - 1, (1 << 8) - 1,
+ &mul, &div);
+ ds90->clkout_mul = mul;
+ ds90->clkout_div = div;
+
+ res = *parent_rate / prediv * ds90->clkout_mul / ds90->clkout_div;
+
+ dev_dbg(dev, "%lu / %lu * %lu / %lu = %lu (wanted %lu)",
+ *parent_rate, prediv, mul, div, res, rate);
+
+ return res;
+}
+
+static int ds90_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct ds90_data *ds90 = container_of(hw, struct ds90_data, clk_out_hw);
+
+ ds90->clkout_ctrl0 = (hs_clk_div << 5) | ds90->clkout_mul;
+ ds90->clkout_ctrl1 = ds90->clkout_div;
+
+ ds90_write(ds90, DS90_REG_CLKOUT_CTRL0, ds90->clkout_ctrl0);
+ ds90_write(ds90, DS90_REG_CLKOUT_CTRL1, ds90->clkout_ctrl1);
+
+ return 0;
+}
+
+static const struct clk_ops ds90_clkout_ops = {
+ .recalc_rate = ds90_clkout_recalc_rate,
+ .round_rate = ds90_clkout_round_rate,
+ .set_rate = ds90_clkout_set_rate,
+};
+
+static int ds90_register_clkout(struct ds90_data *ds90)
+{
+ struct device *dev = &ds90->client->dev;
+ const char *parent_names[1] = { __clk_get_name(ds90->line_rate_clk) };
+ const struct clk_init_data init = {
+ .name = kasprintf(GFP_KERNEL, "%s.clk_out", dev_name(dev)),
+ .ops = &ds90_clkout_ops,
+ .parent_names = parent_names,
+ .num_parents = 1,
+ };
+ int err;
+
+ ds90->clk_out_hw.init = &init;
+
+ err = devm_clk_hw_register(dev, &ds90->clk_out_hw);
+ kfree(init.name); /* clock framework made a copy of the name */
+ if (err)
+ return dev_err_probe(dev, err, "Cannot register clock HW\n");
+
+ err = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &ds90->clk_out_hw);
+ if (err)
+ return dev_err_probe(dev, err, "Cannot add OF clock provider\n");
+
+ return 0;
+}
+
+/* -----------------------------------------------------------------------------
+ * GPIOs
+ */
+
+static void ds90_configure_gpios(struct ds90_data *ds90)
+{
+ u8 gpio_input_ctrl = 0;
+ u8 local_gpio_data = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(ds90->gpio_func); i++) {
+ 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? */
+}
+
+/* -----------------------------------------------------------------------------
+ * Core
+ */
+
+static int ds90_configure(struct ds90_data *ds90)
+{
+ struct device *dev = &ds90->client->dev;
+ s32 rev_mask;
+ int err;
+
+ 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);
+ return err;
+ }
+
+ dev_dbg_once(dev, "rev/mask %02x\n", rev_mask);
+
+ /* 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);
+
+ ds90_write(ds90, DS90_REG_CLKOUT_CTRL0, ds90->clkout_ctrl0);
+ ds90_write(ds90, DS90_REG_CLKOUT_CTRL1, ds90->clkout_ctrl1);
+
+ if (ds90->inv_clock_pol) {
+ 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);
+ }
+
+ ds90_configure_gpios(ds90);
+
+ return 0;
+}
+
+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;
+ int i;
+
+ 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, "ti,gpio-functions", ds90->gpio_func,
+ ARRAY_SIZE(ds90->gpio_func));
+ if (err && err != -EINVAL)
+ dev_err(dev, "DT: invalid ti,gpio-functions property (%d)", err);
+
+ for (i = 0; i < ARRAY_SIZE(ds90->gpio_func); i++) {
+ if (ds90->gpio_func[i] >= DS90_GPIO_N_FUNCS) {
+ dev_err(dev,
+ "Unknown ti,gpio-functions value %u for GPIO%d of %pOF",
+ ds90->gpio_func[i], i, np);
+ return -EINVAL;
+ }
+ }
+
+ ds90->inv_clock_pol = of_property_read_bool(np, "ti,ds90ub953-q1-clk-inv-pol-quirk");
+
+ return 0;
+}
+
+static int ds90_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct ds90_data *ds90;
+ 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);
+
+ /* Default values for clock multiplier and divider registers */
+ ds90->clkout_ctrl0 = 0x41;
+ ds90->clkout_ctrl1 = 0x28;
+
+ ds90->line_rate_clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(ds90->line_rate_clk))
+ return dev_err_probe(dev, PTR_ERR(ds90->line_rate_clk),
+ "Cannot get line rate clock\n");
+ dev_dbg(dev, "line rate: %10lu Hz\n", clk_get_rate(ds90->line_rate_clk));
+
+ err = ds90_register_clkout(ds90);
+ if (err)
+ return err;
+
+ err = ds90_parse_dt(ds90);
+ if (err)
+ goto err_parse_dt;
+
+ err = sysfs_create_group(&dev->kobj, &ds90_attr_group);
+ if (err)
+ goto err_sysfs;
+
+ ds90_soft_reset(ds90);
+ ds90_configure(ds90);
+
+ dev_info(dev, "Ready\n");
+
+ return 0;
+
+err_sysfs:
+err_parse_dt:
+ 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.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
` (5 preceding siblings ...)
2022-02-06 11:59 ` [RFCv3 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer Luca Ceresoli
@ 2022-02-06 12:05 ` Luca Ceresoli
2022-02-07 12:06 ` [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,) Tomi Valkeinen
7 siblings, 0 replies; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-06 12:05 UTC (permalink / raw)
To: linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Tomi Valkeinen, matti.vaittinen
Hi,
apologies for the incorrect cover letter subject. :( It should have been
as in the present e-mail, which I'm sending mostly to help search
engines find the subject...
Regards.
--
Luca
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 4/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
2022-02-06 11:59 ` [RFCv3 4/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
@ 2022-02-06 18:46 ` Rob Herring
2022-02-07 19:39 ` Rob Herring
1 sibling, 0 replies; 24+ messages in thread
From: Rob Herring @ 2022-02-06 18:46 UTC (permalink / raw)
To: Luca Ceresoli
Cc: matti.vaittinen, Jacopo Mondi, Wolfram Sang, Sakari Ailus,
Peter Rosin, Mauro Carvalho Chehab, linux-kernel, linux-media,
linux-i2c, Hans Verkuil, Kieran Bingham, Vladimir Zapolskiy,
Laurent Pinchart, Tomi Valkeinen, Rob Herring, devicetree,
Mark Rutland
On Sun, 06 Feb 2022 12:59:37 +0100, Luca Ceresoli wrote:
> Describe the Texas Instruments DS90UB954-Q1, a 2-input MIPI CSI-2 video
> deserializer with I2C Address Translator and remote GPIOs.
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> ---
>
> Changes RFCv2 -> RFCv3:
>
> - rewrite in yaml
> - use new layout based on remote-chips under the main deser node
> - new clock configuration based on common clock framework
>
> Changes RFCv1 -> RFCv2:
>
> - add explicit aliases for the FPD-link RX ports (optional)
> - add proper remote GPIO description
> ---
> .../bindings/media/i2c/ti,ds90ub954-q1.yaml | 235 ++++++++++++++++++
> MAINTAINERS | 6 +
> 2 files changed, 241 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml: properties:reg-names: {'description': 'Names of I2C address used to communicate with the chip, must match\nthe "reg" values; mandatory if there are 2 or more addresses.\n"main" is the main I2C address, used to access shared registers.\n"rxport0" and "rxport1" are the I2C alias to access FPD-link RX\nport specific registers; must not be used by other slaves on the\nsame bus. "ser0" and "ser1" are the I2C alias to access the remote\nserializer connected on each FPD-link RX port; must not be used by\nother slaves on the same bus.\n', 'minItems': 1, 'maxItems': 5, 'items': [{'const': 'main'}, {'const': 'rxport0'}, {'const': 'rxport1'}, {'const': 'ser0'}, {'const': 'ser1'}]} should not be valid under {'required': ['maxItems']}
hint: "maxItems" is not needed with an "items" list
from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml: ignoring, error in schema: properties: reg-names
Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.example.dt.yaml:0:0: /example-0/i2c@0/deser@3d: failed to match any schema with compatible: ['ti,ds90ub954-q1']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1588972
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
` (6 preceding siblings ...)
2022-02-06 12:05 ` [RFCv3 0/6] TI camera serdes and I2C address translation Luca Ceresoli
@ 2022-02-07 12:06 ` Tomi Valkeinen
2022-02-07 13:21 ` Vaittinen, Matti
7 siblings, 1 reply; 24+ messages in thread
From: Tomi Valkeinen @ 2022-02-07 12:06 UTC (permalink / raw)
To: Luca Ceresoli, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, matti.vaittinen
Hi Luca,
On 06/02/2022 13:59, Luca Ceresoli wrote:
> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
> DS90UB9xx serializer/deserializer chipsets with I2C address translation.
>
> I sent RFCv2 back in 2019 (!). After that I have applied most of the
> improvements proposed during code review, most notably device tree
> representation and proper use of kernel abstractions for clocks and GPIO. I
> have also done many improvements all over the drivers code.
Thanks for sending this! I'll have a closer look at the code in the near
future.
> However I still don't consider these drivers "ready", hence the RFC status.
>
> One reason is that, while the I2C ATR idea has been considered good by
> Wolfram, its implementation requires I2C core changes that have been tried
> but never made it to mainline. I think that discussion needs to be reopened
> and work has to be done on that side. Thus for the time being this code
> still has the alias pool: it is an interim solution until I2C core is
> ready.
>
> Also be aware that the only hardware where I sould test this code runs a
> v4.19 kernel. I cannot guarantee it will work perfectly on mainline.
>
> And since my hardware has only one camera connected to each deserializer I
> dropped support. However I wrote the code to be able to easily add support
> for 2 and 4 camera inputs as well as 2 CSI-2 outputs (DS90UB960).
>
> Finally, I dropped all attempts at supporting hotplug. The goals I had 2+
> years ago are not reasonably doable even with current kernels. Luckily all
> the users that I talked with are happy without hotplug. For this reason I
> simplified the serializer management in the DS90UB954 driver by keeping the
> serializer always instantiated.
>
> Even with the above limitations I felt I'd send this v3 anyway since
> several people have contacted me since v2 asking whether this
> implementation has made progress towards mainline. Some even improved on
> top of my code it their own forks. As I cannot afford to work on this topic
> in the near future, here is the latest and greatest version I can produce,
> with all the improvements I made so far.
I've discussed with Luca in private emails, but I'll add a short status
about my work in this thread:
About a year ago I took Luca's then-latest-patches and started working
on them. The aim was to get full multiplexed streams support to v4l2 so
that we could support CSI-2 bus with multiple virtual channels and
embedded data, and after that, add support for fpdlink devices.
Since then I have sent multiple versions of the v4l2 work (no drivers
yet, only the framework changes) to upstream lists. Some pieces have
already been merged to upstream (e.g. subdev state), but most of it is
still under work. Here's a link to v10 of the streams series:
https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@ideasonboard.com/
It has a link to my (now slightly outdated) git branch which contains
the driver work too.
The fpdlink drivers have diverged from Luca's version quite a bit. The
most obvious difference is the support for multiplexed streams, of
course, but there are lots of other changes too. The drivers support
DS90UB960 (no UB954 at the moment), DS90UB953 and DS90UB913. UB960
supports all the inputs and outputs. I have also dropped some code which
I did not need and which I wasn't sure if it's correctly implemented, to
make it easier to work on the multiplexed streams version. Some of that
code may need to be added back.
I have not changed the i2c-atr driver, and my fpdlink driver uses it
more or less the same way as in Luca's version.
Considering that you're not able to work on this, my suggestion is to
review the i2c-atr patches here (or perhaps send those patches in a
separate series?), but afaics the fpdlink drivers without multiplexed
streams is a dead-end, as they can only support a single camera (and no
embedded data), so I don't see much point in properly reviewing them.
However, I will go through the fpdlink drivers in this series and
cherry-pick the changes that make sense. I was about to start working on
proper fpdlink-clock-rate and clkout support, but I see you've already
done that work =).
But, of course, I'm open to other ideas on how to proceed.
Tomi
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)
2022-02-07 12:06 ` [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,) Tomi Valkeinen
@ 2022-02-07 13:21 ` Vaittinen, Matti
2022-02-07 14:07 ` Luca Ceresoli
0 siblings, 1 reply; 24+ messages in thread
From: Vaittinen, Matti @ 2022-02-07 13:21 UTC (permalink / raw)
To: Tomi Valkeinen, Luca Ceresoli, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab
Hi dee Ho peeps,
On 2/7/22 14:06, Tomi Valkeinen wrote:
> Hi Luca,
>
> On 06/02/2022 13:59, Luca Ceresoli wrote:
>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
>> DS90UB9xx serializer/deserializer chipsets with I2C address translation.
..snip
>> Even with the above limitations I felt I'd send this v3 anyway since
>> several people have contacted me since v2 asking whether this
>> implementation has made progress towards mainline. Some even improved on
>> top of my code it their own forks. As I cannot afford to work on this
>> topic
>> in the near future, here is the latest and greatest version I can
>> produce,
>> with all the improvements I made so far.
>
> I've discussed with Luca in private emails, but I'll add a short status
> about my work in this thread:
Thanks for CC:ing me Luca. We had a small chat during the FOSDEM.
> About a year ago I took Luca's then-latest-patches and started working
> on them. The aim was to get full multiplexed streams support to v4l2 so
> that we could support CSI-2 bus with multiple virtual channels and
> embedded data, and after that, add support for fpdlink devices.
>
> Since then I have sent multiple versions of the v4l2 work (no drivers
> yet, only the framework changes) to upstream lists. Some pieces have
> already been merged to upstream (e.g. subdev state), but most of it is
> still under work. Here's a link to v10 of the streams series:
>
> https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@ideasonboard.com/
>
>
> It has a link to my (now slightly outdated) git branch which contains
> the driver work too.
I have fetched this tree from Tomi and done some experimenting on
another SERDES. That SERDES in not from TI or Maxim, some of you may
guess the company though :) Unfortunately I can't publish the details or
the code for now - I am discussing what I am allowed to publish. My
personal goal is to see if I could write a Linux driver for this
yet-another-Video-SERDES and see if it can one day get merged to
upstream for anyone interested to play with.
> The fpdlink drivers have diverged from Luca's version quite a bit. The
> most obvious difference is the support for multiplexed streams, of
> course, but there are lots of other changes too. The drivers support
> DS90UB960 (no UB954 at the moment), DS90UB953 and DS90UB913. UB960
> supports all the inputs and outputs.
For the record, the SERDES I am working with does also support
connecting 4 cameras (4 SERs) to one DES which provides two CSI-2
outputs. As far as I understand the virtual channel support is also
there (in the HW).
I have also dropped some code which
> I did not need and which I wasn't sure if it's correctly implemented, to
> make it easier to work on the multiplexed streams version. Some of that
> code may need to be added back.
>
> I have not changed the i2c-atr driver, and my fpdlink driver uses it
> more or less the same way as in Luca's version.
>
I have also used the ATR driver as is. The SERDES I am working with does
also the I2C address translation.
> Considering that you're not able to work on this, my suggestion is to
> review the i2c-atr patches here (or perhaps send those patches in a
> separate series?),
It would be _really_ cool to get the ATR upstream.
but afaics the fpdlink drivers without multiplexed
> streams is a dead-end, as they can only support a single camera (and no
> embedded data), so I don't see much point in properly reviewing them.
>
> However, I will go through the fpdlink drivers in this series and
> cherry-pick the changes that make sense. I was about to start working on
> proper fpdlink-clock-rate and clkout support, but I see you've already
> done that work =).
I am not sure if I am poking in the nest of the wasps - but there's one
major difference with the work I've done and with Toni's / Luca's work.
The TI DES drivers (like ub960 driver) packs pretty much everything
under single driver at media/i2c - which (in my opinion) makes the
driver pretty large one.
My approach is/was to utilize MFD - and prepare the regmap + IRQs in the
MFD (as is pretty usual) - and parse that much of the device-tree that
we see how many SER devices are there - and that I get the non I2C
related DES<=>SER link parameters set. After that I do kick alive the
separate MFD cells for ATR, pinctrl/GPIO and media.
The ATR driver instantiates the SER I2C devices like Toni's ub960 does.
The SER compatible is once again matched in MFD (for SER) - which again
provides regmap for SER, does initial I2C writes so SER starts
responding to I2C reads and then kicks cells for media and pinctrl/gpio.
I believe splitting the functionality to MFD subdevices makes drivers
slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual,
regmaps/IRQ-chips under MFD and only media/v4l2 related parts under media.
Anyways - I opened the mail client to just say that the ATR has worked
nicely for me and seems pretty stable - so to me it sounds like a goof
idea to get ATR reviewed/merged even before the drivers have been finalized.
Thanks for showing the way for the rest of us Luca & others! It's much
easier to follow than lead the way ;)
Best Regards
--Matti
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)
2022-02-07 13:21 ` Vaittinen, Matti
@ 2022-02-07 14:07 ` Luca Ceresoli
2022-02-07 14:38 ` Vaittinen, Matti
0 siblings, 1 reply; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-07 14:07 UTC (permalink / raw)
To: Vaittinen, Matti, Tomi Valkeinen, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab
Hi Matti,
On 07/02/22 14:21, Vaittinen, Matti wrote:
> Hi dee Ho peeps,
>
> On 2/7/22 14:06, Tomi Valkeinen wrote:
>> Hi Luca,
>>
>> On 06/02/2022 13:59, Luca Ceresoli wrote:
>>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
>>> DS90UB9xx serializer/deserializer chipsets with I2C address translation.
>
> ..snip
>
>>> Even with the above limitations I felt I'd send this v3 anyway since
>>> several people have contacted me since v2 asking whether this
>>> implementation has made progress towards mainline. Some even improved on
>>> top of my code it their own forks. As I cannot afford to work on this
>>> topic
>>> in the near future, here is the latest and greatest version I can
>>> produce,
>>> with all the improvements I made so far.
>>
>> I've discussed with Luca in private emails, but I'll add a short status
>> about my work in this thread:
>
> Thanks for CC:ing me Luca. We had a small chat during the FOSDEM.
>
>> About a year ago I took Luca's then-latest-patches and started working
>> on them. The aim was to get full multiplexed streams support to v4l2 so
>> that we could support CSI-2 bus with multiple virtual channels and
>> embedded data, and after that, add support for fpdlink devices.
>>
>> Since then I have sent multiple versions of the v4l2 work (no drivers
>> yet, only the framework changes) to upstream lists. Some pieces have
>> already been merged to upstream (e.g. subdev state), but most of it is
>> still under work. Here's a link to v10 of the streams series:
>>
>> https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@ideasonboard.com/
>>
>>
>> It has a link to my (now slightly outdated) git branch which contains
>> the driver work too.
>
> I have fetched this tree from Tomi and done some experimenting on
> another SERDES. That SERDES in not from TI or Maxim, some of you may
> guess the company though :) Unfortunately I can't publish the details or
> the code for now - I am discussing what I am allowed to publish. My
> personal goal is to see if I could write a Linux driver for this
> yet-another-Video-SERDES and see if it can one day get merged to
> upstream for anyone interested to play with.
>
>> The fpdlink drivers have diverged from Luca's version quite a bit. The
>> most obvious difference is the support for multiplexed streams, of
>> course, but there are lots of other changes too. The drivers support
>> DS90UB960 (no UB954 at the moment), DS90UB953 and DS90UB913. UB960
>> supports all the inputs and outputs.
>
> For the record, the SERDES I am working with does also support
> connecting 4 cameras (4 SERs) to one DES which provides two CSI-2
> outputs. As far as I understand the virtual channel support is also
> there (in the HW).
>
> I have also dropped some code which
>> I did not need and which I wasn't sure if it's correctly implemented, to
>> make it easier to work on the multiplexed streams version. Some of that
>> code may need to be added back.
>>
>> I have not changed the i2c-atr driver, and my fpdlink driver uses it
>> more or less the same way as in Luca's version.
>>
>
> I have also used the ATR driver as is. The SERDES I am working with does
> also the I2C address translation.
>
>> Considering that you're not able to work on this, my suggestion is to
>> review the i2c-atr patches here (or perhaps send those patches in a
>> separate series?),
>
> It would be _really_ cool to get the ATR upstream.
>
> but afaics the fpdlink drivers without multiplexed
>> streams is a dead-end, as they can only support a single camera (and no
>> embedded data), so I don't see much point in properly reviewing them.
>>
>> However, I will go through the fpdlink drivers in this series and
>> cherry-pick the changes that make sense. I was about to start working on
>> proper fpdlink-clock-rate and clkout support, but I see you've already
>> done that work =).
>
> I am not sure if I am poking in the nest of the wasps - but there's one
> major difference with the work I've done and with Toni's / Luca's work.
You are. ;)
> The TI DES drivers (like ub960 driver) packs pretty much everything
> under single driver at media/i2c - which (in my opinion) makes the
> driver pretty large one.
>
> My approach is/was to utilize MFD - and prepare the regmap + IRQs in the
> MFD (as is pretty usual) - and parse that much of the device-tree that
> we see how many SER devices are there - and that I get the non I2C
> related DES<=>SER link parameters set. After that I do kick alive the
> separate MFD cells for ATR, pinctrl/GPIO and media.
>
> The ATR driver instantiates the SER I2C devices like Toni's ub960 does.
> The SER compatible is once again matched in MFD (for SER) - which again
> provides regmap for SER, does initial I2C writes so SER starts
> responding to I2C reads and then kicks cells for media and pinctrl/gpio.
>
> I believe splitting the functionality to MFD subdevices makes drivers
> slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual,
> regmaps/IRQ-chips under MFD and only media/v4l2 related parts under media.
There has been quite a fiery discussion about this in the past, you can
grab some popcorn and read
https://lore.kernel.org/linux-media/20181008211205.2900-1-vz@mleia.com/T/#m9b01af81665ac956af3c6d57810239420c3f8cee
TL;DR: there have been strong opposition the the MFD idea.
I personally don't have a super strong opinion: I wrote this as a
monolithic driver because it looked like the most natural implementation
and found it was working fine for me, I never really explored the MFD idea.
> Anyways - I opened the mail client to just say that the ATR has worked
> nicely for me and seems pretty stable - so to me it sounds like a goof
> idea to get ATR reviewed/merged even before the drivers have been finalized.
Sounds like a... what...? A "good idea"? Or a "goofy idea"? :-D
--
Luca
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)
2022-02-07 14:07 ` Luca Ceresoli
@ 2022-02-07 14:38 ` Vaittinen, Matti
2022-02-07 16:23 ` Tomi Valkeinen
0 siblings, 1 reply; 24+ messages in thread
From: Vaittinen, Matti @ 2022-02-07 14:38 UTC (permalink / raw)
To: Luca Ceresoli, Tomi Valkeinen, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab
Hi again Luca,
On 2/7/22 16:07, Luca Ceresoli wrote:
> Hi Matti,
>
> On 07/02/22 14:21, Vaittinen, Matti wrote:
>> Hi dee Ho peeps,
>>
>> On 2/7/22 14:06, Tomi Valkeinen wrote:
>>> Hi Luca,
>>>
>>> On 06/02/2022 13:59, Luca Ceresoli wrote:
>>>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
>>>> DS90UB9xx serializer/deserializer chipsets with I2C address translation.
>>
>>
>> I am not sure if I am poking in the nest of the wasps - but there's one
>> major difference with the work I've done and with Toni's / Luca's work.
>
> You are. ;)
>
>> The TI DES drivers (like ub960 driver) packs pretty much everything
>> under single driver at media/i2c - which (in my opinion) makes the
>> driver pretty large one.
>>
>> My approach is/was to utilize MFD - and prepare the regmap + IRQs in the
>> MFD (as is pretty usual) - and parse that much of the device-tree that
>> we see how many SER devices are there - and that I get the non I2C
>> related DES<=>SER link parameters set. After that I do kick alive the
>> separate MFD cells for ATR, pinctrl/GPIO and media.
>>
>> The ATR driver instantiates the SER I2C devices like Toni's ub960 does.
>> The SER compatible is once again matched in MFD (for SER) - which again
>> provides regmap for SER, does initial I2C writes so SER starts
>> responding to I2C reads and then kicks cells for media and pinctrl/gpio.
>>
>> I believe splitting the functionality to MFD subdevices makes drivers
>> slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual,
>> regmaps/IRQ-chips under MFD and only media/v4l2 related parts under media.
>
> There has been quite a fiery discussion about this in the past, you can
> grab some popcorn and read
> https://lore.kernel.org/linux-media/20181008211205.2900-1-vz@mleia.com/T/#m9b01af81665ac956af3c6d57810239420c3f8cee
>
> TL;DR: there have been strong opposition the the MFD idea.
Hm. I may be missing something but I didn't see opposition to using MFD
or splitting the drivers. I do see opposition to adding _functionality_
in MFD. If I read this correctly, Lee did oppose adding the I2C stuff,
sysfs attributes etc in MFD. Quoting his reply:
"This driver does too much real work ('stuff') to be an MFD driver.
MFD drivers should not need to care of; links, gates, modes, pixels,
frequencies maps or properties. Nor should they contain elaborate
sysfs structures to control the aforementioned 'stuff'.
Granted, there may be some code in there which could be appropriate
for an MFD driver. However most of it needs moving out into a
function driver (or two)."
And I tend to agree with Lee here. I would not put I2C bridge stuff or
sysfs attributes in MFD. But I think it does not mean SERDESes should
not use MFD when they clearly contain more IP blocks than the
video/media ones :) I am confident Lee and others might be much more
welcoming for driver which simply configures regmap and kicks subdriver
for doing the ATR / I2C stuff.
I did add minimal mandatory register initializations in order to avoid
synchronizing the sub-devices - but I hope that would be too much.
(Synchronizing sub-devices to when the I2C reads over the link becomes
available.)
What comes to regmap/regmap IRQ initialization in MFD - that's not
exceptional. I think it's quite standard for MFD to prepare IRQs/regmaps
when many sub-devices use these resources.
> I personally don't have a super strong opinion: I wrote this as a
> monolithic driver because it looked like the most natural implementation
> and found it was working fine for me, I never really explored the MFD idea.
No problem. I am definitely trying to tell you how these TI drivers must
be done. Even I don't have the guts to do that ;D
I am simply saying that the MFD approach could be used. It does have
certain merits if we manage to keep the MFD layer thin enough.
>> Anyways - I opened the mail client to just say that the ATR has worked
>> nicely for me and seems pretty stable - so to me it sounds like a goof
>> idea to get ATR reviewed/merged even before the drivers have been finalized.
>
> Sounds like a... what...? A "good idea"? Or a "goofy idea"? :-D
Let me rephrase. It's greaf idea ;)
(I really meant a "good idea" :])
Best Regards
-- Matti Vaittinen
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)
2022-02-07 14:38 ` Vaittinen, Matti
@ 2022-02-07 16:23 ` Tomi Valkeinen
2022-02-08 6:40 ` Vaittinen, Matti
0 siblings, 1 reply; 24+ messages in thread
From: Tomi Valkeinen @ 2022-02-07 16:23 UTC (permalink / raw)
To: Vaittinen, Matti, Luca Ceresoli, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab
On 07/02/2022 16:38, Vaittinen, Matti wrote:
> Hi again Luca,
>
> On 2/7/22 16:07, Luca Ceresoli wrote:
>> Hi Matti,
>>
>> On 07/02/22 14:21, Vaittinen, Matti wrote:
>>> Hi dee Ho peeps,
>>>
>>> On 2/7/22 14:06, Tomi Valkeinen wrote:
>>>> Hi Luca,
>>>>
>>>> On 06/02/2022 13:59, Luca Ceresoli wrote:
>>>>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
>>>>> DS90UB9xx serializer/deserializer chipsets with I2C address translation.
>>>
>>>
>>> I am not sure if I am poking in the nest of the wasps - but there's one
>>> major difference with the work I've done and with Toni's / Luca's work.
>>
>> You are. ;)
>>
>>> The TI DES drivers (like ub960 driver) packs pretty much everything
>>> under single driver at media/i2c - which (in my opinion) makes the
>>> driver pretty large one.
>>>
>>> My approach is/was to utilize MFD - and prepare the regmap + IRQs in the
>>> MFD (as is pretty usual) - and parse that much of the device-tree that
>>> we see how many SER devices are there - and that I get the non I2C
>>> related DES<=>SER link parameters set. After that I do kick alive the
>>> separate MFD cells for ATR, pinctrl/GPIO and media.
>>>
>>> The ATR driver instantiates the SER I2C devices like Toni's ub960 does.
>>> The SER compatible is once again matched in MFD (for SER) - which again
>>> provides regmap for SER, does initial I2C writes so SER starts
>>> responding to I2C reads and then kicks cells for media and pinctrl/gpio.
>>>
>>> I believe splitting the functionality to MFD subdevices makes drivers
>>> slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual,
>>> regmaps/IRQ-chips under MFD and only media/v4l2 related parts under media.
>>
>> There has been quite a fiery discussion about this in the past, you can
>> grab some popcorn and read
>> https://lore.kernel.org/linux-media/20181008211205.2900-1-vz@mleia.com/T/#m9b01af81665ac956af3c6d57810239420c3f8cee
>>
>> TL;DR: there have been strong opposition the the MFD idea.
>
> Hm. I may be missing something but I didn't see opposition to using MFD
> or splitting the drivers. I do see opposition to adding _functionality_
> in MFD. If I read this correctly, Lee did oppose adding the I2C stuff,
> sysfs attributes etc in MFD. Quoting his reply:
>
> "This driver does too much real work ('stuff') to be an MFD driver.
> MFD drivers should not need to care of; links, gates, modes, pixels,
> frequencies maps or properties. Nor should they contain elaborate
> sysfs structures to control the aforementioned 'stuff'.
>
> Granted, there may be some code in there which could be appropriate
> for an MFD driver. However most of it needs moving out into a
> function driver (or two)."
>
> And I tend to agree with Lee here. I would not put I2C bridge stuff or
> sysfs attributes in MFD. But I think it does not mean SERDESes should
> not use MFD when they clearly contain more IP blocks than the
> video/media ones :) I am confident Lee and others might be much more
> welcoming for driver which simply configures regmap and kicks subdriver
> for doing the ATR / I2C stuff.
I admit that I don't know MFD drivers too well, but I was thinking about
this some time back and I wasn't quite sure about using MFD here.
My thinking was that MFD is fine and good when a device contains more or
less independent functionalities, like a PMIC with, say, gpios and
regulators, both of which just work as long as the PMIC is powered up.
Here all the functionalities depend on the link (fpdlink or some other
"link" =), and the serializers. In other words, the link status or any
changes to the link or the serializers might affect the GPIO/I2C/IRQ
functionalities.
So, I don't have any clear concern here. Just a vague feeling that the
functionalities in this kind of devices may be more tightly tied
together than in normal MFDs. I could be totally wrong here.
Tomi
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 4/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
2022-02-06 11:59 ` [RFCv3 4/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
2022-02-06 18:46 ` Rob Herring
@ 2022-02-07 19:39 ` Rob Herring
1 sibling, 0 replies; 24+ messages in thread
From: Rob Herring @ 2022-02-07 19:39 UTC (permalink / raw)
To: Luca Ceresoli
Cc: linux-media, linux-i2c, devicetree, linux-kernel, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Tomi Valkeinen, matti.vaittinen
On Sun, Feb 06, 2022 at 12:59:37PM +0100, Luca Ceresoli wrote:
> Describe the Texas Instruments DS90UB954-Q1, a 2-input MIPI CSI-2 video
> deserializer with I2C Address Translator and remote GPIOs.
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> ---
>
> Changes RFCv2 -> RFCv3:
>
> - rewrite in yaml
> - use new layout based on remote-chips under the main deser node
> - new clock configuration based on common clock framework
>
> Changes RFCv1 -> RFCv2:
>
> - add explicit aliases for the FPD-link RX ports (optional)
> - add proper remote GPIO description
> ---
> .../bindings/media/i2c/ti,ds90ub954-q1.yaml | 235 ++++++++++++++++++
> MAINTAINERS | 6 +
> 2 files changed, 241 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
> new file mode 100644
> index 000000000000..95dc3d22f5d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
> @@ -0,0 +1,235 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2019 Renesas Electronics Corp.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub954-q1.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments DS90UB954-Q1 dual video deserializer
> +
> +maintainers:
> + - Luca Ceresoli <luca@lucaceresoli.net>
> +
> +description: |
> + 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.
> +
> +properties:
> + compatible:
> + const: ti,ds90ub954-q1
> +
> + reg:
> + description: |
> + 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".
minItems: 1
maxItems: 5
> +
> + reg-names:
> + description: |
> + Names of I2C address used to communicate with the chip, must match
> + the "reg" values; mandatory if there are 2 or more addresses.
> + "main" is the main I2C address, used to access shared registers.
> + "rxport0" and "rxport1" are the I2C alias to access FPD-link RX
> + port specific registers; must not be used by other slaves on the
> + same bus. "ser0" and "ser1" are the 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.
> + minItems: 1
> + maxItems: 5
> + items:
> + - const: main
> + - const: rxport0
> + - const: rxport1
> + - const: ser0
> + - const: ser1
> +
> + clocks:
> + description: provider of the clock on the XIN/REFCLK pin
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + reset-gpios:
> + description: chip reset GPIO connected to PDB pin (active low)
maxItems: 1
> +
> + i2c-alias-pool:
Needs a type.
> + description: |
> + 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 remote I2C chips
> +
> + '#clock-cells':
> + description: |
> + the DS90UB954 provides the FPD line rate clock to the serializer
> + const: 0
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + patternProperties:
> + '^port@[01]$':
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + description: FPD-Link RX port 0 (RIN0+/RIN0- pins)
> +
> + properties:
> + endpoint:
> + $ref: /schemas/media/video-interfaces.yaml#
> + unevaluatedProperties: false
> +
> + '^port@2$':
> + $ref: /schemas/graph.yaml#/properties/port
> + description: MIPI-CSI2 TX port
> +
> + remote-chips:
> + type: object
> +
> + properties:
> + '#address-cells':
> + const: 1
> + '#size-cells':
> + const: 0
> +
> + patternProperties:
> + '^remote-chip@([01]+)$':
> + type: object
> + $ref: /schemas/media/i2c/ti,ds90ub953-q1.yaml#
> +
> + required:
> + - '#address-cells'
> + - '#size-cells'
> +
> + additionalProperties: false
> +
> + i2c-atr:
> + description: |
> + Each child describes the I2C bus on the remote side of an RX port
> + type: object
> +
> + properties:
> + '#address-cells':
> + const: 1
> + '#size-cells':
> + const: 0
> +
> + patternProperties:
> + '^i2c@([01]+)$':
Only 0 or 1 is valid? Then drop the '+'.
This is an i2c bus, so you need:
$ref: /schemas/i2c-controller.yaml#'
unevaluatedProperties: false
> + type: object
> +
> + properties:
> + reg:
> + maxItems: 1
> + '#address-cells':
> + const: 1
> + '#size-cells':
> + const: 0
You can drop these as i2c-controller.yaml covers them.
> + clock-frequency:
> + minimum: 1
1 is already the minimum. (Well, maybe it is 0, but that's not really
useful.)
> + maximum: 1000000
> +
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - ports
> + - remote-chips
> + - i2c-atr
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/media/ds90ub953.h>
> +
> + i2c@0 {
> + reg = <0x0 0x100>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + 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_LOW>;
> + reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
> +
> + #clock-cells = <0>;
> +
> + i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + ds90ub954_fpd3_in0: endpoint {
> + remote-endpoint = <&sensor_0_out>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> + ds90ub954_mipi_out0: endpoint {
> + data-lanes = <1 2 3 4>;
> + link-frequencies = /bits/ 64 <400000000>;
> + remote-endpoint = <&csirx_0_in>;
> + };
> + };
> + };
> +
> + remote-chips {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + des0_ser0: remote-chip@0 {
> + reg = <0>;
> + compatible = "ti,ds90ub953-q1";
> + clocks = <&deser>;
> + ti,gpio-functions =
> + <DS90_GPIO_FUNC_UNUSED
> + DS90_GPIO_FUNC_OUTPUT_REMOTE
> + DS90_GPIO_FUNC_UNUSED
> + DS90_GPIO_FUNC_UNUSED>;
> +
> + gpio-controller;
> + #gpio-cells = <2>;
> + #clock-cells = <0>;
> + };
> + };
> +
> + i2c-atr {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + remote_i2c0: i2c@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + sensor_0@1a {
> + compatible = "sony,imx274";
> + reg = <0x1a>;
> +
> + reset-gpios = <&des0_ser0 1 GPIO_ACTIVE_LOW>;
> +
> + port {
> + sensor_0_out: endpoint {
> + remote-endpoint = <&ds90ub954_fpd3_in0>;
> + };
> + };
> + };
> + };
> + };
> + };
> + };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4429ce035496..f0156062f788 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19097,6 +19097,12 @@ S: Maintained
> F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
> F: include/dt-bindings/media/ds90ub953.h
>
> +TEXAS INSTRUMENTS DS90UB954 VIDEO DESERIALIZER DRIVER
> +M: Luca Ceresoli <luca@lucaceresoli.net>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.yaml
> +
> TEXAS INSTRUMENTS' SYSTEM CONTROL INTERFACE (TISCI) PROTOCOL DRIVER
> M: Nishanth Menon <nm@ti.com>
> M: Tero Kristo <kristo@kernel.org>
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 3/6] media: dt-bindings: add DS90UB953-Q1 video serializer
2022-02-06 11:59 ` [RFCv3 3/6] media: dt-bindings: add DS90UB953-Q1 video serializer Luca Ceresoli
@ 2022-02-07 21:48 ` Rob Herring
0 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2022-02-07 21:48 UTC (permalink / raw)
To: Luca Ceresoli
Cc: linux-media, linux-i2c, devicetree, linux-kernel, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Tomi Valkeinen, matti.vaittinen
On Sun, Feb 06, 2022 at 12:59:36PM +0100, Luca Ceresoli wrote:
> Describe the Texas Instruments DS90UB953-Q1, a MIPI CSI-2 video serializer
> with I2C Address Translator and remote GPIOs.
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> ---
>
> Changes RFCv2 -> RFCv3:
>
> - rewrite in yaml
>
> Changes RFCv1 -> RFCv2: none, this patch is new in RFCv2
> ---
> .../bindings/media/i2c/ti,ds90ub953-q1.yaml | 96 +++++++++++++++++++
> MAINTAINERS | 7 ++
> include/dt-bindings/media/ds90ub953.h | 16 ++++
> 3 files changed, 119 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
> create mode 100644 include/dt-bindings/media/ds90ub953.h
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
> new file mode 100644
> index 000000000000..2a836a3e65e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2019 Renesas Electronics Corp.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub953-q1.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments DS90UB953-Q1 video serializer
> +
> +maintainers:
> + - Luca Ceresoli <luca@lucaceresoli.net>
> +
> +description: |
> + 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/remote-chips/remote-chip@[01]" node.
> +
> +properties:
> + compatible:
> + const: ti,ds90ub953-q1
> +
> + reg:
> + description: |
> + Index of the remote (serializer) RX port that this serializer is
> + connected to.
> + maxItems: 1
> +
> + clocks:
> + description: FPD-Link line rate (provided by the deserializer)
> + maxItems: 1
> +
> + gpio-controller: true
> +
> + '#gpio-cells':
> + const: 2
> +
> + '#clock-cells':
> + const: 0
> +
> + ti,gpio-functions:
> + description: |
> + 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 unused
> + - DS90_GPIO_FUNC_INPUT (1): the GPIO is an input to the ds90ub953,
> + the remote chip (deserializer) can read its value
> + - DS90_GPIO_FUNC_OUTPUT_REMOTE (2): the GPIO is an output from the
> + ds90ub953, the remote chip (deserializer) can set its value
> + For unspecified values the GPIO is assumed to be unused.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + maxItems: 4
> +
> +patternProperties:
> + '^ti,ds90ub953-q1-(clk|d[0-3])-inv-pol-quirk$':
> + description: |
> + The MIPI CSI-2 input clock lane or any of the data lanes has inverted
> + polarity in hardware
What's the type?
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - gpio-controller
> + - '#gpio-cells'
> + - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/media/ds90ub953.h>
> + remote-chips {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + remote-chip@0 {
> + reg = <0>;
> + compatible = "ti,ds90ub953-q1";
> + clocks = <&deser>;
> + ti,gpio-functions =
> + <DS90_GPIO_FUNC_UNUSED
> + DS90_GPIO_FUNC_OUTPUT_REMOTE
> + DS90_GPIO_FUNC_UNUSED
> + DS90_GPIO_FUNC_UNUSED>;
> +
> + gpio-controller;
> + #gpio-cells = <2>;
> + #clock-cells = <0>;
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7383aec87e4a..4429ce035496 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19090,6 +19090,13 @@ F: include/linux/dma/k3-udma-glue.h
> F: include/linux/dma/ti-cppi5.h
> F: include/linux/dma/k3-psil.h
>
> +TEXAS INSTRUMENTS DS90UB953 VIDEO SERIALIZER DRIVER
> +M: Luca Ceresoli <luca@lucaceresoli.net>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/i2c/ti,ds90ub953-q1.yaml
> +F: include/dt-bindings/media/ds90ub953.h
> +
> TEXAS INSTRUMENTS' SYSTEM CONTROL INTERFACE (TISCI) PROTOCOL DRIVER
> M: Nishanth Menon <nm@ti.com>
> M: Tero Kristo <kristo@kernel.org>
> 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 */
Dual license please.
> +/**
> + * 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.25.1
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)
2022-02-07 16:23 ` Tomi Valkeinen
@ 2022-02-08 6:40 ` Vaittinen, Matti
2022-02-08 8:28 ` Tomi Valkeinen
0 siblings, 1 reply; 24+ messages in thread
From: Vaittinen, Matti @ 2022-02-08 6:40 UTC (permalink / raw)
To: Tomi Valkeinen, Luca Ceresoli, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Lee Jones
Morning Tomi,
On 2/7/22 18:23, Tomi Valkeinen wrote:
> On 07/02/2022 16:38, Vaittinen, Matti wrote:
>> Hi again Luca,
>>
>> On 2/7/22 16:07, Luca Ceresoli wrote:
>>> Hi Matti,
>>>
>>> On 07/02/22 14:21, Vaittinen, Matti wrote:
>>>> Hi dee Ho peeps,
>>>>
>>>> On 2/7/22 14:06, Tomi Valkeinen wrote:
>>>>> Hi Luca,
>>>>>
>>>>> On 06/02/2022 13:59, Luca Ceresoli wrote:
>>>>>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
>>>>>> DS90UB9xx serializer/deserializer chipsets with I2C address
>>>>>> translation.
>>>>
>>>>
>>>> I am not sure if I am poking in the nest of the wasps - but there's one
>>>> major difference with the work I've done and with Toni's / Luca's work.
>>>
>>> You are. ;)
>>>
>>>> The TI DES drivers (like ub960 driver) packs pretty much everything
>>>> under single driver at media/i2c - which (in my opinion) makes the
>>>> driver pretty large one.
>>>>
>>>> My approach is/was to utilize MFD - and prepare the regmap + IRQs in
>>>> the
>>>> MFD (as is pretty usual) - and parse that much of the device-tree that
>>>> we see how many SER devices are there - and that I get the non I2C
>>>> related DES<=>SER link parameters set. After that I do kick alive the
>>>> separate MFD cells for ATR, pinctrl/GPIO and media.
>>>>
>>>> The ATR driver instantiates the SER I2C devices like Toni's ub960 does.
>>>> The SER compatible is once again matched in MFD (for SER) - which again
>>>> provides regmap for SER, does initial I2C writes so SER starts
>>>> responding to I2C reads and then kicks cells for media and
>>>> pinctrl/gpio.
>>>>
>>>> I believe splitting the functionality to MFD subdevices makes drivers
>>>> slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual,
>>>> regmaps/IRQ-chips under MFD and only media/v4l2 related parts under
>>>> media.
>>>
>>> There has been quite a fiery discussion about this in the past, you can
>>> grab some popcorn and read
>>> https://lore.kernel.org/linux-media/20181008211205.2900-1-vz@mleia.com/T/#m9b01af81665ac956af3c6d57810239420c3f8cee
>>>
>>>
>>> TL;DR: there have been strong opposition the the MFD idea.
>>
>> Hm. I may be missing something but I didn't see opposition to using MFD
>> or splitting the drivers. I do see opposition to adding _functionality_
>> in MFD. If I read this correctly, Lee did oppose adding the I2C stuff,
>> sysfs attributes etc in MFD. Quoting his reply:
>>
>> "This driver does too much real work ('stuff') to be an MFD driver.
>> MFD drivers should not need to care of; links, gates, modes, pixels,
>> frequencies maps or properties. Nor should they contain elaborate
>> sysfs structures to control the aforementioned 'stuff'.
>>
>> Granted, there may be some code in there which could be appropriate
>> for an MFD driver. However most of it needs moving out into a
>> function driver (or two)."
>>
>> And I tend to agree with Lee here. I would not put I2C bridge stuff or
>> sysfs attributes in MFD. But I think it does not mean SERDESes should
>> not use MFD when they clearly contain more IP blocks than the
>> video/media ones :) I am confident Lee and others might be much more
>> welcoming for driver which simply configures regmap and kicks subdriver
>> for doing the ATR / I2C stuff.
>
> I admit that I don't know MFD drivers too well, but I was thinking about
> this some time back and I wasn't quite sure about using MFD here.
>
> My thinking was that MFD is fine and good when a device contains more or
> less independent functionalities, like a PMIC with, say, gpios and
> regulators, both of which just work as long as the PMIC is powered up.
>
> Here all the functionalities depend on the link (fpdlink or some other
> "link" =), and the serializers. In other words, the link status or any
> changes to the link or the serializers might affect the GPIO/I2C/IRQ
> functionalities.
My use case has been such that once the link between DES & SER
established, it should not go away. If it does it is some kind of an
error and there is no recovery mechanims (at least not yet). Hence I
haven't prepared full solution how to handle dropping/re-connecting the
link or re-initializing des/ser/slaves.
> So, I don't have any clear concern here. Just a vague feeling that the
> functionalities in this kind of devices may be more tightly tied
> together than in normal MFDs. I could be totally wrong here.
I can't prove you're wrong even if that would be so cool :p
I guess a lot of this boils down how the SER behaves when link is
dropped. Does it maintain the configuration or reset to some other
state? And what happens on des & what we need to do in order to reconnect.
My initial feeling is that the DES should always be available as it is
directly connected to I2C. So DES should always be there.
Access to SERs and the devices on remote buses is naturally depending on
the link. So dropping the link means access to SERs and remote devices
start failing - which is probably visible to the MFD sub-devices as
failing regmap accesses. This needs then appropriate handling.
After that being said, I think we can't get over this problem even when
not using MFD. As far as I read your code, the SER and DES have
independent drivers also when MFD is not used. So dropping the link is
still someting that pulls the legs from the SER, right? I also guess the
remote I2C devices like sensors are also implemented as independent drivers.
Well, (I hope) I'll see where I end up with my code... It really makes
this discussion a bit dull when I can't just show the code for
comparison :/ I don't (yet) see why the MFD approach could not work, and
I still think it's worth trying - but I now certainly understand why you
hesitated using MFD. Thanks for taking the time to explain this to me.
Best Regards
--Matti
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)
2022-02-08 6:40 ` Vaittinen, Matti
@ 2022-02-08 8:28 ` Tomi Valkeinen
2022-02-08 9:36 ` Vaittinen, Matti
0 siblings, 1 reply; 24+ messages in thread
From: Tomi Valkeinen @ 2022-02-08 8:28 UTC (permalink / raw)
To: Vaittinen, Matti, Luca Ceresoli, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Lee Jones
Hi,
On 08/02/2022 08:40, Vaittinen, Matti wrote:
> Morning Tomi,
>
> On 2/7/22 18:23, Tomi Valkeinen wrote:
>> On 07/02/2022 16:38, Vaittinen, Matti wrote:
>>> Hi again Luca,
>>>
>>> On 2/7/22 16:07, Luca Ceresoli wrote:
>>>> Hi Matti,
>>>>
>>>> On 07/02/22 14:21, Vaittinen, Matti wrote:
>>>>> Hi dee Ho peeps,
>>>>>
>>>>> On 2/7/22 14:06, Tomi Valkeinen wrote:
>>>>>> Hi Luca,
>>>>>>
>>>>>> On 06/02/2022 13:59, Luca Ceresoli wrote:
>>>>>>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI
>>>>>>> DS90UB9xx serializer/deserializer chipsets with I2C address
>>>>>>> translation.
>>>>>
>>>>>
>>>>> I am not sure if I am poking in the nest of the wasps - but there's one
>>>>> major difference with the work I've done and with Toni's / Luca's work.
>>>>
>>>> You are. ;)
>>>>
>>>>> The TI DES drivers (like ub960 driver) packs pretty much everything
>>>>> under single driver at media/i2c - which (in my opinion) makes the
>>>>> driver pretty large one.
>>>>>
>>>>> My approach is/was to utilize MFD - and prepare the regmap + IRQs in
>>>>> the
>>>>> MFD (as is pretty usual) - and parse that much of the device-tree that
>>>>> we see how many SER devices are there - and that I get the non I2C
>>>>> related DES<=>SER link parameters set. After that I do kick alive the
>>>>> separate MFD cells for ATR, pinctrl/GPIO and media.
>>>>>
>>>>> The ATR driver instantiates the SER I2C devices like Toni's ub960 does.
>>>>> The SER compatible is once again matched in MFD (for SER) - which again
>>>>> provides regmap for SER, does initial I2C writes so SER starts
>>>>> responding to I2C reads and then kicks cells for media and
>>>>> pinctrl/gpio.
>>>>>
>>>>> I believe splitting the functionality to MFD subdevices makes drivers
>>>>> slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual,
>>>>> regmaps/IRQ-chips under MFD and only media/v4l2 related parts under
>>>>> media.
>>>>
>>>> There has been quite a fiery discussion about this in the past, you can
>>>> grab some popcorn and read
>>>> https://lore.kernel.org/linux-media/20181008211205.2900-1-vz@mleia.com/T/#m9b01af81665ac956af3c6d57810239420c3f8cee
>>>>
>>>>
>>>> TL;DR: there have been strong opposition the the MFD idea.
>>>
>>> Hm. I may be missing something but I didn't see opposition to using MFD
>>> or splitting the drivers. I do see opposition to adding _functionality_
>>> in MFD. If I read this correctly, Lee did oppose adding the I2C stuff,
>>> sysfs attributes etc in MFD. Quoting his reply:
>>>
>>> "This driver does too much real work ('stuff') to be an MFD driver.
>>> MFD drivers should not need to care of; links, gates, modes, pixels,
>>> frequencies maps or properties. Nor should they contain elaborate
>>> sysfs structures to control the aforementioned 'stuff'.
>>>
>>> Granted, there may be some code in there which could be appropriate
>>> for an MFD driver. However most of it needs moving out into a
>>> function driver (or two)."
>>>
>>> And I tend to agree with Lee here. I would not put I2C bridge stuff or
>>> sysfs attributes in MFD. But I think it does not mean SERDESes should
>>> not use MFD when they clearly contain more IP blocks than the
>>> video/media ones :) I am confident Lee and others might be much more
>>> welcoming for driver which simply configures regmap and kicks subdriver
>>> for doing the ATR / I2C stuff.
>>
>> I admit that I don't know MFD drivers too well, but I was thinking about
>> this some time back and I wasn't quite sure about using MFD here.
>>
>> My thinking was that MFD is fine and good when a device contains more or
>> less independent functionalities, like a PMIC with, say, gpios and
>> regulators, both of which just work as long as the PMIC is powered up.
>>
>> Here all the functionalities depend on the link (fpdlink or some other
>> "link" =), and the serializers. In other words, the link status or any
>> changes to the link or the serializers might affect the GPIO/I2C/IRQ
>> functionalities.
>
> My use case has been such that once the link between DES & SER
> established, it should not go away. If it does it is some kind of an
> error and there is no recovery mechanims (at least not yet). Hence I
> haven't prepared full solution how to handle dropping/re-connecting the
> link or re-initializing des/ser/slaves.
>
>> So, I don't have any clear concern here. Just a vague feeling that the
>> functionalities in this kind of devices may be more tightly tied
>> together than in normal MFDs. I could be totally wrong here.
>
> I can't prove you're wrong even if that would be so cool :p
>
> I guess a lot of this boils down how the SER behaves when link is
> dropped. Does it maintain the configuration or reset to some other
> state? And what happens on des & what we need to do in order to reconnect.
>
> My initial feeling is that the DES should always be available as it is
> directly connected to I2C. So DES should always be there.
Yes, I don't see how DES would be affected. But all the services offered
by the MFDs are behind the link.
> Access to SERs and the devices on remote buses is naturally depending on
> the link. So dropping the link means access to SERs and remote devices
> start failing - which is probably visible to the MFD sub-devices as
> failing regmap accesses. This needs then appropriate handling.
I was also thinking about cases like BIST or link-analysis which
temporarily affect the link. They're not errors, but I guess from MFD's
point of view they could be handled the same way (whatever that way is).
> After that being said, I think we can't get over this problem even when
> not using MFD. As far as I read your code, the SER and DES have
> independent drivers also when MFD is not used. So dropping the link is
> still someting that pulls the legs from the SER, right? I also guess the
> remote I2C devices like sensors are also implemented as independent drivers.
That's true. I don't think the problem is really different with or
without MFDs. My thinking was just that it's easier to manage all the
problem cases if there are no walls between the components.
> Well, (I hope) I'll see where I end up with my code... It really makes
> this discussion a bit dull when I can't just show the code for
> comparison :/ I don't (yet) see why the MFD approach could not work, and
> I still think it's worth trying - but I now certainly understand why you
> hesitated using MFD. Thanks for taking the time to explain this to me.
I don't think MFD approach could not work. I just don't see why to use
it here.
I'm curious, why do you think using MFDs makes the driver so much
cleaner? The current fpdlink driver is in one file, but, say, if we
split it to multiple files, based on the function, while still keeping
it as a single driver, would that be so much different from an MFD
solution? Is there something in the MFD approach that makes the code
simpler?
Tomi
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,)
2022-02-08 8:28 ` Tomi Valkeinen
@ 2022-02-08 9:36 ` Vaittinen, Matti
0 siblings, 0 replies; 24+ messages in thread
From: Vaittinen, Matti @ 2022-02-08 9:36 UTC (permalink / raw)
To: Tomi Valkeinen, Luca Ceresoli, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Lee Jones
On 2/8/22 10:28, Tomi Valkeinen wrote:
> I'm curious, why do you think using MFDs makes the driver so much
> cleaner? The current fpdlink driver is in one file, but, say, if we
> split it to multiple files, based on the function, while still keeping
> it as a single driver, would that be so much different from an MFD
> solution? Is there something in the MFD approach that makes the code
> simpler?
Fair question.
I personally have two reasons - one technical which I could just throw
here and hope everyone buys it :) But I think the main reason for me to
initially think of MFD is not a technical one.
Last few years I've spent working with PMICs/chargers - which were MFD
to the bone. Before that I worked on a proprietary clocking/all-purpose
FPGA as well as with ASIC routing RP3/RP301 links + providing timing
facilities / clocks. Those were also MFD devices - and one of those used
MFD drivers, the other didn't although it really should have.
So the non technical reason for me is that I am used to seeing
multi-function devices implemented as MFD devices - hence I immediately
saw the SERDES as one too.
One the technical benefit from MFD is that it (in many cases) allows one
to use standard way to re-use code. Eg, it's not a rare case that same
HW blocks are used in many projects. One can for example see three
different PMICs, all having same RTC and clk blocks, while different
regulators and GPIOs - or some just omitting one of those.
MFD allows 'collecting' these IP blocks under different umbrellas by
kicking same subdevices alive via different MFD devices in a standard
way. The IP block (say GPIO controller) we are driving can be
implemented on SER connected by FPDLINK III to DES - or it can be
implemented in PMIC - the absolutely same standrd (mfd sub) platform
GPIO driver can be used in both cases.
Other than that, the use of MFD allows one to write pinctrl/gpio driver
as any pinctrl/gpio platform device driver. It will be looking familiar
to anyone who has worked with pinctrl/gpio - even if he has never seen
media/v4l2 ;) This is where my thought of clarity came from.
Rest is slightly offtopic - you can stop reading.
I am not sure how TI does this and if you know whether same blocks can
be used in other devices. I just have learned never to trust it when a
HW engineer (in Nokia, Rohm or other company) tells me "this is the last
IC using this technology". It may be my problem though as nor do I buy
it if someone says me: "the next version will be just same to this
previous - there is no software impact" :rolleyes: I for example once
heard that when the "next product" maintained same register offsets for
some functions - but did add new ones - and changed the registers from
16bit => 32bit and connecting bus from PCIe => I2C... I remember that
project with a nicknames 're-estimate' 'reschedule' and 'panic-button' :)
Yours
-- Matti
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support
2022-02-06 11:59 ` [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
@ 2022-02-08 11:16 ` Andy Shevchenko
2022-02-16 8:40 ` Luca Ceresoli
2022-03-16 14:11 ` Vaittinen, Matti
1 sibling, 1 reply; 24+ messages in thread
From: Andy Shevchenko @ 2022-02-08 11:16 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Linux Media Mailing List, linux-i2c, devicetree,
Linux Kernel Mailing List, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Tomi Valkeinen, Matti Vaittinen
On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>
> An ATR is a device that looks similar to an i2c-mux: it has an I2C
> slave "upstream" port and N master "downstream" ports, and forwards
> transactions from upstream to the appropriate downstream port. But is
> is different in that the forwarded transaction has a different slave
> address. The address used on the upstream bus is called the "alias"
> and is (potentially) different from the physical slave address of the
> downstream chip.
>
> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
> implementing ATR features in a device driver. The helper takes care or
> adapter creation/destruction and translates addresses at each transaction.
Why I2C mux driver can't be updated to support this feature?
...
> 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:
While this is from a code perspective, maybe i2c mux and this one can
still share some parts?
...
> +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.
What would be the module name?
...
> +/**
Is this a kernel doc formatted documentation?
Haven't you got a warning?
> + * I2C Address Translator
> + *
> + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
2019,2022?
> + *
> + * 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
> + */
Shouldn't this comment be somewhere under Documentation/ ?
...
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +static int i2c_atr_map_msgs(struct i2c_atr_chan *chan,
> + struct i2c_msg msgs[], int num)
foo[] makes not much sense in the function parameter. *foo is what
will be used and it's explicit.
Can this be located on one line (similar question to make compact the
rest of the function declarations)?
> +
Redundant blank line.
...
> + /* 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);
Use kmalloc_array()
> + if (new_buf == NULL)
> + return -ENOMEM;
> +
> + kfree(chan->orig_addrs);
Hmm... is it a reimplementation of krealloc_array()?
> + chan->orig_addrs = new_buf;
> + chan->orig_addrs_size = num;
> + }
...
> + if (c2a) {
> + msgs[i].addr = c2a->alias;
> + } else {
> + dev_err(atr->dev, "client 0x%02x not mapped!\n",
> + msgs[i].addr);
> + return -ENXIO;
> + }
'else' would be redundant if you switch to the traditional pattern,
i.e. check for errors first.
...
> +/*
> + * 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()
> + */
Too sparse formatting of the comment. Can you make it compact?
...
> + int ret = 0;
Unneeded assignment.
> + /* 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;
goto out_unlock_deselect;
> + }
> +
> + /* Perform the transfer */
> + ret = i2c_transfer(parent, msgs, num);
> +
> + /* Restore addresses */
> + i2c_atr_unmap_msgs(chan, msgs, num);
out_unlock_deselct:
> + mutex_unlock(&chan->orig_addrs_lock);
> +out:
out_deselect:
> + if (atr->ops->deselect)
> + atr->ops->deselect(atr, chan->chan_id);
> +
> + return ret;
> +}
...
> + int err = 0;
Be consistent with ret vs. err across the functions.
> + if (atr->ops->select)
> + err = atr->ops->select(atr, chan->chan_id);
> + if (!err)
Perhaps
int ret;
ret = 0;
if (atr->ops->select)
ret = atr->ops->select(atr, chan->chan_id);
if (ret)
goto out_deselect;
> + err = i2c_smbus_xfer(parent, c2a->alias, flags,
> + read_write, command, size, data);
out_deselect:
> + if (atr->ops->deselect)
> + atr->ops->deselect(atr, chan->chan_id);
> +
> + return err;
> +}
...
> + int err = 0;
Same as above: naming, useless assignment.
...
> + c2a = kzalloc(sizeof(struct i2c_atr_cli2alias_pair), GFP_KERNEL);
sizeof(*c2a)
> + if (!c2a) {
> + err = -ENOMEM;
> + goto err_alloc;
Useless label, return directly.
> + }
...
> + c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client);
> + if (c2a != NULL) {
if (c2a)
> + list_del(&c2a->node);
> + kfree(c2a);
> + }
...
> + char symlink_name[20];
Why 20? Do we have a predefined constant for that?
> + if (dev->of_node) {
This check can be dropped, also please use device property and fwnode
APIs. No good of having OF-centric generic modules nowadays.
> + struct device_node *atr_node;
> + struct device_node *child;
> + u32 reg;
> +
> + atr_node = of_get_child_by_name(dev->of_node, "i2c-atr");
atr_node = device_get_named_child_node(...);
fwnode_for_each_child_node() {
}
> + for_each_child_of_node(atr_node, child) {
> + err = of_property_read_u32(child, "reg", ®);
> + if (err)
> + continue;
> + if (chan_id == reg)
> + break;
> + }
> +
> + chan->adap.dev.of_node = child;
> + of_node_put(atr_node);
> + }
On the second thought can you utilize the parser from I2C mux?
...
> + 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);
Doesn't sysfs already has a warning when it's really needed?
...
> + if (atr->adapter[chan_id] == NULL) {
> + dev_err(dev, "Adapter %d does not exist\n", chan_id);
Noisy message. On freeing we usually don't issue such when we try to
free already freeed resource.
> + return;
> + }
...
> + atr = devm_kzalloc(dev, sizeof(*atr)
> + + max_adapters * sizeof(atr->adapter[0]),
> + GFP_KERNEL);
Check overflow.h and use respective macro here.
> + if (!atr)
> + return ERR_PTR(-ENOMEM);
...
> +/**
It's not a kernel doc.
> + * drivers/i2c/i2c-atr.h -- I2C Address Translator
Please, no names of the files inside the files.
> + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>
2019,2022 ?
> + * Based on i2c-mux.h
> + */
...
> +#ifdef __KERNEL__
Why?
...
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
Missed types.h
Missed struct device;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support
2022-02-08 11:16 ` Andy Shevchenko
@ 2022-02-16 8:40 ` Luca Ceresoli
2022-02-17 5:12 ` Vaittinen, Matti
0 siblings, 1 reply; 24+ messages in thread
From: Luca Ceresoli @ 2022-02-16 8:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Media Mailing List, linux-i2c, devicetree,
Linux Kernel Mailing List, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Tomi Valkeinen, Matti Vaittinen
Hi Andy,
thank you for the _very_ detailed review and apologies for not having
found the time to reply until now.
I'm OK with most of your comments, so I'm not commenting on them for
brevity. Below my comments on the remaining topics.
On 08/02/22 12:16, Andy Shevchenko wrote:
> On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>>
>> An ATR is a device that looks similar to an i2c-mux: it has an I2C
>> slave "upstream" port and N master "downstream" ports, and forwards
>> transactions from upstream to the appropriate downstream port. But is
>> is different in that the forwarded transaction has a different slave
>> address. The address used on the upstream bus is called the "alias"
>> and is (potentially) different from the physical slave address of the
>> downstream chip.
>>
>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
>> implementing ATR features in a device driver. The helper takes care or
>> adapter creation/destruction and translates addresses at each transaction.
>
> Why I2C mux driver can't be updated to support this feature?
My first version did that. But it was very complex to shoehorn the ATR
features in the i2c-mux code which already handles various [corner]
cases. If memory serves, code reuse was limited to the trivial code:
allocations, cleanups and the like.
The root reason is that an atr and a mux have a similar electric
topology, but they do very different things. An mux need to be commanded
to switch from one downstream bus to another, an atr does not. An atr
modifies the transaction, including the speed, a mux does not.
>> 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:
>
> While this is from a code perspective, maybe i2c mux and this one can
> still share some parts?
Possibly. I'd have to look into that in more detail.
I must say having a separate file allowed me to be free to implement
whatever is best for atr. With that done I would certainly make sense to
check whether there are still enough commonalities to share code, maybe
in a .c file with shared functions.
>> +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.
>
> What would be the module name?
Isn't the module name written in Kconfig files just to avoid checkpatch
complain about "too few doc lines"? :) Oook, it's i2s-atr anyway.
>> +/**
>
> Is this a kernel doc formatted documentation?
> Haven't you got a warning?
Not from checkpatch, but I got one from the kernel test robot. Will fix.
[...]
>> + *
>> + * 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
>> + */
>
> Shouldn't this comment be somewhere under Documentation/ ?
Uhm, yes, I agree it's a good idea to move this entire comment there.
>> + if (dev->of_node) {
>
> This check can be dropped, also please use device property and fwnode
> APIs. No good of having OF-centric generic modules nowadays.
Sure! This code was written in another decade and I didn't update it...
As you noticed elsewhere it also honors the old, strict 80-chars per
line limit in various places where it makes no sense anymore.
>> + 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);
>
> Doesn't sysfs already has a warning when it's really needed?
I have to check that. I usually don't add unnecessary log messages.
[...]
>> +#include <linux/i2c.h>
>> +#include <linux/mutex.h>
>
> Missed types.h
>
> Missed struct device;
Not sure I got your point here. This file has some 'struct device *',
which do not need a declaration, and has zero non-pointer uses of
'struct device'.
--
Luca
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support
2022-02-16 8:40 ` Luca Ceresoli
@ 2022-02-17 5:12 ` Vaittinen, Matti
0 siblings, 0 replies; 24+ messages in thread
From: Vaittinen, Matti @ 2022-02-17 5:12 UTC (permalink / raw)
To: Luca Ceresoli, Andy Shevchenko
Cc: Linux Media Mailing List, linux-i2c, devicetree,
Linux Kernel Mailing List, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Tomi Valkeinen
On 2/16/22 10:40, Luca Ceresoli wrote:
> On 08/02/22 12:16, Andy Shevchenko wrote:
>> On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>>> +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.
>>
>> What would be the module name?
>
> Isn't the module name written in Kconfig files just to avoid checkpatch
> complain about "too few doc lines"? :) Oook, it's i2s-atr anyway.
Thanks Luca! I have always wondered why people keep adding this
seemingly unnecessary boilerplate. Now I finally get the purpose!
--Matti
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support
2022-02-06 11:59 ` [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
2022-02-08 11:16 ` Andy Shevchenko
@ 2022-03-16 14:11 ` Vaittinen, Matti
2022-03-16 14:25 ` Luca Ceresoli
1 sibling, 1 reply; 24+ messages in thread
From: Vaittinen, Matti @ 2022-03-16 14:11 UTC (permalink / raw)
To: Luca Ceresoli, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Tomi Valkeinen, Mutanen, Mikko
Hi dee Ho peeps!
On 2/6/22 13:59, Luca Ceresoli wrote:
> An ATR is a device that looks similar to an i2c-mux: it has an I2C
> slave "upstream" port and N master "downstream" ports, and forwards
> transactions from upstream to the appropriate downstream port. But is
> is different in that the forwarded transaction has a different slave
> address. The address used on the upstream bus is called the "alias"
> and is (potentially) different from the physical slave address of the
> downstream chip.
>
> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
> implementing ATR features in a device driver. The helper takes care or
> adapter creation/destruction and translates addresses at each transaction.
>
snip
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index 438905e2a1d0..c6d1a345ea6d 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.
> +
I continued playing with the ROHM (de-)serializer and ended up having
.config where the I2C_ATR was ='m', while my ATR driver was ='y' even
though it selects the I2C_ATR.
Yep, most probably my error somewhere.
Anyways, this made me think that most of the I2C_ATR users are likely to
just silently select the I2C_ATR, right? The I2C_ATR has no much reason
to be compiled in w/o users, right? So perhaps the menu entry for
selecting the I2C_ATR could be dropped(?) Do we really need this entry
in already long list of configs to be manually picked?
snip
> +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);
> +
I believe that most of the attach_client implementations will have
similar approach of allocating and populating an address-pool and
searching for first unused address. As a 'further dev' it'd be great to
see a common helper implementation for attach/detach - perhaps so that
the atr drivers would only need to specify the slave-address
configuration register(s) / mask and the use a 'generic' attach/detach
helpers. Well, just thinking how to reduce the code from actual IC
drivers but this is really not something that is required during this
initial series :)
Also, devm-variants would be great - although that falls to the same
category of things that do not need to be done immediately - but would
perhaps be worth considering in the future.
so, perhaps reconsider the Kconfig but for what-ever it is worth:
Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Yours
Matti
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support
2022-03-16 14:11 ` Vaittinen, Matti
@ 2022-03-16 14:25 ` Luca Ceresoli
0 siblings, 0 replies; 24+ messages in thread
From: Luca Ceresoli @ 2022-03-16 14:25 UTC (permalink / raw)
To: Vaittinen, Matti, linux-media, linux-i2c
Cc: devicetree, linux-kernel, Rob Herring, Mark Rutland,
Wolfram Sang, Sakari Ailus, Hans Verkuil, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy, Peter Rosin,
Mauro Carvalho Chehab, Tomi Valkeinen, Mutanen, Mikko
Hi Matti,
On 16/03/22 15:11, Vaittinen, Matti wrote:
> Hi dee Ho peeps!
>
> On 2/6/22 13:59, Luca Ceresoli wrote:
>> An ATR is a device that looks similar to an i2c-mux: it has an I2C
>> slave "upstream" port and N master "downstream" ports, and forwards
>> transactions from upstream to the appropriate downstream port. But is
>> is different in that the forwarded transaction has a different slave
>> address. The address used on the upstream bus is called the "alias"
>> and is (potentially) different from the physical slave address of the
>> downstream chip.
>>
>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
>> implementing ATR features in a device driver. The helper takes care or
>> adapter creation/destruction and translates addresses at each transaction.
>>
>
> snip
>
>> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
>> index 438905e2a1d0..c6d1a345ea6d 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.
>> +
>
> I continued playing with the ROHM (de-)serializer and ended up having
> .config where the I2C_ATR was ='m', while my ATR driver was ='y' even
> though it selects the I2C_ATR.
>
> Yep, most probably my error somewhere.
>
> Anyways, this made me think that most of the I2C_ATR users are likely to
> just silently select the I2C_ATR, right? The I2C_ATR has no much reason
> to be compiled in w/o users, right? So perhaps the menu entry for
> selecting the I2C_ATR could be dropped(?) Do we really need this entry
> in already long list of configs to be manually picked?
Maybe we could make it a blind option, sure. The only reason it could be
useful that it's visible is that one might implement a user driver could
be written out of tree. I don't care very much about that, but it is
possible. Maybe it's the reason for I2C_MUX to be a visible option too.
Peter?
>> +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);
>> +
>
> I believe that most of the attach_client implementations will have
> similar approach of allocating and populating an address-pool and
> searching for first unused address. As a 'further dev' it'd be great to
> see a common helper implementation for attach/detach - perhaps so that
> the atr drivers would only need to specify the slave-address
> configuration register(s) / mask and the use a 'generic' attach/detach
> helpers. Well, just thinking how to reduce the code from actual IC
> drivers but this is really not something that is required during this
> initial series :)
>
> Also, devm-variants would be great - although that falls to the same
> category of things that do not need to be done immediately - but would
> perhaps be worth considering in the future.
Both of your proposals make sense, however I did deliberately not
generalize too much because I knew only one chipset. I don't like trying
to generalize for an unpredictable future use case, it generally leads
(me) to generalizing in the wrong direction. That means you'd be very
welcome to propose helpers and/or devm variants, possibly in the same
patchset as the first Rohm serdes driver. ;)
> Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Thanks for your review!
--
Luca
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-03-16 14:25 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-06 11:59 [RFCv3 0/6] Hi, Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
2022-02-08 11:16 ` Andy Shevchenko
2022-02-16 8:40 ` Luca Ceresoli
2022-02-17 5:12 ` Vaittinen, Matti
2022-03-16 14:11 ` Vaittinen, Matti
2022-03-16 14:25 ` Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 3/6] media: dt-bindings: add DS90UB953-Q1 video serializer Luca Ceresoli
2022-02-07 21:48 ` Rob Herring
2022-02-06 11:59 ` [RFCv3 4/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
2022-02-06 18:46 ` Rob Herring
2022-02-07 19:39 ` Rob Herring
2022-02-06 11:59 ` [RFCv3 5/6] media: ds90ub954: new driver for TI " Luca Ceresoli
2022-02-06 11:59 ` [RFCv3 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer Luca Ceresoli
2022-02-06 12:05 ` [RFCv3 0/6] TI camera serdes and I2C address translation Luca Ceresoli
2022-02-07 12:06 ` [RFCv3 0/6] TI camera serdes and I2C address translation (Was: [RFCv3 0/6] Hi,) Tomi Valkeinen
2022-02-07 13:21 ` Vaittinen, Matti
2022-02-07 14:07 ` Luca Ceresoli
2022-02-07 14:38 ` Vaittinen, Matti
2022-02-07 16:23 ` Tomi Valkeinen
2022-02-08 6:40 ` Vaittinen, Matti
2022-02-08 8:28 ` Tomi Valkeinen
2022-02-08 9:36 ` Vaittinen, Matti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).