Re: [PATCH 1/2] media: dt-bindings: ti,ds90ub960: Add bindings for DS90UB954-Q1

[Tomi Valkeinen]

Hi,

On 28/05/2025 08:35, Yemike Abhilash Chandra wrote:
> Hi Tomi,
> 
> Thanks for the review.
> 
> On 27/05/25 10:30, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 23/05/2025 11:36, 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. Therefore, add support for DS90UB954 within the existing bindings.
>>>
>>> Link: https://www.ti.com/lit/gpn/ds90ub954-q1
>>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@xxxxxx>
>>> ---
>>>   Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/
>>> ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/
>>> ti,ds90ub960.yaml
>>> index 4dcbd2b039a5..b2d4300d7846 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>> @@ -19,6 +19,7 @@ allOf:
>>>   properties:
>>>     compatible:
>>>       enum:
>>> +      - ti,ds90ub954-q1
>>>         - ti,ds90ub960-q1
>>>         - ti,ds90ub9702-q1
>>>   
>>
>> Does this pass the dt checks? The binding lists ports 0-5 as required.
> 
> Thanks for pointing this out. It is passing DT checks since we have marked
> port 4 and port 5 as disabled in DT, but I now understand that approach is
> not acceptable.
> 
> Ports 0–3 are documented as FPD-Link inputs, but on UB954, only ports 0
> and 1 are inputs,
> while port 2 is CSI TX. Should I conditionally modify required ports for
> UB954(0-2) and
> UB960/UB9702 (0-5), even though port 2's description would mismatch?
> (In bindings it would be described as FPD-Link input but it will be
> modeled as CSI TX in DT).
> 
> Alternatively, we can describe the ports block separately for each
> compatible to
> ensure correctness. Please let me know which approach you prefer.

I think you have two options here: either use ports 0, 1, 2 for UB954,
or use ports 0, 1, 4 for UB954. I think the first option makes more sense.

The bindings have to be correct, you can't do "even though port 2's
description would mismatch". Probably the clearest way is just to have a
conditional and define all the ports separately for ub954 and for the rest.

You can check e.g. renesas,du.yaml, which does rather complex conditionals.

I do wonder, though, if the port definitions could be more brief by
somehow defining the RX and TX port details only once, instead of
replicating the same for every port.

Note that the bindings also allow link0-3, which for UB954 should be
link0-1.

 Tomi