Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints

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

Hi Jacopo,

On Friday, 5 October 2018 11:49:45 EEST jacopo mondi wrote:
> On Fri, Oct 05, 2018 at 01:00:47AM +0300, Laurent Pinchart wrote:
> > On Friday, 5 October 2018 00:42:17 EEST Laurent Pinchart wrote:
> >> On Thursday, 4 October 2018 23:41:34 EEST Niklas Söderlund wrote:
> >>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> 
> >>> The CSI-2 transmitters can use a different number of lanes to transmit
> >>> data. Make the data-lanes mandatory for the endpoints describe the
> >> 
> >> s/describe/that describe/ ?
> >> 
> >>> transmitters as no good default can be set to fallback on.
> >>> 
> >>> Signed-off-by: Niklas Söderlund
> >>> <niklas.soderlund+renesas@ragnatech.se>
> >>> ---
> >>> 
> >>>  Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>> 
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> >>> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index
> >>> 5dddc95f9cc46084..f9dac01ab795fc28 100644
> >>> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> >>> @@ -50,6 +50,9 @@ are numbered as follows.
> >>> 
> >>>  The digital output port nodes must contain at least one endpoint.
> >>> 
> >>> +The endpoints described in TXA and TXB ports must if present contain
> >>> +the data-lanes property as described in video-interfaces.txt.
> >>> +
> >> 
> >> Would it make sense to merge those two paragraphs, as they refer to the
> >> same endpoint ?
> >> 
> >> "The digital output port nodes, when present, shall contain at least one
> >> endpoint. Each of those endpoints shall contain the data-lanes property
> >> as described in video-interfaces.txt."
> >> 
> >> (DT bindings normally use "shall" instead of "must", but that hasn't
> >> really been enforced.)
> >> 
> >> If you want to keep the paragraphs separate, I would recommend using
> >> "digital output ports" instead of "TXA and TXB" in the second paragraph
> >> for consistency (or the other way around).
> >> 
> >> I'm fine with any of the above option, so please pick your favourite,
> >> and add
> >> 
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > I just realized that TXB only supports a single data lane, so we may want
> > not to have a data-lanes property for TXB.
> 
> Isn't it better to restrict its value to <1> but make it mandatory
> anyhow? I understand conceptually that property should not be there,
> as it has a single acceptable value, but otherwise we need to traeat
> differently the two output ports, in documentation and code.

The two ports are different, so I wouldn't be shocked if we handled them 
differently :-) I believe it would actually reduce the code size (and save CPU 
cycles at runtime).

> Why not inserting a paragraph with the required endpoint properties
> description?
> 
> Eg:
> 
>  Required endpoint properties:
>  - data-lanes: See "video-interfaces.txt" for description. The property
>    is mandatory for CSI-2 output endpoints and the accepted values
>    depends on which endpoint the property is applied to:
>    - TXA: accepted values are <1>, <2>, <4>
>    - TXB: accepted value is <1>
> 
> >>>  Ports are optional if they are not connected to anything at the
> >>>  hardware level.
> >>> 
> >>>  Example:

-- 
Regards,

Laurent Pinchart