[PATCH 1/2] drm/panel: add of display timing support

[PATCH 1/2] drm/panel: add of display timing support

[Mark Yao]

We want add display support on loader, share the same timing
with kernel side, but the timing is hide into kernel code,
can't be share, avoid config twice display timing, add device-tree
display timing support would be good idea.

With this patch, loader and kernel can share same timing from
device node.

Cc: Thierry Reding <thierry.reding@gmail.com>

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 73 ++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 85143d1..04cd797 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -34,6 +34,7 @@
 #include <drm/drm_panel.h>
 
 #include <video/display_timing.h>
+#include <video/of_display_timing.h>
 #include <video/videomode.h>
 
 struct panel_desc {
@@ -80,6 +81,7 @@ struct panel_simple {
 	bool prepared;
 	bool enabled;
 
+	struct device *dev;
 	const struct panel_desc *desc;
 
 	struct backlight_device *backlight;
@@ -159,6 +161,32 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel)
 	return num;
 }
 
+static int panel_simple_of_get_native_mode(struct panel_simple *panel)
+{
+	struct drm_connector *connector = panel->base.connector;
+	struct drm_device *drm = panel->base.drm;
+	struct drm_display_mode *mode;
+	int ret;
+
+	mode = drm_mode_create(drm);
+	if (!mode)
+		return 0;
+
+	ret = of_get_drm_display_mode(panel->dev->of_node, mode,
+				      OF_USE_NATIVE_MODE);
+	if (ret) {
+		dev_dbg(panel->dev, "failed to find dts display timings\n");
+		drm_mode_destroy(drm, mode);
+		return 0;
+	}
+
+	drm_mode_set_name(mode);
+	mode->type |= DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
 static int panel_simple_disable(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
@@ -172,7 +200,7 @@ static int panel_simple_disable(struct drm_panel *panel)
 		backlight_update_status(p->backlight);
 	}
 
-	if (p->desc->delay.disable)
+	if (p->desc && p->desc->delay.disable)
 		msleep(p->desc->delay.disable);
 
 	p->enabled = false;
@@ -192,7 +220,7 @@ static int panel_simple_unprepare(struct drm_panel *panel)
 
 	regulator_disable(p->supply);
 
-	if (p->desc->delay.unprepare)
+	if (p->desc && p->desc->delay.unprepare)
 		msleep(p->desc->delay.unprepare);
 
 	p->prepared = false;
@@ -217,7 +245,7 @@ static int panel_simple_prepare(struct drm_panel *panel)
 	if (p->enable_gpio)
 		gpiod_set_value_cansleep(p->enable_gpio, 1);
 
-	if (p->desc->delay.prepare)
+	if (p->desc && p->desc->delay.prepare)
 		msleep(p->desc->delay.prepare);
 
 	p->prepared = true;
@@ -232,7 +260,7 @@ static int panel_simple_enable(struct drm_panel *panel)
 	if (p->enabled)
 		return 0;
 
-	if (p->desc->delay.enable)
+	if (p->desc && p->desc->delay.enable)
 		msleep(p->desc->delay.enable);
 
 	if (p->backlight) {
@@ -264,6 +292,9 @@ static int panel_simple_get_modes(struct drm_panel *panel)
 	/* add hard-coded panel modes */
 	num += panel_simple_get_fixed_modes(p);
 
+	/* add device node plane modes */
+	num += panel_simple_of_get_native_mode(p);
+
 	return num;
 }
 
@@ -274,6 +305,9 @@ static int panel_simple_get_timings(struct drm_panel *panel,
 	struct panel_simple *p = to_panel_simple(panel);
 	unsigned int i;
 
+	if (!p->desc)
+		return 0;
+
 	if (p->desc->num_timings < num_timings)
 		num_timings = p->desc->num_timings;
 
@@ -306,6 +340,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	panel->enabled = false;
 	panel->prepared = false;
 	panel->desc = desc;
+	panel->dev = dev;
 
 	panel->supply = devm_regulator_get(dev, "power");
 	if (IS_ERR(panel->supply))
@@ -1515,6 +1550,9 @@ static const struct panel_desc urt_umsh_8596md_parallel = {
 
 static const struct of_device_id platform_of_match[] = {
 	{
+		.compatible = "simple-panel",
+		.data = NULL,
+	}, {
 		.compatible = "ampire,am800480r3tmqwa1h",
 		.data = &ampire_am800480r3tmqwa1h,
 	}, {
@@ -1860,6 +1898,9 @@ static const struct panel_desc_dsi panasonic_vvx10f004b00 = {
 
 static const struct of_device_id dsi_of_match[] = {
 	{
+		.compatible = "simple-panel-dsi",
+		.data = NULL
+	}, {
 		.compatible = "auo,b080uan01",
 		.data = &auo_b080uan01
 	}, {
@@ -1884,6 +1925,8 @@ static int panel_simple_dsi_probe(struct mipi_dsi_device *dsi)
 {
 	const struct panel_desc_dsi *desc;
 	const struct of_device_id *id;
+	const struct panel_desc *pdesc;
+	u32 val;
 	int err;
 
 	id = of_match_node(dsi_of_match, dsi->dev.of_node);
@@ -1892,13 +1935,27 @@ static int panel_simple_dsi_probe(struct mipi_dsi_device *dsi)
 
 	desc = id->data;
 
-	err = panel_simple_probe(&dsi->dev, &desc->desc);
+	if (desc) {
+		dsi->mode_flags = desc->flags;
+		dsi->format = desc->format;
+		dsi->lanes = desc->lanes;
+		pdesc = &desc->desc;
+	} else {
+		pdesc = NULL;
+	}
+
+	err = panel_simple_probe(&dsi->dev, pdesc);
 	if (err < 0)
 		return err;
 
-	dsi->mode_flags = desc->flags;
-	dsi->format = desc->format;
-	dsi->lanes = desc->lanes;
+	if (!of_property_read_u32(dsi->dev.of_node, "dsi,flags", &val))
+		dsi->mode_flags = val;
+
+	if (!of_property_read_u32(dsi->dev.of_node, "dsi,format", &val))
+		dsi->format = val;
+
+	if (!of_property_read_u32(dsi->dev.of_node, "dsi,lanes", &val))
+		dsi->lanes = val;
 
 	return mipi_dsi_attach(dsi);
 }
-- 
1.9.1

[PATCH 2/2] dt-bindings: add simple-panel-dsi and simple-panel

[Mark Yao]

Allow user add display timing on device tree with simple-panel-dsi
or simple-panel.

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 .../bindings/display/panel/simple-panel.txt        | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
index 1341bbf..bc66970 100644
--- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
+++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
@@ -4,10 +4,22 @@ Required properties:
 - power-supply: regulator to provide the supply voltage
 
 Optional properties:
+- compatible: value maybe one of the following
+		"simple-panel";
+		"simple-panel-dsi";
+
 - 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
 
+Required properties when compatible is "simple-panel" or "simple-panel-dsi":
+- display-timings: see display-timing.txt for information
+
+Optional properties when compatible is a dsi devices:
+- dsi,flags: dsi operation mode related flags
+- dsi,format: pixel format for video mode
+- dsi,lanes: number of active data lanes
+
 Example:
 
 	panel: panel {
@@ -19,3 +31,39 @@ Example:
 
 		backlight = <&backlight>;
 	};
+
+Or:
+	panel: panel {
+		compatible = "simple-panel-dsi";
+		ddc-i2c-bus = <&panelddc>;
+
+		power-supply = <&vdd_pnl_reg>;
+		enable-gpios = <&gpio 90 0>;
+
+		backlight = <&backlight>;
+
+		dsi,flags = <MIPI_DSI_MODE_VIDEO |
+			     MIPI_DSI_MODE_VIDEO_BURST |
+			     MIPI_DSI_MODE_VIDEO_SYNC_PULSE>;
+		dsi,format = <MIPI_DSI_FMT_RGB888>;
+		dsi,lanes = <4>;
+
+		display-timings {
+			native-mode = <&timing0>;
+			timing0: timing0 {
+				clock-frequency = <160000000>;
+				hactive = <1200>;
+				vactive = <1920>;
+				hback-porch = <21>;
+				hfront-porch = <120>;
+				vback-porch = <18>;
+				vfront-porch = <21>;
+				hsync-len = <20>;
+				vsync-len = <3>;
+				hsync-active = <0>;
+				vsync-active = <0>;
+				de-active = <0>;
+				pixelclk-active = <0>;
+			};
+		};
+	};
-- 
1.9.1

Re: [PATCH 2/2] dt-bindings: add simple-panel-dsi and simple-panel

[Thierry Reding]

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

On Wed, Jul 20, 2016 at 11:18:50AM +0800, Mark Yao wrote:
> Allow user add display timing on device tree with simple-panel-dsi
> or simple-panel.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> 
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
>  .../bindings/display/panel/simple-panel.txt        | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)

Sorry, not going to happen. Read this for an explanation of why not:

	https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] dt-bindings: add simple-panel-dsi and simple-panel

[Thierry Reding]

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

On Tue, Jul 26, 2016 at 10:01:32AM +0800, Mark yao wrote:
> On 2016年07月25日 23:21, Thierry Reding wrote:
> 
>     On Wed, Jul 20, 2016 at 11:18:50AM +0800, Mark Yao wrote:
> 
>         Allow user add display timing on device tree with simple-panel-dsi
>         or simple-panel.
> 
>         Cc: Thierry Reding <thierry.reding@gmail.com>
>         Cc: Rob Herring <robh+dt@kernel.org>
>         Cc: Mark Rutland <mark.rutland@arm.com>
> 
>         Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>         ---
>          .../bindings/display/panel/simple-panel.txt        | 48 ++++++++++++++++++++++
>          1 file changed, 48 insertions(+)
> 
>     Sorry, not going to happen. Read this for an explanation of why not:
> 
>             https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html
> 
>     Thierry
> 
> 
> Hi Thierry
> 
> The blog actually not persuade me why can't use display timing on
> device tree.

Okay, perhaps read it again, it addresses most of your points below.

> 1, Binding panel as a simple string on device tree seems simple on device tree,
> but it's complex on kernel code, and kernel code would became bigger and
> bigger.

I don't think the video timings in the simple-panel driver are very
complex. They also don't use very much space. And if you're really
concerned about space you can always use conditional compilation and
Kconfig symbols to remove timings for panels that you don't use.

Also, panels are characterized by much more than just video timings.
There were attempts, way back, to fully describe panels in device tree
and that failed. What you propose here is a partial solution to a much
more complex problem.

This is all explained in the blog post.

> 2, Our customer always ask me, where is the display timing? They only find a
> simple panel string on device tree,  need search the kernel code to find the
> actually timing. They are used to find all device info on device tree, but
> panel timing info is not, this would confuse them. They don't want to know how
> code work, just want a easier interface.

That's not a very good argument. There's plenty of data that's not in
device tree for other devices, why should panels be different? Also, I
would hope that any customer of yours knows their way around kernel
code and can therefore easily add video timings for new panels. It's
quite trivial to do, and there are many examples on how to do it.

> 3, I think device tree not only can use for kernel, other module also can use
> it. on our project, we use uboot + kernel, the uboot support fdt, that function
> can parsing device tree. So if describe the display timing on device tree, both
> uboot and kernel can share same display timing, not need to describe twice, it
> would save work and not easy to make mistake.

That's a bit of a stretch. Video timings is fairly straightforward data
and can be easily added to any other piece of code that you want to run.
Yes you will have to duplicate the data, but how is that different from
duplicating all the driver code?

> 4, For differentiation product, we face many different panel, every once in a
> while, need to add a new panel, we can't convert all the panel , code the panel
> on kernel seems too bad, and the kernel image became bigger and bigger.

Why can't you convert all the panels? We already support a bunch of them
and haven't yet run into any problems. If you do encounter any issues
trying to port panels to the DRM panel infrastructure, please let me
know and I can help sort them out.

The kernel image size isn't a problem either. In any modern kernel the
video timing data in the panel driver is tiny compared to the rest.

> Generally, Our customer don't want to do any modify on kernel, they just modify
> device tree to bring up their device. Describe the panel timing on device tree,
> would make customer easy to use and reuse it.

Yes, that would perhaps make it easier for them to bring up the device.
But soon after they'll notice that there are glitches when turning the
panel on and off, and then they'll realize that they can't fix that
using their simple device tree.

All of that said, if you do think this is valuable to your customers,
please feel free to ship these patches in your downstream tree.

Also, though this is a bit off topic, I'm sure that your customers'
customers would be very happy to get all the security and bug fixes that
would automatically be delivered with the frequent kernel updates that
bring in support for new panels.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] dt-bindings: add simple-panel-dsi and simple-panel

[Mark yao]

On 2016年07月26日 17:02, Thierry Reding wrote:
> On Tue, Jul 26, 2016 at 10:01:32AM +0800, Mark yao wrote:
>> On 2016年07月25日 23:21, Thierry Reding wrote:
>>
>>      On Wed, Jul 20, 2016 at 11:18:50AM +0800, Mark Yao wrote:
>>
>>          Allow user add display timing on device tree with simple-panel-dsi
>>          or simple-panel.
>>
>>          Cc: Thierry Reding <thierry.reding@gmail.com>
>>          Cc: Rob Herring <robh+dt@kernel.org>
>>          Cc: Mark Rutland <mark.rutland@arm.com>
>>
>>          Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>          ---
>>           .../bindings/display/panel/simple-panel.txt        | 48 ++++++++++++++++++++++
>>           1 file changed, 48 insertions(+)
>>
>>      Sorry, not going to happen. Read this for an explanation of why not:
>>
>>              https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html
>>
>>      Thierry
>>
>>
>> Hi Thierry
>>
>> The blog actually not persuade me why can't use display timing on
>> device tree.
> Okay, perhaps read it again, it addresses most of your points below.
>
>> 1, Binding panel as a simple string on device tree seems simple on device tree,
>> but it's complex on kernel code, and kernel code would became bigger and
>> bigger.
> I don't think the video timings in the simple-panel driver are very
> complex. They also don't use very much space. And if you're really
> concerned about space you can always use conditional compilation and
> Kconfig symbols to remove timings for panels that you don't use.
>
> Also, panels are characterized by much more than just video timings.
> There were attempts, way back, to fully describe panels in device tree
> and that failed. What you propose here is a partial solution to a much
> more complex problem.
>
> This is all explained in the blog post.
>
>> 2, Our customer always ask me, where is the display timing? They only find a
>> simple panel string on device tree,  need search the kernel code to find the
>> actually timing. They are used to find all device info on device tree, but
>> panel timing info is not, this would confuse them. They don't want to know how
>> code work, just want a easier interface.
> That's not a very good argument. There's plenty of data that's not in
> device tree for other devices, why should panels be different? Also, I
> would hope that any customer of yours knows their way around kernel
> code and can therefore easily add video timings for new panels. It's
> quite trivial to do, and there are many examples on how to do it.
>
>> 3, I think device tree not only can use for kernel, other module also can use
>> it. on our project, we use uboot + kernel, the uboot support fdt, that function
>> can parsing device tree. So if describe the display timing on device tree, both
>> uboot and kernel can share same display timing, not need to describe twice, it
>> would save work and not easy to make mistake.
> That's a bit of a stretch. Video timings is fairly straightforward data
> and can be easily added to any other piece of code that you want to run.
> Yes you will have to duplicate the data, but how is that different from
> duplicating all the driver code?
>
>> 4, For differentiation product, we face many different panel, every once in a
>> while, need to add a new panel, we can't convert all the panel , code the panel
>> on kernel seems too bad, and the kernel image became bigger and bigger.
> Why can't you convert all the panels? We already support a bunch of them
> and haven't yet run into any problems. If you do encounter any issues
> trying to port panels to the DRM panel infrastructure, please let me
> know and I can help sort them out.
>
> The kernel image size isn't a problem either. In any modern kernel the
> video timing data in the panel driver is tiny compared to the rest.
>
>> Generally, Our customer don't want to do any modify on kernel, they just modify
>> device tree to bring up their device. Describe the panel timing on device tree,
>> would make customer easy to use and reuse it.
> Yes, that would perhaps make it easier for them to bring up the device.
> But soon after they'll notice that there are glitches when turning the
> panel on and off, and then they'll realize that they can't fix that
> using their simple device tree.
About the panel on and off, I don't think the panel-simple do the good 
enough.

panel-simple only have one gpio and one regulator, and their sequence is 
hard code, Why not a panel have two gpio or two regulator? On our 
project, we find many customer don't use the RC to do panel reset, they 
directly use gpio reset, so they need a gpio to do panel reset.

the device tree panel's on and off function is what the next step I want 
to upstream, on our downstream kernel, we do like that:

panel {
     power_ctrl {
           power0 {
                   gpios = <xxx>;
                   delay,ms = <3>;
           }
           power1 {
                   regulator = <xxx>;
                   delay,ms = <3>;
           }
           power2 {
                   backlight = <xxx>;
                   delay,ms = <3>;
           }
     }
}
and on driver, power on sequence with power0->power1->power2, power down 
with power2->power1->power0.
if user want to swap the power, can easy do that by  adjust dts power 
sequence.

this method can easy order the gpio, regulator, backlight sequence, 
judge the delay time and add new regulator or gpio.
I think this panel power on and off method is better than panel-simple 
driver currently using.

> All of that said, if you do think this is valuable to your customers,
> please feel free to ship these patches in your downstream tree.
>
> Also, though this is a bit off topic, I'm sure that your customers'
> customers would be very happy to get all the security and bug fixes that
> would automatically be delivered with the frequent kernel updates that
> bring in support for new panels.
>
> Thierry


-- 
Ｍark Yao

Re: [PATCH 2/2] dt-bindings: add simple-panel-dsi and simple-panel

[Thierry Reding]

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

On Wed, Jul 27, 2016 at 11:03:05AM +0800, Mark yao wrote:
> On 2016年07月26日 17:02, Thierry Reding wrote:
> > On Tue, Jul 26, 2016 at 10:01:32AM +0800, Mark yao wrote:
> > > On 2016年07月25日 23:21, Thierry Reding wrote:
> > > 
> > >      On Wed, Jul 20, 2016 at 11:18:50AM +0800, Mark Yao wrote:
> > > 
> > >          Allow user add display timing on device tree with simple-panel-dsi
> > >          or simple-panel.
> > > 
> > >          Cc: Thierry Reding <thierry.reding@gmail.com>
> > >          Cc: Rob Herring <robh+dt@kernel.org>
> > >          Cc: Mark Rutland <mark.rutland@arm.com>
> > > 
> > >          Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> > >          ---
> > >           .../bindings/display/panel/simple-panel.txt        | 48 ++++++++++++++++++++++
> > >           1 file changed, 48 insertions(+)
> > > 
> > >      Sorry, not going to happen. Read this for an explanation of why not:
> > > 
> > >              https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html
> > > 
> > >      Thierry
> > > 
> > > 
> > > Hi Thierry
> > > 
> > > The blog actually not persuade me why can't use display timing on
> > > device tree.
> > Okay, perhaps read it again, it addresses most of your points below.
> > 
> > > 1, Binding panel as a simple string on device tree seems simple on device tree,
> > > but it's complex on kernel code, and kernel code would became bigger and
> > > bigger.
> > I don't think the video timings in the simple-panel driver are very
> > complex. They also don't use very much space. And if you're really
> > concerned about space you can always use conditional compilation and
> > Kconfig symbols to remove timings for panels that you don't use.
> > 
> > Also, panels are characterized by much more than just video timings.
> > There were attempts, way back, to fully describe panels in device tree
> > and that failed. What you propose here is a partial solution to a much
> > more complex problem.
> > 
> > This is all explained in the blog post.
> > 
> > > 2, Our customer always ask me, where is the display timing? They only find a
> > > simple panel string on device tree,  need search the kernel code to find the
> > > actually timing. They are used to find all device info on device tree, but
> > > panel timing info is not, this would confuse them. They don't want to know how
> > > code work, just want a easier interface.
> > That's not a very good argument. There's plenty of data that's not in
> > device tree for other devices, why should panels be different? Also, I
> > would hope that any customer of yours knows their way around kernel
> > code and can therefore easily add video timings for new panels. It's
> > quite trivial to do, and there are many examples on how to do it.
> > 
> > > 3, I think device tree not only can use for kernel, other module also can use
> > > it. on our project, we use uboot + kernel, the uboot support fdt, that function
> > > can parsing device tree. So if describe the display timing on device tree, both
> > > uboot and kernel can share same display timing, not need to describe twice, it
> > > would save work and not easy to make mistake.
> > That's a bit of a stretch. Video timings is fairly straightforward data
> > and can be easily added to any other piece of code that you want to run.
> > Yes you will have to duplicate the data, but how is that different from
> > duplicating all the driver code?
> > 
> > > 4, For differentiation product, we face many different panel, every once in a
> > > while, need to add a new panel, we can't convert all the panel , code the panel
> > > on kernel seems too bad, and the kernel image became bigger and bigger.
> > Why can't you convert all the panels? We already support a bunch of them
> > and haven't yet run into any problems. If you do encounter any issues
> > trying to port panels to the DRM panel infrastructure, please let me
> > know and I can help sort them out.
> > 
> > The kernel image size isn't a problem either. In any modern kernel the
> > video timing data in the panel driver is tiny compared to the rest.
> > 
> > > Generally, Our customer don't want to do any modify on kernel, they just modify
> > > device tree to bring up their device. Describe the panel timing on device tree,
> > > would make customer easy to use and reuse it.
> > Yes, that would perhaps make it easier for them to bring up the device.
> > But soon after they'll notice that there are glitches when turning the
> > panel on and off, and then they'll realize that they can't fix that
> > using their simple device tree.
> About the panel on and off, I don't think the panel-simple do the good
> enough.
> 
> panel-simple only have one gpio and one regulator, and their sequence is
> hard code, Why not a panel have two gpio or two regulator? On our project,
> we find many customer don't use the RC to do panel reset, they directly use
> gpio reset, so they need a gpio to do panel reset.

The driver is called panel-simple for a reason. It's not meant to cover
all possible use-cases, only the simple ones.

> the device tree panel's on and off function is what the next step I want to
> upstream, on our downstream kernel, we do like that:
> 
> panel {
>     power_ctrl {
>           power0 {
>                   gpios = <xxx>;
>                   delay,ms = <3>;
>           }
>           power1 {
>                   regulator = <xxx>;
>                   delay,ms = <3>;
>           }
>           power2 {
>                   backlight = <xxx>;
>                   delay,ms = <3>;
>           }
>     }
> }
> and on driver, power on sequence with power0->power1->power2, power down
> with power2->power1->power0.
> if user want to swap the power, can easy do that by  adjust dts power
> sequence.
> 
> this method can easy order the gpio, regulator, backlight sequence, judge
> the delay time and add new regulator or gpio.
> I think this panel power on and off method is better than panel-simple
> driver currently using.

That's almost exactly like the power sequences that Alex Courbot had
proposed about three years ago. The concept of such a thing was rejected
by device tree binding maintainers, which is why we ended up with what
we have now. I'm sure you can find a link to the discussion if you want
the details about why it got rejected.

All of that's described in the blog, by the way.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] dt-bindings: add simple-panel-dsi and simple-panel

[Rob Herring]

On Tue, Jul 26, 2016 at 4:02 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Jul 26, 2016 at 10:01:32AM +0800, Mark yao wrote:
>> On 2016年07月25日 23:21, Thierry Reding wrote:
>>
>>     On Wed, Jul 20, 2016 at 11:18:50AM +0800, Mark Yao wrote:
>>
>>         Allow user add display timing on device tree with simple-panel-dsi
>>         or simple-panel.
>>
>>         Cc: Thierry Reding <thierry.reding@gmail.com>
>>         Cc: Rob Herring <robh+dt@kernel.org>
>>         Cc: Mark Rutland <mark.rutland@arm.com>
>>
>>         Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>         ---
>>          .../bindings/display/panel/simple-panel.txt        | 48 ++++++++++++++++++++++
>>          1 file changed, 48 insertions(+)
>>
>>     Sorry, not going to happen. Read this for an explanation of why not:
>>
>>             https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html
>>
>>     Thierry
>>
>>
>> Hi Thierry
>>
>> The blog actually not persuade me why can't use display timing on
>> device tree.
>
> Okay, perhaps read it again, it addresses most of your points below.
>
>> 1, Binding panel as a simple string on device tree seems simple on device tree,
>> but it's complex on kernel code, and kernel code would became bigger and
>> bigger.
>
> I don't think the video timings in the simple-panel driver are very
> complex. They also don't use very much space. And if you're really
> concerned about space you can always use conditional compilation and
> Kconfig symbols to remove timings for panels that you don't use.
>
> Also, panels are characterized by much more than just video timings.
> There were attempts, way back, to fully describe panels in device tree
> and that failed. What you propose here is a partial solution to a much
> more complex problem.
>
> This is all explained in the blog post.

While I agree with everything Thierry has said in this thread, there
is an argument to be made for timing information in DT.

Maybe most vendors have now learned that flexible clock frequencies
are needed to generate the correct timings, but I have worked on parts
without fine resolution (i.e. a dedicated pll). Even HiKey with HDMI
suffers from this. In that case you can be tuning the timings
(increasing the h and/or v blanks) to get the desired refresh rates.
So in that case, the panel compatible would not determine the display
timings and you could have the same panel with different timings on
different boards. That's maybe rare enough that putting timing info in
DT still wouldn't make sense.

But that is not the problem here, and everything else about this is wrong.

Rob

Re: [PATCH 2/2] dt-bindings: add simple-panel-dsi and simple-panel

[Thierry Reding]

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

On Thu, Jul 28, 2016 at 02:00:21PM -0500, Rob Herring wrote:
> On Tue, Jul 26, 2016 at 4:02 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Tue, Jul 26, 2016 at 10:01:32AM +0800, Mark yao wrote:
> >> On 2016年07月25日 23:21, Thierry Reding wrote:
> >>
> >>     On Wed, Jul 20, 2016 at 11:18:50AM +0800, Mark Yao wrote:
> >>
> >>         Allow user add display timing on device tree with simple-panel-dsi
> >>         or simple-panel.
> >>
> >>         Cc: Thierry Reding <thierry.reding@gmail.com>
> >>         Cc: Rob Herring <robh+dt@kernel.org>
> >>         Cc: Mark Rutland <mark.rutland@arm.com>
> >>
> >>         Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> >>         ---
> >>          .../bindings/display/panel/simple-panel.txt        | 48 ++++++++++++++++++++++
> >>          1 file changed, 48 insertions(+)
> >>
> >>     Sorry, not going to happen. Read this for an explanation of why not:
> >>
> >>             https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html
> >>
> >>     Thierry
> >>
> >>
> >> Hi Thierry
> >>
> >> The blog actually not persuade me why can't use display timing on
> >> device tree.
> >
> > Okay, perhaps read it again, it addresses most of your points below.
> >
> >> 1, Binding panel as a simple string on device tree seems simple on device tree,
> >> but it's complex on kernel code, and kernel code would became bigger and
> >> bigger.
> >
> > I don't think the video timings in the simple-panel driver are very
> > complex. They also don't use very much space. And if you're really
> > concerned about space you can always use conditional compilation and
> > Kconfig symbols to remove timings for panels that you don't use.
> >
> > Also, panels are characterized by much more than just video timings.
> > There were attempts, way back, to fully describe panels in device tree
> > and that failed. What you propose here is a partial solution to a much
> > more complex problem.
> >
> > This is all explained in the blog post.
> 
> While I agree with everything Thierry has said in this thread, there
> is an argument to be made for timing information in DT.
> 
> Maybe most vendors have now learned that flexible clock frequencies
> are needed to generate the correct timings, but I have worked on parts
> without fine resolution (i.e. a dedicated pll). Even HiKey with HDMI
> suffers from this. In that case you can be tuning the timings
> (increasing the h and/or v blanks) to get the desired refresh rates.
> So in that case, the panel compatible would not determine the display
> timings and you could have the same panel with different timings on
> different boards. That's maybe rare enough that putting timing info in
> DT still wouldn't make sense.

There's another mechanism, though not very popular (possibly because it
is indeed a very rare usecase), that we introduced a while ago. The idea
is that we don't specify the mode per panel, as we do now, but rather a
range of values that are valid for the various video timing parameters.
These set can usually be copied from some datasheet and are represented
by (minimum, typical, maximum) triplets. So drivers for display hardware
would query the timings in this way, try to establish a working mode
from the typical values and tweak as necessary by adjusting the values
within the limits given by (minimum, maximum).

One disadvantage of this is that it becomes somewhat more complicated to
write the display driver, but I think that's an easy problem to solve by
adding some common helpers (I'm thinking one that takes as input the
target clock frequency and the timing ranges and outputs a mode with the
porches adjusted to match the target clock frequency as closely as
possible).

The big advantage is, of course, that device trees become trivial to
write and people don't have to manually stitch together timings to get
to the desired result.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]