[PATCH] of: Add simple panel device tree binding

[PATCH] of: Add simple panel device tree binding

[Thierry Reding]

This binding specifies a set of common properties for display panels. It
can be used as a basis by bindings for specific panels.

Bindings for three specific panels are provided to show how the simple
panel binding can be used.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
This binding has already been discussed earlier. Both Laurent and Tomi
seemed to be generally fine with this. The fact that the binding targets
simple (dumb) panels only and the reduced set of properties make it easy
to be extended in backwards-compatible ways should the need arise, while
at the same time allowing to support a wide variety of panels out there.

 .../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 +++++++++++++++++++++
 4 files changed, 42 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

diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt
new file mode 100644
index 000000000000..72e088a4fb3a
--- /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 000000000000..0ab2c05a4c22
--- /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 000000000000..d328b0341bf4
--- /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 000000000000..1341bbf4aa3d
--- /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>;
+	};
-- 
1.8.4.2

Re: [PATCH] of: Add simple panel device tree binding

[Laurent Pinchart]

Hi Thierry,

Thank you for the patch.

On Friday 22 November 2013 19:41:54 Thierry Reding wrote:
> This binding specifies a set of common properties for display panels. It
> can be used as a basis by bindings for specific panels.
> 
> Bindings for three specific panels are provided to show how the simple
> panel binding can be used.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> This binding has already been discussed earlier. Both Laurent and Tomi
> seemed to be generally fine with this.

That's correct, I'm generally fine with your approach, but there's still one 
point I'd like to see addressed.

As I mentioned previously, 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).

Specifying the full timings in every DT source file would be pretty verbose 
and could be error-prone. However, using a single property to specify one of 
the standard display timings, as orinally proposed by Hans de Goede (CC'ed) 
during a chat at the kernel summit would in my opinion be a good middle-ground 
solution.

Would you consider adding such a property to the simple panel bindings, and 
define a common compatible string ? Each panel should of course list its own 
compatibility string in addition to the common compatible string in case the 
need for extensions arises in the future.

> The fact that the binding targets simple (dumb) panels only and the reduced
> set of properties make it easy to be extended in backwards-compatible ways
> should the need arise, while at the same time allowing to support a wide
> variety of panels out there.
> 
>  .../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 ++++++++++++++++++
>  4 files changed, 42 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
> 
> diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt new file mode
> 100644
> index 000000000000..72e088a4fb3a
> --- /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 000000000000..0ab2c05a4c22
> --- /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 000000000000..d328b0341bf4
> --- /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 000000000000..1341bbf4aa3d
> --- /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>;
> +	};
-- 
Regards,

Laurent Pinchart

Re: [PATCH] of: Add simple panel device tree binding

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2560 bytes --]

On Tue, Nov 26, 2013 at 02:54:41AM +0100, Laurent Pinchart wrote:
> Hi Thierry,
> 
> Thank you for the patch.
> 
> On Friday 22 November 2013 19:41:54 Thierry Reding wrote:
> > This binding specifies a set of common properties for display panels. It
> > can be used as a basis by bindings for specific panels.
> > 
> > Bindings for three specific panels are provided to show how the simple
> > panel binding can be used.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > This binding has already been discussed earlier. Both Laurent and Tomi
> > seemed to be generally fine with this.
> 
> That's correct, I'm generally fine with your approach, but there's still one 
> point I'd like to see addressed.
> 
> As I mentioned previously, 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).
> 
> Specifying the full timings in every DT source file would be pretty verbose 
> and could be error-prone. However, using a single property to specify one of 
> the standard display timings, as orinally proposed by Hans de Goede (CC'ed) 
> during a chat at the kernel summit would in my opinion be a good middle-ground 
> solution.
> 
> Would you consider adding such a property to the simple panel bindings, and 
> define a common compatible string ? Each panel should of course list its own 
> compatibility string in addition to the common compatible string in case the 
> need for extensions arises in the future.

My gripe with this is that it duplicates information. By definition a
simple panel supports a limited number of display modes (typically only
a single one), so once you know the compatible value (which needs to be
there anyway) you can derive the mode from it. Adding a property that
specifies the display mode is redundant.

Dave Airlie proposed something else, namely to keep a list of common
modes within the driver and have each device reference that mode
instead. I think that could work similarily well. It still requires the
driver to be touched for each new panel, but the changes will be very
small. They'll be more of the "add a new table entry" sort of change
that we have in drivers like the 8250/16550-compatible serial. Most of
that could probably be wrapped into a macro to make it concise.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of: Add simple panel device tree binding

[Laurent Pinchart]

[-- Attachment #1.1: Type: text/plain, Size: 4181 bytes --]

Hi Thierry,

On Tuesday 26 November 2013 09:59:12 Thierry Reding wrote:
> On Tue, Nov 26, 2013 at 02:54:41AM +0100, Laurent Pinchart wrote:
> > On Friday 22 November 2013 19:41:54 Thierry Reding wrote:
> > > This binding specifies a set of common properties for display panels. It
> > > can be used as a basis by bindings for specific panels.
> > > 
> > > Bindings for three specific panels are provided to show how the simple
> > > panel binding can be used.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > This binding has already been discussed earlier. Both Laurent and Tomi
> > > seemed to be generally fine with this.
> > 
> > That's correct, I'm generally fine with your approach, but there's still
> > one point I'd like to see addressed.
> > 
> > As I mentioned previously, 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).
> > 
> > Specifying the full timings in every DT source file would be pretty
> > verbose and could be error-prone. However, using a single property to
> > specify one of the standard display timings, as orinally proposed by Hans
> > de Goede (CC'ed) during a chat at the kernel summit would in my opinion be
> > a good middle-ground solution.
> > 
> > Would you consider adding such a property to the simple panel bindings,
> > and define a common compatible string ? Each panel should of course list
> > its own compatibility string in addition to the common compatible string
> > in case the need for extensions arises in the future.
> 
> My gripe with this is that it duplicates information. By definition a simple
> panel supports a limited number of display modes (typically only a single
> one), so once you know the compatible value (which needs to be there anyway)
> you can derive the mode from it. Adding a property that specifies the
> display mode is redundant.

But that could be said of pretty much every device :-) Let's take an example I 
came across today. The SPI DT bindings have a maximum clock speed property for 
every device. This is usually a property of the hardware, which could have 
been stored in the device drivers.

I don't particularly like expressing detailed device information in DT when 
the information is static for a given device and easily stored in the drivers. 
However, in this case, I believe the overhead of adding one mode property to 
DT is worth it, as there are zillion panel models in the wild. Going back to 
the SPI analogy, let's consider the m25p80 SPI flash memory driver. It 
supports many models, each of them implemented by a large number of 
manufacturers. The driver stores information for each model (such as the flash 
size), but the maximum SPI clock frequency is specified in DT as we don't want 
to have a list of all device models for all manufacturers in the driver 
(granted, one of the reasons to specify the max frequency in DT is that it can 
also be limited by the board, not only by the chip, but I believe my point 
remains valid).

> Dave Airlie proposed something else, namely to keep a list of common
> modes within the driver and have each device reference that mode
> instead. I think that could work similarily well. It still requires the
> driver to be touched for each new panel, but the changes will be very
> small. They'll be more of the "add a new table entry" sort of change
> that we have in drivers like the 8250/16550-compatible serial. Most of
> that could probably be wrapped into a macro to make it concise.

That would be better, but not perfect :-) Given the very large number of panel 
models I would like to have a simple panel driver that doesn't need to be 
patched for every model. The odd cases can of course be handled in C, but the 
common case would really benefit from being handled in DT. Imagine how 
annoying it would be to update the USB mass storage driver with the VID:PID of 
every new USB flash device.

-- 
Regards,

Laurent Pinchart

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of: Add simple panel device tree binding

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 6334 bytes --]

On Thu, Dec 05, 2013 at 12:45:17AM +0100, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Tuesday 26 November 2013 09:59:12 Thierry Reding wrote:
> > On Tue, Nov 26, 2013 at 02:54:41AM +0100, Laurent Pinchart wrote:
> > > On Friday 22 November 2013 19:41:54 Thierry Reding wrote:
> > > > This binding specifies a set of common properties for display panels. It
> > > > can be used as a basis by bindings for specific panels.
> > > > 
> > > > Bindings for three specific panels are provided to show how the simple
> > > > panel binding can be used.
> > > > 
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > > This binding has already been discussed earlier. Both Laurent and Tomi
> > > > seemed to be generally fine with this.
> > > 
> > > That's correct, I'm generally fine with your approach, but there's still
> > > one point I'd like to see addressed.
> > > 
> > > As I mentioned previously, 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).
> > > 
> > > Specifying the full timings in every DT source file would be pretty
> > > verbose and could be error-prone. However, using a single property to
> > > specify one of the standard display timings, as orinally proposed by Hans
> > > de Goede (CC'ed) during a chat at the kernel summit would in my opinion be
> > > a good middle-ground solution.
> > > 
> > > Would you consider adding such a property to the simple panel bindings,
> > > and define a common compatible string ? Each panel should of course list
> > > its own compatibility string in addition to the common compatible string
> > > in case the need for extensions arises in the future.
> > 
> > My gripe with this is that it duplicates information. By definition a simple
> > panel supports a limited number of display modes (typically only a single
> > one), so once you know the compatible value (which needs to be there anyway)
> > you can derive the mode from it. Adding a property that specifies the
> > display mode is redundant.
> 
> But that could be said of pretty much every device :-) Let's take an example I 
> came across today. The SPI DT bindings have a maximum clock speed property for 
> every device. This is usually a property of the hardware, which could have 
> been stored in the device drivers.

No it isn't. It's a board-specific property. It depends on all sorts of
factors such as how long the connections are between SPI master and
slave as well as what other electrical components happen to be in that
path.

> I don't particularly like expressing detailed device information in DT when 
> the information is static for a given device and easily stored in the drivers. 
> However, in this case, I believe the overhead of adding one mode property to 
> DT is worth it, as there are zillion panel models in the wild. Going back to 
> the SPI analogy, let's consider the m25p80 SPI flash memory driver. It 
> supports many models, each of them implemented by a large number of 
> manufacturers. The driver stores information for each model (such as the flash 
> size), but the maximum SPI clock frequency is specified in DT as we don't want 
> to have a list of all device models for all manufacturers in the driver 
> (granted, one of the reasons to specify the max frequency in DT is that it can 
> also be limited by the board, not only by the chip, but I believe my point 
> remains valid).

On the contrary, it completely invalidates your point.

And in fact m25p80 is another perfect example to prove my point. Having
display modes encoded within the panel driver and selected depending on
the compatible string isn't anything other than the m25p_ids table.

When you want to support a new type of device, you need to add a new
entry into the m25p_ids table, just like you would add a new display
mode to the panel driver.

> > Dave Airlie proposed something else, namely to keep a list of common
> > modes within the driver and have each device reference that mode
> > instead. I think that could work similarily well. It still requires the
> > driver to be touched for each new panel, but the changes will be very
> > small. They'll be more of the "add a new table entry" sort of change
> > that we have in drivers like the 8250/16550-compatible serial. Most of
> > that could probably be wrapped into a macro to make it concise.
> 
> That would be better, but not perfect :-) Given the very large number of panel
> models I would like to have a simple panel driver that doesn't need to be
> patched for every model. The odd cases can of course be handled in C, but the
> common case would really benefit from being handled in DT.

I really don't think the number of panels supported in mainline will be
overly big. Even if, I will gladly take patches that add them to the
driver. And it isn't even a lot of work, you really just need to copy
this stuff from the datasheet. And the larger the number of panels the
kernel supports, the easier it will get to add new support because you
may either just be able to reuse the compatible string because somebody
else already added support for it, or you may be able to reuse an
existing mode because your panel happens to match that mode.

> Imagine how annoying it would be to update the USB mass storage driver
> with the VID:PID of every new USB flash device.

The electrical interface of USB is well defined and it is further well
specified what USB mass storage means. In addition, USB has very good
support for probing capabilities and characteristics. The equivalent for
panels would be something like EDID. If the panel provided EDID, then of
course you don't need to specify the mode, neither in DT nor in the
driver.

Yet, even with the availability of specifications and standard USB
classes, USB is still one of the areas where doing this is most common:

	$ git grep -n 'USB_DEVICE_.*(' -- drivers/ | wc -l
	829

And then there's things like drivers/hid/usbhid/hid-quirks.c and other
such things.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of: Add simple panel device tree binding

[Sascha Hauer]

On Fri, Dec 06, 2013 at 02:58:00PM +0100, Thierry Reding wrote:
> On Thu, Dec 05, 2013 at 12:45:17AM +0100, Laurent Pinchart wrote:
> > Hi Thierry,
> > 
> > On Tuesday 26 November 2013 09:59:12 Thierry Reding wrote:
> > > On Tue, Nov 26, 2013 at 02:54:41AM +0100, Laurent Pinchart wrote:
> > > > On Friday 22 November 2013 19:41:54 Thierry Reding wrote:
> > > > > This binding specifies a set of common properties for display panels. It
> > > > > can be used as a basis by bindings for specific panels.
> > > > > 
> > > > > Bindings for three specific panels are provided to show how the simple
> > > > > panel binding can be used.
> > > > > 
> > > > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > > ---
> > > > > This binding has already been discussed earlier. Both Laurent and Tomi
> > > > > seemed to be generally fine with this.
> > > > 
> > > > That's correct, I'm generally fine with your approach, but there's still
> > > > one point I'd like to see addressed.
> > > > 
> > > > As I mentioned previously, 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).
> > > > 
> > > > Specifying the full timings in every DT source file would be pretty
> > > > verbose and could be error-prone. However, using a single property to
> > > > specify one of the standard display timings, as orinally proposed by Hans
> > > > de Goede (CC'ed) during a chat at the kernel summit would in my opinion be
> > > > a good middle-ground solution.
> > > > 
> > > > Would you consider adding such a property to the simple panel bindings,
> > > > and define a common compatible string ? Each panel should of course list
> > > > its own compatibility string in addition to the common compatible string
> > > > in case the need for extensions arises in the future.
> > > 
> > > My gripe with this is that it duplicates information. By definition a simple
> > > panel supports a limited number of display modes (typically only a single
> > > one), so once you know the compatible value (which needs to be there anyway)
> > > you can derive the mode from it. Adding a property that specifies the
> > > display mode is redundant.
> > 
> > But that could be said of pretty much every device :-) Let's take an example I 
> > came across today. The SPI DT bindings have a maximum clock speed property for 
> > every device. This is usually a property of the hardware, which could have 
> > been stored in the device drivers.
> 
> No it isn't. It's a board-specific property. It depends on all sorts of
> factors such as how long the connections are between SPI master and
> slave as well as what other electrical components happen to be in that
> path.
> 
> > I don't particularly like expressing detailed device information in DT when 
> > the information is static for a given device and easily stored in the drivers. 
> > However, in this case, I believe the overhead of adding one mode property to 
> > DT is worth it, as there are zillion panel models in the wild. Going back to 
> > the SPI analogy, let's consider the m25p80 SPI flash memory driver. It 
> > supports many models, each of them implemented by a large number of 
> > manufacturers. The driver stores information for each model (such as the flash 
> > size), but the maximum SPI clock frequency is specified in DT as we don't want 
> > to have a list of all device models for all manufacturers in the driver 
> > (granted, one of the reasons to specify the max frequency in DT is that it can 
> > also be limited by the board, not only by the chip, but I believe my point 
> > remains valid).
> 
> On the contrary, it completely invalidates your point.
> 
> And in fact m25p80 is another perfect example to prove my point. Having
> display modes encoded within the panel driver and selected depending on
> the compatible string isn't anything other than the m25p_ids table.
> 
> When you want to support a new type of device, you need to add a new
> entry into the m25p_ids table, just like you would add a new display
> mode to the panel driver.
> 
> > > Dave Airlie proposed something else, namely to keep a list of common
> > > modes within the driver and have each device reference that mode
> > > instead. I think that could work similarily well. It still requires the
> > > driver to be touched for each new panel, but the changes will be very
> > > small. They'll be more of the "add a new table entry" sort of change
> > > that we have in drivers like the 8250/16550-compatible serial. Most of
> > > that could probably be wrapped into a macro to make it concise.
> > 
> > That would be better, but not perfect :-) Given the very large number of panel
> > models I would like to have a simple panel driver that doesn't need to be
> > patched for every model. The odd cases can of course be handled in C, but the
> > common case would really benefit from being handled in DT.
> 
> I really don't think the number of panels supported in mainline will be
> overly big.

I for once have hardly seen a display twice in different projects.

> Even if, I will gladly take patches that add them to the
> driver. And it isn't even a lot of work, you really just need to copy
> this stuff from the datasheet. And the larger the number of panels the
> kernel supports, the easier it will get to add new support because you
> may either just be able to reuse the compatible string because somebody
> else already added support for it, or you may be able to reuse an
> existing mode because your panel happens to match that mode.
> 
> > Imagine how annoying it would be to update the USB mass storage driver
> > with the VID:PID of every new USB flash device.
> 
> The electrical interface of USB is well defined and it is further well
> specified what USB mass storage means. In addition, USB has very good
> support for probing capabilities and characteristics. The equivalent for
> panels would be something like EDID. If the panel provided EDID, then of
> course you don't need to specify the mode, neither in DT nor in the
> driver.

What makes EDID useful is that it contains the modes. We don't have to
maintain a database of all monitors available out there in the kernel.
Still EDID data contains a vendor/model string which makes it possible
to add quirks for a specific monitor should we have to.

So I really vote for putting the mode data into dt for the 98% case and
an exact vendor/model for the remaining quirks. This is simple anyway
since we already have a binding for display modelines. Supporting this
in the panel driver is trivial.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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: [PATCH] of: Add simple panel device tree binding

[Rob Herring]

On Fri, Nov 22, 2013 at 12:41 PM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This binding specifies a set of common properties for display panels. It
> can be used as a basis by bindings for specific panels.
>
> Bindings for three specific panels are provided to show how the simple
> panel binding can be used.
>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Thanks for a "simple" binding that actually provides specific h/w information.

Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Rob

> ---
> This binding has already been discussed earlier. Both Laurent and Tomi
> seemed to be generally fine with this. The fact that the binding targets
> simple (dumb) panels only and the reduced set of properties make it easy
> to be extended in backwards-compatible ways should the need arise, while
> at the same time allowing to support a wide variety of panels out there.
>
>  .../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 +++++++++++++++++++++
>  4 files changed, 42 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
>
> diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> new file mode 100644
> index 000000000000..72e088a4fb3a
> --- /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 000000000000..0ab2c05a4c22
> --- /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 000000000000..d328b0341bf4
> --- /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 000000000000..1341bbf4aa3d
> --- /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>;
> +       };
> --
> 1.8.4.2
>
> --
> 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
--
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: [PATCH] of: Add simple panel device tree binding

[Daniel Kurtz]

On Sat, Nov 23, 2013 at 2:41 AM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This binding specifies a set of common properties for display panels. It
> can be used as a basis by bindings for specific panels.
>
> Bindings for three specific panels are provided to show how the simple
> panel binding can be used.
>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> This binding has already been discussed earlier. Both Laurent and Tomi
> seemed to be generally fine with this. The fact that the binding targets
> simple (dumb) panels only and the reduced set of properties make it easy
> to be extended in backwards-compatible ways should the need arise, while
> at the same time allowing to support a wide variety of panels out there.
>
>  .../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 +++++++++++++++++++++
>  4 files changed, 42 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
>
> diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> new file mode 100644
> index 000000000000..72e088a4fb3a
> --- /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 000000000000..0ab2c05a4c22
> --- /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 000000000000..d328b0341bf4
> --- /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 000000000000..1341bbf4aa3d
> --- /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>;
> +       };


Pardon the question if this has been discussed before, but, would this
also be a good binding in which to store a panel's physical dimensions
(mmWidth, mmHeight)?  Or is there another binding more appropriate?

> --
> 1.8.4.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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: [PATCH] of: Add simple panel device tree binding

[Thierry Reding]

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

On Tue, Nov 26, 2013 at 10:01:54AM +0800, Daniel Kurtz wrote:
> On Sat, Nov 23, 2013 at 2:41 AM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > This binding specifies a set of common properties for display panels. It
> > can be used as a basis by bindings for specific panels.
> >
> > Bindings for three specific panels are provided to show how the simple
> > panel binding can be used.
[...]
> > +Example:
> > +
> > +       panel: panel {
> > +               compatible = "cptt,claa101wb01";
> > +               ddc-i2c-bus = <&panelddc>;
> > +
> > +               power-supply = <&vdd_pnl_reg>;
> > +               enable-gpios = <&gpio 90 0>;
> > +
> > +               backlight = <&backlight>;
> > +       };
> 
> 
> Pardon the question if this has been discussed before, but, would this
> also be a good binding in which to store a panel's physical dimensions
> (mmWidth, mmHeight)?  Or is there another binding more appropriate?

This binding doesn't define those properties because they are implied by
the compatible string. The corresponding driver defines a structure like
this:

	struct panel_desc {
		const struct drm_display_mode *modes;
		unsigned int num_modes;

		struct {
			unsigned int width;
			unsigned int height;
		} size;
	};

The panel_desc.size structure contains the physical panel dimensions,
specified in mm. With that, a panel will be supported by defining
something like this:

	static const struct drm_display_mode foo_mode = {
		...
	};

	static const struct panel_desc foo = {
		.modes = &foo_mode,
		.num_modes = 1,
		.size {
			.width = 223,
			.height = 125,
		},
	};

	static const struct of_device_id platform_of_match[] = {
		...
		{
			.compatible = "vendor,foo",
			.data = &foo,
		},
		...
	};

I assume you have a specific use-case in mind. Does this provide what
you need? The driver patches were posted earlier[0][1]. They have
changed slightly since then to address review comments, but nothing
significant.

Thierry

[0]: http://www.spinics.net/lists/devicetree/msg08498.html
[1]: http://www.spinics.net/lists/devicetree/msg08499.html

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

Re: [PATCH] of: Add simple panel device tree binding

[Tomi Valkeinen]

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

On 2013-11-22 20:41, Thierry Reding wrote:

> +Example:
> +
> +	panel: panel {
> +		compatible = "cptt,claa101wb01";
> +		ddc-i2c-bus = <&panelddc>;
> +
> +		power-supply = <&vdd_pnl_reg>;
> +		enable-gpios = <&gpio 90 0>;
> +
> +		backlight = <&backlight>;
> +	};

I'm somewhat torn with this, as I agree with Thierry that it's correct
to have a panel database in the driver, but, on the other hand, it does
seem impractical.

In my experience, there are lots of panels out there, and each board I
have has a different one. So, while just a gut feeling, we could end up
with lots of panel, each used only on one board.

With a quick thought, things would work fine if we just added the
videomode data to the DT data, instead of a driver database, as Laurent
suggested.

However... I don't think the panels are usually as simple as that. With
the panels I've worked with, the driver has to know things like:

- Does the power supply need to be enabled before the enable gpio, and
if so, how long before? And the same for power off.

- Does the video stream need to be enabled before the enable gpio, and
if so, how long before? And the same for power off.

- Is the gpio enable, power down, or reset? If reset, what are the timings.

Where will those be defined? This goes back to the power sequence stuff
again... (Was the power sequences series forgotten?) And defining such
sequences in DT data is, I think, bad idea. But having them in the
driver would be fine. If we have those in the driver, it's better to
have the video modes there also.

The sequence for power, gpio and video stream is most likely common for
most panels. Power-on is done with enabling, in order: power, video,
gpio. And power-off is vice versa. But we need delays in between, and I
don't know if we can have some kind of common delays that'd work for
most of the panels.

If we can, we could perhaps define in detail our "common simple panel"
spec, which would contain sequences, timings, powers, gpios, and so on,
so it would be easy to see if a particular panel fits into our simple
panel spec.

 Tomi



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

Re: [PATCH] of: Add simple panel device tree binding

[Alex Courbot]

On 12/11/2013 11:16 PM, Tomi Valkeinen wrote:
> * PGP Signed by an unknown key
>
> On 2013-11-22 20:41, Thierry Reding wrote:
>
>> +Example:
>> +
>> +	panel: panel {
>> +		compatible = "cptt,claa101wb01";
>> +		ddc-i2c-bus = <&panelddc>;
>> +
>> +		power-supply = <&vdd_pnl_reg>;
>> +		enable-gpios = <&gpio 90 0>;
>> +
>> +		backlight = <&backlight>;
>> +	};
>
> I'm somewhat torn with this, as I agree with Thierry that it's correct
> to have a panel database in the driver, but, on the other hand, it does
> seem impractical.
>
> In my experience, there are lots of panels out there, and each board I
> have has a different one. So, while just a gut feeling, we could end up
> with lots of panel, each used only on one board.
>
> With a quick thought, things would work fine if we just added the
> videomode data to the DT data, instead of a driver database, as Laurent
> suggested.
>
> However... I don't think the panels are usually as simple as that. With
> the panels I've worked with, the driver has to know things like:
>
> - Does the power supply need to be enabled before the enable gpio, and
> if so, how long before? And the same for power off.
>
> - Does the video stream need to be enabled before the enable gpio, and
> if so, how long before? And the same for power off.
>
> - Is the gpio enable, power down, or reset? If reset, what are the timings.
>
> Where will those be defined? This goes back to the power sequence stuff
> again... (Was the power sequences series forgotten?)

Sorry for the very late reply - power sequences are forgotten for now, 
but I don't mind reviving them if a clear use-case emerges. The main 
point of power seqs (at least in my mind) was to be able to define them 
in the DT to avoid things like the panel DB you mention. This idea has 
been dismissed for good reasons, and anyway in the case of the panel it 
is clear that this is not what we want to do.

Now if we make a power sequences framework without DT support, we will 
end up with something that would still require a panel database, and can 
anyway be expressed by functions. The gain would be automated 
error-handling, and reduced footprint for power sequences. Not sure 
that's enough to justify resurrecting the power seqs.

Alex.