Re: [PATCH 2/2] media: i2c: ds90ub960: Add support for DS90UB954-Q1

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

Hi Jai,

Thanks for the review.

On 23/05/25 22:23, Jai Luthra wrote:
> Hi Abhilash,
> 
> Thanks for the patch.
> 
> Quoting Yemike Abhilash Chandra (2025-05-23 14:06:55)
>> 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.
>>
>> Some other registers are marked as reserved in the datasheet as well,
>> notably around CSI-TX frame and line-count monitoring and some other
>> status registers. The datasheet also does not mention anything about
> 
> So what happens when userspace calls LOG_STATUS and the driver tries to
> read these monitoring registers? Are these populated in the device but just
> marked as reserved in the datasheet?
> 
> Whatever is the case, please make sure the driver doesn't crash, and update
> the commit message with the reality if the datasheet is wrong.
> 

I don't see a crash while doing a log-status [1]. In the driver, we 
check what
TX and RX ports are active from the HW data and the do a register read
accordingly. That should be fine I believe.

>> setting strobe position, and fails to lock the RX ports if we forcefully
>> set it, so disable it through the hw_data.
>>
>> Link: https://www.ti.com/lit/gpn/ds90ub954-q1
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> ---
>>   drivers/media/i2c/Kconfig     |  2 +-
>>   drivers/media/i2c/ds90ub960.c | 46 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index e68202954a8f..6e265e1cec20 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -1662,7 +1662,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.
> 
> nit:
>             Device driver for the Texas Instruments DS90UB954, DS90UB960
>             FPD-Link III Deserializers and DS90UB9702 FPD-Link IV Deserializer.
> 
>>   
>>   config VIDEO_MAX96714
>> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
>> index ed2cf9d247d1..38e4f006d098 100644
>> --- a/drivers/media/i2c/ds90ub960.c
>> +++ b/drivers/media/i2c/ds90ub960.c
>> @@ -460,6 +460,7 @@ struct ub960_hw_data {
>>          u8 num_txports;
>>          bool is_ub9702;
>>          bool is_fpdlink4;
>> +       bool is_ub954;
>>   };
>>   
>>   enum ub960_rxport_mode {
>> @@ -982,6 +983,10 @@ static int ub960_txport_select(struct ub960_data *priv, u8 nport)
>>   
>>          lockdep_assert_held(&priv->reg_lock);
>>   
>> +       /* TX port registers are shared for UB954*/
>> +       if (priv->hw_data->is_ub954)
>> +               return 0;
>> +
> 
> nit: This could be moved above the assertion

Will do that in next revision.

> 
>>          if (priv->reg_current.txport == nport)
>>                  return 0;
>>   
>> @@ -1415,6 +1420,13 @@ static int ub960_parse_dt_txport(struct ub960_data *priv,
>>                  goto err_free_vep;
>>          }
>>   
>> +       /* UB954 does not support 1.2 Gbps */
>> +       if (priv->tx_data_rate == MHZ(1200) && priv->hw_data->is_ub954) {
>> +               dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport);
>> +               ret = -EINVAL;
>> +               goto err_free_vep;
>> +       }
>> +
> 
> The error handling is exactly the same as the previous if {} block that
> checks the allowed data rates for UB960. IMO cleaner to move this condition
> in that block.
> 

Noted, will try to do that in a cleaner way in next revision.

> Maybe even a separate table for allowed data-rates for each chip, but that
> is probably overkill.
> 
>>          v4l2_fwnode_endpoint_free(&vep);
>>   
>>          priv->txports[nport] = txport;
>> @@ -1572,6 +1584,10 @@ static int ub960_rxport_set_strobe_pos(struct ub960_data *priv,
>>          u8 clk_delay, data_delay;
>>          int ret = 0;
>>   
>> +       /* FIXME: After writing to this area the UB954 chip no longer responds */
>> +       if (priv->hw_data->is_ub954)
>> +               return 0;
>> +
> 
> It would be good to understand if this is a hardware limitation or not.
> Tomi, do you have any idea?
> 
>>          clk_delay = UB960_IR_RX_ANA_STROBE_SET_CLK_NO_EXTRA_DELAY;
>>          data_delay = UB960_IR_RX_ANA_STROBE_SET_DATA_NO_EXTRA_DELAY;
>>   
>> @@ -5021,6 +5037,27 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
>>          if (priv->hw_data->is_ub9702)
>>                  ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq,
>>                                   NULL);
>> +       else if (priv->hw_data->is_ub954) {
>> +               /* From DS90UB954-Q1 datasheet:
>> +                * "REFCLK_FREQ measurement is not synchronized. Value in this
>> +                * register should read twice and only considered valid if
> 
>                     * register should be read twice and only considered valid if
> 
>> +                * REFCLK_FREQ is unchanged between reads."
>> +                */
>> +               unsigned long timeout = jiffies + msecs_to_jiffies(100);
>> +
>> +               do {
>> +                       u8 refclk_new;
>> +
>> +                       ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_new,
>> +                                        NULL);
>> +                       if (ret)
>> +                               goto err_pd_gpio;
>> +
>> +                       if (refclk_new == refclk_freq)
>> +                               break;
>> +                       refclk_freq = refclk_new;
>> +               } while (time_before(jiffies, timeout));
>> +       }
> 
> Hmm.. in your testing did you find this actually requiring more than one
> read?
> 
> I'm surprised because this is missing from UB960 which is an older device.
> 

In my testing (around 20 reboots) , I had to do only 1 check i.e just 2
iterations. I am not sure on how to proceed but the data sheet at7.6.121 
REFCLK_FREQ Register clearly specifies the below.

"REFCLK_FREQ measurement is not synchronized. Value in this
register should read twice and only considered valid if
REFCLK_FREQ is unchanged between reads."


Thanks and Regards,
Abhilash Chandra

[1]: 
https://gist.github.com/Yemike-Abhilash-Chandra/dc07a6389d06648d9e80de23d8cae954

>>          else
>>                  ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq,
>>                                   NULL);
>> @@ -5177,6 +5214,13 @@ static void ub960_remove(struct i2c_client *client)
>>          mutex_destroy(&priv->reg_lock);
>>   }
>>   
>> +static const struct ub960_hw_data ds90ub954_hw = {
>> +       .model = "ub954",
>> +       .num_rxports = 2,
>> +       .num_txports = 1,
>> +       .is_ub954 = true,
>> +};
>> +
>>   static const struct ub960_hw_data ds90ub960_hw = {
>>          .model = "ub960",
>>          .num_rxports = 4,
>> @@ -5192,6 +5236,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 },
>>          {}
>> @@ -5199,6 +5244,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 },
>>          {}
>> -- 
>> 2.34.1
>>
>>
> 
> Thanks,
> Jai