Hi Thierry,

On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote:

On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote:

quoted

On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:

quoted

On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:

quoted

On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:

quoted

On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:

quoted

On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:

quoted

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

 .../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]

quoted

quoted

quoted

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

And I believe that's what we should avoid ;-) First of all, let's not forget 
that Linux models control busses, not data busses. DSI is a special case as it 
combines the control and data busses, but in the general case the same 
implementation isn't possible. An LVDS panel controlled through I2C needs to 
be an I2C device sitting on an I2C bus.

Then, I believe it would make all drivers simpler if we had a single object 
type to deal with, with proper abstractions for bus types. A drm_panel that 
can model panels regardless of the data bus type, with one operation that 
conveys bus-specific information, makes storing the objects and communicating 
with them simpler than having to deal with different kind of devices.

-- 
Regards,

Laurent Pinchart