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

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 3211 bytes --]

Hi Laurent, Niklas,

On Fri, Oct 05, 2018 at 01:00:47AM +0300, Laurent Pinchart wrote:
> Hello again,
>
> 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.

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
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]