Re: [PATCH v2 1/2] media: i2c: ds90ub960: Enable CSI TX1 port

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi,

On 20/07/2026 14:09, Rishikesh Donadkar wrote:
> From: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> 
> 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.
I think you could expend the above a bit to explain how the HW handles 
the I2C routing.

> 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@ti.com>
> Co-developed-by: Rishikesh Donadkar <r-donadkar@ti.com>
> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> ---
>   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;