| Message ID | 1353920848-1705-3-git-send-email-s.trumtrar@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2012-11-26 11:07, Steffen Trumtrar wrote: > +/* > + * Subsystem independent description of a videomode. > + * Can be generated from struct display_timing. > + */ > +struct videomode { > + u32 pixelclock; /* pixelclock in Hz */ I don't know if this is of any importance, but the linux clock framework manages clock rates with unsigned long. Would it be better to use the same type here? > + u32 hactive; > + u32 hfront_porch; > + u32 hback_porch; > + u32 hsync_len; > + > + u32 vactive; > + u32 vfront_porch; > + u32 vback_porch; > + u32 vsync_len; > + > + u32 hah; /* hsync active high */ > + u32 vah; /* vsync active high */ > + u32 de; /* data enable */ > + u32 pixelclk_pol; What values will these 4 have? Aren't these booleans? The data enable comment should be "data enable active high". The next patch says in the devtree binding documentation that the values may be on/off/ignored. Is that represented here somehow, i.e. values are 0/1/2 or so? If not, what does it mean if the value is left out from devtree data, meaning "not used on hardware"? There's also a "doubleclk" value mentioned in the devtree bindings doc, but not shown here. I think this videomode struct is the one that display drivers are going to use (?), in addition to the display_timing, so it would be good to document it well. Tomi
On Mon, Nov 26, 2012 at 02:37:26PM +0200, Tomi Valkeinen wrote: > On 2012-11-26 11:07, Steffen Trumtrar wrote: > > > +/* > > + * Subsystem independent description of a videomode. > > + * Can be generated from struct display_timing. > > + */ > > +struct videomode { > > + u32 pixelclock; /* pixelclock in Hz */ > > I don't know if this is of any importance, but the linux clock framework > manages clock rates with unsigned long. Would it be better to use the > same type here? > Hm, I don't know. Anyone? u32 should be large enough for a pixelclock. > > + u32 hactive; > > + u32 hfront_porch; > > + u32 hback_porch; > > + u32 hsync_len; > > + > > + u32 vactive; > > + u32 vfront_porch; > > + u32 vback_porch; > > + u32 vsync_len; > > + > > + u32 hah; /* hsync active high */ > > + u32 vah; /* vsync active high */ > > + u32 de; /* data enable */ > > + u32 pixelclk_pol; > > What values will these 4 have? Aren't these booleans? > > The data enable comment should be "data enable active high". > > The next patch says in the devtree binding documentation that the values > may be on/off/ignored. Is that represented here somehow, i.e. values are > 0/1/2 or so? If not, what does it mean if the value is left out from > devtree data, meaning "not used on hardware"? > Hm, I think you might be right here. It is no problem in the DT part of the code. The properties are optional, left out means "not used on hardware". But this does not propagate correctly to the videomode. I should set the fields to -EINVAL in case they are omitted, than everything should work nicely. > There's also a "doubleclk" value mentioned in the devtree bindings doc, > but not shown here. > Argh. That slipped through. I have patches that add this property to all structs and the devicetree binding. But it is not supposed to be in this series, because I want to at least have the binding stable for now. > I think this videomode struct is the one that display drivers are going > to use (?), in addition to the display_timing, so it would be good to > document it well. > Yes. Maybe I have to put more of the devicetree documentation in the code. Regards, Steffen
On Mon, 26 Nov 2012 16:39:58 +0100, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > On Mon, Nov 26, 2012 at 02:37:26PM +0200, Tomi Valkeinen wrote: > > On 2012-11-26 11:07, Steffen Trumtrar wrote: > > > > > +/* > > > + * Subsystem independent description of a videomode. > > > + * Can be generated from struct display_timing. > > > + */ > > > +struct videomode { > > > + u32 pixelclock; /* pixelclock in Hz */ > > > > I don't know if this is of any importance, but the linux clock framework > > manages clock rates with unsigned long. Would it be better to use the > > same type here? > > > > Hm, I don't know. Anyone? u32 should be large enough for a pixelclock. 4GHz is a pretty large pixel clock. I have no idea how conceivable it is that hardware will get to that speed. However, if it will ever be larger, then you'll need to account for that in the DT binding so that the pixel clock can be specified using 2 cells. g. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012-12-06 12:07, Grant Likely wrote: > On Mon, 26 Nov 2012 16:39:58 +0100, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: >> On Mon, Nov 26, 2012 at 02:37:26PM +0200, Tomi Valkeinen wrote: >>> On 2012-11-26 11:07, Steffen Trumtrar wrote: >>> >>>> +/* >>>> + * Subsystem independent description of a videomode. >>>> + * Can be generated from struct display_timing. >>>> + */ >>>> +struct videomode { >>>> + u32 pixelclock; /* pixelclock in Hz */ >>> >>> I don't know if this is of any importance, but the linux clock framework >>> manages clock rates with unsigned long. Would it be better to use the >>> same type here? >>> >> >> Hm, I don't know. Anyone? u32 should be large enough for a pixelclock. > > 4GHz is a pretty large pixel clock. I have no idea how conceivable it is > that hardware will get to that speed. However, if it will ever be > larger, then you'll need to account for that in the DT binding so that > the pixel clock can be specified using 2 cells. I didn't mention the type because of the size of the field, but only because to me it makes sense to use the same type for clock rates all around the kernel. In many cases the value will be passed to clk_set_rate(). I can't see any real issues with u32, though. Tomi
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index d08d799..2a23b18 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 23e948e..fc30439 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -167,3 +167,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..ac9bbbc --- /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/display_timing.h> +#include <linux/export.h> +#include <linux/slab.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..784eaad --- /dev/null +++ b/drivers/video/videomode.c @@ -0,0 +1,44 @@ +/* + * generic display timing functions + * + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix + * + * This file is released under the GPLv2 + */ + +#include <linux/display_timing.h> +#include <linux/errno.h> +#include <linux/export.h> +#include <linux/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->vah = dt->vsync_pol_active; + vm->hah = dt->hsync_pol_active; + vm->de = dt->de_pol_active; + vm->pixelclk_pol = dt->pixelclk_pol; + + vm->interlaced = dt->interlaced; + vm->doublescan = dt->doublescan; + + return 0; +} +EXPORT_SYMBOL_GPL(videomode_from_timing); diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h new file mode 100644 index 0000000..4986364 --- /dev/null +++ b/include/linux/display_timing.h @@ -0,0 +1,104 @@ +/* + * 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/types.h> + +/* + * A single signal can be specified via a range with a typical value, that lies + * somewhere inbetween. Do not use an array, to prevent any confusion about the + * meaning of every entry. + */ +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/linux/videomode.h). As each timing_entry can be defined as a + * range, one struct display_timing may become multiple struct videomodes. + */ +struct display_timing { + struct timing_entry pixelclock; + + struct timing_entry hactive; + struct timing_entry hfront_porch; + struct timing_entry hback_porch; + struct timing_entry hsync_len; + + struct timing_entry vactive; + struct timing_entry vfront_porch; + struct timing_entry vback_porch; + struct timing_entry vsync_len; + + unsigned int vsync_pol_active; + unsigned int hsync_pol_active; + unsigned int de_pol_active; + unsigned int pixelclk_pol; + bool interlaced; + bool doublescan; +}; + +/* + * This describes all timing settings a display provides. + * The native_mode is the default setting for this display. It can be specified + * in the devicetree or will be the first that is provided. Drivers that can + * handle multiple videomode 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/linux/videomode.h b/include/linux/videomode.h new file mode 100644 index 0000000..562f490 --- /dev/null +++ b/include/linux/videomode.h @@ -0,0 +1,54 @@ +/* + * 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> + +struct display_timings; + +/* + * Subsystem independent description of a videomode. + * Can be generated from struct display_timing. + */ +struct videomode { + u32 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 hah; /* hsync active high */ + u32 vah; /* vsync active high */ + u32 de; /* data enable */ + u32 pixelclk_pol; + + bool interlaced; + bool doublescan; +}; + +/** + * 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