Re: [PATCH 4/7] dt-bindings: drm/bridge: anx7625: Add mode-switch support

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 08/06/2022 23:56, Prashant Malani wrote:
> On Wed, Jun 8, 2022 at 10:08 AM Prashant Malani <pmalani@chromium.org> wrote:
>>
>> Hi Krzysztof,
>>
>> Thank you for looking at the patch.
>>
>> On Jun 08 11:24, Krzysztof Kozlowski wrote:
>>> On 07/06/2022 21:00, Prashant Malani wrote:
>>>> Analogix 7625 can be used in systems to switch USB Type-C DisplayPort
>>>> alternate mode lane traffic between 2 Type-C ports.
>>>>
>>>> Update the binding to accommodate this usage by introducing a switch
>>>> property.
>>>>
>>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
>>>> ---
>>>>  .../display/bridge/analogix,anx7625.yaml      | 56 +++++++++++++++++++
>>>>  1 file changed, 56 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
>>>> index 35a48515836e..7e1f655ddfcc 100644
>>>> --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
>>>> @@ -105,6 +105,26 @@ properties:
>>>>        - port@0
>>>>        - port@1
>>>>
>>>> +  switches:
>>>> +    type: object
>>>> +    description: Set of switches controlling DisplayPort traffic on
>>>> +      outgoing RX/TX lanes to Type C ports.
>>>> +
>>>> +    properties:
>>>> +      switch:
>>>
>>> You allow only one switch with such schema, so no need for "switches"...
>>
>> See below comment (summary: we'd like to allow 1 or 2 switches).
>>>
>>>> +        $ref: /schemas/usb/typec-switch.yaml#
>>>> +        maxItems: 2
>>>
>>> Are you sure this works? what are you limiting here with maxItems? I
>>> think you wanted patternProperties...
>>
>> Yeah, I might not have used the DT syntax correctly here.
>> What I'm aiming for is:
>> "switches" should can contain 1 or 2 "switch" nodes.
>> 2 is the maximum (limitation of the hardware).
>>
>>>
>>>> +
>>>> +        properties:
>>>> +          reg:
>>>> +            maxItems: 1
>>>> +
>>>> +        required:
>>>> +          - reg
>>>> +
>>>> +    required:
>>>> +      - switch@0
>>>
>>> This does not match the property.
>>>
>>> You also need unevaluatedProperties:false
>>
>> Ack, will update this in the next version.
> 
> Actually, could you kindly clarify which of the two needs this?
> "switches" or "switch" ?
> I interpreted "switch" as requiring it, but I thought it better to confirm.

Depends what do you want to have there. If two properties called
"switch", then "switches" is ok. However old code had only one property
thus switches with maximum one switch is a bit weird.

Looking at example you wanted to switch@[01], so you need to use
patternProperties.


Best regards,
Krzysztof