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