| Message ID | 20181004204138.2784-2-niklas.soderlund@ragnatech.se |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints | expand |
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:
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:
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 > > >
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:
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 > > >
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 >>
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: