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@xxxxxxxxx> wrote:
> On Fri, Jun 10, 2016 at 7:36 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
>> On Thu, Jun 09, 2016 at 03:33:19PM +0200, Andrea Merello wrote:
>>> Signed-off-by: Andrea Merello <andrea.merello@xxxxxxxxx>
>>> Cc: Stefan Kristiansson <stefan.kristiansson@xxxxxxxxxxxxx>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
>>> Cc: Francesco Diotalevi <francesco.diotalevi@xxxxxx>
>>> Cc: Claudio Lorini <claudio.lorini@xxxxxx>
>>> ---
>>>  .../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