[PATCH 2/2] drm: Add DT bindings documentation for OpenCores VGA/LCD controller

[PATCH 2/2] drm: Add DT bindings documentation for OpenCores VGA/LCD controller

[Andrea Merello]

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Francesco Diotalevi <francesco.diotalevi@iit.it>
Cc: Claudio Lorini <claudio.lorini@iit.it>
---
 .../bindings/display/opencores,ocdrm.txt           | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/opencores,ocdrm.txt

diff --git a/Documentation/devicetree/bindings/display/opencores,ocdrm.txt b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
new file mode 100644
index 0000000..8d36de5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
@@ -0,0 +1,27 @@
+OpenCores VGA/LCD controller
+
+Required properties:
+- compatible: "opencores,ocdrm"
+- reg: Physical base address and length of the controller's registers.
+- clocks: Must contain an entry for the pixelclock generator.
+  See ../clocks/clock-bindings.txt for details.
+
+Required sub-nodes:
+- port: the connection to a DRM bridge.  The connection is modelled
+  using the OF graph bindings specified in
+  Documentation/devicetree/bindings/graph.txt.
+
+Example:
+
+ocdrm@43c10000 {
+	compatible = "opencores,ocfb-drm";
+	reg = <0x43c10000 0x10000>;
+	clocks = <&pix_clock>;
+	ports {
+		port@0 {
+			rgb_out: endpoint {
+				remote-endpoint = <&bridge_in>;
+			};
+		};
+	};
+};
-- 
1.9.1

Re: [PATCH 2/2] drm: Add DT bindings documentation for OpenCores VGA/LCD controller

[Rob Herring]

On Thu, Jun 09, 2016 at 03:33:19PM +0200, Andrea Merello wrote:
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Francesco Diotalevi <francesco.diotalevi@iit.it>
> Cc: Claudio Lorini <claudio.lorini@iit.it>
> ---
>  .../bindings/display/opencores,ocdrm.txt           | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/opencores,ocdrm.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/opencores,ocdrm.txt b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
> new file mode 100644
> index 0000000..8d36de5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
> @@ -0,0 +1,27 @@
> +OpenCores VGA/LCD controller
> +
> +Required properties:
> +- compatible: "opencores,ocdrm"

Name this based on what the h/w block is called, not a Linux subsystem.

Is there any sort of versioning for OpenCore IP that you can include in 
the name?

> +- reg: Physical base address and length of the controller's registers.
> +- clocks: Must contain an entry for the pixelclock generator.
> +  See ../clocks/clock-bindings.txt for details.
> +
> +Required sub-nodes:
> +- port: the connection to a DRM bridge.  The connection is modelled

Don't include Linux driver details (DRM) in bindings.

> +  using the OF graph bindings specified in
> +  Documentation/devicetree/bindings/graph.txt.
> +
> +Example:
> +
> +ocdrm@43c10000 {
> +	compatible = "opencores,ocfb-drm";

Doesn't match the above and even worse naming.

> +	reg = <0x43c10000 0x10000>;
> +	clocks = <&pix_clock>;
> +	ports {
> +		port@0 {

Single port doesn't need a unit address.

> +			rgb_out: endpoint {
> +				remote-endpoint = <&bridge_in>;
> +			};
> +		};
> +	};
> +};
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] drm: Add DT bindings documentation for OpenCores VGA/LCD controller

[Andrea Merello]

On Fri, Jun 10, 2016 at 7:36 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Jun 09, 2016 at 03:33:19PM +0200, Andrea Merello wrote:
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: Francesco Diotalevi <francesco.diotalevi@iit.it>
>> Cc: Claudio Lorini <claudio.lorini@iit.it>
>> ---
>>  .../bindings/display/opencores,ocdrm.txt           | 27 ++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/display/opencores,ocdrm.txt b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>> new file mode 100644
>> index 0000000..8d36de5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>> @@ -0,0 +1,27 @@
>> +OpenCores VGA/LCD controller
>> +
>> +Required properties:
>> +- compatible: "opencores,ocdrm"
>
> Name this based on what the h/w block is called, not a Linux subsystem.

Ok.

Something like "opencores,video" ?

The older framebuffer driver has "opencores,ocfb"; I assumed the
suffix -fb was because of it is a "framebuffer" _driver_, but maybe it
is because they consider it a "framebuffer" _device_.

Maybe I can keep the same "compatible" string? Is it ok to have two
drivers (maybe temporarily, until the older one fades out) with the
same "compatible" ?

> Is there any sort of versioning for OpenCore IP that you can include in
> the name?

Not sure about this. I'll check.

>> +- reg: Physical base address and length of the controller's registers.
>> +- clocks: Must contain an entry for the pixelclock generator.
>> +  See ../clocks/clock-bindings.txt for details.
>> +
>> +Required sub-nodes:
>> +- port: the connection to a DRM bridge.  The connection is modelled
>
> Don't include Linux driver details (DRM) in bindings.

You mean telling that the port must be a connection to a DRM bridge,
leaving out further details?

>> +  using the OF graph bindings specified in
>> +  Documentation/devicetree/bindings/graph.txt.
>> +
>> +Example:
>> +
>> +ocdrm@43c10000 {
>> +     compatible = "opencores,ocfb-drm";
>
> Doesn't match the above and even worse naming.

OK.

>> +     reg = <0x43c10000 0x10000>;
>> +     clocks = <&pix_clock>;
>> +     ports {
>> +             port@0 {
>
> Single port doesn't need a unit address.

OK.

>> +                     rgb_out: endpoint {
>> +                             remote-endpoint = <&bridge_in>;
>> +                     };
>> +             };
>> +     };
>> +};
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] drm: Add DT bindings documentation for OpenCores VGA/LCD controller

[Rob Herring]

On Mon, Jun 27, 2016 at 6:21 AM, Andrea Merello
<andrea.merello@gmail.com> wrote:
> On Fri, Jun 10, 2016 at 7:36 PM, Rob Herring <robh@kernel.org> wrote:
>> On Thu, Jun 09, 2016 at 03:33:19PM +0200, Andrea Merello wrote:
>>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>>> Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: Francesco Diotalevi <francesco.diotalevi@iit.it>
>>> Cc: Claudio Lorini <claudio.lorini@iit.it>
>>> ---
>>>  .../bindings/display/opencores,ocdrm.txt           | 27 ++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/opencores,ocdrm.txt b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>> new file mode 100644
>>> index 0000000..8d36de5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>> @@ -0,0 +1,27 @@
>>> +OpenCores VGA/LCD controller
>>> +
>>> +Required properties:
>>> +- compatible: "opencores,ocdrm"
>>
>> Name this based on what the h/w block is called, not a Linux subsystem.
>
> Ok.
>
> Something like "opencores,video" ?

The VGA/LCD controller IP block is just called "video"?

> The older framebuffer driver has "opencores,ocfb"; I assumed the
> suffix -fb was because of it is a "framebuffer" _driver_, but maybe it
> is because they consider it a "framebuffer" _device_.

Could be either. The FB and DRM "bindings" have been flawed in
general. The bindings should describe hardware blocks and the
connections between them, not what the kernel subsystem wants.

> Maybe I can keep the same "compatible" string? Is it ok to have two
> drivers (maybe temporarily, until the older one fades out) with the
> same "compatible" ?

In principal, that is exactly how it should work. However, the FB
binding is probably incomplete or flawed. If keeping the compatible
string is enough for it to work, then that is fine.

>> Is there any sort of versioning for OpenCore IP that you can include in
>> the name?
>
> Not sure about this. I'll check.
>
>>> +- reg: Physical base address and length of the controller's registers.
>>> +- clocks: Must contain an entry for the pixelclock generator.
>>> +  See ../clocks/clock-bindings.txt for details.
>>> +
>>> +Required sub-nodes:
>>> +- port: the connection to a DRM bridge.  The connection is modelled
>>
>> Don't include Linux driver details (DRM) in bindings.
>
> You mean telling that the port must be a connection to a DRM bridge,
> leaving out further details?

The binding itself and description should not contain the word DRM.
Just say connection to bridge chip or connector.

Rob