Re: [PATCH v2 1/2] media: i2c: ds90ub960: Enable CSI TX1 port
From: Tomi Valkeinen
Date: Mon Jul 20 2026 - 10:39:16 EST
Hi,
On 20/07/2026 14:09, Rishikesh Donadkar wrote:
From: Yemike Abhilash Chandra <y-abhilashchandra@xxxxxx>I think you could expend the above a bit to explain how the HW handles the I2C routing.
The DS90UB960 chip has two CSI-2 transmit ports (TX0 and TX1), but the
current driver implementation only utilizes TX0. To enable TX1 as the
active output port, the I2C Bidirectional Control Channel (BCC) mapping in
the RX_PORT_CTL register must be configured to use I2C Slave Port 1.
This patch adds a new function, ub960_parse_active_ports(), which is
called during driver initialization to scan the device tree and identify
which RX and TX ports are enabled. The function creates bitmasks
representing the active ports and uses these masks to correctly configure
the RX_PORT_CTL register, ensuring proper port routing for whichever TX
port is in use.
This doesn't quite explain how the decisions about routing are done. What if two TX ports are enabled?
DS90UB960 data sheet: https://www.ti.com/lit/ds/symlink/ds90ub960-q1.pdf
I don't think there's a need to link to the datasheet in the commit. Unless, say, you add support for a new device.
Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@xxxxxx>
Co-developed-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
Signed-off-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
---
drivers/media/i2c/ds90ub960.c | 46 ++++++++++++++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 15a9797b47ac..653dc7a4eee5 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -26,6 +26,7 @@
* - i2c-atr could be made embeddable instead of allocatable.
*/
+#include <linux/bitfield.h>
#include <linux/bitops.h>
#include <linux/cleanup.h>
#include <linux/clk.h>
@@ -76,6 +77,8 @@
#define UB960_NUM_BC_GPIOS 4
+#define UB960_CSI_TX0 BIT(4)
This is a very strange looking define.
+
/*
* Register map
*
@@ -114,6 +117,7 @@
#define UB960_SR_SCL_HIGH_TIME 0x0a
#define UB960_SR_SCL_LOW_TIME 0x0b
#define UB960_SR_RX_PORT_CTL 0x0c
+#define UB960_SR_RX_PORT_CTL_BCC_MAP GENMASK(7, 4)
#define UB960_SR_IO_CTL 0x0d
#define UB960_SR_GPIO_PIN_STS 0x0e
#define UB960_SR_GPIO_INPUT_CTL 0x0f
@@ -589,6 +593,9 @@ struct ub960_data {
u32 tx_data_rate; /* Nominal data rate (Gb/s) */
s64 tx_link_freq[1];
+ u8 rx_mask;
+ u8 tx_mask;
+
struct i2c_atr *atr;
struct {
@@ -2538,7 +2545,18 @@ static int ub960_init_rx_ports_ub960(struct ub960_data *priv)
struct device *dev = &priv->client->dev;
unsigned int port_lock_mask;
unsigned int port_mask;
- int ret;
+ u8 enabled_rxports_mask;
+ u8 enabled_rxports;
+ int ret = 0;
+
+ /* Configure I2C interface for RX ports */
+ enabled_rxports_mask = FIELD_PREP(UB960_SR_RX_PORT_CTL_BCC_MAP, priv->rx_mask);
+ enabled_rxports = (priv->tx_mask & UB960_CSI_TX0) ? 0x00 : enabled_rxports_mask;
tx_mask should have bits 0 or 1 set, for TX0 and TX1.
This looks quite confusing. I think the comment could at least say what's the strategy here, how does it configure the ports.
Isn't this just a boolean switch? If TX0 is used, all ports are routed there? Else TX1 is used for all ports. Why do you even need tx_mask and rx_mask? And why the enabled_rxports_mask. Or maybe I just fail to read the code, thus a better comment or patch desc might help.
+ ret = ub960_update_bits(priv, UB960_SR_RX_PORT_CTL, enabled_rxports_mask,
+ enabled_rxports, &ret);
+ if (ret)
+ return ret;
for_each_active_rxport(priv, it) {
ret = ub960_init_rx_port_ub960(priv, it.rxport);
@@ -4789,6 +4807,30 @@ static int ub960_parse_dt_txports(struct ub960_data *priv)
return 0;
}
+static void ub960_parse_active_ports(struct ub960_data *priv)
+{
+ struct device *dev = &priv->client->dev;
+ int nport;
+
+ priv->rx_mask = 0;
+ priv->tx_mask = 0;
+
+ for (nport = 0; nport < priv->hw_data->num_rxports + priv->hw_data->num_txports; nport++) {
+ struct fwnode_handle *ep_fwnode;
+
+ ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), nport, 0, 0);
+ if (!ep_fwnode)
+ continue;
+
+ if (nport < priv->hw_data->num_rxports)
+ priv->rx_mask |= BIT(nport);
+ else
+ priv->tx_mask |= BIT(nport);
+
+ fwnode_handle_put(ep_fwnode);
Does this work (or rather, the user of tx_mask) on UB954, with 2 rx ports?
+ }
+}
+
static int ub960_parse_dt(struct ub960_data *priv)
{
int ret;
@@ -5162,6 +5204,8 @@ static int ub960_probe(struct i2c_client *client)
if (ret)
goto err_mutex_destroy;
+ ub960_parse_active_ports(priv);
+
We already parse rx and tx ports in ub960_parse_dt(). Wouldn't that be a better place to collect the masks? The existing functions are doing almost exactly what you function above does, aren't they?
ret = ub960_parse_dt(priv);
if (ret)
goto err_disable_core_hw;