[RFR 2/2] drm/panel: Add simple panel support

[Thierry Reding]

On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Thursday 17 October 2013 13:05:18 Thierry Reding wrote:
> > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
> > > On 17/10/13 11:53, Thierry Reding wrote:
> > > > I keep wondering why we absolutely must have compatibility between CDF
> > > > and this simple panel binding. DT content is supposed to concern itself
> > > > with the description of hardware only. What about the following isn't an
> > > > accurate description of the panel hardware?
> > > > 
> > > > 	panel: panel {
> > > > 		compatible = "cptt,claa101wb01";
> > > > 		
> > > > 		power-supply = <&vdd_pnl_reg>;
> > > > 		enable-gpios = <&gpio 90 0>;
> > > > 		
> > > > 		backlight = <&backlight>;
> > > > 	};
> > > > 	
> > > > 	dsi-controller {
> > > > 		panel = <&panel>;
> > > > 	};
> > > 
> > > That's quite similar to how my current out-of-mainline OMAP DSS DT
> > > bindings work. The difference is that I have a reference from the panel
> > > node to the video source (dsi-controller), instead of the other way
> > > around. I just find it more natural. It works the same way as, say,
> > > regulators, gpios, etc.
> > 
> > I suppose that depends on the way you look at it. In the above proposal
> > I consider the output (DSI controller) to use the panel as a resource
> > that provides a certain set of services (query mode timings, provide a
> > way to enable or disable the panel). Similarly the panel uses the
> > backlight as a resource that provides a certain set of services (such as
> > changing the brightness).
> > 
> > The above also follows the natural order of control. The panel has no
> > way to control the DSI output. However, it is the output that controls
> > when a panel is required and turn it on as appropriate.
> 
> I'm no DSI expert, but I know enough about it to be sure that Tomi will 
> disagree. DSI panels can have complex power sequences that require the input 
> stream to be finely controlled (for instance it might require a clock without 
> video frames for some time, switch a GPIO or regulator, send a command to the 
> panel, and then only get video frames). For that reason all developers I've 
> talked to who have an in-depth knowledge of DSI and DSI panels told me that 
> the panel needs to control the video bus, and request the video source to 
> start/stop the video stream.

Oh, I'm very well aware of the various flavours of funkiness that DSI
panels come in. But it's wrong to say that the panel needs to control
the video bus. There's simply no way that a panel can actually do that.
It is true, however, that in order to make this work in a maintainable
fashion, the DSI panel *driver* may need to control the DSI bus. That's
an entirely different story.

Again, that's exactly how I2C works as well. I2C slaves, by definition,
do not control the I2C bus. However, I2C client drivers can do so by
using the (master) services provided by the I2C controller.

So for DSI what we could easily do is provide a DSI "adapter" to the
panel when it is attached to the DSI controller. If the DSI adapter
implements all of the required services, then the DSI panel driver is
easily capable of handling all the required special quirks.

I'm Cc'ing Rob and Daniel, both of whom have been working on this
lately. Actually I'm not sure if Daniel was directly involved, but at
least somebody from Intel's team was working on it.

> > > However, one thing unclear is where the panel node should be. You seem
> > > to have a DSI panel here. If the panel is truly not controlled in any
> > > way, maybe having the panel as a normal platform device is ok. But if
> > > the DSI panel is controlled with DSI messages, should it be a child of
> > > the dsi-controller node, the same way i2c peripherals are children of
> > > i2c master?
> > 
> > That's one possibility. In this particular case it's not even necessary
> > to use any of DSIs control methods to talk to the panel. The panel only
> > receives the data stream and "just works".
> > 
> > Even if the panel were to be controlled via DSI, I think keeping it as
> > a separate device node is equally valid. It's really boils down to what
> > I already said above. The output uses the panel as a resource, similar
> > to how other devices might use a GPIO controller to use a GPIO pin, or
> > an interrupt controller to request an IRQ.
> 
> Except that the display controller can also provides resources to the panel. 
> For DSI panels that support DSI commands, the control bus is provided by the 
> display controller. Much like an I2C device must be a child of its I2C 
> controller node, the DSI panel device should then be a child of the display 
> controller node.

I don't think resources is the right term here. The DSI controller
provides services. That's traditionally been handled in DT by handing
out phandles to the service providers.

That said I see why it would make sense to make a DSI panel the child of
the DSI controller node, so we agree at least partially. I don't like it
all that much because it makes handling of panels inconsistent across
various types of output and therefore makes it more complicated to
support it drivers, but I'm sure I can live with it if somebody wants to
enforce that.

> > > > I do agree though that it might be useful to tweak the mode in case the
> > > > default one doesn't work. How about we provide a means to override the
> > > > mode encoded in the driver using one specified in the DT? That's
> > > > obviously a backwards-compatible change, so it could be added if or when
> > > > it becomes necessary.
> > > 
> > > This sounds good to me.
> > > 
> > > Although maybe it'd be good to have the driver compatible with something
> > > like "generic-panel", so that you could have:
> > > 
> > > compatible = "foo,specific-panel", "generic-panel";
> > > 
> > > and if there's no need for any power/gpio setup for your board, you may
> > > skip adding "foo,specific-panel" support to the panel driver. Later,
> > > somebody else may need to implement fine grained power/gpio support for
> > > "foo,specific-panel", and it would just work.
> > > 
> > > Maybe that would help us with the problem of adding support in the
> > > driver for a hundred different simple panels each used only once on a
> > > specific board.
> > 
> > Sure, that all sounds like reasonable additions. All of the suggestions
> > are even backwards-compatible and hence can be added when needed without
> > breaking compatibility with existing users of the binding.
> 
> What's wrong with moving the three hardcoded modes we already have in the 
> driver to DT before pushing this to mainline ?

I think I've answered that in a different subthread.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20131017/63a1008f/attachment.pgp>