| Message ID | 1352734626-27412-2-git-send-email-s.trumtrar@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote: [...] > 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 DISPLAY_TIMINGS? > #video output switch sysfs driver > obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o > +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o display_timings.o? > +obj-$(CONFIG_VIDEOMODE) += videomode.o > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c display_timings.c? > +int videomode_from_timing(struct display_timings *disp, struct videomode *vm, > + unsigned int index) I find the indexing API a bit confusing. But that's perhaps just a matter of personal preference. Also the ordering of arguments seems a little off. I find it more natural to have the destination pointer in the first argument, similar to the memcpy() function, so this would be: int videomode_from_timing(struct videomode *vm, struct display_timings *disp, unsigned int index); Actually, when reading videomode_from_timing() I'd expect the argument list to be: int videomode_from_timing(struct videomode *vm, struct display_timing *timing); Am I the only one confused by this? > diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h display_timings.h? > +/* placeholder function until ranges are really needed The above line has trailing whitespace. Also the block comment should have the opening /* on a separate line. > + * the index parameter should then be used to select one of [min typ max] If index is supposed to select min, typ or max, then maybe an enum would be a better candidate? Or alternatively provide separate accessors, like display_timing_get_{minimum,typical,maximum}(). > + */ > +static inline u32 display_timing_get_value(struct timing_entry *te, > + unsigned int index) > +{ > + return te->typ; > +} > + > +static inline struct display_timing *display_timings_get(struct display_timings *disp, > + unsigned int index) > +{ > + if (disp->num_timings > index) > + return disp->timings[index]; > + else > + return NULL; > +} > + > +void timings_release(struct display_timings *disp); This function no longer exists. Thierry
On Tue, Nov 13, 2012 at 11:41:59AM +0100, Thierry Reding wrote: > On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote: > [...] > > 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 > > DISPLAY_TIMINGS? > > > #video output switch sysfs driver > > obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o > > +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o > > display_timings.o? > > > +obj-$(CONFIG_VIDEOMODE) += videomode.o > > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c > > display_timings.c? > I originally had that and changed it by request to the singular form. (Can't find the mail atm). And I think this fits better with all the other drivers. > > +int videomode_from_timing(struct display_timings *disp, struct videomode *vm, > > + unsigned int index) > > I find the indexing API a bit confusing. But that's perhaps just a > matter of personal preference. > > Also the ordering of arguments seems a little off. I find it more > natural to have the destination pointer in the first argument, similar > to the memcpy() function, so this would be: > > int videomode_from_timing(struct videomode *vm, struct display_timings *disp, > unsigned int index); > > Actually, when reading videomode_from_timing() I'd expect the argument > list to be: > > int videomode_from_timing(struct videomode *vm, struct display_timing *timing); > > Am I the only one confused by this? > I went with the of_xxx-functions that have fname(from_node, to_property) and personally prefer it this way. Therefore I'd like to keep it as is. > > diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h > > display_timings.h? > > > +/* placeholder function until ranges are really needed > > The above line has trailing whitespace. Also the block comment should > have the opening /* on a separate line. > Okay. > > + * the index parameter should then be used to select one of [min typ max] > > If index is supposed to select min, typ or max, then maybe an enum would > be a better candidate? Or alternatively provide separate accessors, like > display_timing_get_{minimum,typical,maximum}(). > Hm, I'm not so sure about this one. I'd prefer the enum. > > + */ > > +static inline u32 display_timing_get_value(struct timing_entry *te, > > + unsigned int index) > > +{ > > + return te->typ; > > +} > > + > > +static inline struct display_timing *display_timings_get(struct display_timings *disp, > > + unsigned int index) > > +{ > > + if (disp->num_timings > index) > > + return disp->timings[index]; > > + else > > + return NULL; > > +} > > + > > +void timings_release(struct display_timings *disp); > > This function no longer exists. > Right. Steffen > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss
On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote: [...] > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c [...] > +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); > +} I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't be used from modules. Thierry
On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote: > On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote: > [...] > > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c > [...] > > +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); > > +} > > I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't > be used from modules. > > Thierry Yes. Just in time. I was just starting to type the send-email command ;-) Regards, Steffen
On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote: > On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote: > > On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote: > > [...] > > > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c > > [...] > > > +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); > > > +} > > > > I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't > > be used from modules. > > > > Thierry > > Yes. Just in time. I was just starting to type the send-email command ;-) Great! In that case don't forget to also look at my other email before sending. =) Thierry
On Wed, Nov 14, 2012 at 12:02:15PM +0100, Thierry Reding wrote: > On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote: > > On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote: > > > On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote: > > > [...] > > > > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c > > > [...] > > > > +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); > > > > +} > > > > > > I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't > > > be used from modules. > > > > > > Thierry > > > > Yes. Just in time. I was just starting to type the send-email command ;-) > > Great! In that case don't forget to also look at my other email before > sending. =) > Sure.
On Wed, Nov 14, 2012 at 12:10:15PM +0100, Steffen Trumtrar wrote: > On Wed, Nov 14, 2012 at 12:02:15PM +0100, Thierry Reding wrote: > > On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote: > > > On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote: > > > > On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote: > > > > [...] > > > > > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c > > > > [...] > > > > > +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); > > > > > +} > > > > > > > > I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't > > > > be used from modules. > > > > > > > > Thierry > > > > > > Yes. Just in time. I was just starting to type the send-email command ;-) > > > > Great! In that case don't forget to also look at my other email before > > sending. =) > > > Sure. Besides those comments (and those from other people) I think your patchset is in pretty good shape. Have you thought about how and when you want to get things merged? Thierry
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..04b7b69 --- /dev/null +++ b/drivers/video/display_timing.c @@ -0,0 +1,22 @@ +/* + * 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/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); +} diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c new file mode 100644 index 0000000..087374a --- /dev/null +++ b/drivers/video/videomode.c @@ -0,0 +1,45 @@ +/* + * 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/errno.h> +#include <linux/display_timing.h> +#include <linux/kernel.h> +#include <linux/videomode.h> + +int videomode_from_timing(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, 0); + vm->hactive = display_timing_get_value(&dt->hactive, 0); + vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0); + vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0); + vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0); + + vm->vactive = display_timing_get_value(&dt->vactive, 0); + vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0); + vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0); + vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0); + + 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..0ed2a1e --- /dev/null +++ b/include/linux/display_timing.h @@ -0,0 +1,69 @@ +/* + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de> + * + * description of display timings + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_DISPLAY_TIMINGS_H +#define __LINUX_DISPLAY_TIMINGS_H + +#include <linux/types.h> + +struct timing_entry { + u32 min; + u32 typ; + u32 max; +}; + +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; +}; + +struct display_timings { + unsigned int num_timings; + unsigned int native_mode; + + struct display_timing **timings; +}; + +/* placeholder function until ranges are really needed + * the index parameter should then be used to select one of [min typ max] + */ +static inline u32 display_timing_get_value(struct timing_entry *te, + unsigned int index) +{ + return te->typ; +} + +static inline struct display_timing *display_timings_get(struct display_timings *disp, + unsigned int index) +{ + if (disp->num_timings > index) + return disp->timings[index]; + else + return NULL; +} + +void timings_release(struct display_timings *disp); +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..0b87fbb --- /dev/null +++ b/include/linux/videomode.h @@ -0,0 +1,39 @@ +/* + * 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/display_timing.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; + + u32 hah; + u32 vah; + u32 de; + u32 pixelclk_pol; + + bool interlaced; + bool doublescan; +}; + +int videomode_from_timing(struct display_timings *disp, struct videomode *vm, + unsigned int index); +#endif
Add display_timing structure and the according helper functions. This allows the description of a display via its supported timing parameters. Every timing parameter can be specified as a single value or a range <min typ max>. Also, add helper functions to convert from display timings to a generic videomode structure. This videomode can then be converted to the corresponding subsystem mode representation (e.g. fb_videomode). Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> --- drivers/video/Kconfig | 6 ++++ drivers/video/Makefile | 2 ++ drivers/video/display_timing.c | 22 +++++++++++++ drivers/video/videomode.c | 45 ++++++++++++++++++++++++++ include/linux/display_timing.h | 69 ++++++++++++++++++++++++++++++++++++++++ include/linux/videomode.h | 39 +++++++++++++++++++++++ 6 files changed, 183 insertions(+) create mode 100644 drivers/video/display_timing.c create mode 100644 drivers/video/videomode.c create mode 100644 include/linux/display_timing.h create mode 100644 include/linux/videomode.h