Re: [PATCH v3] of: Add videomode helper
On Fri, Sep 14, 2012 at 11:24 AM, Steffen Trumtrar
s.trumt...@pengutronix.de wrote:
+/* FIXME */
+static u32 of_video_get_value(struct mode_property *prop)
+{
+ return (prop-min = prop-typ) ? prop-min : prop-typ;
+}
+
+/* read property into new mode_property */
+static int of_video_parse_property(struct device_node *np, char *name,
+ struct mode_property *result)
+{
+ struct property *prop;
+ int length;
+ int cells;
+ int ret;
+
+ prop = of_find_property(np, name, length);
+ if (!prop)
+ return -EINVAL;
+
+ cells = length / sizeof(u32);
+
+ memset(result, 0, sizeof(*result));
+
+ ret = of_property_read_u32_array(np, name, result-min, cells);
+ of_video_get_value(result);
What's the point of calling of_video_get_value() here? It doesn't do anything.
+ return ret;
+}
+
+int videomode_to_display_mode(struct display *disp, struct drm_display_mode
*dmode, int index)
+{
+ struct videomode *vm;
+
+memset(dmode, 0, sizeof(*dmode));
Indentation problem.
+int of_get_video_mode(struct device_node *np, struct display *disp)
+{
+ struct device_node *display_np;
+ struct device_node *mode_np;
+ struct device_node *modes_np;
+ char *default_mode;
+
+ int ret = 0;
+
+ memset(disp, 0, sizeof(*disp));
+ disp-modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
+ if (!np)
+ return -EINVAL;
+
+ display_np = of_parse_phandle(np, display, 0);
+
+ if (!display_np)
+ return -EINVAL;
+
+ of_property_read_u32(display_np, width-mm, disp-width_mm);
+ of_property_read_u32(display_np, height-mm, disp-height_mm);
+
+ mode_np = of_parse_phandle(np, default-mode, 0);
+
+ if (!mode_np)
+ mode_np = of_find_node_by_name(np, mode);
+
+ if (!mode_np)
+ return -EINVAL;
+
+ default_mode = (char *)mode_np-full_name;
+
+ modes_np = of_find_node_by_name(np, modes);
+ for_each_child_of_node(modes_np, mode_np) {
+ struct videomode *vm;
Blank line after variable declarations, please.
+ vm = kmalloc(sizeof(struct videomode), GFP_KERNEL);
+ disp-modes[disp-num_modes] = kmalloc(sizeof(struct
videomode*), GFP_KERNEL);
Are you sure this is right?
+ ret |= of_video_parse_property(mode_np, hback-porch,
vm-hback_porch);
+ ret |= of_video_parse_property(mode_np, hfront-porch,
vm-hfront_porch);
+ ret |= of_video_parse_property(mode_np, hactive,
vm-hactive);
+ ret |= of_video_parse_property(mode_np, hsync-len,
vm-hsync_len);
+ ret |= of_video_parse_property(mode_np, vback-porch,
vm-vback_porch);
+ ret |= of_video_parse_property(mode_np, vfront-porch,
vm-vfront_porch);
+ ret |= of_video_parse_property(mode_np, vactive,
vm-vactive);
+ ret |= of_video_parse_property(mode_np, vsync-len,
vm-vsync_len);
+ ret |= of_video_parse_property(mode_np, clock, vm-clock);
+
+ if (ret)
+ return -EINVAL;
+
+ vm-hah = of_property_read_bool(mode_np, hsync-active-high);
+ vm-vah = of_property_read_bool(mode_np, vsync-active-high);
+ vm-interlaced = of_property_read_bool(mode_np, interlaced);
+ vm-doublescan = of_property_read_bool(mode_np, doublescan);
+
+ if (strcmp(default_mode,mode_np-full_name) == 0) {
+ printk(%s: default_node = %d\n, __func__,
disp-num_modes);
Please use a KERN_ macro here, or a pr_xxx function. Even better
would be to use a dev_xxx function.
In general, I'd like to see more error reporting of bad device tree
properties, to help debugging.
+ disp-default_mode = disp-num_modes;
+ }
+
+ disp-modes[disp-num_modes] = vm;
Isn't this a memory leak?
+ disp-num_modes++;
+ }
+ of_node_put(display_np);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_video_mode);
--
Timur Tabi
Linux kernel developer at Freescale
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3] of: Add videomode helper
On Mon, Sep 17, 2012 at 06:35:43PM +, Tabi Timur-B04825 wrote:
On Fri, Sep 14, 2012 at 11:24 AM, Steffen Trumtrar
s.trumt...@pengutronix.de wrote:
+/* FIXME */
+static u32 of_video_get_value(struct mode_property *prop)
+{
+ return (prop-min = prop-typ) ? prop-min : prop-typ;
+}
+
+/* read property into new mode_property */
+static int of_video_parse_property(struct device_node *np, char *name,
+ struct mode_property *result)
+{
+ struct property *prop;
+ int length;
+ int cells;
+ int ret;
+
+ prop = of_find_property(np, name, length);
+ if (!prop)
+ return -EINVAL;
+
+ cells = length / sizeof(u32);
+
+ memset(result, 0, sizeof(*result));
+
+ ret = of_property_read_u32_array(np, name, result-min, cells);
+ of_video_get_value(result);
What's the point of calling of_video_get_value() here? It doesn't do
anything.
You're right. That definitely does not belong there.
+ return ret;
+}
+
+int videomode_to_display_mode(struct display *disp, struct
drm_display_mode *dmode, int index)
+{
+ struct videomode *vm;
+
+memset(dmode, 0, sizeof(*dmode));
Indentation problem.
Okay.
+int of_get_video_mode(struct device_node *np, struct display *disp)
+{
+ struct device_node *display_np;
+ struct device_node *mode_np;
+ struct device_node *modes_np;
+ char *default_mode;
+
+ int ret = 0;
+
+ memset(disp, 0, sizeof(*disp));
+ disp-modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
+ if (!np)
+ return -EINVAL;
+
+ display_np = of_parse_phandle(np, display, 0);
+
+ if (!display_np)
+ return -EINVAL;
+
+ of_property_read_u32(display_np, width-mm, disp-width_mm);
+ of_property_read_u32(display_np, height-mm, disp-height_mm);
+
+ mode_np = of_parse_phandle(np, default-mode, 0);
+
+ if (!mode_np)
+ mode_np = of_find_node_by_name(np, mode);
+
+ if (!mode_np)
+ return -EINVAL;
+
+ default_mode = (char *)mode_np-full_name;
+
+ modes_np = of_find_node_by_name(np, modes);
+ for_each_child_of_node(modes_np, mode_np) {
+ struct videomode *vm;
Blank line after variable declarations, please.
+ vm = kmalloc(sizeof(struct videomode), GFP_KERNEL);
+ disp-modes[disp-num_modes] = kmalloc(sizeof(struct
videomode*), GFP_KERNEL);
Are you sure this is right?
I implemented disp-modes as struct videomode** modes. So I guess the first
kmalloc is wrong. Right?!
+ ret |= of_video_parse_property(mode_np, hback-porch,
vm-hback_porch);
+ ret |= of_video_parse_property(mode_np, hfront-porch,
vm-hfront_porch);
+ ret |= of_video_parse_property(mode_np, hactive,
vm-hactive);
+ ret |= of_video_parse_property(mode_np, hsync-len,
vm-hsync_len);
+ ret |= of_video_parse_property(mode_np, vback-porch,
vm-vback_porch);
+ ret |= of_video_parse_property(mode_np, vfront-porch,
vm-vfront_porch);
+ ret |= of_video_parse_property(mode_np, vactive,
vm-vactive);
+ ret |= of_video_parse_property(mode_np, vsync-len,
vm-vsync_len);
+ ret |= of_video_parse_property(mode_np, clock,
vm-clock);
+
+ if (ret)
+ return -EINVAL;
+
+ vm-hah = of_property_read_bool(mode_np,
hsync-active-high);
+ vm-vah = of_property_read_bool(mode_np,
vsync-active-high);
+ vm-interlaced = of_property_read_bool(mode_np,
interlaced);
+ vm-doublescan = of_property_read_bool(mode_np,
doublescan);
+
+ if (strcmp(default_mode,mode_np-full_name) == 0) {
+ printk(%s: default_node = %d\n, __func__,
disp-num_modes);
Please use a KERN_ macro here, or a pr_xxx function. Even better
would be to use a dev_xxx function.
In general, I'd like to see more error reporting of bad device tree
properties, to help debugging.
Okay. Actually, the printk also was not supposed to be in the final patch.
I can fix that and add some dev_xxx.
+ disp-default_mode = disp-num_modes;
+ }
+
+ disp-modes[disp-num_modes] = vm;
Isn't this a memory leak?
I think I get you. I will fix that.
+ disp-num_modes++;
+ }
+ of_node_put(display_np);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_video_mode);
Thank you for your review.
Regards,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions |
[PATCH v3] of: Add videomode helper
This patch adds a helper function for parsing videomodes from the devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
---
Hi!
The original patch was done by Sascha Hauer. I reworked his v2 a little bit.
Changes since v2:
- use hardware-near property-names
- provide a videomode structure
- allow ranges for all properties (min,typ,max)
- functions to get display_mode or fb_videomode
Regards,
Steffen
.../devicetree/bindings/video/displaymode | 74 ++
drivers/of/Kconfig |5 +
drivers/of/Makefile|1 +
drivers/of/of_videomode.c | 236
include/linux/of_videomode.h | 56 +
5 files changed, 372 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/displaymode
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode
new file mode 100644
index 000..990ca52
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,74 @@
+videomode bindings
+==
+
+Required properties:
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
+ in pixels
+ vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
+ lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree
representation
+corresponds to the one commonly found in datasheets for displays.
+The description of the display and its mode is split in two parts: first the
display
+properties like size in mm and (optionally) multiple subnodes with the
supported modes.
+
+Example:
+
+ display@0 {
+ width-mm = 800;
+ height-mm = 480;
+ modes {
+ mode0: mode@0 {
+ /* 1920x1080p24 */
+ clock = 5200;
+ hactive = 1920;
+ vactive = 1080;
+ hfront-porch = 25;
+ hback-porch = 25;
+ hsync-len = 25;
+ vback-porch = 2;
+ vfront-porch = 2;
+ vsync-len = 2;
+ hsync-active-high;
+ };
+ };
+ };
+
+Every property also supports the use of ranges, so the commonly used datasheet
+description with min typ max-tuples can be used.
+
+Example:
+
+ mode1: mode@1 {
+ /* 1920x1080p24 */
+ clock = 14850;
+ hactive = 1920;
+ vactive = 1080;
+ hsync-len = 0 44 60;
+ hfront-porch = 80 88 95;
+ hback-porch = 100 148 160;
+ vfront-porch = 0 4 6;
+ vback-porch = 0 36 50;
+ vsync-len = 0 5 6;
+ };
+
+The videomode can be linked to a connector via phandles. The connector has to
+support the display- and default-mode-property to link to and select a mode.
+
+Example:
+
+ hdmi@0012 {
+ status = okay;
+ display = benq;
+ default-mode = mode1;
+ };
+
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..a3acaa3 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,9 @@ config OF_MTD
depends on MTD
def_bool y
+config OF_VIDEOMODE
+ def_bool y
+ help
+ helper to parse videomodes from the devicetree
+
endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..80e6db3 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
obj-$(CONFIG_OF_PCI) += of_pci.o
obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
obj-$(CONFIG_OF_MTD) += of_mtd.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 000..217edf8
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,236 @@
+/*
+ * OF helpers for parsing display modes
+ *
+ * Copyright (c) 2012 Sascha Hauer s.ha...@pengutronix.de, Pengutronix
+ * Copyright (c) 2012 Steffen Trumtrar
