Message ID | 1359104515-8907-3-git-send-email-s.trumtrar@pengutronix.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Steffen, On 2013-01-25 11:01, Steffen Trumtrar wrote: > +/* 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) <snip> > + unsigned int dmt_flags; /* VESA DMT flags */ > + unsigned int data_flags; /* video data flags */ Why did you go for this approach? To be able to represent true/false/not-specified? Would it be simpler to just have "flags" field? What does it give us to have those two separately? Should the above say raising edge/falling edge instead of positive edge/negative edge? Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ping. On 2013-02-18 16:09, Tomi Valkeinen wrote: > Hi Steffen, > > On 2013-01-25 11:01, Steffen Trumtrar wrote: > >> +/* 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) > > <snip> > >> + unsigned int dmt_flags; /* VESA DMT flags */ >> + unsigned int data_flags; /* video data flags */ > > Why did you go for this approach? To be able to represent > true/false/not-specified? > > Would it be simpler to just have "flags" field? What does it give us to > have those two separately? > > Should the above say raising edge/falling edge instead of positive > edge/negative edge? > > Tomi >
Ah, sorry. Forgot to answer this. On Wed, Feb 27, 2013 at 05:45:31PM +0200, Tomi Valkeinen wrote: > Ping. > > On 2013-02-18 16:09, Tomi Valkeinen wrote: > > Hi Steffen, > > > > On 2013-01-25 11:01, Steffen Trumtrar wrote: > > > >> +/* 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) > > > > <snip> > > > >> + unsigned int dmt_flags; /* VESA DMT flags */ > >> + unsigned int data_flags; /* video data flags */ > > > > Why did you go for this approach? To be able to represent > > true/false/not-specified? > > We decided somewhere between v3 and v8 (I think), that those flags can be high/low/ignored. > > Would it be simpler to just have "flags" field? What does it give us to > > have those two separately? > > I decided to split them, so it is clear that some flags are VESA defined and the others are "invented" for the display-timings framework and may be extended. > > Should the above say raising edge/falling edge instead of positive > > edge/negative edge? > > Hm, I used posedge/negedge because it is shorter (and because of my Verilog past pretty natural to me :-) ). I don't know what others are thinking though. Regards, Steffen
On 2013-02-27 18:05, Steffen Trumtrar wrote: > Ah, sorry. Forgot to answer this. > > On Wed, Feb 27, 2013 at 05:45:31PM +0200, Tomi Valkeinen wrote: >> Ping. >> >> On 2013-02-18 16:09, Tomi Valkeinen wrote: >>> Hi Steffen, >>> >>> On 2013-01-25 11:01, Steffen Trumtrar wrote: >>> >>>> +/* 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) >>> >>> <snip> >>> >>>> + unsigned int dmt_flags; /* VESA DMT flags */ >>>> + unsigned int data_flags; /* video data flags */ >>> >>> Why did you go for this approach? To be able to represent >>> true/false/not-specified? >>> > > We decided somewhere between v3 and v8 (I think), that those flags can be > high/low/ignored. Okay. Why aren't they enums, though? That always makes more clear which defines are to be used with which fields. >>> Would it be simpler to just have "flags" field? What does it give us to >>> have those two separately? >>> > > I decided to split them, so it is clear that some flags are VESA defined and > the others are "invented" for the display-timings framework and may be > extended. Hmm... Okay. Is it relevant that they are VESA defined? It just feels to complicate handling the flags =). >>> Should the above say raising edge/falling edge instead of positive >>> edge/negative edge? >>> > > Hm, I used posedge/negedge because it is shorter (and because of my Verilog past > pretty natural to me :-) ). I don't know what others are thinking though. I guess it's quite clear, but it's still different terms than used elsewhere, e.g. documentation for videomodes. Another thing I noticed while using the new videomode, display_timings.h has a few names that are quite short and generic. Like "TE_MIN", which is now a global define. And "timing_entry". Either name could be well used internally in some .c file, and could easily clash. Tomi
Hi! On Wed, Feb 27, 2013 at 06:13:49PM +0200, Tomi Valkeinen wrote: > On 2013-02-27 18:05, Steffen Trumtrar wrote: > > Ah, sorry. Forgot to answer this. > > > > On Wed, Feb 27, 2013 at 05:45:31PM +0200, Tomi Valkeinen wrote: > >> Ping. > >> > >> On 2013-02-18 16:09, Tomi Valkeinen wrote: > >>> Hi Steffen, > >>> > >>> On 2013-01-25 11:01, Steffen Trumtrar wrote: > >>> > >>>> +/* 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) > >>> > >>> <snip> > >>> > >>>> + unsigned int dmt_flags; /* VESA DMT flags */ > >>>> + unsigned int data_flags; /* video data flags */ > >>> > >>> Why did you go for this approach? To be able to represent > >>> true/false/not-specified? > >>> > > > > We decided somewhere between v3 and v8 (I think), that those flags can be > > high/low/ignored. > > Okay. Why aren't they enums, though? That always makes more clear which > defines are to be used with which fields. > Hm... > >>> Would it be simpler to just have "flags" field? What does it give us to > >>> have those two separately? > >>> > > > > I decided to split them, so it is clear that some flags are VESA defined and > > the others are "invented" for the display-timings framework and may be > > extended. > > Hmm... Okay. Is it relevant that they are VESA defined? It just feels to > complicate handling the flags =). > > >>> Should the above say raising edge/falling edge instead of positive > >>> edge/negative edge? > >>> > > > > Hm, I used posedge/negedge because it is shorter (and because of my Verilog past > > pretty natural to me :-) ). I don't know what others are thinking though. > > I guess it's quite clear, but it's still different terms than used > elsewhere, e.g. documentation for videomodes. > > Another thing I noticed while using the new videomode, display_timings.h > has a few names that are quite short and generic. Like "TE_MIN", which > is now a global define. And "timing_entry". Either name could be well > used internally in some .c file, and could easily clash. > Yes. You are correct. Everyone using this is welcome to send patches now :-) Regards, Steffen
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index e7068c5..09a8f0d 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -33,6 +33,12 @@ config VIDEO_OUTPUT_CONTROL This framework adds support for low-level control of the video output switch. +config DISPLAY_TIMING + bool + +config VIDEOMODE + bool + menuconfig FB tristate "Support for frame buffer devices" ---help--- diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 768a137..e0dd820 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -168,3 +168,5 @@ 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_VIDEOMODE) += videomode.o diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c new file mode 100644 index 0000000..5e1822c --- /dev/null +++ b/drivers/video/display_timing.c @@ -0,0 +1,24 @@ +/* + * generic display timing functions + * + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix + * + * This file is released under the GPLv2 + */ + +#include <linux/export.h> +#include <linux/slab.h> +#include <video/display_timing.h> + +void display_timings_release(struct display_timings *disp) +{ + if (disp->timings) { + unsigned int i; + + for (i = 0; i < disp->num_timings; i++) + kfree(disp->timings[i]); + kfree(disp->timings); + } + kfree(disp); +} +EXPORT_SYMBOL_GPL(display_timings_release); diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c new file mode 100644 index 0000000..21c47a2 --- /dev/null +++ b/drivers/video/videomode.c @@ -0,0 +1,39 @@ +/* + * generic display timing functions + * + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix + * + * This file is released under the GPLv2 + */ + +#include <linux/errno.h> +#include <linux/export.h> +#include <video/display_timing.h> +#include <video/videomode.h> + +int videomode_from_timing(const struct display_timings *disp, + struct videomode *vm, unsigned int index) +{ + struct display_timing *dt; + + dt = display_timings_get(disp, index); + 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->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->dmt_flags = dt->dmt_flags; + vm->data_flags = dt->data_flags; + + return 0; +} +EXPORT_SYMBOL_GPL(videomode_from_timing); diff --git a/include/video/display_timing.h b/include/video/display_timing.h new file mode 100644 index 0000000..71e9a38 --- /dev/null +++ b/include/video/display_timing.h @@ -0,0 +1,124 @@ +/* + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de> + * + * description of display timings + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_DISPLAY_TIMING_H +#define __LINUX_DISPLAY_TIMING_H + +#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) + +/* + * A single signal can be specified via a range of minimal and maximal values + * with a typical value, that lies somewhere inbetween. + */ +struct timing_entry { + u32 min; + u32 typ; + 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 + * (see include/video/videomode.h). As each timing_entry can be defined as a + * range, one struct display_timing may become multiple struct videomodes. + * + * Example: hsync active high, vsync active low + * + * Active Video + * Video ______________________XXXXXXXXXXXXXXXXXXXXXX_____________________ + * |<- sync ->|<- back ->|<----- active ----->|<- front ->|<- sync.. + * | | porch | | porch | + * + * HSync _|¯¯¯¯¯¯¯¯¯¯|___________________________________________|¯¯¯¯¯¯¯¯¯ + * + * VSync ¯|__________|¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯|_________ + */ +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 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 */ + + unsigned int dmt_flags; /* VESA DMT flags */ + unsigned int data_flags; /* video data flags */ +}; + +/* + * This describes all timing settings a display provides. + * The native_mode is the default setting for this display. + * Drivers that can handle multiple videomodes should work with this struct and + * convert each entry to the desired end result. + */ +struct display_timings { + unsigned int num_timings; + unsigned int native_mode; + + 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, + unsigned int index) +{ + if (disp->num_timings > index) + return disp->timings[index]; + else + return NULL; +} + +void display_timings_release(struct display_timings *disp); + +#endif diff --git a/include/video/videomode.h b/include/video/videomode.h new file mode 100644 index 0000000..a421562 --- /dev/null +++ b/include/video/videomode.h @@ -0,0 +1,48 @@ +/* + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de> + * + * generic videomode description + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_VIDEOMODE_H +#define __LINUX_VIDEOMODE_H + +#include <linux/types.h> +#include <video/display_timing.h> + +/* + * Subsystem independent description of a videomode. + * Can be generated from struct display_timing. + */ +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; + + unsigned int dmt_flags; /* VESA DMT flags */ + unsigned int data_flags; /* video data flags */ +}; + +/** + * videomode_from_timing - convert display timing to videomode + * @disp: structure with all possible timing entries + * @vm: return value + * @index: index into the list of display timings in devicetree + * + * DESCRIPTION: + * This function converts a struct display_timing to a struct videomode. + */ +int videomode_from_timing(const struct display_timings *disp, + struct videomode *vm, unsigned int index); + +#endif