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(+)
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>; + }; + }; + };
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;
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
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;
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
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
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
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
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Boris BREZILLON -
Thierry Reding