Re: [RESEND PATCH v3 06/11] drm: add DT bindings documentation for atmel-hlcdc-dc driver

[Boris BREZILLON <boris.brezillon@free-electrons.com>]

Hello Thierry,

On Mon, 14 Jul 2014 12:05:43 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > controller device.
> > 
> > The HLCDC block provides a single RGB output port, and only supports LCD
> > panels connection to LCD panels for now.
> > 
> > The atmel,panel property link the HLCDC RGB output with the LCD panel
> > connected on this port (note that the HLCDC RGB connector implementation
> > makes use of the DRM panel framework).
> > 
> > Connection to other external devices (DRM bridges) might be added later by
> > mean of a new atmel,xxx (atmel,bridge) property.
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 ++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> 
> This is the wrong directory. Device tree bindings describe hardware, but
> DRM is a Linux-specific framework. And yes, there are already files in
> that directory, I know, but that doesn't make it any better.
> 
> I suggest either devicetree/bindings/gpu or devicetree/bindings/video.

No problem, I'll move the documentation into devicetree/bindings/video
(the HLCDC does not provide any 3D rendering functionality and thus
I'm not sure moving the bindings documentation into
devicetree/bindings/gpu makes sense).

> 
> > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> [...]
> > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> 
> I think it's better to refer to these using relative filenames. When the
> device tree bindings are moved out of the kernel tree, they may no
> longer use the same hierarchy.

Sure.
By relative path you mean ../../mfd/atmel-hlcdc.txt or just
mfd/atmel-hlcdc.txt ?

> 
> > +Required properties:
> > + - compatible: value should be one of the following:
> > +   "atmel,hlcdc-dc"
> 
> There's only one value, so perhaps: should be "atmel,hlcdc-dc".

Yes, I'll fix that.

> 
> > + - atmel,panel: Should contain a phandle with 2 parameters.
> > +   The first cell is a phandle to a DRM panel device
> > +   The second cell encodes the RGB mode, which can take the following values:
> > +   * 0: RGB444
> > +   * 1: RGB565
> > +   * 2: RGB666
> > +   * 3: RGB888
> 
> These are properties of the panel and should be obtained from the panel
> directly rather than an additional cell in this specifier.

Okay.
What's the preferred way of doing this ?
What about defining an rgb-mode property in the panel node.

BTW, have you received this series [1] adding support for the LCD panel
I'm testing this driver with.

[1] https://lkml.org/lkml/2014/6/5/612

> 
> > +   The third cell encodes specific flags describing LCD signals configuration
> > +   (see Atmel's datasheet for a full description of these fields):
> > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > +   * bit 4: DISPPOL: Display Signal Polarity
> > +   * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization
> > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration
> > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration
> > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> 
> Similarly for most of these: HSPOL and VSPOL seem to correspond to the
> DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And
> VSPDLYS as well as VSPDLYE sound like they may be vsync_start and
> vsync_end of the same structure.

I agree with HSPOL and VSPOL.

> 
> As for the others, maybe if you could explain what exactly they are we
> may be able to find a better fit.

Atmel datasheets include several timing diagrams [2] (chapter "32.6.17
Output Timing Generation" page 603), and I think you will get more
informations from these diagrams than if I try to explain what I
understood ;-).

Best Regards,

Boris

[1]https://lkml.org/lkml/2014/6/5/612
[2]http://www.atmel.com/Images/Atmel_11121_32-bit-Cortex-A5-Microcontroller_SAMA5D3_Datasheet.pdf

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com