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

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

[Niklas Söderlund]

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
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.
+
 Ports are optional if they are not connected to anything at the hardware level.
 
 Example:
-- 
2.19.0

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

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>

>  Ports are optional if they are not connected to anything at the hardware
> level.
> 
>  Example:

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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.

> >  Ports are optional if they are not connected to anything at the hardware
> > 
> > level.
> > 
> >  Example:

-- 
Regards,

Laurent Pinchart

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

[jacopo mondi]

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

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

[Laurent Pinchart]

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

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

[Niklas Söderlund]

Hi Laurent, Jacopo

Thanks for your comments.

On 2018-10-05 13:02:53 +0300, Laurent Pinchart wrote:
> 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).

I'm leaning towards Jacopo on this that I think it's more clear to treat 
the two the same. I also think it adheres to the notion that DT shall 
describe hardware which I think this adds value. Also I only have 
datasheets for adv7482 so I can't be sure other adv748x don't support 
more then one lane on TXB.

I do not feel strongly about this so I'm open to treating them 
differently. I might keep it as is for v3 if no one screams to loud :-)

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

-- 
Regards,
Niklas S�derlund

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

[Kieran Bingham]

Hi Niklas,

On 02/11/2018 10:34, Niklas Söderlund wrote:
> Hi Laurent, Jacopo
> 
> Thanks for your comments.
> 
> On 2018-10-05 13:02:53 +0300, Laurent Pinchart wrote:
>> 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).
> 
> I'm leaning towards Jacopo on this that I think it's more clear to treat 
> the two the same. I also think it adheres to the notion that DT shall 
> describe hardware which I think this adds value. Also I only have 
> datasheets for adv7482 so I can't be sure other adv748x don't support 
> more then one lane on TXB.
> 
> I do not feel strongly about this so I'm open to treating them 
> differently. I might keep it as is for v3 if no one screams to loud :-)

I personally think that TXA and TXB are 'the same' entities, just in
different configurations, and so I would say they are 'the same' too.

For reference, from the adv748x.h header file:

 * The ADV748x range of receivers have the following configurations:
 *
 *                  Analog   HDMI  MHL  4-Lane  1-Lane
 *                    In      In         CSI     CSI
 *       ADV7480               X    X     X
 *       ADV7481      X        X    X     X       X
 *       ADV7482      X        X          X       X


So both the ADV7481 and ADV7482 have both a TXA and a TXB, where the
ADV7480 has only the TXA.

TXB is always only 'one-lane'

--
Kieran



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