Re: [PATCH V2 1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Marek,

Thank you for the patch.

On Wed, Jun 02, 2021 at 10:37:30PM +0200, Marek Vasut wrote:
> Decoder input LVDS format is a property of the decoder chip or even
> its strapping. Add DT property data-mapping the same way lvds-panel
> does, to define the LVDS data mapping.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: devicetree@vger.kernel.org
> To: dri-devel@lists.freedesktop.org
> ---
> V2: - Use allOf
>     - Move the data-mapping to endpoint
> ---
>  .../bindings/display/bridge/lvds-codec.yaml   | 53 ++++++++++++++-----
>  1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> index cacafa61e3f52..c493d007785ca 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -68,6 +68,18 @@ properties:
>            For LVDS encoders, port 1 is the LVDS output
>            For LVDS decoders, port 1 is the parallel output
>  
> +        properties:
> +          endpoint:
> +            properties:
> +              data-mapping:
> +                enum:
> +                  - jeida-18
> +                  - jeida-24
> +                  - vesa-24
> +                description: |
> +                  The color signals mapping order. See details in
> +                  Documentation/devicetree/bindings/display/panel/lvds.yaml
> +
>      required:
>        - port@0
>        - port@1
> @@ -79,21 +91,38 @@ properties:
>  
>    power-supply: true
>  
> -if:
> -  not:
> -    properties:
> -      compatible:
> -        contains:
> -          const: lvds-encoder
> -then:
> -  properties:
> -    ports:
> +allOf:
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: lvds-encoder
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint:
> +                  properties:
> +                    pclk-sample: false
> +
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: lvds-decoder

Is this correct ? The condition says that if it's an encoder (not a
decoder), then the data-mapping property on port 1 (the output port,
thus and LVDS port) is not allowed. I think this should be for decoders,
not encoders. You can thus combine the two conditions:

allOf:
  - if:
      not:
        properties:
          compatible:
            contains:
              const: lvds-encoder
    then:
      properties:
        ports:
          properties:
            port@0:
              properties:
                endpoint:
                  properties:
                    pclk-sample: false
            port@1:
              properties:
                endpoint:
                  properties:
                    data-mapping: false

You could also drop the allOf, or keep it to avoid whitespace churn when
a new condition will need to be added.

> +    then:
>        properties:
> -        port@0:
> +        ports:
>            properties:
> -            endpoint:
> +            port@1:
>                properties:
> -                pclk-sample: false
> +                endpoint:
> +                  properties:
> +                    data-mapping: false
>  
>  required:
>    - compatible

-- 
Regards,

Laurent Pinchart