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
> 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
Re: [PATCH v3] of: Add videomode helper
On Fri, Sep 14, 2012 at 11:24 AM, Steffen Trumtrar
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
[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
Signed-off-by: Steffen Trumtrar
---
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 ()
- 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 -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 , Pengutronix
+ * Copyright (c) 2012 Steffen Trumtrar , Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
