diff mbox

[v1,1/7] dt-bindings: display: add STM32 LTDC driver

Message ID 1484573344-11609-2-git-send-email-yannick.fertre@st.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yannick FERTRE Jan. 16, 2017, 1:28 p.m. UTC 
Signed-off-by: Yannick Fertre <yannick.fertre@st.com>
---
 .../devicetree/bindings/display/st,ltdc.txt        | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/st,ltdc.txt

Comments

Laurent Pinchart Jan. 16, 2017, 8:30 p.m. UTC | #1 
Hi Yannick,

Thank you for the patch.

On Monday 16 Jan 2017 14:28:58 Yannick Fertre wrote:
> Signed-off-by: Yannick Fertre <yannick.fertre@st.com>
> ---
>  .../devicetree/bindings/display/st,ltdc.txt        | 57 ++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/st,ltdc.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/st,ltdc.txt
> b/Documentation/devicetree/bindings/display/st,ltdc.txt new file mode
> 100644
> index 0000000..20e89da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/st,ltdc.txt
> @@ -0,0 +1,57 @@
> +* STMicroelectronics STM32 lcd-tft display controller
> +
> +- st-display-subsystem: Master device for DRM sub-components
> +  This device must be the parent of all the sub-components and is
> responsible
> +  of bind them.

Why do you need this ? At a quick glance the ltdc node should be enough.

> +  Required properties:
> +  - compatible: "st,display-subsystem"
> +  - ranges: to allow probing of subdevices
> +
> +- ltdc_host: lcd-tft display controller host
> +  must be a sub-node of st-display-subsystem
> +  Required properties:
> +  - compatible: "st,ltdc"
> +  - reg: Physical base address of the IP registers and length of memory
> mapped region.
> +  - clocks: from common clock binding: handle hardware IP needed clocks,
> the
> +    number of clocks may depend of the SoC type.
> +    See ../clocks/clock-bindings.txt for details.
> +  - clock-names: names of the clocks listed in clocks property in the same
> +    order.

You need to define the required/optional clocks with their names here. If they 
vary depending on the SoC, the DT bindings document need to list them for each 
SoC.

> +  - resets: resets to be used by the device
> +    See ../reset/reset.txt for details.
> +  - reset-names: names of the resets listed in resets property in the same
> +    order.
> +  Required nodes:
> +    - Video port for RGB output.
> +
> +Example:
> +
> +/ {
> +	...
> +	soc {
> +	...
> +		st-display-subsystem {
> +			compatible = "st,display-subsystem";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			dma-ranges;
> +
> +			ltdc_host: stm32-ltdc@40016800 {
> +				compatible = "st,ltdc";
> +				reg = <0x40016800 0x200>;
> +				interrupts = <88>, <89>;
> +				resets = <&rcc 314>;
> +				clocks = <&rcc 1 8>;
> +				clock-names = "clk-lcd";
> +				status = "disabled";
> +
> +				port {
> +					ltdc_out_rgb: endpoint {
> +					};
> +				};
> +			};
> +		};
> +	...
> +	};
> +};
Yannick FERTRE Jan. 19, 2017, 4:16 p.m. UTC | #2 
On 01/16/2017 09:30 PM, Laurent Pinchart wrote:
> Hi Yannick,
>
> Thank you for the patch.
>
> On Monday 16 Jan 2017 14:28:58 Yannick Fertre wrote:
>> Signed-off-by: Yannick Fertre <yannick.fertre@st.com>
>> ---
>>  .../devicetree/bindings/display/st,ltdc.txt        | 57 ++++++++++++++++++
>>  1 file changed, 57 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/st,ltdc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/display/st,ltdc.txt
>> b/Documentation/devicetree/bindings/display/st,ltdc.txt new file mode
>> 100644
>> index 0000000..20e89da
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/st,ltdc.txt
>> @@ -0,0 +1,57 @@
>> +* STMicroelectronics STM32 lcd-tft display controller
>> +
>> +- st-display-subsystem: Master device for DRM sub-components
>> +  This device must be the parent of all the sub-components and is
>> responsible
>> +  of bind them.
>
> Why do you need this ? At a quick glance the ltdc node should be enough.
Hi Laurent,
To prepare next device upstream of dsi, we need a master node 
"st-display-subsystem" and a sub node "st,ltdc". It's the same kind of 
node than st,stih4xx.
>
>> +  Required properties:
>> +  - compatible: "st,display-subsystem"
>> +  - ranges: to allow probing of subdevices
>> +
>> +- ltdc_host: lcd-tft display controller host
>> +  must be a sub-node of st-display-subsystem
>> +  Required properties:
>> +  - compatible: "st,ltdc"
>> +  - reg: Physical base address of the IP registers and length of memory
>> mapped region.
>> +  - clocks: from common clock binding: handle hardware IP needed clocks,
>> the
>> +    number of clocks may depend of the SoC type.
>> +    See ../clocks/clock-bindings.txt for details.
>> +  - clock-names: names of the clocks listed in clocks property in the same
>> +    order.
>
> You need to define the required/optional clocks with their names here. If they
> vary depending on the SoC, the DT bindings document need to list them for each
> SoC.
>
Ok, I'll push news bindings with pack V2.
>> +  - resets: resets to be used by the device
>> +    See ../reset/reset.txt for details.
>> +  - reset-names: names of the resets listed in resets property in the same
>> +    order.
>> +  Required nodes:
>> +    - Video port for RGB output.
>> +
>> +Example:
>> +
>> +/ {
>> +	...
>> +	soc {
>> +	...
>> +		st-display-subsystem {
>> +			compatible = "st,display-subsystem";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges;
>> +			dma-ranges;
>> +
>> +			ltdc_host: stm32-ltdc@40016800 {
>> +				compatible = "st,ltdc";
>> +				reg = <0x40016800 0x200>;
>> +				interrupts = <88>, <89>;
>> +				resets = <&rcc 314>;
>> +				clocks = <&rcc 1 8>;
>> +				clock-names = "clk-lcd";
>> +				status = "disabled";
>> +
>> +				port {
>> +					ltdc_out_rgb: endpoint {
>> +					};
>> +				};
>> +			};
>> +		};
>> +	...
>> +	};
>> +};
>
Rob Herring (Arm) Jan. 19, 2017, 4:29 p.m. UTC | #3 
On Mon, Jan 16, 2017 at 02:28:58PM +0100, Yannick Fertre wrote:
> Signed-off-by: Yannick Fertre <yannick.fertre@st.com>

Changelog?

> ---
>  .../devicetree/bindings/display/st,ltdc.txt        | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/st,ltdc.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/st,ltdc.txt b/Documentation/devicetree/bindings/display/st,ltdc.txt
> new file mode 100644
> index 0000000..20e89da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/st,ltdc.txt
> @@ -0,0 +1,57 @@
> +* STMicroelectronics STM32 lcd-tft display controller
> +
> +- st-display-subsystem: Master device for DRM sub-components
> +  This device must be the parent of all the sub-components and is responsible
> +  of bind them.
> +  Required properties:
> +  - compatible: "st,display-subsystem"
> +  - ranges: to allow probing of subdevices

You have a single sub device. There is no need for this node.


> +- ltdc_host: lcd-tft display controller host
> +  must be a sub-node of st-display-subsystem
> +  Required properties:
> +  - compatible: "st,ltdc"

Too generic. Needs to be SoC specific.

> +  - reg: Physical base address of the IP registers and length of memory mapped region.
> +  - clocks: from common clock binding: handle hardware IP needed clocks, the
> +    number of clocks may depend of the SoC type.

No. You must be explicit on how many clocks and their order.

> +    See ../clocks/clock-bindings.txt for details.
> +  - clock-names: names of the clocks listed in clocks property in the same
> +    order.
> +  - resets: resets to be used by the device

ditto.

> +    See ../reset/reset.txt for details.
> +  - reset-names: names of the resets listed in resets property in the same
> +    order.

You are missing this in the example. However, for a single entry, -names 
is pointless.

> +  Required nodes:
> +    - Video port for RGB output.
> +
> +Example:
> +
> +/ {
> +	...
> +	soc {
> +	...
> +		st-display-subsystem {
> +			compatible = "st,display-subsystem";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			dma-ranges;
> +
> +			ltdc_host: stm32-ltdc@40016800 {
> +				compatible = "st,ltdc";
> +				reg = <0x40016800 0x200>;
> +				interrupts = <88>, <89>;
> +				resets = <&rcc 314>;
> +				clocks = <&rcc 1 8>;
> +				clock-names = "clk-lcd";

The 'clk-' part is redundant.

> +				status = "disabled";
> +
> +				port {
> +					ltdc_out_rgb: endpoint {
> +					};
> +				};
> +			};
> +		};
> +	...
> +	};
> +};
> -- 
> 1.9.1
>
Rob Herring (Arm) Jan. 19, 2017, 4:31 p.m. UTC | #4 
On Thu, Jan 19, 2017 at 04:16:01PM +0000, Yannick FERTRE wrote:
> On 01/16/2017 09:30 PM, Laurent Pinchart wrote:
> > Hi Yannick,
> >
> > Thank you for the patch.
> >
> > On Monday 16 Jan 2017 14:28:58 Yannick Fertre wrote:
> >> Signed-off-by: Yannick Fertre <yannick.fertre@st.com>
> >> ---
> >>  .../devicetree/bindings/display/st,ltdc.txt        | 57 ++++++++++++++++++
> >>  1 file changed, 57 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/display/st,ltdc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/st,ltdc.txt
> >> b/Documentation/devicetree/bindings/display/st,ltdc.txt new file mode
> >> 100644
> >> index 0000000..20e89da
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/st,ltdc.txt
> >> @@ -0,0 +1,57 @@
> >> +* STMicroelectronics STM32 lcd-tft display controller
> >> +
> >> +- st-display-subsystem: Master device for DRM sub-components
> >> +  This device must be the parent of all the sub-components and is
> >> responsible
> >> +  of bind them.
> >
> > Why do you need this ? At a quick glance the ltdc node should be enough.
> Hi Laurent,
> To prepare next device upstream of dsi, we need a master node 
> "st-display-subsystem" and a sub node "st,ltdc". It's the same kind of 
> node than st,stih4xx.

No, you don't. That's what OF graph is for. The DRM driver binds to the 
LCD controller node and walks the graph to find the DSI node and then 
the panel.

Rob
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/display/st,ltdc.txt b/Documentation/devicetree/bindings/display/st,ltdc.txt
new file mode 100644
index 0000000..20e89da
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/st,ltdc.txt
@@ -0,0 +1,57 @@ 
+* STMicroelectronics STM32 lcd-tft display controller
+
+- st-display-subsystem: Master device for DRM sub-components
+  This device must be the parent of all the sub-components and is responsible
+  of bind them.
+  Required properties:
+  - compatible: "st,display-subsystem"
+  - ranges: to allow probing of subdevices
+
+- ltdc_host: lcd-tft display controller host
+  must be a sub-node of st-display-subsystem
+  Required properties:
+  - compatible: "st,ltdc"
+  - reg: Physical base address of the IP registers and length of memory mapped region.
+  - clocks: from common clock binding: handle hardware IP needed clocks, the
+    number of clocks may depend of the SoC type.
+    See ../clocks/clock-bindings.txt for details.
+  - clock-names: names of the clocks listed in clocks property in the same
+    order.
+  - resets: resets to be used by the device
+    See ../reset/reset.txt for details.
+  - reset-names: names of the resets listed in resets property in the same
+    order.
+  Required nodes:
+    - Video port for RGB output.
+
+Example:
+
+/ {
+	...
+	soc {
+	...
+		st-display-subsystem {
+			compatible = "st,display-subsystem";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			dma-ranges;
+
+			ltdc_host: stm32-ltdc@40016800 {
+				compatible = "st,ltdc";
+				reg = <0x40016800 0x200>;
+				interrupts = <88>, <89>;
+				resets = <&rcc 314>;
+				clocks = <&rcc 1 8>;
+				clock-names = "clk-lcd";
+				status = "disabled";
+
+				port {
+					ltdc_out_rgb: endpoint {
+					};
+				};
+			};
+		};
+	...
+	};
+};