This patch simplifies videomode related Kconfig and Makefile. After this patch, there's only one non-user selectable Kconfig option left, VIDEOMODE_HELPERS. The reasons for the change:
* Videomode helper functions are not something that should be shown in the kernel configuration options. The related code should just be included if it's needed, i.e. selected by drivers using videomode.
* There's no need to have separate Kconfig options for videomode and display_timing. First of all, the amount of code for both is quite small. Second, videomode depends on display_timing, and display_timing in itself is not really useful, so both would be included in any case.
* CONFIG_VIDEOMODE is a bit vague name, and CONFIG_VIDEOMODE_HELPERS describes better what's included.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/gpu/drm/drm_modes.c | 8 ++++---- drivers/gpu/drm/tilcdc/Kconfig | 3 +-- drivers/video/Kconfig | 22 ++-------------------- drivers/video/Makefile | 8 ++++---- drivers/video/fbmon.c | 8 ++++---- 5 files changed, 15 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 04fa6f1..0698c0e 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -506,7 +506,7 @@ drm_gtf_mode(struct drm_device *dev, int hdisplay, int vdisplay, int vrefresh, } EXPORT_SYMBOL(drm_gtf_mode);
-#if IS_ENABLED(CONFIG_VIDEOMODE) +#ifdef CONFIG_VIDEOMODE_HELPERS int drm_display_mode_from_videomode(const struct videomode *vm, struct drm_display_mode *dmode) { @@ -540,9 +540,8 @@ int drm_display_mode_from_videomode(const struct videomode *vm, return 0; } EXPORT_SYMBOL_GPL(drm_display_mode_from_videomode); -#endif
-#if IS_ENABLED(CONFIG_OF_VIDEOMODE) +#ifdef CONFIG_OF /** * of_get_drm_display_mode - get a drm_display_mode from devicetree * @np: device_node with the timing specification @@ -572,7 +571,8 @@ int of_get_drm_display_mode(struct device_node *np, return 0; } EXPORT_SYMBOL_GPL(of_get_drm_display_mode); -#endif +#endif /* CONFIG_OF */ +#endif /* CONFIG_VIDEOMODE_HELPERS */
/** * drm_mode_set_name - set the name on a mode diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig index d24d040..e461e99 100644 --- a/drivers/gpu/drm/tilcdc/Kconfig +++ b/drivers/gpu/drm/tilcdc/Kconfig @@ -4,8 +4,7 @@ config DRM_TILCDC select DRM_KMS_HELPER select DRM_KMS_CMA_HELPER select DRM_GEM_CMA_HELPER - select OF_VIDEOMODE - select OF_DISPLAY_TIMING + select VIDEOMODE_HELPERS select BACKLIGHT_CLASS_DEVICE help Choose this option if you have an TI SoC with LCDC display diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 4c1546f..2a81b11 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -31,26 +31,8 @@ config VIDEO_OUTPUT_CONTROL This framework adds support for low-level control of the video output switch.
-config DISPLAY_TIMING - bool - -config VIDEOMODE - bool - -config OF_DISPLAY_TIMING - bool "Enable device tree display timing support" - depends on OF - select DISPLAY_TIMING - help - helper to parse display timings from the devicetree - -config OF_VIDEOMODE - bool "Enable device tree videomode support" - depends on OF - select VIDEOMODE - select OF_DISPLAY_TIMING - help - helper to get videomodes from the devicetree +config VIDEOMODE_HELPERS + bool
config HDMI bool diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 9df3873..e414378 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -171,7 +171,7 @@ obj-$(CONFIG_FB_VIRTUAL) += vfb.o
#video output switch sysfs driver obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o -obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o -obj-$(CONFIG_OF_DISPLAY_TIMING) += of_display_timing.o -obj-$(CONFIG_VIDEOMODE) += videomode.o -obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o +obj-$(CONFIG_VIDEOMODE_HELPERS) += display_timing.o videomode.o +ifeq ($(CONFIG_OF),y) +obj-$(CONFIG_VIDEOMODE_HELPERS) += of_display_timing.o of_videomode.o +endif diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index 94ad0f7..368cedf 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -1376,7 +1376,7 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf return err; }
-#if IS_ENABLED(CONFIG_VIDEOMODE) +#ifdef CONFIG_VIDEOMODE_HELPERS int fb_videomode_from_videomode(const struct videomode *vm, struct fb_videomode *fbmode) { @@ -1424,9 +1424,8 @@ int fb_videomode_from_videomode(const struct videomode *vm, return 0; } EXPORT_SYMBOL_GPL(fb_videomode_from_videomode); -#endif
-#if IS_ENABLED(CONFIG_OF_VIDEOMODE) +#ifdef CONFIG_OF static inline void dump_fb_videomode(const struct fb_videomode *m) { pr_debug("fb_videomode = %ux%u@%uHz (%ukHz) %u %u %u %u %u %u %u %u %u\n", @@ -1465,7 +1464,8 @@ int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, return 0; } EXPORT_SYMBOL_GPL(of_get_fb_videomode); -#endif +#endif /* CONFIG_OF */ +#endif /* CONFIG_VIDEOMODE_HELPERS */
#else int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
Both videomode and display_timing contain flags describing the modes. These are stored in dmt_flags and data_flags. There's no need to separate these flags, and having separate fields just makes the flags more difficult to use.
This patch combines the fields and renames VESA_DMT_* flags to DISPLAY_FLAGS_*.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/gpu/drm/drm_modes.c | 12 ++++++------ drivers/video/fbmon.c | 8 ++++---- drivers/video/of_display_timing.c | 19 +++++++++---------- drivers/video/videomode.c | 3 +-- include/video/display_timing.h | 26 +++++++++++--------------- include/video/videomode.h | 3 +-- 6 files changed, 32 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 0698c0e..f83f071 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -523,17 +523,17 @@ int drm_display_mode_from_videomode(const struct videomode *vm, dmode->clock = vm->pixelclock / 1000;
dmode->flags = 0; - if (vm->dmt_flags & VESA_DMT_HSYNC_HIGH) + if (vm->flags & DISPLAY_FLAGS_HSYNC_HIGH) dmode->flags |= DRM_MODE_FLAG_PHSYNC; - else if (vm->dmt_flags & VESA_DMT_HSYNC_LOW) + else if (vm->flags & DISPLAY_FLAGS_HSYNC_LOW) dmode->flags |= DRM_MODE_FLAG_NHSYNC; - if (vm->dmt_flags & VESA_DMT_VSYNC_HIGH) + if (vm->flags & DISPLAY_FLAGS_VSYNC_HIGH) dmode->flags |= DRM_MODE_FLAG_PVSYNC; - else if (vm->dmt_flags & VESA_DMT_VSYNC_LOW) + else if (vm->flags & DISPLAY_FLAGS_VSYNC_LOW) dmode->flags |= DRM_MODE_FLAG_NVSYNC; - if (vm->data_flags & DISPLAY_FLAGS_INTERLACED) + if (vm->flags & DISPLAY_FLAGS_INTERLACED) dmode->flags |= DRM_MODE_FLAG_INTERLACE; - if (vm->data_flags & DISPLAY_FLAGS_DOUBLESCAN) + if (vm->flags & DISPLAY_FLAGS_DOUBLESCAN) dmode->flags |= DRM_MODE_FLAG_DBLSCAN; drm_mode_set_name(dmode);
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index 368cedf..e5cc2fd 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -1398,13 +1398,13 @@ int fb_videomode_from_videomode(const struct videomode *vm,
fbmode->sync = 0; fbmode->vmode = 0; - if (vm->dmt_flags & VESA_DMT_HSYNC_HIGH) + if (vm->flags & DISPLAY_FLAGS_HSYNC_HIGH) fbmode->sync |= FB_SYNC_HOR_HIGH_ACT; - if (vm->dmt_flags & VESA_DMT_HSYNC_HIGH) + if (vm->flags & DISPLAY_FLAGS_HSYNC_HIGH) fbmode->sync |= FB_SYNC_VERT_HIGH_ACT; - if (vm->data_flags & DISPLAY_FLAGS_INTERLACED) + if (vm->flags & DISPLAY_FLAGS_INTERLACED) fbmode->vmode |= FB_VMODE_INTERLACED; - if (vm->data_flags & DISPLAY_FLAGS_DOUBLESCAN) + if (vm->flags & DISPLAY_FLAGS_DOUBLESCAN) fbmode->vmode |= FB_VMODE_DOUBLE; fbmode->flag = 0;
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c index 13ecd98..56009bc 100644 --- a/drivers/video/of_display_timing.c +++ b/drivers/video/of_display_timing.c @@ -79,25 +79,24 @@ static struct display_timing *of_get_display_timing(struct device_node *np) ret |= parse_timing_property(np, "vsync-len", &dt->vsync_len); ret |= parse_timing_property(np, "clock-frequency", &dt->pixelclock);
- dt->dmt_flags = 0; - dt->data_flags = 0; + dt->flags = 0; if (!of_property_read_u32(np, "vsync-active", &val)) - dt->dmt_flags |= val ? VESA_DMT_VSYNC_HIGH : - VESA_DMT_VSYNC_LOW; + dt->flags |= val ? DISPLAY_FLAGS_VSYNC_HIGH : + DISPLAY_FLAGS_VSYNC_LOW; if (!of_property_read_u32(np, "hsync-active", &val)) - dt->dmt_flags |= val ? VESA_DMT_HSYNC_HIGH : - VESA_DMT_HSYNC_LOW; + dt->flags |= val ? DISPLAY_FLAGS_HSYNC_HIGH : + DISPLAY_FLAGS_HSYNC_LOW; if (!of_property_read_u32(np, "de-active", &val)) - dt->data_flags |= val ? DISPLAY_FLAGS_DE_HIGH : + dt->flags |= val ? DISPLAY_FLAGS_DE_HIGH : DISPLAY_FLAGS_DE_LOW; if (!of_property_read_u32(np, "pixelclk-active", &val)) - dt->data_flags |= val ? DISPLAY_FLAGS_PIXDATA_POSEDGE : + dt->flags |= val ? DISPLAY_FLAGS_PIXDATA_POSEDGE : DISPLAY_FLAGS_PIXDATA_NEGEDGE;
if (of_property_read_bool(np, "interlaced")) - dt->data_flags |= DISPLAY_FLAGS_INTERLACED; + dt->flags |= DISPLAY_FLAGS_INTERLACED; if (of_property_read_bool(np, "doublescan")) - dt->data_flags |= DISPLAY_FLAGS_DOUBLESCAN; + dt->flags |= DISPLAY_FLAGS_DOUBLESCAN;
if (ret) { pr_err("%s: error reading timing properties\n", diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c index 21c47a2..810afff 100644 --- a/drivers/video/videomode.c +++ b/drivers/video/videomode.c @@ -31,8 +31,7 @@ int videomode_from_timing(const struct display_timings *disp, vm->vback_porch = display_timing_get_value(&dt->vback_porch, TE_TYP); vm->vsync_len = display_timing_get_value(&dt->vsync_len, TE_TYP);
- vm->dmt_flags = dt->dmt_flags; - vm->data_flags = dt->data_flags; + vm->flags = dt->flags;
return 0; } diff --git a/include/video/display_timing.h b/include/video/display_timing.h index 71e9a38..a8a4be5 100644 --- a/include/video/display_timing.h +++ b/include/video/display_timing.h @@ -12,19 +12,16 @@ #include <linux/bitops.h> #include <linux/types.h>
-/* VESA display monitor timing parameters */ -#define VESA_DMT_HSYNC_LOW BIT(0) -#define VESA_DMT_HSYNC_HIGH BIT(1) -#define VESA_DMT_VSYNC_LOW BIT(2) -#define VESA_DMT_VSYNC_HIGH BIT(3) - -/* display specific flags */ -#define DISPLAY_FLAGS_DE_LOW BIT(0) /* data enable flag */ -#define DISPLAY_FLAGS_DE_HIGH BIT(1) -#define DISPLAY_FLAGS_PIXDATA_POSEDGE BIT(2) /* drive data on pos. edge */ -#define DISPLAY_FLAGS_PIXDATA_NEGEDGE BIT(3) /* drive data on neg. edge */ -#define DISPLAY_FLAGS_INTERLACED BIT(4) -#define DISPLAY_FLAGS_DOUBLESCAN BIT(5) +#define DISPLAY_FLAGS_HSYNC_LOW BIT(0) +#define DISPLAY_FLAGS_HSYNC_HIGH BIT(1) +#define DISPLAY_FLAGS_VSYNC_LOW BIT(2) +#define DISPLAY_FLAGS_VSYNC_HIGH BIT(3) +#define DISPLAY_FLAGS_DE_LOW BIT(4) /* data enable flag */ +#define DISPLAY_FLAGS_DE_HIGH BIT(5) +#define DISPLAY_FLAGS_PIXDATA_POSEDGE BIT(6) /* drive data on pos. edge */ +#define DISPLAY_FLAGS_PIXDATA_NEGEDGE BIT(7) /* drive data on neg. edge */ +#define DISPLAY_FLAGS_INTERLACED BIT(8) +#define DISPLAY_FLAGS_DOUBLESCAN BIT(9)
/* * A single signal can be specified via a range of minimal and maximal values @@ -72,8 +69,7 @@ struct display_timing { struct timing_entry vback_porch; /* ver. back porch */ struct timing_entry vsync_len; /* ver. sync len */
- unsigned int dmt_flags; /* VESA DMT flags */ - unsigned int data_flags; /* video data flags */ + unsigned int flags; /* display flags */ };
/* diff --git a/include/video/videomode.h b/include/video/videomode.h index a421562..f4ae6ed 100644 --- a/include/video/videomode.h +++ b/include/video/videomode.h @@ -29,8 +29,7 @@ struct videomode { u32 vback_porch; u32 vsync_len;
- unsigned int dmt_flags; /* VESA DMT flags */ - unsigned int data_flags; /* video data flags */ + unsigned int flags; /* display flags */ };
/**
Instead of having plain defines for the videomode's flags, add an enum for the flags. This makes the flags clearer to use, as the enum tells which values can be used with the flags field.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de --- include/video/display_timing.h | 28 +++++++++++++++++----------- include/video/videomode.h | 2 +- 2 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/include/video/display_timing.h b/include/video/display_timing.h index a8a4be5..b63471d 100644 --- a/include/video/display_timing.h +++ b/include/video/display_timing.h @@ -12,16 +12,22 @@ #include <linux/bitops.h> #include <linux/types.h>
-#define DISPLAY_FLAGS_HSYNC_LOW BIT(0) -#define DISPLAY_FLAGS_HSYNC_HIGH BIT(1) -#define DISPLAY_FLAGS_VSYNC_LOW BIT(2) -#define DISPLAY_FLAGS_VSYNC_HIGH BIT(3) -#define DISPLAY_FLAGS_DE_LOW BIT(4) /* data enable flag */ -#define DISPLAY_FLAGS_DE_HIGH BIT(5) -#define DISPLAY_FLAGS_PIXDATA_POSEDGE BIT(6) /* drive data on pos. edge */ -#define DISPLAY_FLAGS_PIXDATA_NEGEDGE BIT(7) /* drive data on neg. edge */ -#define DISPLAY_FLAGS_INTERLACED BIT(8) -#define DISPLAY_FLAGS_DOUBLESCAN BIT(9) +enum display_flags { + DISPLAY_FLAGS_HSYNC_LOW = BIT(0), + DISPLAY_FLAGS_HSYNC_HIGH = BIT(1), + DISPLAY_FLAGS_VSYNC_LOW = BIT(2), + DISPLAY_FLAGS_VSYNC_HIGH = BIT(3), + + /* data enable flag */ + DISPLAY_FLAGS_DE_LOW = BIT(4), + DISPLAY_FLAGS_DE_HIGH = BIT(5), + /* drive data on pos. edge */ + DISPLAY_FLAGS_PIXDATA_POSEDGE = BIT(6), + /* drive data on neg. edge */ + DISPLAY_FLAGS_PIXDATA_NEGEDGE = BIT(7), + DISPLAY_FLAGS_INTERLACED = BIT(8), + DISPLAY_FLAGS_DOUBLESCAN = BIT(9), +};
/* * A single signal can be specified via a range of minimal and maximal values @@ -69,7 +75,7 @@ struct display_timing { struct timing_entry vback_porch; /* ver. back porch */ struct timing_entry vsync_len; /* ver. sync len */
- unsigned int flags; /* display flags */ + enum display_flags flags; /* display flags */ };
/* diff --git a/include/video/videomode.h b/include/video/videomode.h index f4ae6ed..8b12e60 100644 --- a/include/video/videomode.h +++ b/include/video/videomode.h @@ -29,7 +29,7 @@ struct videomode { u32 vback_porch; u32 vsync_len;
- unsigned int flags; /* display flags */ + enum display_flags flags; /* display flags */ };
/**
Display timing's fields have minimum, typical and maximum values. These can be accessed by using timing_entry_index enum, and display_timing_get_value() function.
There's no real need for this extra complexity. The values can be accessed more easily by just using the min/typ/max fields.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/video/videomode.c | 18 +++++++++--------- include/video/display_timing.h | 25 ------------------------- 2 files changed, 9 insertions(+), 34 deletions(-)
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c index 810afff..a3d95f2 100644 --- a/drivers/video/videomode.c +++ b/drivers/video/videomode.c @@ -20,16 +20,16 @@ int videomode_from_timing(const struct display_timings *disp, if (!dt) return -EINVAL;
- vm->pixelclock = display_timing_get_value(&dt->pixelclock, TE_TYP); - vm->hactive = display_timing_get_value(&dt->hactive, TE_TYP); - vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, TE_TYP); - vm->hback_porch = display_timing_get_value(&dt->hback_porch, TE_TYP); - vm->hsync_len = display_timing_get_value(&dt->hsync_len, TE_TYP); + vm->pixelclock = dt->pixelclock.typ; + vm->hactive = dt->hactive.typ; + vm->hfront_porch = dt->hfront_porch.typ; + vm->hback_porch = dt->hback_porch.typ; + vm->hsync_len = dt->hsync_len.typ;
- vm->vactive = display_timing_get_value(&dt->vactive, TE_TYP); - vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, TE_TYP); - vm->vback_porch = display_timing_get_value(&dt->vback_porch, TE_TYP); - vm->vsync_len = display_timing_get_value(&dt->vsync_len, TE_TYP); + vm->vactive = dt->vactive.typ; + vm->vfront_porch = dt->vfront_porch.typ; + vm->vback_porch = dt->vback_porch.typ; + vm->vsync_len = dt->vsync_len.typ;
vm->flags = dt->flags;
diff --git a/include/video/display_timing.h b/include/video/display_timing.h index b63471d..5d0259b 100644 --- a/include/video/display_timing.h +++ b/include/video/display_timing.h @@ -39,12 +39,6 @@ struct timing_entry { u32 max; };
-enum timing_entry_index { - TE_MIN = 0, - TE_TYP = 1, - TE_MAX = 2, -}; - /* * Single "mode" entry. This describes one set of signal timings a display can * have in one setting. This struct can later be converted to struct videomode @@ -91,25 +85,6 @@ struct display_timings { struct display_timing **timings; };
-/* get value specified by index from struct timing_entry */ -static inline u32 display_timing_get_value(const struct timing_entry *te, - enum timing_entry_index index) -{ - switch (index) { - case TE_MIN: - return te->min; - break; - case TE_TYP: - return te->typ; - break; - case TE_MAX: - return te->max; - break; - default: - return te->typ; - } -} - /* get one entry from struct display_timings */ static inline struct display_timing *display_timings_get(const struct display_timings *disp,
Structs videomode and display_timing have rather long field names for the timing values. Nothing wrong with that as such, but this patch changes them to abbreviations for the following reasons:
* The timing values often need to be used in calculations, and long field names makes their direct use clumsier.
* The current names are a bit of a mishmash: some words are used as such, some are shortened, and for some only first letter is used. Some names use underscode, some don't. All this makes it difficult to remember what the field names are.
* The abbreviations used in this patch are very common, and there shouldn't be any misunderstanding about their meaning.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/gpu/drm/drm_modes.c | 18 +++++++++--------- drivers/video/fbmon.c | 24 +++++++++++------------- drivers/video/of_display_timing.c | 16 ++++++++-------- drivers/video/videomode.c | 16 ++++++++-------- include/video/display_timing.h | 16 ++++++++-------- include/video/videomode.h | 18 +++++++++--------- 6 files changed, 53 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f83f071..d744e95 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -510,15 +510,15 @@ EXPORT_SYMBOL(drm_gtf_mode); int drm_display_mode_from_videomode(const struct videomode *vm, struct drm_display_mode *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->hdisplay = vm->hact; + dmode->hsync_start = dmode->hdisplay + vm->hfp; + dmode->hsync_end = dmode->hsync_start + vm->hsl; + dmode->htotal = dmode->hsync_end + vm->hbp;
- 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->vdisplay = vm->vact; + dmode->vsync_start = dmode->vdisplay + vm->vfp; + dmode->vsync_end = dmode->vsync_start + vm->vsl; + dmode->vtotal = dmode->vsync_end + vm->vbp;
dmode->clock = vm->pixelclock / 1000;
@@ -565,7 +565,7 @@ int of_get_drm_display_mode(struct device_node *np, drm_display_mode_from_videomode(&vm, dmode);
pr_debug("%s: got %dx%d display mode from %s\n", - of_node_full_name(np), vm.hactive, vm.vactive, np->name); + of_node_full_name(np), vm.hact, vm.vact, np->name); drm_mode_debug_printmodeline(dmode);
return 0; diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index e5cc2fd..8103fc9 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -1382,15 +1382,15 @@ int fb_videomode_from_videomode(const struct videomode *vm, { unsigned int htotal, vtotal;
- fbmode->xres = vm->hactive; - fbmode->left_margin = vm->hback_porch; - fbmode->right_margin = vm->hfront_porch; - fbmode->hsync_len = vm->hsync_len; + fbmode->xres = vm->hact; + fbmode->left_margin = vm->hbp; + fbmode->right_margin = vm->hfp; + fbmode->hsync_len = vm->hsl;
- fbmode->yres = vm->vactive; - fbmode->upper_margin = vm->vback_porch; - fbmode->lower_margin = vm->vfront_porch; - fbmode->vsync_len = vm->vsync_len; + fbmode->yres = vm->vact; + fbmode->upper_margin = vm->vbp; + fbmode->lower_margin = vm->vfp; + fbmode->vsync_len = vm->vsl;
/* prevent division by zero in KHZ2PICOS macro */ fbmode->pixclock = vm->pixelclock ? @@ -1408,10 +1408,8 @@ int fb_videomode_from_videomode(const struct videomode *vm, fbmode->vmode |= FB_VMODE_DOUBLE; fbmode->flag = 0;
- htotal = vm->hactive + vm->hfront_porch + vm->hback_porch + - vm->hsync_len; - vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch + - vm->vsync_len; + htotal = vm->hact + vm->hfp + vm->hbp + vm->hsl; + vtotal = vm->vact + vm->vfp + vm->vbp + vm->vsl; /* prevent division by zero */ if (htotal && vtotal) { fbmode->refresh = vm->pixelclock / (htotal * vtotal); @@ -1458,7 +1456,7 @@ int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, fb_videomode_from_videomode(&vm, fb);
pr_debug("%s: got %dx%d display mode from %s\n", - of_node_full_name(np), vm.hactive, vm.vactive, np->name); + of_node_full_name(np), vm.hact, vm.vact, np->name); dump_fb_videomode(fb);
return 0; diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c index 56009bc..79660937 100644 --- a/drivers/video/of_display_timing.c +++ b/drivers/video/of_display_timing.c @@ -69,14 +69,14 @@ static struct display_timing *of_get_display_timing(struct device_node *np) return NULL; }
- ret |= parse_timing_property(np, "hback-porch", &dt->hback_porch); - ret |= parse_timing_property(np, "hfront-porch", &dt->hfront_porch); - ret |= parse_timing_property(np, "hactive", &dt->hactive); - ret |= parse_timing_property(np, "hsync-len", &dt->hsync_len); - ret |= parse_timing_property(np, "vback-porch", &dt->vback_porch); - ret |= parse_timing_property(np, "vfront-porch", &dt->vfront_porch); - ret |= parse_timing_property(np, "vactive", &dt->vactive); - ret |= parse_timing_property(np, "vsync-len", &dt->vsync_len); + ret |= parse_timing_property(np, "hback-porch", &dt->hbp); + ret |= parse_timing_property(np, "hfront-porch", &dt->hfp); + ret |= parse_timing_property(np, "hactive", &dt->hact); + ret |= parse_timing_property(np, "hsync-len", &dt->hsl); + ret |= parse_timing_property(np, "vback-porch", &dt->vbp); + ret |= parse_timing_property(np, "vfront-porch", &dt->vfp); + ret |= parse_timing_property(np, "vactive", &dt->vact); + ret |= parse_timing_property(np, "vsync-len", &dt->vsl); ret |= parse_timing_property(np, "clock-frequency", &dt->pixelclock);
dt->flags = 0; diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c index a3d95f2..c42689a 100644 --- a/drivers/video/videomode.c +++ b/drivers/video/videomode.c @@ -21,15 +21,15 @@ int videomode_from_timing(const struct display_timings *disp, return -EINVAL;
vm->pixelclock = dt->pixelclock.typ; - vm->hactive = dt->hactive.typ; - vm->hfront_porch = dt->hfront_porch.typ; - vm->hback_porch = dt->hback_porch.typ; - vm->hsync_len = dt->hsync_len.typ; + vm->hact = dt->hact.typ; + vm->hfp = dt->hfp.typ; + vm->hbp = dt->hbp.typ; + vm->hsl = dt->hsl.typ;
- vm->vactive = dt->vactive.typ; - vm->vfront_porch = dt->vfront_porch.typ; - vm->vback_porch = dt->vback_porch.typ; - vm->vsync_len = dt->vsync_len.typ; + vm->vact = dt->vact.typ; + vm->vfp = dt->vfp.typ; + vm->vbp = dt->vbp.typ; + vm->vsl = dt->vsl.typ;
vm->flags = dt->flags;
diff --git a/include/video/display_timing.h b/include/video/display_timing.h index 5d0259b..db98ef9 100644 --- a/include/video/display_timing.h +++ b/include/video/display_timing.h @@ -59,15 +59,15 @@ struct timing_entry { struct display_timing { struct timing_entry pixelclock;
- struct timing_entry hactive; /* hor. active video */ - struct timing_entry hfront_porch; /* hor. front porch */ - struct timing_entry hback_porch; /* hor. back porch */ - struct timing_entry hsync_len; /* hor. sync len */ + struct timing_entry hact; /* hor. active video */ + struct timing_entry hfp; /* hor. front porch */ + struct timing_entry hbp; /* hor. back porch */ + struct timing_entry hsl; /* hor. sync len */
- struct timing_entry vactive; /* ver. active video */ - struct timing_entry vfront_porch; /* ver. front porch */ - struct timing_entry vback_porch; /* ver. back porch */ - struct timing_entry vsync_len; /* ver. sync len */ + struct timing_entry vact; /* ver. active video */ + struct timing_entry vfp; /* ver. front porch */ + struct timing_entry vbp; /* ver. back porch */ + struct timing_entry vsl; /* ver. sync len */
enum display_flags flags; /* display flags */ }; diff --git a/include/video/videomode.h b/include/video/videomode.h index 8b12e60..b601c0c 100644 --- a/include/video/videomode.h +++ b/include/video/videomode.h @@ -19,15 +19,15 @@ struct videomode { unsigned long pixelclock; /* pixelclock in Hz */
- u32 hactive; - u32 hfront_porch; - u32 hback_porch; - u32 hsync_len; - - u32 vactive; - u32 vfront_porch; - u32 vback_porch; - u32 vsync_len; + u32 hact; + u32 hfp; + u32 hbp; + u32 hsl; + + u32 vact; + u32 vfp; + u32 vbp; + u32 vsl;
enum display_flags flags; /* display flags */ };
Hi Tomi,
Thanks for the patch.
On Tuesday 12 March 2013 12:19:38 Tomi Valkeinen wrote:
Structs videomode and display_timing have rather long field names for the timing values. Nothing wrong with that as such, but this patch changes them to abbreviations for the following reasons:
The timing values often need to be used in calculations, and long field names makes their direct use clumsier.
The current names are a bit of a mishmash: some words are used as such, some are shortened, and for some only first letter is used. Some names use underscode, some don't. All this makes it difficult to remember what the field names are.
The abbreviations used in this patch are very common, and there shouldn't be any misunderstanding about their meaning.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de
I have no strong opinion on this, but I find the existing names easier to read. I might be biased by having read them often though.
Hi,
On 2013-03-12 15:37, Laurent Pinchart wrote:
Hi Tomi,
Thanks for the patch.
On Tuesday 12 March 2013 12:19:38 Tomi Valkeinen wrote:
Structs videomode and display_timing have rather long field names for the timing values. Nothing wrong with that as such, but this patch changes them to abbreviations for the following reasons:
The timing values often need to be used in calculations, and long field names makes their direct use clumsier.
The current names are a bit of a mishmash: some words are used as such, some are shortened, and for some only first letter is used. Some names use underscode, some don't. All this makes it difficult to remember what the field names are.
The abbreviations used in this patch are very common, and there shouldn't be any misunderstanding about their meaning.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de
I have no strong opinion on this, but I find the existing names easier to read. I might be biased by having read them often though.
Yes, the last patch was a bit of a "teaser" =). I found myself typoing them a lot, using helper local variables to shorten the code lines, and as I mention in the description, I find them a bit of a mishmash. So, while they're not used in any drivers yet, I thought it'd be worth a shot to change them.
Tomi
On Tue, Mar 12, 2013 at 03:40:14PM +0200, Tomi Valkeinen wrote:
Hi,
On 2013-03-12 15:37, Laurent Pinchart wrote:
Hi Tomi,
Thanks for the patch.
On Tuesday 12 March 2013 12:19:38 Tomi Valkeinen wrote:
Structs videomode and display_timing have rather long field names for the timing values. Nothing wrong with that as such, but this patch changes them to abbreviations for the following reasons:
The timing values often need to be used in calculations, and long field names makes their direct use clumsier.
The current names are a bit of a mishmash: some words are used as such, some are shortened, and for some only first letter is used. Some names use underscode, some don't. All this makes it difficult to remember what the field names are.
The abbreviations used in this patch are very common, and there shouldn't be any misunderstanding about their meaning.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de
I have no strong opinion on this, but I find the existing names easier to read. I might be biased by having read them often though.
Yes, the last patch was a bit of a "teaser" =). I found myself typoing them a lot, using helper local variables to shorten the code lines, and as I mention in the description, I find them a bit of a mishmash. So, while they're not used in any drivers yet, I thought it'd be worth a shot to change them.
Imo the new names look quite a bit more ugly and less readable, for very few saved chars. And at least for i915.ko development it's been a long time since I've last had to type them ... Maybe the real problem is that we have a few too many video mode structures and can't properly share them? -Daniel
On 2013-03-18 10:00, Daniel Vetter wrote:
On Tue, Mar 12, 2013 at 03:40:14PM +0200, Tomi Valkeinen wrote:
Hi,
On 2013-03-12 15:37, Laurent Pinchart wrote:
Hi Tomi,
Thanks for the patch.
On Tuesday 12 March 2013 12:19:38 Tomi Valkeinen wrote:
Structs videomode and display_timing have rather long field names for the timing values. Nothing wrong with that as such, but this patch changes them to abbreviations for the following reasons:
The timing values often need to be used in calculations, and long field names makes their direct use clumsier.
The current names are a bit of a mishmash: some words are used as such, some are shortened, and for some only first letter is used. Some names use underscode, some don't. All this makes it difficult to remember what the field names are.
The abbreviations used in this patch are very common, and there shouldn't be any misunderstanding about their meaning.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de
I have no strong opinion on this, but I find the existing names easier to read. I might be biased by having read them often though.
Yes, the last patch was a bit of a "teaser" =). I found myself typoing them a lot, using helper local variables to shorten the code lines, and as I mention in the description, I find them a bit of a mishmash. So, while they're not used in any drivers yet, I thought it'd be worth a shot to change them.
Imo the new names look quite a bit more ugly and less readable, for very few saved chars. And at least for i915.ko development it's been a long time since I've last had to type them ... Maybe the real problem is that we have a few too many video mode structures and can't properly share them?
I guess it's a matter of taste. But I've received three "I don't like the new names" comments, and no positive comments, so I'm dropping the last patch. The first four should be fine, though.
And yes, we should definitely try to use only this new videomode struct in the future everywhere in the kernel. It sure would make me remember the names better =).
Tomi
On Tue, Mar 12, 2013 at 12:19:38PM +0200, Tomi Valkeinen wrote:
Structs videomode and display_timing have rather long field names for the timing values. Nothing wrong with that as such, but this patch changes them to abbreviations for the following reasons:
The timing values often need to be used in calculations, and long field names makes their direct use clumsier.
The current names are a bit of a mishmash: some words are used as such, some are shortened, and for some only first letter is used. Some names use underscode, some don't. All this makes it difficult to remember what the field names are.
The abbreviations used in this patch are very common, and there shouldn't be any misunderstanding about their meaning.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de
drivers/gpu/drm/drm_modes.c | 18 +++++++++--------- drivers/video/fbmon.c | 24 +++++++++++------------- drivers/video/of_display_timing.c | 16 ++++++++-------- drivers/video/videomode.c | 16 ++++++++-------- include/video/display_timing.h | 16 ++++++++-------- include/video/videomode.h | 18 +++++++++--------- 6 files changed, 53 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f83f071..d744e95 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -510,15 +510,15 @@ EXPORT_SYMBOL(drm_gtf_mode); int drm_display_mode_from_videomode(const struct videomode *vm, struct drm_display_mode *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->hdisplay = vm->hact;
- dmode->hsync_start = dmode->hdisplay + vm->hfp;
- dmode->hsync_end = dmode->hsync_start + vm->hsl;
- dmode->htotal = dmode->hsync_end + vm->hbp;
- 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->vdisplay = vm->vact;
dmode->vsync_start = dmode->vdisplay + vm->vfp;
dmode->vsync_end = dmode->vsync_start + vm->vsl;
dmode->vtotal = dmode->vsync_end + vm->vbp;
dmode->clock = vm->pixelclock / 1000;
@@ -565,7 +565,7 @@ int of_get_drm_display_mode(struct device_node *np, drm_display_mode_from_videomode(&vm, dmode);
pr_debug("%s: got %dx%d display mode from %s\n",
of_node_full_name(np), vm.hactive, vm.vactive, np->name);
of_node_full_name(np), vm.hact, vm.vact, np->name);drm_mode_debug_printmodeline(dmode);
return 0;
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index e5cc2fd..8103fc9 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -1382,15 +1382,15 @@ int fb_videomode_from_videomode(const struct videomode *vm, { unsigned int htotal, vtotal;
- fbmode->xres = vm->hactive;
- fbmode->left_margin = vm->hback_porch;
- fbmode->right_margin = vm->hfront_porch;
- fbmode->hsync_len = vm->hsync_len;
- fbmode->xres = vm->hact;
- fbmode->left_margin = vm->hbp;
- fbmode->right_margin = vm->hfp;
- fbmode->hsync_len = vm->hsl;
- fbmode->yres = vm->vactive;
- fbmode->upper_margin = vm->vback_porch;
- fbmode->lower_margin = vm->vfront_porch;
- fbmode->vsync_len = vm->vsync_len;
fbmode->yres = vm->vact;
fbmode->upper_margin = vm->vbp;
fbmode->lower_margin = vm->vfp;
fbmode->vsync_len = vm->vsl;
/* prevent division by zero in KHZ2PICOS macro */ fbmode->pixclock = vm->pixelclock ?
@@ -1408,10 +1408,8 @@ int fb_videomode_from_videomode(const struct videomode *vm, fbmode->vmode |= FB_VMODE_DOUBLE; fbmode->flag = 0;
- htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
vm->hsync_len;- vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
vm->vsync_len;
- htotal = vm->hact + vm->hfp + vm->hbp + vm->hsl;
- vtotal = vm->vact + vm->vfp + vm->vbp + vm->vsl; /* prevent division by zero */ if (htotal && vtotal) { fbmode->refresh = vm->pixelclock / (htotal * vtotal);
@@ -1458,7 +1456,7 @@ int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, fb_videomode_from_videomode(&vm, fb);
pr_debug("%s: got %dx%d display mode from %s\n",
of_node_full_name(np), vm.hactive, vm.vactive, np->name);
of_node_full_name(np), vm.hact, vm.vact, np->name);dump_fb_videomode(fb);
return 0;
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c index 56009bc..79660937 100644 --- a/drivers/video/of_display_timing.c +++ b/drivers/video/of_display_timing.c @@ -69,14 +69,14 @@ static struct display_timing *of_get_display_timing(struct device_node *np) return NULL; }
- ret |= parse_timing_property(np, "hback-porch", &dt->hback_porch);
- ret |= parse_timing_property(np, "hfront-porch", &dt->hfront_porch);
- ret |= parse_timing_property(np, "hactive", &dt->hactive);
- ret |= parse_timing_property(np, "hsync-len", &dt->hsync_len);
- ret |= parse_timing_property(np, "vback-porch", &dt->vback_porch);
- ret |= parse_timing_property(np, "vfront-porch", &dt->vfront_porch);
- ret |= parse_timing_property(np, "vactive", &dt->vactive);
- ret |= parse_timing_property(np, "vsync-len", &dt->vsync_len);
ret |= parse_timing_property(np, "hback-porch", &dt->hbp);
ret |= parse_timing_property(np, "hfront-porch", &dt->hfp);
ret |= parse_timing_property(np, "hactive", &dt->hact);
ret |= parse_timing_property(np, "hsync-len", &dt->hsl);
ret |= parse_timing_property(np, "vback-porch", &dt->vbp);
ret |= parse_timing_property(np, "vfront-porch", &dt->vfp);
ret |= parse_timing_property(np, "vactive", &dt->vact);
ret |= parse_timing_property(np, "vsync-len", &dt->vsl); ret |= parse_timing_property(np, "clock-frequency", &dt->pixelclock);
dt->flags = 0;
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c index a3d95f2..c42689a 100644 --- a/drivers/video/videomode.c +++ b/drivers/video/videomode.c @@ -21,15 +21,15 @@ int videomode_from_timing(const struct display_timings *disp, return -EINVAL;
vm->pixelclock = dt->pixelclock.typ;
- vm->hactive = dt->hactive.typ;
- vm->hfront_porch = dt->hfront_porch.typ;
- vm->hback_porch = dt->hback_porch.typ;
- vm->hsync_len = dt->hsync_len.typ;
- vm->hact = dt->hact.typ;
- vm->hfp = dt->hfp.typ;
- vm->hbp = dt->hbp.typ;
- vm->hsl = dt->hsl.typ;
- vm->vactive = dt->vactive.typ;
- vm->vfront_porch = dt->vfront_porch.typ;
- vm->vback_porch = dt->vback_porch.typ;
- vm->vsync_len = dt->vsync_len.typ;
vm->vact = dt->vact.typ;
vm->vfp = dt->vfp.typ;
vm->vbp = dt->vbp.typ;
vm->vsl = dt->vsl.typ;
vm->flags = dt->flags;
diff --git a/include/video/display_timing.h b/include/video/display_timing.h index 5d0259b..db98ef9 100644 --- a/include/video/display_timing.h +++ b/include/video/display_timing.h @@ -59,15 +59,15 @@ struct timing_entry { struct display_timing { struct timing_entry pixelclock;
- struct timing_entry hactive; /* hor. active video */
- struct timing_entry hfront_porch; /* hor. front porch */
- struct timing_entry hback_porch; /* hor. back porch */
- struct timing_entry hsync_len; /* hor. sync len */
- struct timing_entry hact; /* hor. active video */
- struct timing_entry hfp; /* hor. front porch */
- struct timing_entry hbp; /* hor. back porch */
- struct timing_entry hsl; /* hor. sync len */
- struct timing_entry vactive; /* ver. active video */
- struct timing_entry vfront_porch; /* ver. front porch */
- struct timing_entry vback_porch; /* ver. back porch */
- struct timing_entry vsync_len; /* ver. sync len */
struct timing_entry vact; /* ver. active video */
struct timing_entry vfp; /* ver. front porch */
struct timing_entry vbp; /* ver. back porch */
struct timing_entry vsl; /* ver. sync len */
enum display_flags flags; /* display flags */
}; diff --git a/include/video/videomode.h b/include/video/videomode.h index 8b12e60..b601c0c 100644 --- a/include/video/videomode.h +++ b/include/video/videomode.h @@ -19,15 +19,15 @@ struct videomode { unsigned long pixelclock; /* pixelclock in Hz */
- u32 hactive;
- u32 hfront_porch;
- u32 hback_porch;
- u32 hsync_len;
- u32 vactive;
- u32 vfront_porch;
- u32 vback_porch;
- u32 vsync_len;
u32 hact;
u32 hfp;
u32 hbp;
u32 hsl;
u32 vact;
u32 vfp;
u32 vbp;
u32 vsl;
enum display_flags flags; /* display flags */
};
Hi,
I really don't like this. It may be shorter, but I think it makes it really hard to read. And the direct mapping of DT<->C is lost.
Regards, Steffen
Hi Tomi,
Thanks for the patch.
On Tuesday 12 March 2013 12:19:34 Tomi Valkeinen wrote:
This patch simplifies videomode related Kconfig and Makefile. After this patch, there's only one non-user selectable Kconfig option left, VIDEOMODE_HELPERS. The reasons for the change:
Videomode helper functions are not something that should be shown in the kernel configuration options. The related code should just be included if it's needed, i.e. selected by drivers using videomode.
There's no need to have separate Kconfig options for videomode and display_timing. First of all, the amount of code for both is quite small. Second, videomode depends on display_timing, and display_timing in itself is not really useful, so both would be included in any case.
CONFIG_VIDEOMODE is a bit vague name, and CONFIG_VIDEOMODE_HELPERS describes better what's included.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Steffen Trumtrar s.trumtrar@pengutronix.de
I like that.
Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Laurent Pinchart -
Steffen Trumtrar -
Tomi Valkeinen