Message-ID: <c387b053-82b1-4de4-946a-5f2b9270224f@ideasonboard.com>
Date: Fri, 5 Dec 2025 13:10:29 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Yemike Abhilash Chandra <y-abhilashchandra@...com>
Cc: hansg@...nel.org, mehdi.djait@...ux.intel.com, ribalda@...omium.org,
 git@...tzsch.eu, vladimir.zapolskiy@...aro.org,
 benjamin.mugnier@...s.st.com, dongcheng.yan@...el.com, u-kumar1@...com,
 jai.luthra@...ux.dev, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 mchehab@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, hverkuil@...all.nl, sakari.ailus@...ux.intel.com,
 laurent.pinchart@...asonboard.com
Subject: Re: [PATCH V2 4/4] media: i2c: ds90ub960: Add support for
 DS90UB954-Q1

Hi,

On 02/12/2025 12:22, Yemike Abhilash Chandra wrote:
> DS90UB954-Q1 is an FPDLink-III deserializer that is mostly register
> compatible with DS90UB960-Q1. The main difference is that it supports half
> of the RX and TX ports, i.e. 2x FPDLink RX ports and 1x CSI TX port.
> 
> A couple of differences are between the status registers and the
> strobe setting registers. Hence accommodate these differences in
> the UB960 driver so that we can reuse a large part of the existing code.
> 
> Link: https://www.ti.com/lit/gpn/ds90ub954-q1
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@...com>
> ---
> Refer table 5.2.1 STROBE_SET Register in [1] for DS90UB954 strobe
> setting register.
> 
> [1]: https://www.ti.com/lit/an/snla301/snla301.pdf
> 
>  drivers/media/i2c/Kconfig     |   4 +-
>  drivers/media/i2c/ds90ub960.c | 165 +++++++++++++++++++++++++---------
>  2 files changed, 125 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 745819c625d6..52104f76e371 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1703,8 +1703,8 @@ config VIDEO_DS90UB960
>  	select V4L2_FWNODE
>  	select VIDEO_V4L2_SUBDEV_API
>  	help
> -	  Device driver for the Texas Instruments DS90UB960
> -	  FPD-Link III Deserializer and DS90UB9702 FPD-Link IV Deserializer.
> +	  Device driver for the Texas Instruments DS90UB954, DS90UB960
> +	  FPD-Link III Deserializers and DS90UB9702 FPD-Link IV Deserializer.
>  
>  config VIDEO_MAX96714
>  	tristate "Maxim MAX96714 GMSL2 deserializer"
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index 45494fcaf095..7d3e5a87bb17 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -396,6 +396,12 @@
>  #define UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY	BIT(3)
>  #define UB960_IR_RX_ANA_STROBE_SET_DATA_DELAY_MASK	GENMASK(2, 0)
>  
> +#define UB954_IR_RX_ANA_STROBE_SET_CLK_DATA		0x08
> +#define UB954_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY	BIT(3)
> +#define UB954_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY	BIT(7)
> +#define UB954_IR_RX_ANA_STROBE_SET_CLK_DELAY_MASK	GENMASK(2, 0)
> +#define UB954_IR_RX_ANA_STROBE_SET_DATA_DELAY_MASK	GENMASK(4, 6)
> +
>  /* UB9702 Registers */
>  
>  #define UB9702_SR_CSI_EXCLUSIVE_FWD2		0x3c
> @@ -455,6 +461,7 @@
>  #define UB960_NUM_EQ_LEVELS (UB960_MAX_EQ_LEVEL - UB960_MIN_EQ_LEVEL + 1)
>  
>  enum chip_type {
> +	UB954,
>  	UB960,
>  	UB9702,
>  };
> @@ -1000,6 +1007,10 @@ static int ub960_txport_select(struct ub960_data *priv, u8 nport)
>  
>  	lockdep_assert_held(&priv->reg_lock);
>  
> +	/* UB954 has only 1 CSI TX. Hence, no need to select */
> +	if (priv->hw_data->chip_type == UB954)
> +		return 0;
> +
>  	if (priv->reg_current.txport == nport)
>  		return 0;
>  
> @@ -1424,10 +1435,11 @@ static int ub960_parse_dt_txport(struct ub960_data *priv,
>  	priv->tx_link_freq[0] = vep.link_frequencies[0];
>  	priv->tx_data_rate = priv->tx_link_freq[0] * 2;
>  
> -	if (priv->tx_data_rate != MHZ(1600) &&
> -	    priv->tx_data_rate != MHZ(1200) &&
> -	    priv->tx_data_rate != MHZ(800) &&
> -	    priv->tx_data_rate != MHZ(400)) {
> +	if ((priv->tx_data_rate != MHZ(1600) &&
> +	     priv->tx_data_rate != MHZ(1200) &&
> +	     priv->tx_data_rate != MHZ(800) &&
> +	     priv->tx_data_rate != MHZ(400)) ||
> +	     (priv->hw_data->chip_type == UB954 && priv->tx_data_rate == MHZ(1200))) {
>  		dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport);
>  		ret = -EINVAL;
>  		goto err_free_vep;
> @@ -1551,22 +1563,44 @@ static int ub960_rxport_get_strobe_pos(struct ub960_data *priv,
>  	u8 clk_delay, data_delay;
>  	int ret;
>  
> -	ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> -			     UB960_IR_RX_ANA_STROBE_SET_CLK, &v, NULL);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * DS90UB960 has two separate registers for clk and data delay whereas
> +	 * DS90UB954 has a single combined register. Hence read accordingly
> +	 */

Why do you read the single register twice? In any case, I don't think
the comment is needed, as it's quite clear from the code. Unless there's
some extra complication with the registers.

> +	if (priv->hw_data->chip_type == UB954) {
> +		ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> +				     UB954_IR_RX_ANA_STROBE_SET_CLK_DATA, &v, NULL);
> +		if (ret)
> +			return ret;
>  
> -	clk_delay = (v & UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY) ?
> -			    0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
> +		clk_delay = (v & UB954_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY) ?
> +			     0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
>  
> -	ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> -			     UB960_IR_RX_ANA_STROBE_SET_DATA, &v, NULL);
> -	if (ret)
> -		return ret;
> +		ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> +				     UB954_IR_RX_ANA_STROBE_SET_CLK_DATA, &v, NULL);
> +		if (ret)
> +			return ret;
> +
> +		data_delay = (v & UB954_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY) ?
> +			      0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
> +	} else {
> +		ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> +				     UB960_IR_RX_ANA_STROBE_SET_CLK, &v, NULL);
> +		if (ret)
> +			return ret;
>  
> -	data_delay = (v & UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY) ?
> +		clk_delay = (v & UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY) ?
>  			     0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
>  
> +		ret = ub960_read_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> +				     UB960_IR_RX_ANA_STROBE_SET_DATA, &v, NULL);
> +		if (ret)
> +			return ret;
> +
> +		data_delay = (v & UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY) ?
> +			      0 : UB960_MANUAL_STROBE_EXTRA_DELAY;
> +	}
> +
>  	ret = ub960_rxport_read(priv, nport, UB960_RR_SFILTER_STS_0, &v, NULL);
>  	if (ret)
>  		return ret;
> @@ -1590,8 +1624,17 @@ static int ub960_rxport_set_strobe_pos(struct ub960_data *priv,
>  	u8 clk_delay, data_delay;
>  	int ret = 0;
>  
> -	clk_delay = UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY;
> -	data_delay = UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
> +	/*
> +	 * DS90UB960 has two separate registers for clk and data delay whereas
> +	 * DS90UB954 has a single combined register. Hence assign accordingly.
> +	 */
> +	if (priv->hw_data->chip_type == UB954) {
> +		clk_delay = UB954_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY;
> +		data_delay = UB954_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
> +	} else {
> +		clk_delay = UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY;
> +		data_delay = UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
> +	}
>  
>  	if (strobe_pos < UB960_MIN_AEQ_STROBE_POS)
>  		clk_delay = abs(strobe_pos) - UB960_MANUAL_STROBE_EXTRA_DELAY;
> @@ -1602,11 +1645,25 @@ static int ub960_rxport_set_strobe_pos(struct ub960_data *priv,
>  	else if (strobe_pos > 0)
>  		data_delay = strobe_pos | UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
>  
> -	ub960_write_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> -			UB960_IR_RX_ANA_STROBE_SET_CLK, clk_delay, &ret);
> -
> -	ub960_write_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> -			UB960_IR_RX_ANA_STROBE_SET_DATA, data_delay, &ret);
> +	/*
> +	 * DS90UB960 has two separate registers for clk and data delay whereas
> +	 * DS90UB954 has a single combined register. Hence write the registers accordingly.
> +	 */
> +	if (priv->hw_data->chip_type == UB954) {
> +		ub960_ind_update_bits(priv, UB960_IND_TARGET_RX_ANA(nport),
> +				      UB954_IR_RX_ANA_STROBE_SET_CLK_DATA,
> +				      UB954_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY,
> +				      clk_delay, &ret);
> +		ub960_ind_update_bits(priv, UB960_IND_TARGET_RX_ANA(nport),
> +				      UB954_IR_RX_ANA_STROBE_SET_CLK_DATA,
> +				      UB954_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY,
> +				      data_delay, &ret);

Here, too. It's a single register, why write it twice?

And I don't think this is correct at all... Did you validate this? The
above only sets the EXTRA_DELAY bits, not the values at all. And the
code that sets clk_delay and data_delay use UB960's bit positions, which
are not the same on UB954.

> +	} else {
> +		ub960_write_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> +				UB960_IR_RX_ANA_STROBE_SET_CLK, clk_delay, &ret);
> +		ub960_write_ind(priv, UB960_IND_TARGET_RX_ANA(nport),
> +				UB960_IR_RX_ANA_STROBE_SET_DATA, data_delay, &ret);
> +	}
>  
>  	return ret;
>  }
> @@ -4176,33 +4233,40 @@ static int ub960_log_status(struct v4l2_subdev *sd)
>  		dev_info(dev, "\tsync %u, pass %u\n", v & (u8)BIT(1),
>  			 v & (u8)BIT(0));
>  
> -		ret = ub960_read16(priv, UB960_SR_CSI_FRAME_COUNT_HI(nport),
> -				   &v16, NULL);
> -		if (ret)
> -			return ret;
> +		/*
> +		 * Frame counter, frame error counter, line counter and line error counter
> +		 * registers are marked as reserved in the UB954 datasheet. Hence restrict
> +		 * the following register reads only for UB960 and UB9702.
> +		 */
> +		if (priv->hw_data->chip_type != UB954) {

It is better to check for the chips that have the registers, unless
we're sure that this particular chip, ub954, is and will be the only
outlier.

> +			ret = ub960_read16(priv, UB960_SR_CSI_FRAME_COUNT_HI(nport),
> +					   &v16, NULL);
> +			if (ret)
> +				return ret;
>  
> -		dev_info(dev, "\tframe counter %u\n", v16);
> +			dev_info(dev, "\tframe counter %u\n", v16);
>  
> -		ret = ub960_read16(priv, UB960_SR_CSI_FRAME_ERR_COUNT_HI(nport),
> -				   &v16, NULL);
> -		if (ret)
> -			return ret;
> +			ret = ub960_read16(priv, UB960_SR_CSI_FRAME_ERR_COUNT_HI(nport),
> +					   &v16, NULL);
> +			if (ret)
> +				return ret;
>  
> -		dev_info(dev, "\tframe error counter %u\n", v16);
> +			dev_info(dev, "\tframe error counter %u\n", v16);
>  
> -		ret = ub960_read16(priv, UB960_SR_CSI_LINE_COUNT_HI(nport),
> -				   &v16, NULL);
> -		if (ret)
> -			return ret;
> +			ret = ub960_read16(priv, UB960_SR_CSI_LINE_COUNT_HI(nport),
> +					   &v16, NULL);
> +			if (ret)
> +				return ret;
>  
> -		dev_info(dev, "\tline counter %u\n", v16);
> +			dev_info(dev, "\tline counter %u\n", v16);
>  
> -		ret = ub960_read16(priv, UB960_SR_CSI_LINE_ERR_COUNT_HI(nport),
> -				   &v16, NULL);
> -		if (ret)
> -			return ret;
> +			ret = ub960_read16(priv, UB960_SR_CSI_LINE_ERR_COUNT_HI(nport),
> +					   &v16, NULL);
> +			if (ret)
> +				return ret;
>  
> -		dev_info(dev, "\tline error counter %u\n", v16);
> +			dev_info(dev, "\tline error counter %u\n", v16);
> +		}
>  	}
>  
>  	for_each_rxport(priv, it) {
> @@ -5023,6 +5087,9 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
>  	}
>  
>  	switch (priv->hw_data->chip_type) {
> +	case UB954:
> +		model = "UB954";
> +		break;
>  	case UB960:
>  		model = "UB960";
>  		break;
> @@ -5039,6 +5106,11 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
>  	if (ret)
>  		goto err_pd_gpio;
>  
> +	/*
> +	 * UB954 REFCLK_FREQ is not synchronized, so multiple reads are recommended
> +	 * by the datasheet. However, we use the same logic as UB960 (single read),
> +	 * as practical testing showed this is sufficient and stable for UB954 as well.
> +	 */

I think the important point is that the clk rate is only used for a
debug print.

>  	if (priv->hw_data->chip_type == UB9702)
>  		ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq,
>  				 NULL);
> @@ -5198,6 +5270,13 @@ static void ub960_remove(struct i2c_client *client)
>  	mutex_destroy(&priv->reg_lock);
>  }
>  
> +static const struct ub960_hw_data ds90ub954_hw = {
> +	.chip_type = UB954,
> +	.chip_family = FAMILY_FPD3,
> +	.num_rxports = 2,
> +	.num_txports = 1,
> +};
> +
>  static const struct ub960_hw_data ds90ub960_hw = {
>  	.chip_type = UB960,
>  	.chip_family = FAMILY_FPD3,
> @@ -5213,6 +5292,7 @@ static const struct ub960_hw_data ds90ub9702_hw = {
>  };
>  
>  static const struct i2c_device_id ub960_id[] = {
> +	{ "ds90ub954-q1", (kernel_ulong_t)&ds90ub954_hw },
>  	{ "ds90ub960-q1", (kernel_ulong_t)&ds90ub960_hw },
>  	{ "ds90ub9702-q1", (kernel_ulong_t)&ds90ub9702_hw },
>  	{}
> @@ -5220,6 +5300,7 @@ static const struct i2c_device_id ub960_id[] = {
>  MODULE_DEVICE_TABLE(i2c, ub960_id);
>  
>  static const struct of_device_id ub960_dt_ids[] = {
> +	{ .compatible = "ti,ds90ub954-q1", .data = &ds90ub954_hw },
>  	{ .compatible = "ti,ds90ub960-q1", .data = &ds90ub960_hw },
>  	{ .compatible = "ti,ds90ub9702-q1", .data = &ds90ub9702_hw },
>  	{}

 Tomi

Powered by blists - more mailing lists