Hi Steffen,
Thanks for the patch.
On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> Get videomode from devicetree in a format appropriate for the
> backend. drm_display_mode and fb_videomode are supported atm.
> Uses the display signal timings from of_display_timings
>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> ---
> drivers/of/Kconfig | 5 +
> drivers/of/Makefile | 1 +
> drivers/of/of_videomode.c | 212 +++++++++++++++++++++++++++++++++++++++
> include/linux/of_videomode.h | 41 ++++++++
> 4 files changed, 259 insertions(+)
> create mode 100644 drivers/of/of_videomode.c
> create mode 100644 include/linux/of_videomode.h
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 646deb0..74282e2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
> help
> helper to parse display timings from the devicetree
>
> +config OF_VIDEOMODE
> + def_bool y
> + help
> + helper to get videomodes from the devicetree
> +
> endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c8e9603..09d556f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,3 +12,4 @@ 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_DISPLAY_TIMINGS) += of_display_timings.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 0000000..76ac16e
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,212 @@
> +/*
> + * generic videomode helper
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>,
> Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/fb.h>
> +#include <linux/slab.h>
> +#include <drm/drm_mode.h>
> +#include <linux/of_display_timings.h>
> +#include <linux/of_videomode.h>
> +
> +void dump_fb_videomode(struct fb_videomode *m)
> +{
> + pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
That's going to be pretty difficult to read :-) Would it make sense to group
several attributes logically (for instance using %ux%u for m->xres, m->yres) ?
> + m->refresh, m->xres, m->yres, m->pixclock, m->left_margin,
> + m->right_margin, m->upper_margin, m->lower_margin, +
> m->hsync_len, m->vsync_len, m->sync, m->vmode, m->flag);
> +}
Shouldn't this (and the other non exported functions below) be static ?
> +void dump_drm_displaymode(struct drm_display_mode *m)
> +{
> + pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
> + m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
> + m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
> + m->clock);
> +}
> +
> +int videomode_from_timing(struct display_timings *disp, struct videomode
> *vm,
> + int index)
> +{
> + struct signal_timing *st = NULL;
> +
> + if (!vm)
> + return -EINVAL;
> +
What about making vm a mandatory argument ? It looks to me like a caller bug
if vm is NULL.
> + st = display_timings_get(disp, index);
> +
You can remove the blank line.
> + if (!st) {
> + pr_err("%s: no signal timings found\n", __func__);
> + return -EINVAL;
> + }
> +
> + vm->pixelclock = signal_timing_get_value(&st->pixelclock, 0);
> + vm->hactive = signal_timing_get_value(&st->hactive, 0);
> + vm->hfront_porch = signal_timing_get_value(&st->hfront_porch, 0);
> + vm->hback_porch = signal_timing_get_value(&st->hback_porch, 0);
> + vm->hsync_len = signal_timing_get_value(&st->hsync_len, 0);
> +
> + vm->vactive = signal_timing_get_value(&st->vactive, 0);
> + vm->vfront_porch = signal_timing_get_value(&st->vfront_porch, 0);
> + vm->vback_porch = signal_timing_get_value(&st->vback_porch, 0);
> + vm->vsync_len = signal_timing_get_value(&st->vsync_len, 0);
> +
> + vm->vah = st->vsync_pol_active_high;
> + vm->hah = st->hsync_pol_active_high;
> + vm->interlaced = st->interlaced;
> + vm->doublescan = st->doublescan;
> +
> + return 0;
> +}
> +
> +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> index)
I wonder how to avoid abuse of this functions. It's a useful helper for
drivers that need to get a video mode once only, but would result in lower
performances if a driver calls it for every mode. Drivers must call
of_get_display_timing_list instead in that case and case the display timings.
I'm wondering whether we should really expose of_get_videomode.
> +{
> + struct display_timings *disp;
> + int ret = 0;
No need to assign ret to 0 here.
> +
> + disp = of_get_display_timing_list(np);
> +
You can remove the blank line.
> + if (!disp) {
> + pr_err("%s: no timings specified\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (index == OF_DEFAULT_TIMING)
> + index = disp->default_timing;
> +
> + ret = videomode_from_timing(disp, vm, index);
> +
No need for a blank line.
> + if (ret)
> + return ret;
> +
> + display_timings_release(disp);
> +
> + if (!vm) {
> + pr_err("%s: could not get videomode %d\n", __func__, index);
> + return -EINVAL;
> + }
This can't happen. If vm is NULL the videomode_from_timing call above will
return -EINVAL, and this function will then return immediately without
reaching this code block.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_videomode);
> +
> +#if defined(CONFIG_DRM)
> +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode
> *dmode)
> +{
> + memset(dmode, 0, sizeof(*dmode));
> +
> + dmode->hdisplay = vm->hactive;
> + dmode->hsync_start = dmode->hdisplay + vm->hfront_porch;
> + dmode->hsync_end = dmode->hsync_start + vm->hsync_len;
> + dmode->htotal = dmode->hsync_end + vm->hback_porch;
> +
> + dmode->vdisplay = vm->vactive;
> + dmode->vsync_start = dmode->vdisplay + vm->vfront_porch;
> + dmode->vsync_end = dmode->vsync_start + vm->vsync_len;
> + dmode->vtotal = dmode->vsync_end + vm->vback_porch;
> +
> + dmode->clock = vm->pixelclock / 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 of_get_drm_display_mode(struct device_node *np, struct drm_display_mode
> *dmode,
> + int index)
Same as above, do we really need to expose this helper function ? If so we
should at least clearly document (using kerneldoc for instance) that drivers
should only use it if they need to get a single mode once.
> +{
> + struct videomode vm;
> + int ret;
> +
> + ret = of_get_videomode(np, &vm, index);
> +
> + if (ret)
> + return ret;
> +
> + videomode_to_display_mode(&vm, dmode);
> +
> + pr_info("%s: got %dx%d display mode from %s\n", __func__, vm.hactive,
> + vm.vactive, np->name);
> + dump_drm_displaymode(dmode);
> +
> + return 0;
> +
> +}
> +EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
> +#else
> +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode
> *dmode)
> +{
> + return 0;
> +}
> +
> +int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode
> *dmode,
> + int index)
> +{
> + return 0;
> +}
What about not defining those if CONFIG_DRM is not set ? No driver should call
these functions in that case. If we really need those stubs they should return
an error.
> +#endif
> +
> +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode
> *fbmode)
> +{
> + memset(fbmode, 0, sizeof(*fbmode));
> +
> + fbmode->xres = vm->hactive;
> + fbmode->left_margin = vm->hback_porch;
> + fbmode->right_margin = vm->hfront_porch;
> + fbmode->hsync_len = vm->hsync_len;
> +
> + fbmode->yres = vm->vactive;
> + fbmode->upper_margin = vm->vback_porch;
> + fbmode->lower_margin = vm->vfront_porch;
> + fbmode->vsync_len = vm->vsync_len;
> +
> + fbmode->pixclock = KHZ2PICOS(vm->pixelclock) / 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_videomode);
> +
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
> + int index)
> +{
> + struct videomode vm;
> + int ret;
> +
> + ret = of_get_videomode(np, &vm, index);
> + if (ret)
> + return ret;
> +
> + videomode_to_fb_videomode(&vm, fb);
> +
> + pr_info("%s: got %dx%d display mode from %s\n", __func__, vm.hactive,
> + vm.vactive, np->name);
> + dump_fb_videomode(fb);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..96efe01
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> + *
> + * generic videomode description
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_VIDEOMODE_H
> +#define __LINUX_VIDEOMODE_H
> +
> +#include <drm/drmP.h>
> +
> +struct videomode {
> + u32 pixelclock;
> + u32 refreshrate;
> +
> + u32 hactive;
> + u32 hfront_porch;
> + u32 hback_porch;
> + u32 hsync_len;
> +
> + u32 vactive;
> + u32 vfront_porch;
> + u32 vback_porch;
> + u32 vsync_len;
> +
> + bool hah;
> + bool vah;
> + bool interlaced;
> + bool doublescan;
> +
> +};
> +
> +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode
> *dmode); +int videomode_to_fb_videomode(struct videomode *vm, struct
> fb_videomode *fbmode); +int of_get_videomode(struct device_node *np, struct
> videomode *vm, int index); +int of_get_drm_display_mode(struct device_node
> *np, struct drm_display_mode *dmode, + int index);
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
> int index); +#endif /* __LINUX_VIDEOMODE_H */
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
