[RFC PATCH 0/2] drm/panel: add simple-panel description using DT

[RFC PATCH 0/2] drm/panel: add simple-panel description using DT

[Boris BREZILLON]

Hello Thierry,

I noticed you're describing each new panel with a new entry in the
of_platform_match table and a new compatible string.
I guess you have a good reason to do it this way, because retrieving
panel description from DT would be pretty easy (see this series ;-)).

Could tell me why you chose this approach ?

Best Regards,

Boris

Boris BREZILLON (2):
  drm/panel: add support for simple-panel description definition using
    DT
  drm/panel: update simple-panel DT bindings doc with panel desc
    properties

 .../devicetree/bindings/panel/simple-panel.txt     | 34 +++++++++++
 drivers/gpu/drm/panel/Kconfig                      |  1 +
 drivers/gpu/drm/panel/panel-simple.c               | 70 ++++++++++++++++++++++
 3 files changed, 105 insertions(+)

-- 
1.8.3.2

[RFC PATCH 1/2] drm/panel: add support for simple-panel description definition using DT

[Boris BREZILLON]

Currently, the only way to add new panel descriptions to the simple panel
driver is to add new entries in the platform_of_match table.

Add support for panel description retrieval from the DT.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/gpu/drm/panel/Kconfig        |  1 +
 drivers/gpu/drm/panel/panel-simple.c | 70 ++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 4ec874d..4fe3d48 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -1,6 +1,7 @@
 config DRM_PANEL
 	bool
 	depends on DRM
+	select VIDEOMODE_HELPERS
 	help
 	  Panel registration and lookup framework.
 
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 309f29e..fcf648d 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -33,6 +33,10 @@
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_panel.h>
 
+#include <video/display_timing.h>
+#include <video/of_display_timing.h>
+#include <video/videomode.h>
+
 struct panel_desc {
 	const struct drm_display_mode *modes;
 	unsigned int num_modes;
@@ -168,6 +172,61 @@ static const struct drm_panel_funcs panel_simple_funcs = {
 	.get_modes = panel_simple_get_modes,
 };
 
+static struct panel_desc *of_panel_desc_init(struct device *dev)
+{
+	struct display_timings *timings;
+	struct panel_desc *desc;
+	u32 width, height;
+	int err;
+	int i;
+
+	err = of_property_read_u32(dev->of_node, "width", &width);
+	if (err)
+		return ERR_PTR(err);
+
+	err = of_property_read_u32(dev->of_node, "height", &height);
+	if (err)
+		return ERR_PTR(err);
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return ERR_PTR(-ENOMEM);
+
+	timings = of_get_display_timings(dev->of_node);
+	if (timings) {
+		struct drm_display_mode *modes =
+				devm_kzalloc(dev,
+					     timings->num_timings *
+					     sizeof(*desc->modes),
+					     GFP_KERNEL);
+
+		if (!modes)
+			return ERR_PTR(-ENOMEM);
+
+		for (i = 0; i < timings->num_timings; i++) {
+			struct videomode vm;
+
+			if (videomode_from_timings(timings, &vm, i))
+				break;
+
+			drm_display_mode_from_videomode(&vm, modes + i);
+
+			modes[i].type = DRM_MODE_TYPE_DRIVER;
+
+			if (timings->native_mode == i)
+				modes[i].type |= DRM_MODE_TYPE_PREFERRED;
+			desc->num_modes++;
+		}
+
+		kfree(timings);
+	}
+
+	desc->size.width = width;
+	desc->size.height = height;
+
+	return desc;
+}
+
 static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 {
 	struct device_node *backlight, *ddc;
@@ -178,6 +237,17 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	if (!panel)
 		return -ENOMEM;
 
+	if (!desc) {
+		desc = of_panel_desc_init(dev);
+		/*
+		 * Do not fail on DT panel desc retrieval error because
+		 * some systems do not need a panel description to work
+		 * properly.
+		 */
+		if (IS_ERR(desc))
+			desc = NULL;
+	}
+
 	panel->enabled = false;
 	panel->desc = desc;
 
-- 
1.8.3.2

[RFC PATCH 2/2] drm/panel: update simple-panel DT bindings doc with panel desc properties

[Boris BREZILLON]

Document simple-panel description properties and subnodes.

The display-timings definition is the one described in
Documentation/devicetree/bindings/video/display-timing.txt.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/panel/simple-panel.txt     | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt b/Documentation/devicetree/bindings/panel/simple-panel.txt
index 1341bbf..3440f78 100644
--- a/Documentation/devicetree/bindings/panel/simple-panel.txt
+++ b/Documentation/devicetree/bindings/panel/simple-panel.txt
@@ -7,6 +7,13 @@ 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
+- height: the screen height in pixels
+- width: the screen width in pixels
+
+Optional children nodes:
+- display-timings: encode the panel timings.
+  see Documentation/devicetree/bindings/video/display-timing.txt for a full
+  description.
 
 Example:
 
@@ -19,3 +26,30 @@ Example:
 
 		backlight = <&backlight>;
 	};
+
+or
+        panel: panel {
+                compatible = "simple-panel";
+		enable-gpios = <&gpio 90 0>;
+
+		backlight = <&backlight>;
+
+		width = 1920;
+		height = 1080;
+		display-timings {
+	                native-mode = <&timing0>;
+			timing0: 1080p24 {
+				/* 1920x1080p24 */
+				clock-frequency = <52000000>;
+				hactive = <1920>;
+				vactive = <1080>;
+				hfront-porch = <25>;
+				hback-porch = <25>;
+				hsync-len = <25>;
+				vback-porch = <2>;
+				vfront-porch = <2>;
+				vsync-len = <2>;
+				hsync-active = <1>;
+			};
+		};
+	};
-- 
1.8.3.2

Re: [RFC PATCH 1/2] drm/panel: add support for simple-panel description definition using DT

[Boris BREZILLON]

On 09/05/2014 16:16, Boris BREZILLON wrote:
> Currently, the only way to add new panel descriptions to the simple panel
> driver is to add new entries in the platform_of_match table.
>
> Add support for panel description retrieval from the DT.
>
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/gpu/drm/panel/Kconfig        |  1 +
>  drivers/gpu/drm/panel/panel-simple.c | 70 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 4ec874d..4fe3d48 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -1,6 +1,7 @@
>  config DRM_PANEL
>  	bool
>  	depends on DRM
> +	select VIDEOMODE_HELPERS
>  	help
>  	  Panel registration and lookup framework.
>  
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 309f29e..fcf648d 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -33,6 +33,10 @@
>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_panel.h>
>  
> +#include <video/display_timing.h>
> +#include <video/of_display_timing.h>
> +#include <video/videomode.h>
> +
>  struct panel_desc {
>  	const struct drm_display_mode *modes;
>  	unsigned int num_modes;
> @@ -168,6 +172,61 @@ static const struct drm_panel_funcs panel_simple_funcs = {
>  	.get_modes = panel_simple_get_modes,
>  };
>  
> +static struct panel_desc *of_panel_desc_init(struct device *dev)
> +{
> +	struct display_timings *timings;
> +	struct panel_desc *desc;
> +	u32 width, height;
> +	int err;
> +	int i;
> +
> +	err = of_property_read_u32(dev->of_node, "width", &width);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	err = of_property_read_u32(dev->of_node, "height", &height);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
> +	if (!desc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	timings = of_get_display_timings(dev->of_node);
> +	if (timings) {
> +		struct drm_display_mode *modes =
> +				devm_kzalloc(dev,
> +					     timings->num_timings *
> +					     sizeof(*desc->modes),
> +					     GFP_KERNEL);
> +
> +		if (!modes)
> +			return ERR_PTR(-ENOMEM);
> +
> +		for (i = 0; i < timings->num_timings; i++) {
> +			struct videomode vm;
> +
> +			if (videomode_from_timings(timings, &vm, i))
> +				break;
> +
> +			drm_display_mode_from_videomode(&vm, modes + i);
> +
> +			modes[i].type = DRM_MODE_TYPE_DRIVER;
> +
> +			if (timings->native_mode == i)
> +				modes[i].type |= DRM_MODE_TYPE_PREFERRED;
> +			desc->num_modes++;
> +		}

Oops, I forgot to add:
                          desc->modes = modes;

> +
> +		kfree(timings);
> +	}
> +
> +	desc->size.width = width;
> +	desc->size.height = height;
> +
> +	return desc;
> +}
> +
>  static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  {
>  	struct device_node *backlight, *ddc;
> @@ -178,6 +237,17 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  	if (!panel)
>  		return -ENOMEM;
>  
> +	if (!desc) {
> +		desc = of_panel_desc_init(dev);
> +		/*
> +		 * Do not fail on DT panel desc retrieval error because
> +		 * some systems do not need a panel description to work
> +		 * properly.
> +		 */
> +		if (IS_ERR(desc))
> +			desc = NULL;
> +	}
> +
>  	panel->enabled = false;
>  	panel->desc = desc;
>  

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC PATCH 0/2] drm/panel: add simple-panel description using DT

[Thierry Reding]

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

On Fri, May 09, 2014 at 04:16:40PM +0200, Boris BREZILLON wrote:
> Hello Thierry,
> 
> I noticed you're describing each new panel with a new entry in the
> of_platform_match table and a new compatible string.
> I guess you have a good reason to do it this way, because retrieving
> panel description from DT would be pretty easy (see this series ;-)).
> 
> Could tell me why you chose this approach ?

The reason is that devicetree mandates that a device be identified using
a compatible value and that compatible value should be as specific as
possible. That compatible value should give the device driver enough
information to know everything it needs (resolution, timings, physical
dimension).

Having all of that data in the device tree is redundant.

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: [RFC PATCH 2/2] drm/panel: update simple-panel DT bindings doc with panel desc properties

[Thierry Reding]

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

On Fri, May 09, 2014 at 04:16:42PM +0200, Boris BREZILLON wrote:
> Document simple-panel description properties and subnodes.
> 
> The display-timings definition is the one described in
> Documentation/devicetree/bindings/video/display-timing.txt.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  .../devicetree/bindings/panel/simple-panel.txt     | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt b/Documentation/devicetree/bindings/panel/simple-panel.txt
> index 1341bbf..3440f78 100644
> --- a/Documentation/devicetree/bindings/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt
> @@ -7,6 +7,13 @@ 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
> +- height: the screen height in pixels
> +- width: the screen width in pixels

width and height are not in pixels. They are in millimeters.

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: [RFC PATCH 0/2] drm/panel: add simple-panel description using DT

[Andrzej Hajda]

On 05/13/2014 09:51 AM, Thierry Reding wrote:
> On Fri, May 09, 2014 at 04:16:40PM +0200, Boris BREZILLON wrote:
>> Hello Thierry,
>>
>> I noticed you're describing each new panel with a new entry in the
>> of_platform_match table and a new compatible string.
>> I guess you have a good reason to do it this way, because retrieving
>> panel description from DT would be pretty easy (see this series ;-)).
>>
>> Could tell me why you chose this approach ?
> 
> The reason is that devicetree mandates that a device be identified using
> a compatible value and that compatible value should be as specific as
> possible. That compatible value should give the device driver enough
> information to know everything it needs (resolution, timings, physical
> dimension).
> 
> Having all of that data in the device tree is redundant.
> 

Many panels I have encountered have no single timings, they accepts
ranges of timings. With 'compatible' approach there is no place to
configure timings specific for particular hw configuration, unless you
abuse somehow compatible string.

However it seems there are not so many cases the same panel must be used
with different timings on different boards, anyway it is a potential issue.

Regards
Andrzej

Re: [RFC PATCH 0/2] drm/panel: add simple-panel description using DT

[Thierry Reding]

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

On Tue, May 13, 2014 at 11:09:00AM +0200, Andrzej Hajda wrote:
> On 05/13/2014 09:51 AM, Thierry Reding wrote:
> > On Fri, May 09, 2014 at 04:16:40PM +0200, Boris BREZILLON wrote:
> >> Hello Thierry,
> >>
> >> I noticed you're describing each new panel with a new entry in the
> >> of_platform_match table and a new compatible string.
> >> I guess you have a good reason to do it this way, because retrieving
> >> panel description from DT would be pretty easy (see this series ;-)).
> >>
> >> Could tell me why you chose this approach ?
> > 
> > The reason is that devicetree mandates that a device be identified using
> > a compatible value and that compatible value should be as specific as
> > possible. That compatible value should give the device driver enough
> > information to know everything it needs (resolution, timings, physical
> > dimension).
> > 
> > Having all of that data in the device tree is redundant.
> > 
> 
> Many panels I have encountered have no single timings, they accepts
> ranges of timings. With 'compatible' approach there is no place to
> configure timings specific for particular hw configuration, unless you
> abuse somehow compatible string.
> 
> However it seems there are not so many cases the same panel must be used
> with different timings on different boards, anyway it is a potential issue.

Right. I think it makes sense, and I've said so in the past, to add
support for overriding the default timings specified by the driver using
a display-timings node in DT. But that should be reserved as a means to
override the timings if that's required on some specific configuration,
not abused as a means to add support for new panels altogether.

Note that there's also the possibility to use the drm_crtc_helper_funcs'
.mode_fixup() to adjust a mode's timings if the display hardware doesn't
support the mode as-is.

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: [RFC PATCH 0/2] drm/panel: add simple-panel description using DT

[Boris BREZILLON]

On 13/05/2014 09:51, Thierry Reding wrote:
> On Fri, May 09, 2014 at 04:16:40PM +0200, Boris BREZILLON wrote:
>> Hello Thierry,
>>
>> I noticed you're describing each new panel with a new entry in the
>> of_platform_match table and a new compatible string.
>> I guess you have a good reason to do it this way, because retrieving
>> panel description from DT would be pretty easy (see this series ;-)).
>>
>> Could tell me why you chose this approach ?
> The reason is that devicetree mandates that a device be identified using
> a compatible value and that compatible value should be as specific as
> possible. That compatible value should give the device driver enough
> information to know everything it needs (resolution, timings, physical
> dimension).
>
> Having all of that data in the device tree is redundant.

Okay, thanks for your answer.

As a result, you'll see a patch adding support for the FL500WVR00-A0T
(foxlink) panel soon ;-).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com