[2/3] media: i2c: ds90ub960: Add DS90UB954 support

  Add support for TI DS90UB954 FPD-Link III Deserializer.

Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
---
 drivers/media/i2c/Kconfig     |  2 +-
 drivers/media/i2c/ds90ub960.c | 52 +++++++++++++++++++++++++++++------
 2 files changed, 44 insertions(+), 10 deletions(-)
  

Hi,

On 30/08/2024 10:00, Alexander Shiyan wrote:
> Add support for TI DS90UB954 FPD-Link III Deserializer.
> 
> Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
> ---
>   drivers/media/i2c/Kconfig     |  2 +-
>   drivers/media/i2c/ds90ub960.c | 52 +++++++++++++++++++++++++++++------
>   2 files changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 8ba096b8ebca..18766898280b 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1604,7 +1604,7 @@ config VIDEO_DS90UB960
>   	select V4L2_FWNODE
>   	select VIDEO_V4L2_SUBDEV_API
>   	help
> -	  Device driver for the Texas Instruments DS90UB960
> +	  Device driver for the Texas Instruments DS90UB954/DS90UB960
>   	  FPD-Link III Deserializer and DS90UB9702 FPD-Link IV Deserializer.
>   
>   config VIDEO_MAX96714
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index e9f9abf439ee..9edc7e8ceebd 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -403,6 +403,7 @@
>   #define UB960_NUM_EQ_LEVELS (UB960_MAX_EQ_LEVEL - UB960_MIN_EQ_LEVEL + 1)
>   
>   enum chip_type {
> +	UB954,
>   	UB960,
>   	UB9702,
>   };
> @@ -1154,10 +1155,17 @@ 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)) {
> +	switch (priv->tx_data_rate) {
> +	case MHZ(1600):
> +	case MHZ(800):
> +	case MHZ(400):
> +		break;
> +	case MHZ(1200):
> +		/* UB954 does not support 1.2 Gbps */
> +		if (priv->hw_data->chip_type != UB954)
> +			break;

Here, and in a few other places, don't check for model != UB954, but 
rather check for model == UB954. Otherwise if we add a new chip model 
these won't necessarily go right.

> +		fallthrough;
> +	default:
>   		dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport);
>   		ret = -EINVAL;
>   		goto err_free_vep;
> @@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport)
>   
>   	if (priv->strobe.manual)
>   		ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos);
> -	else
> +	else if (priv->hw_data->chip_type != UB954)
>   		ub960_rxport_set_strobe_pos(priv, nport, 0);

This looks odd. Manually set strobe pos is ok, but not the default?

What is the reason for this if?

>   
>   	if (rxport->eq.manual_eq) {
> @@ -3807,7 +3815,7 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
>   	u8 rev_mask;
>   	int ret;
>   	u8 dev_sts;
> -	u8 refclk_freq;
> +	u8 refclk_freq[2];

Instead of an array, I think the code will be clearer if you just add a 
new variable (refclk_freq_new?).

>   
>   	ret = regulator_enable(priv->vddio);
>   	if (ret)
> @@ -3839,6 +3847,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;
> @@ -3856,12 +3867,26 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
>   	if (ret)
>   		goto err_pd_gpio;
>   
> -	ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq);
> +	ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq[0]);
>   	if (ret)
>   		goto err_pd_gpio;
>   
> +	/* From DS90UB954-Q1 datasheet:
> +	 * "REFCLK_FREQ measurement is not synchronized. Value in this register
> +	 * should read twice and only considered valid if
> +	 * REFCLK_FREQ is unchanged between reads."
> +	*/

The coding style says the multiline comments are like:

/*
  * Foo
  */

> +	while (priv->hw_data->chip_type == UB954) {
> +		ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq[1]);
> +		if (ret)
> +			goto err_pd_gpio;
> +		if (refclk_freq[0] == refclk_freq[1])
> +			break;
> +		refclk_freq[0] = refclk_freq[1];
> +	};

This is potentially an infinite loop, which is not a good idea. Also, 
don't loop with "while (priv->hw_data->chip_type == UB954)"... Just use 
an if for the chip_type, and loop with a proper condition.

> +
>   	dev_dbg(dev, "refclk valid %u freq %u MHz (clk fw freq %lu MHz)\n",
> -		!!(dev_sts & BIT(4)), refclk_freq,
> +		!!(dev_sts & BIT(4)), refclk_freq[0],
>   		clk_get_rate(priv->refclk) / 1000000);
>   
>   	/* Disable all RX ports by default */
> @@ -3923,7 +3948,8 @@ static int ub960_probe(struct i2c_client *client)
>   	 */
>   	priv->reg_current.indirect_target = 0xff;
>   	priv->reg_current.rxport = 0xff;
> -	priv->reg_current.txport = 0xff;
> +	/* Avoid using UB960_SR_CSI_PORT_SEL register for single TX channel */
> +	priv->reg_current.txport = priv->hw_data->num_txports > 1 ? 0xff : 0x00;

No, don't do this. Just do a proper check in ub960_txport_select() and 
skip the reg write there.

  Tomi

>   	ret = ub960_get_hw_resources(priv);
>   	if (ret)
> @@ -4031,6 +4057,12 @@ static void ub960_remove(struct i2c_client *client)
>   	mutex_destroy(&priv->reg_lock);
>   }
>   
> +static const struct ub960_hw_data ds90ub954_hw = {
> +	.chip_type = UB954,
> +	.num_rxports = 2,
> +	.num_txports = 1,
> +};
> +
>   static const struct ub960_hw_data ds90ub960_hw = {
>   	.chip_type = UB960,
>   	.num_rxports = 4,
> @@ -4045,6 +4077,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 },
>   	{}
> @@ -4052,6 +4085,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 },
>   	{}
  

Hello.

вт, 10 сент. 2024 г. в 11:40, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>:
> On 30/08/2024 10:00, Alexander Shiyan wrote:
> > Add support for TI DS90UB954 FPD-Link III Deserializer.
...
> > @@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport)
> >
> >       if (priv->strobe.manual)
> >               ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos);
> > -     else
> > +     else if (priv->hw_data->chip_type != UB954)
> >               ub960_rxport_set_strobe_pos(priv, nport, 0);
>
> This looks odd. Manually set strobe pos is ok, but not the default?
> What is the reason for this if?

In fact, these registers are described as reserved in the datasheet.
(We are speaking about indirect page 1).
Here is an excerpt from datasheet UB960:
Indirect Access Register Select:
Selects target for register access
0000: Pattern Generator and CSI-2 Timing (PATGEN_AND_CSI-2) Registers
xxxx: RESERVED

In UB954 datasheet this area is described as "FPD-Link III Channel 0
Reserved Registers: Test and Debug registers".

I tested the UB954 and when writing to this area an error occurs and
the chip no longer responds.
When disabling ub960_rxport_set_strobe_pos() everything works as expected.