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:
quoted
On 17/10/13 11:53, Thierry Reding wrote:
quoted
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.
quoted
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.
quoted
quoted
quoted
quoted
+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..." =).
quoted
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 ?
--
Regards,
Laurent Pinchart
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:
quoted
On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
quoted
On 17/10/13 11:53, Thierry Reding wrote:
quoted
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.
quoted
quoted
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.
quoted
quoted
quoted
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