Re: [PATCH v10 09/12] dt-bindings: display: bridge: lvds-codec: Add new bus-width prop

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: dri-devel@lists.freedesktop.org,
	Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Andrey Smirnov <andrew.smirnov@gmail.com>,
	Jonas Karlman <jonas@kwiboo.se>, Rob Herring <robh+dt@kernel.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	devicetree@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	intel-gfx-trybot@lists.freedesktop.org, kernel@collabora.com,
	Sam Ravnborg <sam@ravnborg.org>, Chris Healy <cphealy@gmail.com>
Subject: Re: [PATCH v10 09/12] dt-bindings: display: bridge: lvds-codec: Add new bus-width prop
Date: Tue, 25 Feb 2020 00:31:39 +0200	[thread overview]
Message-ID: <20200224223139.GA29578@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20200128135514.108171-10-boris.brezillon@collabora.com>

Hi Boris,

Thank you for the patch.

On Tue, Jan 28, 2020 at 02:55:11PM +0100, Boris Brezillon wrote:
> Add the bus-width property to describe the input bus format.
> 
> v10:
> * Add changelog to the commit message
> * Add Rob's R-b
> 
> v8 -> v9:
> * No changes
> 
> v7:
> * Rebase on top of lvds-codec changes
> * Drop the data-mapping property
> 
> v4 -> v6:
> * Not part of the series
> 
> v3:
> * New patch
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/display/bridge/lvds-codec.yaml    | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> index 8f373029f5d2..7c4e42f4de61 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -55,6 +55,14 @@ properties:
>          description: |
>            For LVDS encoders, port 0 is the parallel input
>            For LVDS decoders, port 0 is the LVDS input
> +        properties:
> +          bus-width:
> +            allOf:
> +              - $ref: /schemas/types.yaml#/definitions/uint32
> +              - enum: [18, 24]
> +              - default: 24
> +          description:
> +            Number of data lines used to transmit the RGB data.

This is a bit unclear. First of all, depending on whether the node is an
LVDS encoder or decoder, port@0 is either a parallel input or an LVDS
input. The property mentiones RGB data, does it mean it apply to LVDS
encoders only ? Or should it be in port@1 for LVDS decoders ?

Then, I'm not sure what the property describes. Is it the number of data
lanes that the chip has ? Or the number of lanes routed on the board ?
Should it be specified only if the number of lanes on the board is
different than the maximum number of lanes of the hardware ? A more
detailed description is needed.

Updating the example would also be useful.

>  
>        port@1:
>          type: object

-- 
Regards,

Laurent Pinchart