Hi Boris,

On Wednesday 16 July 2014 15:05:22 Boris BREZILLON wrote:

On Tue, 15 Jul 2014 13:07:58 +0200 Laurent Pinchart wrote:

quoted

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

quoted

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
[off-list ref]
---

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

Could you detail a bit what you mean by "single object type" ?

Is this about making a common abstraction class (by mean of
drm_xxx and drm_xxx_funcs) that could represent any display device
(drm_bridge, drm_panel, ...) ?

Exactly :-) This is similar to what exists in V4L, with a v4l2_subdev object 
able to model any kind of IP core or external chip.

I don't think we will get there in one go, but I'd like to start by merging 
drm_encoder and drm_bridge on the kernel side. Both objects model the same 
hardware, a drm_encoder on one board could be a drm_bridge on another one. 

From a userspace point of view drm_encoder won't go away, and we can't chain 

multiple encoders, so the change would be internal to the kernel only.

Then, as a next step, I believe using the same object to model panels would be 
a good idea, but there's no consensus on that.

-- 
Regards,

Laurent Pinchart