Re: [PATCH V3 3/4] media: dt-bindings: ti,ds90ub960: Add support for DS90UB954-Q1

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

Hi Tomi,

Thank you for the review.

On 22/12/25 16:59, Tomi Valkeinen wrote:
> Hi,
> 
> On 19/12/2025 14:29, 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@ti.com>
>> ---
>> Changelog:
>> Changes in v3:
>> - Remove the example added for DS90UB954, as it is just a subset of the DS90UB960 example. (Rob)
>>
>>   .../bindings/media/i2c/ti,ds90ub960.yaml      | 113 ++++++++++++------
>>   1 file changed, 77 insertions(+), 36 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>> index cc61604eca37..8e2b82d6dc81 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>> @@ -13,12 +13,10 @@ description:
>>     The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
>>     forwarding.
>>   
>> -allOf:
>> -  - $ref: /schemas/i2c/i2c-atr.yaml#
>> -
>>   properties:
>>     compatible:
>>       enum:
>> +      - ti,ds90ub954-q1
>>         - ti,ds90ub960-q1
>>         - ti,ds90ub9702-q1
>>   
>> @@ -129,39 +127,6 @@ properties:
>>         Ports represent FPD-Link inputs to the deserializer and CSI TX outputs
>>         from the deserializer. The number of ports is model-dependent.
>>   
>> -    properties:
>> -      port@0:
>> -        $ref: '#/$defs/FPDLink-input-port'
>> -        description: FPD-Link input 0
>> -
>> -      port@1:
>> -        $ref: '#/$defs/FPDLink-input-port'
>> -        description: FPD-Link input 1
>> -
>> -      port@2:
>> -        $ref: '#/$defs/FPDLink-input-port'
>> -        description: FPD-Link input 2
>> -
>> -      port@3:
>> -        $ref: '#/$defs/FPDLink-input-port'
>> -        description: FPD-Link input 3
>> -
>> -      port@4:
>> -        $ref: '#/$defs/CSI2-output-port'
>> -        description: CSI-2 Output 0
>> -
>> -      port@5:
>> -        $ref: '#/$defs/CSI2-output-port'
>> -        description: CSI-2 Output 1
>> -
>> -    required:
>> -      - port@0
>> -      - port@1
>> -      - port@2
>> -      - port@3
>> -      - port@4
>> -      - port@5
>> -
>>   required:
>>     - compatible
>>     - reg
>> @@ -204,6 +169,82 @@ $defs:
>>             - data-lanes
>>             - link-frequencies
>>   
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-atr.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - ti,ds90ub960-q1
>> +              - ti,ds90ub9702-q1
>> +    then:
>> +      properties:
>> +        ports:
>> +          properties:
>> +            port@0:
>> +              $ref: '#/$defs/FPDLink-input-port'
>> +              description: FPD-Link input 0
>> +
>> +            port@1:
>> +              $ref: '#/$defs/FPDLink-input-port'
>> +              description: FPD-Link input 1
>> +
>> +            port@2:
>> +              $ref: '#/$defs/FPDLink-input-port'
>> +              description: FPD-Link input 2
>> +
>> +            port@3:
>> +              $ref: '#/$defs/FPDLink-input-port'
>> +              description: FPD-Link input 3
>> +
>> +            port@4:
>> +              $ref: '#/$defs/CSI2-output-port'
>> +              description: CSI-2 Output 0
>> +
>> +            port@5:
>> +              $ref: '#/$defs/CSI2-output-port'
>> +              description: CSI-2 Output 1
>> +
>> +          required:
>> +            - port@0
>> +            - port@1
>> +            - port@2
>> +            - port@3
>> +            - port@4
>> +            - port@5
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: ti,ds90ub954-q1
>> +    then:
>> +      properties:
>> +        ports:
>> +          properties:
>> +            port@0:
>> +              $ref: '#/$defs/FPDLink-input-port'
>> +              description: FPD-Link input 0
>> +
>> +            port@1:
>> +              $ref: '#/$defs/FPDLink-input-port'
>> +              description: FPD-Link input 1
>> +
>> +            port@2:
>> +              $ref: '#/$defs/CSI2-output-port'
>> +              description: CSI-2 Output 0
>> +
>> +          required:
>> +            - port@0
>> +            - port@1
>> +            - port@2
>> +
>> +        links:
>> +          properties:
>> +            link@2: false
>> +            link@3: false
> I can't help but think if this is good or not. In other words, if we
> specifically add ports per compatible, why wouldn't we also add
> specifically links per compatible? Or, if we just disable links as
> above, why don't we do it the same way for ports?
> 

Quoting writing schemas:

"When bindings cover multiple similar devices that differ in some 
properties,
those properties should be constrained for each device. This usually means:

  * In top level 'properties' define the property with the broadest 
constraints.
  * In 'if:then:' blocks, further narrow the constraints for those 
properties.
  * Do not define the properties within an 'if:then:' block (note that
    'additionalItems' also won't allow that)."


Since new properties cannot be introduced inside allOf / if:then, it is 
not possible to define
device-specific patternProperties for links directly under each 
condition like below

- if:
     properties:
       compatible:
         contains:
           enum:
             - ti,ds90ub960-q1
             - ti,ds90ub9702-q1
   then:
     properties:
       links:
         patternProperties:
           '^link@[0-3]$':

- if:
     properties:
       compatible:
         contains:
           const: ti,ds90ub954-q1

   then:
     properties:
       links:
	patternProperties:
	  '^link@[0-1]$':

Therefore, a broad top-level definition such as:

patternProperties:
   '^link@[0-3]$':

is required, with device-specific constraints applied later via 
conditional logic

This works for ports since we have a

patternProperties:
   '^port@[0-9a-f]+$':

already defined at /schemas/graph.yaml#/properties/ports which we refer 
in the top level schema

Note that an alternative could also be modeled by explicitly disallowing 
unused ports for ds90ub954,
namely port 2, port3 and port 5 but that approach would require changes 
in the driver, as it currently
assumes that TX ports start immediately after RX ports. Hence I 
preferred having ports 0, 1 and 2 for ds90ub954

The approach that was used in this patch was also hinted at in our 
earlier discussion at
https://lore.kernel.org/all/58b309d9-de03-4818-8d38-a27cc68466db@ideasonboard.com/

Thanks and Regards,
Yemike Abhilash Chandra


>   Tomi
>