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

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

[Thierry Reding]

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

Adding the dri-devel mailing list on Cc, since this discussion is
outgrowing its original intent.

On Thu, Oct 17, 2013 at 02:47:52AM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> Thank you for the patch.
> 
> On Wednesday 16 October 2013 20:25:12 Thierry Reding wrote:
> > Add a driver for simple panels. Such panels can have a regulator that
> > provides the supply voltage and a separate GPIO to enable the panel.
> > Optionally the panels can have a backlight associated with them so it
> > can be enabled or disabled according to the panel's power management
> > mode.
> > 
> > Support is added for three panels: An AU Optronics 10.1" WSVGA, a
> > Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD
> > panel.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../devicetree/bindings/panel/auo,b101aw03.txt     |   7 +
> >  .../bindings/panel/chunghwa,claa101wb03.txt        |   7 +
> >  .../bindings/panel/panasonic,vvx10f004b00.txt      |   7 +
> >  .../devicetree/bindings/panel/simple-panel.txt     |  21 ++
> >  drivers/gpu/drm/Makefile                           |   1 +
> >  drivers/gpu/drm/panel/Kconfig                      |  13 +
> >  drivers/gpu/drm/panel/Makefile                     |   1 +
> >  drivers/gpu/drm/panel/panel-simple.c               | 335 ++++++++++++++++++
> >  8 files changed, 392 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> > create mode 100644
> > Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt create
> > mode 100644
> > Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt create
> > mode 100644 Documentation/devicetree/bindings/panel/simple-panel.txt create
> > mode 100644 drivers/gpu/drm/panel/Makefile
> >  create mode 100644 drivers/gpu/drm/panel/panel-simple.c
> > 
> > diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> > b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt new file mode
> > 100644
> > index 0000000..72e088a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> > @@ -0,0 +1,7 @@
> > +AU Optronics Corporation 10.1" WSVGA TFT LCD panel
> > +
> > +Required properties:
> > +- compatible: should be "auo,b101aw03"
> > +
> > +This binding is compatible with the simple-panel binding, which is
> > specified +in simple-panel.txt in this directory.
> > diff --git
> > a/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt
> > b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt new file
> > mode 100644
> > index 0000000..0ab2c05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt
> > @@ -0,0 +1,7 @@
> > +Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel
> > +
> > +Required properties:
> > +- compatible: should be "chunghwa,claa101wb03"
> > +
> > +This binding is compatible with the simple-panel binding, which is
> > specified +in simple-panel.txt in this directory.
> > diff --git
> > a/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt
> > b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt new
> > file mode 100644
> > index 0000000..d328b03
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt
> > @@ -0,0 +1,7 @@
> > +Panasonic Corporation 10.1" WUXGA TFT LCD panel
> > +
> > +Required properties:
> > +- compatible: should be "panasonic,vvx10f004b00"
> > +
> > +This binding is compatible with the simple-panel binding, which is
> > specified +in simple-panel.txt in this directory.
> > diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt
> > b/Documentation/devicetree/bindings/panel/simple-panel.txt new file mode
> > 100644
> > index 0000000..1341bbf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt
> > @@ -0,0 +1,21 @@
> > +Simple display panel
> > +
> > +Required properties:
> > +- power-supply: regulator to provide the supply voltage
> > +
> > +Optional properties:
> > +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> > +- enable-gpios: GPIO pin to enable or disable the panel
> > +- backlight: phandle of the backlight device attached to the panel
> > +
> > +Example:
> > +
> > +	panel: panel {
> > +		compatible = "cptt,claa101wb01";
> > +		ddc-i2c-bus = <&panelddc>;
> > +
> > +		power-supply = <&vdd_pnl_reg>;
> > +		enable-gpios = <&gpio 90 0>;
> > +
> > +		backlight = <&backlight>;
> > +	};
> 
> My biggest concern here is that this will not be compatible with the CDF DT 
> bindings. They're not complete yet, but they will require connections between 
> entities to be described in DT, in a way very similar (or actually identical) 
> to the V4L2 DT bindings, documented in 
> Documentation/devicetree/bindings/media/video-interfaces.txt. Could you have a 
> look at that ? Please ignore all optional properties beside remote-endpoint, 
> as they're V4L2 specific.

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? That seems like an overly complex amount of data
to describe just a simple panel where the only connection between it and
the display hardware is the data bus and I was sort of expecting the CDF
to provide some sort of shortcut for setups where the connection is 1:1
with no means to perform any configuration of the bus.

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

> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
[...]
> > +/*
> > + * Copyright (C) 2013, NVIDIA Corporation.  All rights reserved.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sub
> > license,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > + * next paragraph) shall be included in all copies or substantial portions
> > + * of the Software.
> 
> This contradicts MODULE_LICENSE("GPL v2") below.

True, I'll fix that up.

> > +/* TODO: convert to gpiod_*() API once it's been merged */
> > +#define GPIO_ACTIVE_LOW	(1 << 0)
> 
> Why can't you just #include <dt-bindings/gpio/gpio.h> ?

This is supposed to be something completely driver internal. Keeping it
here make that more explicit. It shouldn't matter much anyway, since by
now these patches have about 0 chance of making it into 3.13, so I'll
just convert them to gpiod.

> > +		backlight_update_status(p->backlight);
> > +	}
> > +
> > +	if (gpio_is_valid(p->enable_gpio)) {
> > +		if (p->enable_gpio_flags & GPIO_ACTIVE_LOW)
> > +			gpio_set_value(p->enable_gpio, 1);
> > +		else
> > +			gpio_set_value(p->enable_gpio, 0);
> > +	}
> > +
> > +	regulator_disable(p->supply);
> > +	p->enabled = false;
> > +}
> > +
> > +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.

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

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Tomi Valkeinen]

[-- 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 --]

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

[Thierry Reding]

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

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Laurent Pinchart]

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

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.

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

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

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Thierry Reding]

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

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Laurent Pinchart]

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

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.

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

Then I might not have understood your answer :-)

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Thierry Reding]

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

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Tomi Valkeinen]

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

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.

 Tomi



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

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

[Thierry Reding]

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

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Laurent Pinchart]

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

Hi Thierry,

On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
> Adding the dri-devel mailing list on Cc, since this discussion is
> outgrowing its original intent.
> 
> On Thu, Oct 17, 2013 at 02:47:52AM +0200, Laurent Pinchart wrote:
> > Hi Thierry,
> > 
> > Thank you for the patch.
> > 
> > On Wednesday 16 October 2013 20:25:12 Thierry Reding wrote:
> > > Add a driver for simple panels. Such panels can have a regulator that
> > > provides the supply voltage and a separate GPIO to enable the panel.
> > > Optionally the panels can have a backlight associated with them so it
> > > can be enabled or disabled according to the panel's power management
> > > mode.
> > > 
> > > Support is added for three panels: An AU Optronics 10.1" WSVGA, a
> > > Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD
> > > panel.
> > > 
> > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > ---
> > > 
> > >  .../devicetree/bindings/panel/auo,b101aw03.txt     |   7 +
> > >  .../bindings/panel/chunghwa,claa101wb03.txt        |   7 +
> > >  .../bindings/panel/panasonic,vvx10f004b00.txt      |   7 +
> > >  .../devicetree/bindings/panel/simple-panel.txt     |  21 ++
> > >  drivers/gpu/drm/Makefile                           |   1 +
> > >  drivers/gpu/drm/panel/Kconfig                      |  13 +
> > >  drivers/gpu/drm/panel/Makefile                     |   1 +
> > >  drivers/gpu/drm/panel/panel-simple.c               | 335 ++++++++++++++
> > >  8 files changed, 392 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/panel/simple-panel.txt
> > >  create mode 100644 drivers/gpu/drm/panel/Makefile
> > >  create mode 100644 drivers/gpu/drm/panel/panel-simple.c

[snip]

> > > diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt
> > > b/Documentation/devicetree/bindings/panel/simple-panel.txt new file mode
> > > 100644
> > > index 0000000..1341bbf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt
> > > @@ -0,0 +1,21 @@
> > > +Simple display panel
> > > +
> > > +Required properties:
> > > +- power-supply: regulator to provide the supply voltage
> > > +
> > > +Optional properties:
> > > +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> > > +- enable-gpios: GPIO pin to enable or disable the panel
> > > +- backlight: phandle of the backlight device attached to the panel
> > > +
> > > +Example:
> > > +
> > > +	panel: panel {
> > > +		compatible = "cptt,claa101wb01";
> > > +		ddc-i2c-bus = <&panelddc>;
> > > +
> > > +		power-supply = <&vdd_pnl_reg>;
> > > +		enable-gpios = <&gpio 90 0>;
> > > +
> > > +		backlight = <&backlight>;
> > > +	};
> > 
> > My biggest concern here is that this will not be compatible with the CDF
> > DT bindings. They're not complete yet, but they will require connections
> > between entities to be described in DT, in a way very similar (or
> > actually identical) to the V4L2 DT bindings, documented in
> > Documentation/devicetree/bindings/media/video-interfaces.txt. Could you
> > have a look at that ? Please ignore all optional properties beside
> > remote-endpoint, as they're V4L2 specific.
> 
> 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.

> That seems like an overly complex amount of data to describe just a simple
> panel where the only connection between it and the display hardware is the
> data bus and I was sort of expecting the CDF to provide some sort of
> shortcut for setups where the connection is 1:1 with no means to perform any
> configuration of the bus.

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.

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

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

> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c

[...]

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

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

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Tomi Valkeinen]

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

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



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

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

[Laurent Pinchart]

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

Hi Tomi,

On Thursday 17 October 2013 14:35:49 Tomi Valkeinen wrote:
> 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.

My bad, you'r absolutely right. More sleep is needed.

(And while we're at it, the remote-endpoint properties must point to an 
endpoint, not the device DT node.

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

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

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Tomi Valkeinen]

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

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



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

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

[Laurent Pinchart]

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

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 ?

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Tomi Valkeinen]

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

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



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

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

[Sylwester Nawrocki]

On 10/17/2013 02:32 PM, 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>;
> 		};
> 	};
> };
>
> 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.

Each endpoint node was originally supposed to contain the port configuration
properties specific to a single remote device. So there would be more than
one endpoint sub-mode of a port node only if, e.g. a display controller 
could
output data through a single port to more than one panel (not necessarily
simultaneously).

If it is known in the display subsystems there is no use case where single
data output (port) is being switched between (connected to) multiple data
sinks we could probably omit the endpoint nodes in order to simplify the
bindings a bit.

We could have something like:

dispc {
	disp_port: port {
		remote-port = <&p_port>;
		common-video-property = <>;
	};
};

panel {
	p_port: port {
		remote-port = <&disp_port>;
		common-video-property = <>;
	};
};

Alternatively we could say, that for only one endpoint node needed such an
endpoint node can be omitted and the common video properties can be 
specified
directly in the port node.

This would require introducing 'remote-port' phandle property, as
remote-endpoint is supposed to point to an endpoint, not a port node.

Regarding the reverse link phandle, I was wondering if it shouldn't be made
optional. We would have to think twice about such a change though, since
making an optional property back mandatory wouldn't be possible.

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

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

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.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Tomi Valkeinen]

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

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

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?

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.

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.

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.

 Tomi



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

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

[Sylwester Nawrocki]

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

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

[Thierry Reding]

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

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?

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

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

Of course none of that is relevant to the topic of DT bindings, which is
what we should probably be concentrating on. 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.

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

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

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

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Laurent Pinchart]

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

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.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Thierry Reding]

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

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:
> > [...]
> > 
> > > 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.

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

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

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

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

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

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

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

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Laurent Pinchart]

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

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.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Thierry Reding]

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

On Thu, Oct 24, 2013 at 01:05:49PM +0200, Laurent Pinchart wrote:
> 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).

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?

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

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

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

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.

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

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Laurent Pinchart]

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

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.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]