Hi,
(CC'ing the linux-media mailing list, as video modes are of interest there as
well)
On Wednesday 19 September 2012 12:19:22 Tomi Valkeinen wrote:
On Wed, 2012-09-19 at 10:20 +0200, Steffen Trumtrar wrote:
quoted
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.hauer@pengutronix.de>
Signed-off-by: Steffen Trumtrar <redacted>
---
Hi!
changes since v3:
- print error messages
- free alloced memory
- general cleanup
Regards
Steffen
.../devicetree/bindings/video/displaymode | 74 +++++
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 283 +++++++++++++++
include/linux/of_videomode.h | 56 ++++
5 files changed, 419 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 0000000..990ca52
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,74 @@
+videomode bindings
+=========
+
+Required properties:
165 more quoted lines
I agree. We should flag errors and fail instead of trying to fix them
silently. That's the only way to make sure that device tree authors will get
it right (or at least less wrong). We might need to accept wrong DT data later
to support buggy devices found in the wild, but we should not start that way.
And I think the above also trashes memory if there happens to be 4+ cells.
quoted
+}
+
+static int of_video_free(struct display *disp)
+{
+ int i;
+
+ for (i=0; i<disp->num_modes; i++)
Spaces around = and < please.
quoted
+ kfree(disp->modes[i]);
+ kfree(disp->modes);
+
+ return 0;
+}
+
+int videomode_to_display_mode(struct display *disp, struct
drm_display_mode *dmode, int index)
Shouldn't this use drm_mode_modeinfo instead of drm_display_mode ?
quoted
+{
+ struct videomode *vm;
+
+ memset(dmode, 0, sizeof(*dmode));
+
+ if (index > disp->num_modes) {
+ pr_err("%s: wrong index: %d from %d\n", __func__, index,
disp->num_modes); + return -EINVAL;
+ }
+
+ vm = disp->modes[index];
+
+ dmode->hdisplay = of_video_get_value(&vm->hactive);
+ dmode->hsync_start = of_video_get_value(&vm->hactive) +
of_video_get_value(&vm->hfront_porch);
You could replace of_video_get_value(&vm->hactive) with dmode->hdisplay here,
and similarly below (hsync_end = hsync_start + of_video_get_value(&vm-
hsync_len), ...). Beside shortening lines, it would save calls to
of_video_get_value().
quoted
+ dmode->hsync_end = of_video_get_value(&vm->hactive) +
of_video_get_value(&vm->hfront_porch)
+ + of_video_get_value(&vm->hsync_len);
+ dmode->htotal = of_video_get_value(&vm->hactive) +
of_video_get_value(&vm->hfront_porch)
+ + of_video_get_value(&vm->hsync_len) +
of_video_get_value(&vm->hback_porch);
+ dmode->vdisplay = of_video_get_value(&vm->vactive);
+ dmode->vsync_start = of_video_get_value(&vm->vactive) +
of_video_get_value(&vm->vfront_porch); + dmode->vsync_end > > of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
+ + of_video_get_value(&vm->vsync_len);
+ dmode->vtotal = of_video_get_value(&vm->vactive) +
of_video_get_value(&vm->vfront_porch) +
+ of_video_get_value(&vm->vsync_len) +
of_video_get_value(&vm->vback_porch); +
+ dmode->width_mm = disp->width_mm;
+ dmode->height_mm = disp->height_mm;
+
+ dmode->clock = of_video_get_value(&vm->clock) / 1000;
+
+ if (vm->hah)
+ dmode->flags |= DRM_MODE_FLAG_PHSYNC;
+ else
+ dmode->flags |= DRM_MODE_FLAG_NHSYNC;
+ if (vm->vah)
+ dmode->flags |= DRM_MODE_FLAG_PVSYNC;
+ else
+ dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+ if (vm->interlaced)
+ dmode->flags |= DRM_MODE_FLAG_INTERLACE;
+ if (vm->doublescan)
+ dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
+
+ drm_mode_set_name(dmode);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_display_mode);
+
+int videomode_to_fb_mode(struct display *disp, struct fb_videomode
1 more quoted lines
disp is only used to retrieve the mode, you could pass a struct videomode
instead of disp and index.
Thinking about it, I would make videomode_to_display_mode take a struct
videomode as well. The only reason it needs struct display is to get the
display physical dimensions. Those are only used in very specific cases in the
DRM subsystem, I don't think they should be copied to every struct
drm_mode_modeinfo.
quoted
+{
+ struct videomode *vm;
+
+ memset(fbmode, 0, sizeof(*fbmode));
+
+ if (index > disp->num_modes) {
+ pr_err("%s: wrong index: %d from %d\n", __func__, index,
disp->num_modes);
+ return -EINVAL;
+ }
+
+ vm = disp->modes[index];
+
+ fbmode->xres = of_video_get_value(&vm->hactive);
+ fbmode->left_margin = of_video_get_value(&vm->hback_porch);
+ fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
+ fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
+
+ fbmode->yres = of_video_get_value(&vm->vactive);
+ fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
+ fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
+ fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
+
+ fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
+
+ if (vm->hah)
+ fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
+ if (vm->vah)
+ fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+ if (vm->interlaced)
+ fbmode->vmode |= FB_VMODE_INTERLACED;
+ if (vm->doublescan)
+ fbmode->vmode |= FB_VMODE_DOUBLE;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
+
+int of_get_video_modes(struct device_node *np, struct display *disp)
+{
150 more quoted lines
What are those two functions used for ? Aren't drivers expected to parse the
modes into a struct display and then operate on that structure, instead of
getting individual modes from the DT node ?
quoted
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..5571ce3
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,56 @@
+/*
+ * Copyright 2012 Sascha Hauer <s.hauer@pengutronix.de>
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * OF helpers for videomodes.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+#define OF_MODE_SELECTION -1
+
+struct mode_property {
+ u32 min;
+ u32 typ;
+ u32 max;
+};
+
+struct display {
+ u32 width_mm;
+ u32 height_mm;
+ struct videomode **modes;
+ int default_mode;
+ int num_modes;
default_mode and num_modes are non-negative, what about using an unsigned int
type for them ?
quoted
+};
+
+/* describe videomode in terms of hardware parameters */
+struct videomode {
+ struct mode_property hback_porch;
+ struct mode_property hfront_porch;
+ struct mode_property hactive;
+ struct mode_property hsync_len;
+
+ struct mode_property vback_porch;
+ struct mode_property vfront_porch;
+ struct mode_property vactive;
+ struct mode_property vsync_len;
+
+ struct mode_property clock;
+
+ bool hah;
+ bool vah;
+ bool interlaced;
+ bool doublescan;
+};
I think the names display and videomode are a bit too generic here, and
could clash with names from kernel drivers. Also, the videomode is not
really a videomode, but a "template" (or something) for videomode. A
real videomode doesn't have min & max values, only the actual value.
Perhaps these should be prefixed with "of_"? Then again, they are not
really of specific either...
If feel this is an important topic.
A generic video mode structure is definitely needed, as well as helper
functions to convert between the new structure and existing video mode
structures (struct fb_videomode, struct drm_mode_modeinfo and struct
v4l2_bt_timings). The structure and the helper functions should be generic, as
the goal is to gradually replace subsystem-specific video mode structures
where possible (userspace APIs will still need to keep the old structures). We
thus need to drop the dependency on OF for everything but the DT parsing code.
For that reason an of_ prefix wouldn't be a good idea. As the goal is to
create a truly generic video mode structure, a generic name is a good idea (I
might prefer struct video_mode instead of struct videomode, but that's just
bikeshedding).
We might need two video mode structures, one to represent a video mode, and
one to represent a range of video modes. I don't have enough experience with
video modes to have a really strong opinion on this, having a single structure
to represent both could be useful as well. This particular topic needs to be
discussed.
--
Regards,
Laurent Pinchart