diff mbox series

[RESEND,v5,2/3] dt-bindings: display: atmel: optional video-interface of endpoints

Message ID 20180803072308.14962-3-peda@axentia.se (mailing list archive)
State New, archived
Headers show
Series drm/atmel-hlcdc: bus-width override support | expand

Commit Message

Peter Rosin Aug. 3, 2018, 7:23 a.m. UTC
With bus-type/bus-width properties in the endpoint nodes, the video-
interface of the connection can be specified for cases where the
heuristic fails to select the correct output mode. This can happen
e.g. if not all RGB pins are routed on the PCB; the driver has no
way of knowing this, and needs to be told explicitly.

This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Jacopo Mondi Aug. 3, 2018, 8:11 a.m. UTC | #1
Hi Peter!

On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
> With bus-type/bus-width properties in the endpoint nodes, the video-
> interface of the connection can be specified for cases where the
> heuristic fails to select the correct output mode. This can happen
> e.g. if not all RGB pins are routed on the PCB; the driver has no
> way of knowing this, and needs to be told explicitly.
>
> This is critical for the devices that have the "conflicting output
> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> RGB bits move around depending on the selected output mode. For
> devices that do not have the "conflicting output formats" issue
> (SAMA5D2, SAMA5D4), this is completely irrelevant.
>
> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> index 82f2acb3d374..9de434a8f523 100644
> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> @@ -15,6 +15,14 @@ Required children nodes:
>   to external devices using the OF graph reprensentation (see ../graph.txt).
>   At least one port node is required.
>
> +Optional properties in grandchild nodes:
> + Any endpoint grandchild node may specify a desired video interface
> + according to ../../media/video-interfaces.txt, specifically
> + - bus-type: must be <0>.

Is there any value in specifying this, if it has a fixed value to
"autodetect"? I understand it's optional, so if nobody else objects,
feels free to keep it there.


> + - bus-width: recognized values are <12>, <16>, <18> and <24>, and
> +   override any output mode selection heuristic, forcing "rgb444",
> +   "rgb565", "rgb666" and "rgb888" respectively.
> +
>  Example:
>
>  	hlcdc: hlcdc@f0030000 {
> @@ -50,3 +58,21 @@ Example:
>  			#pwm-cells = <3>;
>  		};
>  	};
> +

Two blank lines here.

> +
> +Example 2: With a video interface override to force rgb565; as above
> +but with these changes/additions:
> +
> +	&hlcdc {
> +		hlcdc-display-controller {
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
> +
> +			port@0 {

The node has a unit address specified, you're missing a reg = <0>
property (no big deal, it's an example, but the other one has it)

> +				hlcdc_panel_output: endpoint@0 {

Missing reg here too.

Minors apart:

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

> +					bus-type = <0>;
> +					bus-width = <16>;
> +				};
> +			};
> +		};
> +	};
> --
> 2.11.0
>
Peter Rosin Aug. 3, 2018, 8:40 a.m. UTC | #2
On 2018-08-03 10:11, jacopo mondi wrote:
> Hi Peter!
> 
> On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
>> With bus-type/bus-width properties in the endpoint nodes, the video-
>> interface of the connection can be specified for cases where the
>> heuristic fails to select the correct output mode. This can happen
>> e.g. if not all RGB pins are routed on the PCB; the driver has no
>> way of knowing this, and needs to be told explicitly.
>>
>> This is critical for the devices that have the "conflicting output
>> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
>> RGB bits move around depending on the selected output mode. For
>> devices that do not have the "conflicting output formats" issue
>> (SAMA5D2, SAMA5D4), this is completely irrelevant.
>>
>> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>> index 82f2acb3d374..9de434a8f523 100644
>> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>> @@ -15,6 +15,14 @@ Required children nodes:
>>   to external devices using the OF graph reprensentation (see ../graph.txt).
>>   At least one port node is required.
>>
>> +Optional properties in grandchild nodes:
>> + Any endpoint grandchild node may specify a desired video interface
>> + according to ../../media/video-interfaces.txt, specifically
>> + - bus-type: must be <0>.
> 
> Is there any value in specifying this, if it has a fixed value to
> "autodetect"? I understand it's optional, so if nobody else objects,
> feels free to keep it there.

That's just how media/video-interfaces.txt works.

bus-type 0 means that other properties describe the bus type. In this
case bus-width is specified, so that means a parallel bus. But bus-width
has no meaning (or may not have) if bus-type is non-zero. But checking
that bus-type for zero in the code seemed like overkill to me since the
driver already knows that it is a parallel bus...

TL;DR I'd like to keep it.

> 
>> + - bus-width: recognized values are <12>, <16>, <18> and <24>, and
>> +   override any output mode selection heuristic, forcing "rgb444",
>> +   "rgb565", "rgb666" and "rgb888" respectively.
>> +
>>  Example:
>>
>>  	hlcdc: hlcdc@f0030000 {
>> @@ -50,3 +58,21 @@ Example:
>>  			#pwm-cells = <3>;
>>  		};
>>  	};
>> +
> 
> Two blank lines here.
> 
>> +
>> +Example 2: With a video interface override to force rgb565; as above
>> +but with these changes/additions:
>> +
>> +	&hlcdc {
>> +		hlcdc-display-controller {
>> +			pinctrl-names = "default";
>> +			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
>> +
>> +			port@0 {
> 
> The node has a unit address specified, you're missing a reg = <0>
> property (no big deal, it's an example, but the other one has it)
> 
>> +				hlcdc_panel_output: endpoint@0 {
> 
> Missing reg here too.

I'll fix those (I think they appeared for the original example after I
wrote the patch).

Cheers,
Peter

> Minors apart:
> 
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Thanks
>    j
> 
>> +					bus-type = <0>;
>> +					bus-width = <16>;
>> +				};
>> +			};
>> +		};
>> +	};
>> --
>> 2.11.0
>>
Jacopo Mondi Aug. 3, 2018, 8:51 a.m. UTC | #3
On Fri, Aug 03, 2018 at 10:40:02AM +0200, Peter Rosin wrote:
> On 2018-08-03 10:11, jacopo mondi wrote:
> > Hi Peter!
> >
> > On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
> >> With bus-type/bus-width properties in the endpoint nodes, the video-
> >> interface of the connection can be specified for cases where the
> >> heuristic fails to select the correct output mode. This can happen
> >> e.g. if not all RGB pins are routed on the PCB; the driver has no
> >> way of knowing this, and needs to be told explicitly.
> >>
> >> This is critical for the devices that have the "conflicting output
> >> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> >> RGB bits move around depending on the selected output mode. For
> >> devices that do not have the "conflicting output formats" issue
> >> (SAMA5D2, SAMA5D4), this is completely irrelevant.
> >>
> >> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >> Reviewed-by: Rob Herring <robh@kernel.org>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
> >>  1 file changed, 26 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> index 82f2acb3d374..9de434a8f523 100644
> >> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> @@ -15,6 +15,14 @@ Required children nodes:
> >>   to external devices using the OF graph reprensentation (see ../graph.txt).
> >>   At least one port node is required.
> >>
> >> +Optional properties in grandchild nodes:
> >> + Any endpoint grandchild node may specify a desired video interface
> >> + according to ../../media/video-interfaces.txt, specifically
> >> + - bus-type: must be <0>.
> >
> > Is there any value in specifying this, if it has a fixed value to
> > "autodetect"? I understand it's optional, so if nobody else objects,
> > feels free to keep it there.
>
> That's just how media/video-interfaces.txt works.
>
> bus-type 0 means that other properties describe the bus type. In this
> case bus-width is specified, so that means a parallel bus. But bus-width
> has no meaning (or may not have) if bus-type is non-zero. But checking
> that bus-type for zero in the code seemed like overkill to me since the
> driver already knows that it is a parallel bus...
>

Yeah, I felt like pointing that out since you're not cheking for its value,
and that property is only used by v4l2-fwnode to handle some
not-that-used-anymore bus as CCP2 is.

> TL;DR I'd like to keep it.
>

Fine with me then.

> >
> >> + - bus-width: recognized values are <12>, <16>, <18> and <24>, and
> >> +   override any output mode selection heuristic, forcing "rgb444",
> >> +   "rgb565", "rgb666" and "rgb888" respectively.
> >> +
> >>  Example:
> >>
> >>  	hlcdc: hlcdc@f0030000 {
> >> @@ -50,3 +58,21 @@ Example:
> >>  			#pwm-cells = <3>;
> >>  		};
> >>  	};
> >> +
> >
> > Two blank lines here.
> >
> >> +
> >> +Example 2: With a video interface override to force rgb565; as above
> >> +but with these changes/additions:
> >> +
> >> +	&hlcdc {
> >> +		hlcdc-display-controller {
> >> +			pinctrl-names = "default";
> >> +			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
> >> +
> >> +			port@0 {
> >
> > The node has a unit address specified, you're missing a reg = <0>
> > property (no big deal, it's an example, but the other one has it)
> >
> >> +				hlcdc_panel_output: endpoint@0 {
> >
> > Missing reg here too.
>
> I'll fix those (I think they appeared for the original example after I
> wrote the patch).
>

Ok, then please consider also describing the port@0 node cell sizes too
since it has a child endpoint node.

Cheers
  j

> Cheers,
> Peter
>
> > Minors apart:
> >
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > Thanks
> >    j
> >
> >> +					bus-type = <0>;
> >> +					bus-width = <16>;
> >> +				};
> >> +			};
> >> +		};
> >> +	};
> >> --
> >> 2.11.0
> >>
>
Laurent Pinchart Aug. 3, 2018, 8:55 a.m. UTC | #4
Hi Peter,

(CC'ing Sakari Ailus)

On Friday, 3 August 2018 11:40:02 EEST Peter Rosin wrote:
> On 2018-08-03 10:11, jacopo mondi wrote:
> > On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
> >> With bus-type/bus-width properties in the endpoint nodes, the video-
> >> interface of the connection can be specified for cases where the
> >> heuristic fails to select the correct output mode. This can happen
> >> e.g. if not all RGB pins are routed on the PCB; the driver has no
> >> way of knowing this, and needs to be told explicitly.
> >> 
> >> This is critical for the devices that have the "conflicting output
> >> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> >> RGB bits move around depending on the selected output mode. For
> >> devices that do not have the "conflicting output formats" issue
> >> (SAMA5D2, SAMA5D4), this is completely irrelevant.
> >> 
> >> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >> Reviewed-by: Rob Herring <robh@kernel.org>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >> 
> >>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26
> >>  ++++++++++++++++++++++ 1 file changed, 26 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt index
> >> 82f2acb3d374..9de434a8f523 100644
> >> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >> 
> >> @@ -15,6 +15,14 @@ Required children nodes:
> >>   to external devices using the OF graph reprensentation (see
> >>   ../graph.txt).
> >>   At least one port node is required.
> >> 
> >> +Optional properties in grandchild nodes:
> >> + Any endpoint grandchild node may specify a desired video interface
> >> + according to ../../media/video-interfaces.txt, specifically
> >> + - bus-type: must be <0>.
> > 
> > Is there any value in specifying this, if it has a fixed value to
> > "autodetect"? I understand it's optional, so if nobody else objects,
> > feels free to keep it there.
> 
> That's just how media/video-interfaces.txt works.
> 
> bus-type 0 means that other properties describe the bus type. In this
> case bus-width is specified, so that means a parallel bus. But bus-width
> has no meaning (or may not have) if bus-type is non-zero. But checking
> that bus-type for zero in the code seemed like overkill to me since the
> driver already knows that it is a parallel bus...
> 
> TL;DR I'd like to keep it.

Sakari told me recently that he was planning to introduce explicit bus types 
for parallel buses (both external sync and BT.656). Sakari, do you want to 
comment on this ?

> >> + - bus-width: recognized values are <12>, <16>, <18> and <24>, and
> >> +   override any output mode selection heuristic, forcing "rgb444",
> >> +   "rgb565", "rgb666" and "rgb888" respectively.

[snip]
Peter Rosin Aug. 16, 2018, 12:52 p.m. UTC | #5
On 2018-08-03 10:51, jacopo mondi wrote:
> On Fri, Aug 03, 2018 at 10:40:02AM +0200, Peter Rosin wrote:
>> On 2018-08-03 10:11, jacopo mondi wrote:
>>> Hi Peter!
>>>
>>> On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
>>>> With bus-type/bus-width properties in the endpoint nodes, the video-
>>>> interface of the connection can be specified for cases where the
>>>> heuristic fails to select the correct output mode. This can happen
>>>> e.g. if not all RGB pins are routed on the PCB; the driver has no
>>>> way of knowing this, and needs to be told explicitly.
>>>>
>>>> This is critical for the devices that have the "conflicting output
>>>> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
>>>> RGB bits move around depending on the selected output mode. For
>>>> devices that do not have the "conflicting output formats" issue
>>>> (SAMA5D2, SAMA5D4), this is completely irrelevant.
>>>>
>>>> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
>>>>  1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>>>> index 82f2acb3d374..9de434a8f523 100644
>>>> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>>>> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
>>>> @@ -15,6 +15,14 @@ Required children nodes:
>>>>   to external devices using the OF graph reprensentation (see ../graph.txt).
>>>>   At least one port node is required.
>>>>
>>>> +Optional properties in grandchild nodes:
>>>> + Any endpoint grandchild node may specify a desired video interface
>>>> + according to ../../media/video-interfaces.txt, specifically
>>>> + - bus-type: must be <0>.
>>>
>>> Is there any value in specifying this, if it has a fixed value to
>>> "autodetect"? I understand it's optional, so if nobody else objects,
>>> feels free to keep it there.
>>
>> That's just how media/video-interfaces.txt works.
>>
>> bus-type 0 means that other properties describe the bus type. In this
>> case bus-width is specified, so that means a parallel bus. But bus-width
>> has no meaning (or may not have) if bus-type is non-zero. But checking
>> that bus-type for zero in the code seemed like overkill to me since the
>> driver already knows that it is a parallel bus...
>>
> 
> Yeah, I felt like pointing that out since you're not cheking for its value,
> and that property is only used by v4l2-fwnode to handle some
> not-that-used-anymore bus as CCP2 is.
> 
>> TL;DR I'd like to keep it.
>>
> 
> Fine with me then.
> 
>>>
>>>> + - bus-width: recognized values are <12>, <16>, <18> and <24>, and
>>>> +   override any output mode selection heuristic, forcing "rgb444",
>>>> +   "rgb565", "rgb666" and "rgb888" respectively.
>>>> +
>>>>  Example:
>>>>
>>>>  	hlcdc: hlcdc@f0030000 {
>>>> @@ -50,3 +58,21 @@ Example:
>>>>  			#pwm-cells = <3>;
>>>>  		};
>>>>  	};
>>>> +
>>>
>>> Two blank lines here.
>>>
>>>> +
>>>> +Example 2: With a video interface override to force rgb565; as above
>>>> +but with these changes/additions:
>>>> +
>>>> +	&hlcdc {
>>>> +		hlcdc-display-controller {
>>>> +			pinctrl-names = "default";
>>>> +			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
>>>> +
>>>> +			port@0 {
>>>
>>> The node has a unit address specified, you're missing a reg = <0>
>>> property (no big deal, it's an example, but the other one has it)
>>>
>>>> +				hlcdc_panel_output: endpoint@0 {
>>>
>>> Missing reg here too.
>>
>> I'll fix those (I think they appeared for the original example after I
>> wrote the patch).
>>
> 
> Ok, then please consider also describing the port@0 node cell sizes too
> since it has a child endpoint node.

Ok, I have now figured out why this was as it were, and I no longer agree
with adding the extra properties. The whole of example 2 is inside a
reference (using the &hlcdc notation) to the hlcdc node in example 1, and
therefore these "missing" properties are not missing. I think they are
just clutter that hides what is really needed/different between example 1
and 2, and apparently Boris and Rob agreed when they acked/reviewed. The
description of example 2 also clearly states that example 2 is changes
and additions on top of example 1. So, I plan to have this in the next
iteration:

	&hlcdc {
		hlcdc-display-controller {
			pinctrl-names = "default";
			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;

			port@0 {
				hlcdc_panel_output: endpoint@0 {
					bus-width = <16>;
				};
			};
		};
	};

Jacopo, please let me know if you want me to keep your review tag anyway...


>> Cheers,
>> Peter
>>
>>> Minors apart:

...because I interpret this to mean that I could add your tag if I made the
changes you suggested. Or did it mean that I could add your tag regardless
because the issues were minor?

Cheers,
Peter

>>>
>>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>
>>> Thanks
>>>    j
>>>
>>>> +					bus-type = <0>;
>>>> +					bus-width = <16>;
>>>> +				};
>>>> +			};
>>>> +		};
>>>> +	};
>>>> --
>>>> 2.11.0
>>>>
>>
Jacopo Mondi Aug. 16, 2018, 7:46 p.m. UTC | #6
Hi Peter,

On Thu, Aug 16, 2018 at 02:52:41PM +0200, Peter Rosin wrote:
> On 2018-08-03 10:51, jacopo mondi wrote:
> > On Fri, Aug 03, 2018 at 10:40:02AM +0200, Peter Rosin wrote:
> >> On 2018-08-03 10:11, jacopo mondi wrote:
> >>> Hi Peter!
> >>>
> >>> On Fri, Aug 03, 2018 at 09:23:07AM +0200, Peter Rosin wrote:
> >>>> With bus-type/bus-width properties in the endpoint nodes, the video-
> >>>> interface of the connection can be specified for cases where the
> >>>> heuristic fails to select the correct output mode. This can happen
> >>>> e.g. if not all RGB pins are routed on the PCB; the driver has no
> >>>> way of knowing this, and needs to be told explicitly.
> >>>>
> >>>> This is critical for the devices that have the "conflicting output
> >>>> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> >>>> RGB bits move around depending on the selected output mode. For
> >>>> devices that do not have the "conflicting output formats" issue
> >>>> (SAMA5D2, SAMA5D4), this is completely irrelevant.
> >>>>
> >>>> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >>>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>> ---
> >>>>  .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
> >>>>  1 file changed, 26 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >>>> index 82f2acb3d374..9de434a8f523 100644
> >>>> --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >>>> +++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> >>>> @@ -15,6 +15,14 @@ Required children nodes:
> >>>>   to external devices using the OF graph reprensentation (see ../graph.txt).
> >>>>   At least one port node is required.
> >>>>
> >>>> +Optional properties in grandchild nodes:
> >>>> + Any endpoint grandchild node may specify a desired video interface
> >>>> + according to ../../media/video-interfaces.txt, specifically
> >>>> + - bus-type: must be <0>.
> >>>
> >>> Is there any value in specifying this, if it has a fixed value to
> >>> "autodetect"? I understand it's optional, so if nobody else objects,
> >>> feels free to keep it there.
> >>
> >> That's just how media/video-interfaces.txt works.
> >>
> >> bus-type 0 means that other properties describe the bus type. In this
> >> case bus-width is specified, so that means a parallel bus. But bus-width
> >> has no meaning (or may not have) if bus-type is non-zero. But checking
> >> that bus-type for zero in the code seemed like overkill to me since the
> >> driver already knows that it is a parallel bus...
> >>
> >
> > Yeah, I felt like pointing that out since you're not cheking for its value,
> > and that property is only used by v4l2-fwnode to handle some
> > not-that-used-anymore bus as CCP2 is.
> >
> >> TL;DR I'd like to keep it.
> >>
> >
> > Fine with me then.
> >
> >>>
> >>>> + - bus-width: recognized values are <12>, <16>, <18> and <24>, and
> >>>> +   override any output mode selection heuristic, forcing "rgb444",
> >>>> +   "rgb565", "rgb666" and "rgb888" respectively.
> >>>> +
> >>>>  Example:
> >>>>
> >>>>  	hlcdc: hlcdc@f0030000 {
> >>>> @@ -50,3 +58,21 @@ Example:
> >>>>  			#pwm-cells = <3>;
> >>>>  		};
> >>>>  	};
> >>>> +
> >>>
> >>> Two blank lines here.
> >>>
> >>>> +
> >>>> +Example 2: With a video interface override to force rgb565; as above
> >>>> +but with these changes/additions:
> >>>> +
> >>>> +	&hlcdc {
> >>>> +		hlcdc-display-controller {
> >>>> +			pinctrl-names = "default";
> >>>> +			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
> >>>> +
> >>>> +			port@0 {
> >>>
> >>> The node has a unit address specified, you're missing a reg = <0>
> >>> property (no big deal, it's an example, but the other one has it)
> >>>
> >>>> +				hlcdc_panel_output: endpoint@0 {
> >>>
> >>> Missing reg here too.
> >>
> >> I'll fix those (I think they appeared for the original example after I
> >> wrote the patch).
> >>
> >
> > Ok, then please consider also describing the port@0 node cell sizes too
> > since it has a child endpoint node.
>
> Ok, I have now figured out why this was as it were, and I no longer agree
> with adding the extra properties. The whole of example 2 is inside a
> reference (using the &hlcdc notation) to the hlcdc node in example 1, and
> therefore these "missing" properties are not missing. I think they are
> just clutter that hides what is really needed/different between example 1
> and 2, and apparently Boris and Rob agreed when they acked/reviewed. The
> description of example 2 also clearly states that example 2 is changes
> and additions on top of example 1. So, I plan to have this in the next
> iteration:
>
> 	&hlcdc {
> 		hlcdc-display-controller {
> 			pinctrl-names = "default";
> 			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
>
> 			port@0 {
> 				hlcdc_panel_output: endpoint@0 {
> 					bus-width = <16>;

Right, I thought that even when re-defining a node, reg properties
should be there, but yes, I had no reasons to think so :)

> 				};
> 			};
> 		};
> 	};
>
> Jacopo, please let me know if you want me to keep your review tag anyway...
>

Sure

>
> >> Cheers,
> >> Peter
> >>
> >>> Minors apart:
>
> ...because I interpret this to mean that I could add your tag if I made the
> changes you suggested. Or did it mean that I could add your tag regardless
> because the issues were minor?
>v

Sorry, I was not clear. That was a minor nit, you could have add my
tag anyhow.

Thanks
   j

> Cheers,
> Peter
>
> >>>
> >>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>>
>  >>> Thanks
> >>>    j
> >>>
> >>>> +					bus-type = <0>;
> >>>> +					bus-width = <16>;
> >>>> +				};
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> --
> >>>> 2.11.0
> >>>>
> >>
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
index 82f2acb3d374..9de434a8f523 100644
--- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
+++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
@@ -15,6 +15,14 @@  Required children nodes:
  to external devices using the OF graph reprensentation (see ../graph.txt).
  At least one port node is required.
 
+Optional properties in grandchild nodes:
+ Any endpoint grandchild node may specify a desired video interface
+ according to ../../media/video-interfaces.txt, specifically
+ - bus-type: must be <0>.
+ - bus-width: recognized values are <12>, <16>, <18> and <24>, and
+   override any output mode selection heuristic, forcing "rgb444",
+   "rgb565", "rgb666" and "rgb888" respectively.
+
 Example:
 
 	hlcdc: hlcdc@f0030000 {
@@ -50,3 +58,21 @@  Example:
 			#pwm-cells = <3>;
 		};
 	};
+
+
+Example 2: With a video interface override to force rgb565; as above
+but with these changes/additions:
+
+	&hlcdc {
+		hlcdc-display-controller {
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
+
+			port@0 {
+				hlcdc_panel_output: endpoint@0 {
+					bus-type = <0>;
+					bus-width = <16>;
+				};
+			};
+		};
+	};