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

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

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 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