Re: [PATCH v8 1/6] video: add display_timing and videomode

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

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 #2: Type: application/pgp-signature, Size: 836 bytes --]