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
Attachment:
pgpYw0o5T8Kk2.pgp
Description: PGP signature
