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

[boris.brezillon@free-electrons.com (Boris BREZILLON)]

On Fri, 18 Jul 2014 16:51:52 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> Hi Thierry,
> 
> Oops, I missed this reply.
> 
> On Tue, 15 Jul 2014 12:31:37 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> > > 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:
> > [...]
> > > > > 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 ?
> > 
> > I think the former is more explicit.
> 
> Okay.
> 
> > 
> > > > > + - 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.
> > 
> > There's .bpc in struct drm_display_info, I suspect that it could be used
> > for this. Alternatively, maybe we could extend the list of color formats
> > that go into drm_display_info.color_formats? RGB444 is already covered.
> 

I forgot to ask about bpc meaning. If, as I think, it means "bits per
color" then it cannot be used to encode RGB565 where green color is
encoded on 6 bits and red and blue are encoded on 5 bits. 

> I don't think this color_formats field is intended to represent data
> stream format going through the bus.
> Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma
> subsampling rate) and not 12 bits signals (4 bits for each color).
> 
> Anyway I'll propose a patch series adding a new field to
> drm_display_info to encode the mediabus format (as discussed with
> Laurent and you).
> 
> > 
> > Also, like Laurent said, this shouldn't go into the device tree, since
> > it's already implied by the panel's compatible value, so we'd be
> > duplicating information.
> 
> Again, this is not necessarily true (depending on your board design).
> One can decide to connect an RGB888 panel on an RGB666 bus and connect
> the missing pins to ground.
> 
> > 
> > > 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
> > 
> > I don't think I've seen it in my inbox, let me check my archives.
> > 
> > > > > +   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 ;-).
> > 
> > These look like knobs to tune the signal in a very fine-grained manner.
> > To be honest, maybe the best way to solve this would be by omitting them
> > for now and choose some default that's likely to work on most devices.
> > Does the panel that you use specify how it expects HSYNC to be timed vs.
> > VSYNC?
> 
> 
> No it doesn't, and I agree that we should leave these specific timing
> tweaks unimplemented until we really need them.
> 
> Regards,
> 
> Boris
> 
> 



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