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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3623 bytes --]

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.

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?

>>> +static const struct drm_display_mode auo_b101aw03_mode = {
>>> +	.clock = 51450,
>>> +	.hdisplay = 1024,
>>> +	.hsync_start = 1024 + 156,
>>> +	.hsync_end = 1024 + 156 + 8,
>>> +	.htotal = 1024 + 156 + 8 + 156,
>>> +	.vdisplay = 600,
>>> +	.vsync_start = 600 + 16,
>>> +	.vsync_end = 600 + 16 + 6,
>>> +	.vtotal = 600 + 16 + 6 + 16,
>>> +	.vrefresh = 60,
>>> +};
>>
>> Instead of hardcoding the modes in the driver, which would then require to be 
>> updated for every new simple panel model (and we know there are lots of them), 
>> why don't you specify the modes in the panel DT node ? The simple panel driver 
>> would then become much more generic. It would also allow board designers to 
>> tweak h/v sync timings depending on the system requirements.
> 
> Sigh... we keep second-guessing ourselves. Back at the time when power
> sequences were designed (and NAKed at the last minute), we all decided
> that the right thing to do would be to use specific compatible values
> for individual panels, because that would allow us to encode the power
> sequencing within the driver. And when we already have the panel model
> encoded in the compatible value, we might just as well encode the mode
> within the driver for that panel. Otherwise we'll have to keep adding
> the same mode timings for every board that uses the same panel.

Oh, I didn't feel "we all decided that the right thing..." =).

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

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]