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

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

On Tue, 15 Jul 2014 12:52:54 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote:
> > Hi Thierry,
> > 
> > On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:
> > > On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> > > > On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> > > >> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding 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
> > > > 
> > > > [snip]
> > > > 
> > > >>>> + - 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.
> > > > 
> > > > You could do that, but it won't help you much, as the HLCDC driver must
> > > > not parse properties from the panel node. You should instead extend the
> > > > drm_panel API with a function to retrieve panel properties. The HLCDC
> > > > driver will then query the panel driver at runtime for the interface
> > > > type. The panel driver will get the information from hardcoded data in
> > > > the driver, from DT or possibly in some cases by querying the panel
> > > > hardware directly.
> > > 
> > > My preference for this would be that we either add this to some existing
> > > structure (struct drm_display_info seems like a good candidate) or if
> > > the number of parameters grows out of hands, then maybe even introduce a
> > > new type of device that's specific for the interface. DRM panels are an
> > > abstraction for panels, that is, interface-agnostic, and if we start
> > > exposing interface specific parameters things will start to become very
> > > unwieldy.
> > 
> > I agree with the goal of keeping drm_panel interface-agnostic. However, one 
> > way or another, interface parameters will need to be communicated between the 
> > panel driver and the controller driver. My preference, if we need to extend 
> > the number and/or scope of parameters beyond what drm_display_info could 
> > reasonably contain, would be to implement a new drm_panel operation to 
> > query/configure interface parameters, using a structure that contains the 
> > interface type and a union of type-specific structures. This would keep the 
> > API generic in the sense of not requiring explicit knowledge of all interfaces 
> > in the drivers, while offering the flexibility we need with a way to easily 
> > detect the interface type at runtime and react on unknown/unsupported types.
> 
> That's exactly what I was hoping could be avoided. If instead we modeled
> the interface type as a bus, we could for example have an lvds_bus along
> with an lvds_device and then use that as the natural place to store
> these properties. Much like we do for DSI.

I understand this is not a simple case here, and this is why I left
RGB mode config in the HLCDC node in the first place.

Anyway, I agree that this rgb mode should not be defined in the hlcdc
node but rather in the slave device. I said slave device and not panel
device here because the device connected to the RGB connector is not
necessarily an LCD panel (i.e. atmel is connecting a raw RGB to HDMI
bridge on the RGB connector). And given that I definitely think an
interface bus architecture is better: this way we could configure RGB
mode no matter what kind of device is connected on this bus and we
could keep slave devices interface-agnostic.

This being said, I guess modeling interface (or connector) types as
busses is not that simple.

I really want to help here, so let me know what I can do.

Just a side note: you are saying that RGB mode is a panel property, and
this is not entirely true (it might depends on board design) :-). In
some HW designs, LSB bits of the RGB connector are either connected to
ground or to the first available MSB bit. This way you can use an LCD
panel supporting RGB888 mode with an display controller supporting lower
modes (RGB555 or RGB666).

Best Regards,

Boris

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