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.

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.

...

...

...

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

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.

Thierry

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

On Thu, Oct 17, 2013 at 02:23:32PM +0200, Laurent Pinchart wrote:

Hi Thierry,

On Thursday 17 October 2013 14:06:47 Thierry Reding wrote:

...

On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote:

...

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.

Sure, but I don't think that's really related to the DT bindings. We don't have to model every electrical signal in a detailed way that matches the direction of the electrons flow :-) What we need to model is a connection between a display controller and a panel (possibly with a direction). What I'd like to do is to express that link in a way that can also express more complex pipeline topologies. I don't want to make it overly complex, I had hoped that my DT bindings proposal would be a good approach as it's both generic and still pretty simple.

I get that, and for what it's worth I do think that your proposal looks simple enough and if it can solve any of the problem you're facing with CDF, then that's great.

But I don't think we should force inclusion of these properties on every panel, even if it doesn't use any of the graph functionality. Is there any problem with making them optional?

Thierry

On Thu, Oct 17, 2013 at 02:50:31PM +0300, Tomi Valkeinen wrote:

On 17/10/13 14:05, Thierry Reding wrote:

...

...

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've discussed this issue to death with other people, and for some reason I still see this the other way =).

I see the panel using the DSI controller as a resource. The panel driver uses the DSI controller to send the video data to the panel device. Compare this to, say, i2c client driver, which uses i2c master to send data to the i2c device.

In the OMAP DSS driver model, the panel driver is in control. The panel driver tells the DSI controller which kind of video timings to use, or which bus speed to use, or whether to use low-power or high-speed mode, when to enable the DSI clock or DSI video stream, etc.

With a simple panel the model you describe works usually fine. The reason I went with the different model is that we had rather complex display peripherals, that needed more specific control of the bus.

A simple example could be the enable sequence. I presume in your model the DSI controller's enable would be maybe something like:

configure_dsi_timings(panel->get_timings()); enable_dsi_output(); panel->enable();

What if the DSI peripheral requires an enable sequence of, say:

• enable DSI clock
• use LP mode
• send some config messages
• re-configure DSI clock to higher speed
• send some more config messages
• enable HS mode
• send some more config messages
• enable DSI video stream

It gets a bit difficult to manage that kind of setup in a generic way in the DSI controller driver. As I see it, the DSI peripheral driver has to be in control of the nuances. The same goes for other video buses also, but DSI is perhaps one of the most complex ones to support.

I've briefly gone over this in another subthread, but here it is again for the sake of completeness.

The difference in perception I think comes from the fact that the DSI panel *driver* may need to control the DSI bus. However, DT describes the hardware in terms of devices. And from that point of view it is still the DSI _control_ler that _control_s the panel. There's no electrical way that the panel can take possession of the bus. Well, there's BTA, but even that happens under supervision of the DSI controller.

Thierry

On 17/10/13 14:02, Laurent Pinchart wrote:

...

Okay, so if I understand correctly, translating those bindings to panel nodes would look somewhat like this:

dc: display-controller { ports { port@0 { remote-endpoint = <&panel>; }; }; };

panel: panel { ports { port@0 { remote-endpoint = <&dc>; }; }; };

The above leaves out any of the other, non-relevant properties. Does that sound about right?

Yes it does.

It does?

Shouldn't it be something like:

panel { ports { port@0 { endpoint@0 { remote = <&dc>; }; }; }; };

And simplified:

panel { port { endpoint@0 { remote = <&dc>; }; }; };

You do need a node for the endpoint, a remote-endpoint property is not enough.

Please note that, when a device has as single port, the ports node can be omitted, and the port doesn't need to be numbered. You would then end up with

dc: display-controller { port { remote-endpoint = <&panel>; }; };

panel: panel { port { remote-endpoint = <&dc>; }; };

I don't think there's a way to simplify it further.

I'm not sure if there's a specific need for the port or endpoint nodes in cases like the above. Even if we have common properties describing the endpoint, I guess they could just be in the parent node.

panel { remote = <&dc>; common-video-property = <asd>; };

The above would imply one port and one endpoint. Would that work? If we had a function like parse_endpoint(node), we could just point it to either a real endpoint node, or to the device's node.

Tomi

On 17/10/13 14:51, Laurent Pinchart wrote:

...

I'm not sure if there's a specific need for the port or endpoint nodes in cases like the above. Even if we have common properties describing the endpoint, I guess they could just be in the parent node.

panel { remote = <&dc>; common-video-property = <asd>; };

The above would imply one port and one endpoint. Would that work? If we had a function like parse_endpoint(node), we could just point it to either a real endpoint node, or to the device's node.

You reference the display controller here, not a specific display controller output. Don't most display controllers have several outputs ?

Sure. Then the display controller could have more verbose description. But the panel could still have what I wrote above, except the 'remote' property would point to a real endpoint node inside the dispc node, not to the dispc node.

This would, of course, need some extra code to handle the different cases, but just from DT point of view, I think all the relevant information is there.

Tomi

On 17/10/13 15:17, Laurent Pinchart wrote:

Hi Tomi,

On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote:

...

On 17/10/13 14:51, Laurent Pinchart wrote:

...

...

I'm not sure if there's a specific need for the port or endpoint nodes in cases like the above. Even if we have common properties describing the endpoint, I guess they could just be in the parent node.

panel { remote = <&dc>; common-video-property = <asd>; };

The above would imply one port and one endpoint. Would that work? If we had a function like parse_endpoint(node), we could just point it to either a real endpoint node, or to the device's node.

You reference the display controller here, not a specific display controller output. Don't most display controllers have several outputs ?

Sure. Then the display controller could have more verbose description. But the panel could still have what I wrote above, except the 'remote' property would point to a real endpoint node inside the dispc node, not to the dispc node.

This would, of course, need some extra code to handle the different cases, but just from DT point of view, I think all the relevant information is there.

There's many ways to describe the same information in DT. While we could have DT bindings that use different descriptions for different devices and still support all of them in our code, why should we opt for that option that will make the implementation much more complex when we can describe connections in a simple and generic way ?

My suggestion was simple and generic. I'm not proposing per-device custom bindings.

My point was, if we can describe the connections as I described above, which to me sounds possible, we can simplify the panel DT data for 99.9% of the cases.

To me, the first of these looks much nicer:

panel { remote = <&remote-endpoint>; common-video-property = <asd>; };

panel { port { endpoint { remote = <&remote-endpoint>; common-video-property = <asd>; }; }; };

If that can be supported in the SW by adding complexity to a few functions, and it covers practically all the panels, isn't it worth it?

Note that I have not tried this, so I don't know if there are issues. It's just a thought. Even if there's need for a endpoint node, perhaps the port node can be made optional.

Tomi

Hi Tomi,

On Thursday 17 October 2013 15:32:29 Tomi Valkeinen wrote:

On 17/10/13 15:17, Laurent Pinchart wrote:

...

On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote:

...

On 17/10/13 14:51, Laurent Pinchart wrote:

...

...

I'm not sure if there's a specific need for the port or endpoint nodes in cases like the above. Even if we have common properties describing the endpoint, I guess they could just be in the parent node.

panel { remote = <&dc>; common-video-property = <asd>; };

The above would imply one port and one endpoint. Would that work? If we had a function like parse_endpoint(node), we could just point it to either a real endpoint node, or to the device's node.

You reference the display controller here, not a specific display controller output. Don't most display controllers have several outputs ?

Sure. Then the display controller could have more verbose description. But the panel could still have what I wrote above, except the 'remote' property would point to a real endpoint node inside the dispc node, not to the dispc node.

This would, of course, need some extra code to handle the different cases, but just from DT point of view, I think all the relevant information is there.

There's many ways to describe the same information in DT. While we could have DT bindings that use different descriptions for different devices and still support all of them in our code, why should we opt for that option that will make the implementation much more complex when we can describe connections in a simple and generic way ?

My suggestion was simple and generic. I'm not proposing per-device custom bindings.

My point was, if we can describe the connections as I described above, which to me sounds possible, we can simplify the panel DT data for 99.9% of the cases.

To me, the first of these looks much nicer:

panel { remote = <&remote-endpoint>; common-video-property = <asd>; };

panel { port { endpoint { remote = <&remote-endpoint>; common-video-property = <asd>; }; }; };

Please note that the common video properties would be in the panel node, not in the endpoint node (unless you have specific requirements to do so, which isn't the common case).

If that can be supported in the SW by adding complexity to a few functions, and it covers practically all the panels, isn't it worth it?

Note that I have not tried this, so I don't know if there are issues. It's just a thought. Even if there's need for a endpoint node, perhaps the port node can be made optional.

It can be worth it, as long as we make sure that simplified bindings cover the needs of the generic code.

We could assume that, if the port subnode isn't present, the device will have a single port, with a single endpoint. However, isn't the number of endpoints a system property rather than a device property ? If a port of a device is connected to two remote ports it will require two endpoints. We could select the simplified or full bindings based on the system topology though.

I've CC'ed Sylwester Nawrocki and Guennadi Liakhovetski, the V4L2 DT bindings authors, as well as the linux-media list, to get their opinion on this.

On 10/24/2013 12:52 PM, Tomi Valkeinen wrote:

On 24/10/13 13:40, Laurent Pinchart wrote:

...

...

panel { remote =<&remote-endpoint>; common-video-property =<asd>; };

panel { port { endpoint { remote =<&remote-endpoint>; common-video-property =<asd>; }; }; };

Please note that the common video properties would be in the panel node, not in the endpoint node (unless you have specific requirements to do so, which isn't the common case).

Hmm, well, the panel driver must look for its properties either in the panel node, or in the endpoint node (I guess it could look them from both, but that doesn't sound good).

Presumably the OS could be searching for port node and any endpoint node inside it first. If that's not found then it could be parsing the panel node.

Please note that a port node may be required even if there is only one port, when there are multiple physical bus interfaces, e.g. at an LCD controller and only one of them is used. The reg property would select the physical bus interface.

I wonder if a property like #video-port or #video-endpoint could be used to indicate that a node contains video bus properties. Probably it's too late to introduce it now and make it a required property for the endpoint nodes or nodes containing the common video properties.

If you write the panel driver, and in all your cases the properties work fine in the panel node, does that mean they'll work fine with everybody?

It's likely not safe to assume so. In V4L data bus properties are specified a both the receiver and the transmitter endpoint nodes separately.

I guess there are different kinds of properties. Something like a regulator is obviously property of the panel. But anything related to the video itself, like DPI's bus width, or perhaps even something like "orientation" if the panel supports such, could need to be in the endpoint node.

If we use port/endpoint nodes it all seems clear, the video bus properties are put in an endpoint node.

But since we are considering a simplified binding all the properties would be placed in the panel or display controller node.

But yes, I understand what you mean. With "common-video-property" I meant common properties like DPI bus width.

...

...

If that can be supported in the SW by adding complexity to a few functions, and it covers practically all the panels, isn't it worth it?

Note that I have not tried this, so I don't know if there are issues. It's just a thought. Even if there's need for a endpoint node, perhaps the port node can be made optional.

It can be worth it, as long as we make sure that simplified bindings cover the needs of the generic code.

We could assume that, if the port subnode isn't present, the device will have a single port, with a single endpoint. However, isn't the number of endpoints

Right.

...

a system property rather than a device property ? If a port of a device is

Yes.

...

connected to two remote ports it will require two endpoints. We could select the simplified or full bindings based on the system topology though.

Yes, I guess it's all about the system topology. Any simplified binding would work only for very simple configuration like single-output LCD controller with single panel attached to it.

The drivers should not know about simplified/normal bindings. They should use CDF DT helper functions to get the port and endpoint information. The helper functions would do the assuming.

Yes, anyway all the parsing is supposed to be done within the helpers.

-- Thanks, Sylwester

On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:

On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:

...

On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:

[...]

...

CDF needs to model video pipelines that can be more complex than just a display controller with a single output connected to a panel with a single input. For that reason the DT bindings need to describe connections in a generic enough way. The above DT fragment might be slightly more complex than one would expect when thinking about the really simple case only, but it's nowhere near overly complex in my opinion.

Please note that, when a device has as single port, the ports node can be omitted, and the port doesn't need to be numbered. You would then end up with> dc: display-controller { port { remote-endpoint = <&panel>; }; };

panel: panel { port { remote-endpoint = <&dc>; }; };

I don't think there's a way to simplify it further.

That binding looks completely inconsistent to me. If we can write it either using a ports node with port@X subnodes, in my opinion we can equally well just use a unidirectional link for the case where we don't need the link back from the panel to the output. Where's the difference?

Consider for example the case where an LVDS panel is connected to a simple LVDS output. For what possible reason would the panel need to access the output?

The point is that the video pipeline must be described in DT. Having a per- device way to represent connections would be a nightmare to support from an implementation point of view, hence the need for a generic way to describe them.

...

...

...

I also plan to specify video bus parameters in DT for CDF, but this hasn't been finalized yet.

That effectively blocks any of the code that I've been working on until CDF has been merged. It's been discussed for over a year now, and there has even been significant pushback on using CDF in DRM, so even if CDF is eventually merged, that will still leave DRM with no way to turn on a simple panel.

I'm probably even more concerned about the long time all this took and will still take than you are, sincerely. I'm obviously to blame for not being able to explain the problems and solutions correctly, as I see no way why people would push back if they realized that the other solutions that have been proposed, far from being future-proof, won't even be present-proof. We would be shooting ourselves in the foot, but what can I do when too many people want to do so ?

I'm curious why you think this current proposal isn't present-proof. All three panels that I've tested with work with this proposal currently and I'm convinced that many more setups can be covered using something as simple as this.

My apologies for not being clear enough. I wasn't implying that your proposal isn't present-proof, I was talking about the counter-proposals that have been made in reaction to CDF.

...

I'll still keep trying of course, but not for years.I have yet to see someone proposing a viable solution to share drivers between DRM and V4L2, which is a real pressing requirement, partly due to DT.

Yeah... I still don't get that side of the argument. Why exactly do we need to share the drivers again?

Think about a scaler IP in a SoC or FPGA, with one instance used in a camera capture pipeline, supported by a V4L2 driver, and one instance used in a video output pipeline, supported by a DRM driver. We want a single driver for that IP core. Same story for video encoders.

If we really need to support display output within V4L2, shouldn't we instead improve interoperability between the two and use DRM/KMS exclusively for the output part? Then the problem of having to share drivers between subsystems goes away and we'll actually be using the right subsystem for the right job at hand.

In the long term we definitely should improve interoperability between the two. I'd go further than that, having one API for video capture and one API for video output doesn't make much sense, except for historical reasons.

Of course none of that is relevant to the topic of DT bindings, which is what we should probably be concentrating on.

It actually is. Given that DT bindings must describe hardware, the scaler (or encoder, or other entity) would use the same bindings regardless of the Linux subsystem that will be involved. We thus need to make sure the bindings will be usable on both sides.

Nobody's been able to come up with a valid reason why the proposed binding is broken. It works for the tested devices (likely many more as well) and I don't forsee why it can't be future-proof. Why would it suddenly need to change? And even if it needed to change, why would the changes be backwards- incompatible rather than simply additional features that don't impact existing users?

...

Regarding the use of CDF in DRM, please note that it will be optional for drivers to switch to the new framework. Nothing should hopefully require existing drivers to be converted, drivers that want to make use of CDF to support panels will be welcome to do so, but they big DRM drivers for desktop GPUs won't be affected.

I'm not at all opposed to the idea of CDF or the problems it tries to address. I don't think it necessarily should be a separate framework for reasons explained earlier. But even if it were to end up in a separate framework there's absolutely nothing preventing us from adding a DRM panel driver that hooks into CDF as a "backend" of sorts for panels that require something more complex than the simple-panel binding.

Once again it's not about panel having complex needs, but more about using simple panels in complex pipelines. I'm fine with the drm_panel infrastructure, what I would like is DT bindings that describe connections in a more future-proof way. The rest is fine.

...

...

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>; };

Note how the above is also not incompatible with anything that the CDF (to be) mandates, so it could easily be extended with any nodes or properties that CDF needs to work properly without breaking backwards- compatibility.

As mentioned above, CDF needs to describe more complex pipelines. To make the driver life as simple as possible the core code will handle the pipeline description in the DT bindings, and it does require it to be generic enough. You can of course describe connections in various ways depending on the hardware, which would make the DT-related code needlessly overcomplex. One of the issue with the above description is that there's no way to go from the panel to the display controller, which might be a show stopper in the future when code requires that. A workaround would be to look through the whole DT for the reverse link, which would be pretty inefficient.

But that's precisely the point. Why would we need to go back from the panel to the display controller? Especially for dumb panels that can't or don't have to be configured in any way. Even if they needed some sort of setup, why can't that be done from the display controller/output.

Even given a setup where a DSI controller needs to write some registers in a panel upon initialization, I don't see why the reverse connection needs to be described. The fact alone that an output dereferences a panel's phandle should be enough to connect both of them and have any panel driver use the DSI controller that it's been attached to for the programming.

That's very much analogous to how I2C works. There are no connections back to the I2C master from the slaves. Yet each I2C client driver manages to use the services provided by the I2C master to perform transactions on the I2C bus. In a similar way the DSI controller is the bus master that talks to DSI panels. DSI panels don't actively talk to the DSI controller.

It's all about modeling video pipeline graphs in DT. To be able to walk the graph we need to describe connections. Not having bidirectional information means that we restrict the points at which we can start walking the graph, potentially making our life much more difficult in the future.

What's so wrong about adding a port node and link information to the panel DT node ? It describe what's there: the panel has one input, why not make that explicit ?

...

...

...

...

+static void panel_simple_enable(struct drm_panel *panel) +{

• struct panel_simple *p = to_panel_simple(panel);
• int err;
• if (p->enabled)

• return;
  

• err = regulator_enable(p->supply);
• if (err < 0)

• dev_err(panel->dev, "failed to enable supply: %d\n", err);
  

Is that really a non-fatal error ? Shouldn't the enable operation return an int ?

There's no way to propagate this in DRM, so why go through the trouble of returning the error? Furthermore, there's nothing that the caller could do to remedy the situation anyway.

That's a DRM issue, which could be fixed. While the caller can't remedy the situation, it should at least report the error to the application instead of silently ignoring it.

Perhaps. It's not really relevant to the discussion and can always be fixed in a subsequent patch.

Most things can be fixed by a subsequent patch, that's not a good enough reason not to address the known problems before pushing the code to mainline (to clarify my point of view, there's no need to fix DRM to use the return value before pushing this patch set to mainline, but I'd like a v2 with an int return value).

...

...

...

...

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

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.

I share Tomi's point of view here. Your three panels use the same power sequence, so I believe we should have a generic panel compatible string that would use modes described in DT for the common case. Only if a panel required something more complex which can't (or which could, but won't for any reason) accurately be described in DT would you need to extend the driver.

I don't see the point quite frankly. You seem to be assuming that every panel will only be used in a single board.

No, but in practice that's often the case, at least for boards with .dts files in the mainline kernel.

However what you're proposing will require the mode timings to be repeated in every board's DT file, if the same panel is ever used on more than a single board.

Is that a problem ? You could #include a .dtsi for the panel that would specify the video mode if you want to avoid multiple copies.

Hi Thierry,

On Thursday 17 October 2013 14:46:20 Thierry Reding wrote:

On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote:

...

On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:

...

On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:

...

On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:

[snip]

...

The point is that the video pipeline must be described in DT. Having a per-device way to represent connections would be a nightmare to support from an implementation point of view, hence the need for a generic way to describe them.

Okay, so we're back to the need to describe pipelines in DT. At the risk of sounding selfish: I don't care about pipelines. I just want us to settle on a way to represent a dumb panel in DT so that it can be enabled when it needs to. Furthermore I don't have any hardware that exhibits any of these "advanced" features, so I'm totally unqualified to work on any of this.

Can we please try to be a little pragmatic here and solve one problem at a time? I am aware that solving this for panels may require some amount of foresight, but let's not try to solve everything at once at the risk of not getting anything done at all.

I won't force you to care about pipelines, and I don't think this is selfish at all :-)

What I would like to ensure is that whatever bindings we come up with, they will not preclude us from adding pipeline support when needed (as I'm pretty sure it will be needed at some point).

...

...

...

I'll still keep trying of course, but not for years.I have yet to see someone proposing a viable solution to share drivers between DRM and V4L2, which is a real pressing requirement, partly due to DT.

Yeah... I still don't get that side of the argument. Why exactly do we need to share the drivers again?

Think about a scaler IP in a SoC or FPGA, with one instance used in a camera capture pipeline, supported by a V4L2 driver, and one instance used in a video output pipeline, supported by a DRM driver. We want a single driver for that IP core. Same story for video encoders.

Yes, I see. This doesn't immediately concern the simple panel problem that I'm trying to solve, so perhaps we can have a separate discussion about it. Perhaps this would more easily be solved by providing some sort of helper library for the lower level control of the device and wrapping that with V4L2 or DRM APIs.

That's more or less the idea of CDF ;-)

...

...

If we really need to support display output within V4L2, shouldn't we instead improve interoperability between the two and use DRM/KMS exclusively for the output part? Then the problem of having to share drivers between subsystems goes away and we'll actually be using the right subsystem for the right job at hand.

In the long term we definitely should improve interoperability between the two. I'd go further than that, having one API for video capture and one API for video output doesn't make much sense, except for historical reasons.

Having two separate subsystems has worked out pretty well so far. In my opinion having strong boundaries between subsystems helps with design.

I'm not saying we've made a mistake. A unified subsystem wouldn't have got it right in the first place either anyway. My point is that the boundary has collapsed on the hardware side, so I believe the software side will need to follow somehow.

...

...

Of course none of that is relevant to the topic of DT bindings, which is what we should probably be concentrating on.

It actually is. Given that DT bindings must describe hardware, the scaler (or encoder, or other entity) would use the same bindings regardless of the Linux subsystem that will be involved. We thus need to make sure the bindings will be usable on both sides.

Perhaps a single driver could expose both interfaces?

That's one idea that has been proposed. I believe it would be simpler to standardize a common API and have translation layers specific to V4L and KMS in the core, instead of pushing that translation to all panel/bridges/encoders/whatever drivers.

...

...

I'm not at all opposed to the idea of CDF or the problems it tries to address. I don't think it necessarily should be a separate framework for reasons explained earlier. But even if it were to end up in a separate framework there's absolutely nothing preventing us from adding a DRM panel driver that hooks into CDF as a "backend" of sorts for panels that require something more complex than the simple-panel binding.

Once again it's not about panel having complex needs, but more about using simple panels in complex pipelines. I'm fine with the drm_panel infrastructure, what I would like is DT bindings that describe connections in a more future-proof way. The rest is fine.

And I've already said elsewhere that the bindings in their current form are easily extensible to cater for the needs of CDF.

The simple panel bindings do not include any connection information, so we could add that later when needed without having to deprecate, remove or repurpose existing properties. The simple panel driver would need to be extended, which isn't much of a problem (except that extending it with CDF support might require changes to the users of the simple panel driver, which I believe won't be happily accepted, but that's a different issue).

My concern is also on the other side. In the patches you've sent the tegra driver uses a custom nvidia,panel property to reference the panel. That would of course not be CDF-compatible, but there's no way around that at the moment if we don't want to keep development of all ARM KMS drivers stalled for the next 6 months. It boils down to the question of whether DT should be a stable ABI, and I'm increasingly tempted to say that I don't care. I want to solve issues we have on the display side, the firmware interface isn't my main concern.

...

...

But that's precisely the point. Why would we need to go back from the panel to the display controller? Especially for dumb panels that can't or don't have to be configured in any way. Even if they needed some sort of setup, why can't that be done from the display controller/output.

Even given a setup where a DSI controller needs to write some registers in a panel upon initialization, I don't see why the reverse connection needs to be described. The fact alone that an output dereferences a panel's phandle should be enough to connect both of them and have any panel driver use the DSI controller that it's been attached to for the programming.

That's very much analogous to how I2C works. There are no connections back to the I2C master from the slaves. Yet each I2C client driver manages to use the services provided by the I2C master to perform transactions on the I2C bus. In a similar way the DSI controller is the bus master that talks to DSI panels. DSI panels don't actively talk to the DSI controller.

It's all about modeling video pipeline graphs in DT. To be able to walk the graph we need to describe connections. Not having bidirectional information means that we restrict the points at which we can start walking the graph, potentially making our life much more difficult in the future.

What's so wrong about adding a port node and link information to the panel DT node ? It describe what's there: the panel has one input, why not make that explicit ?

What's wrong with it is that there's no way to verify the soundness of the design by means of a full implementation because we're missing the majority of the pieces. Just putting the nodes into DT to provide some imaginary future-proofness isn't helpful either. Without any code that actually uses them they are useless.

And again, why should we add them right away (while not needed) when they can easily be added in a backwards-compatible manner at some later point when there's actually any use for them and they can actually be tested in some larger framework?

It's the "easily" part I'm not sure about. I doubt we'll ever have any easy to solve DT backward compatibility issue. However, as mentioned above, this shouldn't be a show stopper. I'm thus fine with the way the proposed bindings describe (or rather don't describe) the connection. However, I will then expect your support in the future to implement the "easy" extension of the bindings to support CDF. De we have a deal ? ;-)

...

...

...

...

...

> +static void panel_simple_enable(struct drm_panel *panel) > +{ > + struct panel_simple *p = to_panel_simple(panel); > + int err; > + > + if (p->enabled) > + return; > + > + err = regulator_enable(p->supply); > + if (err < 0) > + dev_err(panel->dev, "failed to enable supply: %d\n",

err);

...

...

...

...

...

Is that really a non-fatal error ? Shouldn't the enable operation return an int ?

There's no way to propagate this in DRM, so why go through the trouble of returning the error? Furthermore, there's nothing that the caller could do to remedy the situation anyway.

That's a DRM issue, which could be fixed. While the caller can't remedy the situation, it should at least report the error to the application instead of silently ignoring it.

Perhaps. It's not really relevant to the discussion and can always be fixed in a subsequent patch.

Most things can be fixed by a subsequent patch, that's not a good enough reason not to address the known problems before pushing the code to mainline (to clarify my point of view, there's no need to fix DRM to use the return value before pushing this patch set to mainline, but I'd like a v2 with an int return value).

Why? What's the use of returning an error if you know up front that it can't be used? This should be changed if or when we "fix" DRM to propagate errors.

Because not doing so now will require us to change (potentially) lots of panel drivers at that time. It's much easier to have each panel driver developer implement the required code in his/her driver than having a single developer refactoring the code later and have to touch all drivers. If your concern is that the error paths won't be testable at the moment, you could easily already add a WARN_ON() to the caller to catch problems.

...

...

...

...

...

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.

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.

I share Tomi's point of view here. Your three panels use the same power sequence, so I believe we should have a generic panel compatible string that would use modes described in DT for the common case. Only if a panel required something more complex which can't (or which could, but won't for any reason) accurately be described in DT would you need to extend the driver.

I don't see the point quite frankly. You seem to be assuming that every panel will only be used in a single board.

No, but in practice that's often the case, at least for boards with .dts files in the mainline kernel.

...

However what you're proposing will require the mode timings to be repeated in every board's DT file, if the same panel is ever used on more than a single board.

Is that a problem ? You could #include a .dtsi for the panel that would specify the video mode if you want to avoid multiple copies.

Yes, I don't think it's desirable to duplicate data needlessly in DT. It also makes the binding more difficult to use. If I know that the panel is one supported by the simple-panel binding, I can just go and add the right compatible value without having to bother looking up the mode timings and duplicating them. That way DT writers get to concern themselves only with the variable data.

I've had a quick chat with Dave Airlie and Hans de Goede yesterday about this. As most panels will use standard timings, Hans proposed adding a timings property that would reference the standard timings the panel uses (this could be a string or integer defined in include/dt-bindings/...). In most case DT would just have a single property to select the timings, and only in more complex cases where the system designer wants to use custom timings would the timings need to be manually defined.

Hi Thierry,

On Thursday 24 October 2013 13:48:24 Thierry Reding wrote:

On Thu, Oct 24, 2013 at 01:05:49PM +0200, Laurent Pinchart wrote:

...

On Thursday 17 October 2013 14:46:20 Thierry Reding wrote:

...

On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote:

...

On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:

...

On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:

...

On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:

[snip]

...

...

The point is that the video pipeline must be described in DT. Having a per-device way to represent connections would be a nightmare to support from an implementation point of view, hence the need for a generic way to describe them.

Okay, so we're back to the need to describe pipelines in DT. At the risk of sounding selfish: I don't care about pipelines. I just want us to settle on a way to represent a dumb panel in DT so that it can be enabled when it needs to. Furthermore I don't have any hardware that exhibits any of these "advanced" features, so I'm totally unqualified to work on any of this.

Can we please try to be a little pragmatic here and solve one problem at a time? I am aware that solving this for panels may require some amount of foresight, but let's not try to solve everything at once at the risk of not getting anything done at all.

I won't force you to care about pipelines, and I don't think this is selfish at all :-)

What I would like to ensure is that whatever bindings we come up with, they will not preclude us from adding pipeline support when needed (as I'm pretty sure it will be needed at some point).

Sure. I think I've explained elsewhere that I consider this easily possible. Other people have said the same thing. If all we do is add properties to a binding, then drivers written for an old binding will have no problem continuing to work.

I also think that we're mostly being too scared. If ever the next device comes around that requires explicit pipeline support to work, how likely is it to have the exact same display as prior devices?

The exact same panel, probably low, but a panel supported by the same driver using the same DT bindings (such as the generic simple panel driver), quite high.

...

...

...

...

I'm not at all opposed to the idea of CDF or the problems it tries to address. I don't think it necessarily should be a separate framework for reasons explained earlier. But even if it were to end up in a separate framework there's absolutely nothing preventing us from adding a DRM panel driver that hooks into CDF as a "backend" of sorts for panels that require something more complex than the simple-panel binding.

Once again it's not about panel having complex needs, but more about using simple panels in complex pipelines. I'm fine with the drm_panel infrastructure, what I would like is DT bindings that describe connections in a more future-proof way. The rest is fine.

And I've already said elsewhere that the bindings in their current form are easily extensible to cater for the needs of CDF.

The simple panel bindings do not include any connection information, so we could add that later when needed without having to deprecate, remove or repurpose existing properties.

Yes, I agree.

...

The simple panel driver would need to be extended, which isn't much of a problem (except that extending it with CDF support might require changes to the users of the simple panel driver, which I believe won't be happily accepted, but that's a different issue).

That might be true. But that's just the way kernel development works. We sometimes require major rework of APIs and we're actually pretty good at pulling that off, so I don't worry too much about it.

Also, being the original author of the driver makes me the maintainer, and if it should prove to be necessary I'm absolutely willing to add CDF support.

Much appreciated :-)

...

My concern is also on the other side. In the patches you've sent the tegra driver uses a custom nvidia,panel property to reference the panel. That would of course not be CDF-compatible, but there's no way around that at the moment if we don't want to keep development of all ARM KMS drivers stalled for the next 6 months.

Well, the nvidia,panel property is part of the display output and it is an optional property. So the driver already needs to cope with it being absent. If the display output driver is ever extended with CDF support then we can just go and use CDF if CDF-specific properties exist, or we fall back to nvidia,panel if that doesn't exist.

...

It boils down to the question of whether DT should be a stable ABI, and I'm increasingly tempted to say that I don't care. I want to solve issues we have on the display side, the firmware interface isn't my main concern.

I wholeheartedly agree. In my opinion it is much more useful to come up with a solution that works now, rather than discussing and arguing things to death and not get anything done. If that means that I'll have to maintain additional code, then so be it.

...

...

...

...

But that's precisely the point. Why would we need to go back from the panel to the display controller? Especially for dumb panels that can't or don't have to be configured in any way. Even if they needed some sort of setup, why can't that be done from the display controller/output.

Even given a setup where a DSI controller needs to write some registers in a panel upon initialization, I don't see why the reverse connection needs to be described. The fact alone that an output dereferences a panel's phandle should be enough to connect both of them and have any panel driver use the DSI controller that it's been attached to for the programming.

That's very much analogous to how I2C works. There are no connections back to the I2C master from the slaves. Yet each I2C client driver manages to use the services provided by the I2C master to perform transactions on the I2C bus. In a similar way the DSI controller is the bus master that talks to DSI panels. DSI panels don't actively talk to the DSI controller.

It's all about modeling video pipeline graphs in DT. To be able to walk the graph we need to describe connections. Not having bidirectional information means that we restrict the points at which we can start walking the graph, potentially making our life much more difficult in the future.

What's so wrong about adding a port node and link information to the panel DT node ? It describe what's there: the panel has one input, why not make that explicit ?

What's wrong with it is that there's no way to verify the soundness of the design by means of a full implementation because we're missing the majority of the pieces. Just putting the nodes into DT to provide some imaginary future-proofness isn't helpful either. Without any code that actually uses them they are useless.

And again, why should we add them right away (while not needed) when they can easily be added in a backwards-compatible manner at some later point when there's actually any use for them and they can actually be tested in some larger framework?

It's the "easily" part I'm not sure about. I doubt we'll ever have any easy to solve DT backward compatibility issue. However, as mentioned above, this shouldn't be a show stopper. I'm thus fine with the way the proposed bindings describe (or rather don't describe) the connection. However, I will then expect your support in the future to implement the "easy" extension of the bindings to support CDF.

Again, I don't expect that to ever happen. But if it ever happens anyway, then...

...

Do we have a deal ? ;-)

Yes, we do.

Great!

...

...

...

...

...

> > > +static void panel_simple_enable(struct drm_panel *panel) > > > +{ > > > + struct panel_simple *p = to_panel_simple(panel); > > > + int err; > > > + > > > + if (p->enabled) > > > + return; > > > + > > > + err = regulator_enable(p->supply); > > > + if (err < 0) > > > + dev_err(panel->dev, "failed to enable supply:

%d\n",

...

err);

...

...

...

...

> > Is that really a non-fatal error ? Shouldn't the enable > > operation return an int ? > > There's no way to propagate this in DRM, so why go through the > trouble of returning the error? Furthermore, there's nothing > that the caller could do to remedy the situation anyway.

That's a DRM issue, which could be fixed. While the caller can't remedy the situation, it should at least report the error to the application instead of silently ignoring it.

Perhaps. It's not really relevant to the discussion and can always be fixed in a subsequent patch.

Most things can be fixed by a subsequent patch, that's not a good enough reason not to address the known problems before pushing the code to mainline (to clarify my point of view, there's no need to fix DRM to use the return value before pushing this patch set to mainline, but I'd like a v2 with an int return value).

Why? What's the use of returning an error if you know up front that it can't be used? This should be changed if or when we "fix" DRM to propagate errors.

Because not doing so now will require us to change (potentially) lots of panel drivers at that time. It's much easier to have each panel driver developer implement the required code in his/her driver than having a single developer refactoring the code later and have to touch all drivers. If your concern is that the error paths won't be testable at the moment, you could easily already add a WARN_ON() to the caller to catch problems.

I don't mind fixing potentially many drivers. The conversion is pretty mechanical and therefore easy. We even have tools such as coccinelle to help with it. But we've probably spent more time arguing the point than it would take to make this simple change, so I'll just go and change the return value to an int and return an error.

Thank you.

Perhaps if I'm in the mood I'll even write up a patch to propagate the error further.

...

...

...

...

...

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

I share Tomi's point of view here. Your three panels use the same power sequence, so I believe we should have a generic panel compatible string that would use modes described in DT for the common case. Only if a panel required something more complex which can't (or which could, but won't for any reason) accurately be described in DT would you need to extend the driver.

I don't see the point quite frankly. You seem to be assuming that every panel will only be used in a single board.

No, but in practice that's often the case, at least for boards with .dts files in the mainline kernel.

...

However what you're proposing will require the mode timings to be repeated in every board's DT file, if the same panel is ever used on more than a single board.

Is that a problem ? You could #include a .dtsi for the panel that would specify the video mode if you want to avoid multiple copies.

Yes, I don't think it's desirable to duplicate data needlessly in DT. It also makes the binding more difficult to use. If I know that the panel is one supported by the simple-panel binding, I can just go and add the right compatible value without having to bother looking up the mode timings and duplicating them. That way DT writers get to concern themselves only with the variable data.

I've had a quick chat with Dave Airlie and Hans de Goede yesterday about this. As most panels will use standard timings, Hans proposed adding a timings property that would reference the standard timings the panel uses (this could be a string or integer defined in include/dt-bindings/...). In most case DT would just have a single property to select the timings, and only in more complex cases where the system designer wants to use custom timings would the timings need to be manually defined.

We can do the same thing within the kernel. We already have a list of standard EDID/HDMI/CEA display modes, so we could similarly add a new list of common display panel modes and make each driver reference that instead of defining a custom one.

Sure. My point is that I would like to avoid adding zillions of compatible properties to the driver, when we could use a single property in the DT bindings that would specify the timings instead. This would lower the amount of changes made to the simple panel driver, while keeping DT simple enough (at least in my opinion).

And that still enables us to add a property that would allow DT writers to override the display mode if they need to.