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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

Hi,

On 2012-11-20 17:54, Steffen Trumtrar wrote:
> 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).

Sorry for reviewing this so late.

One thing I'd like to see is some explanation of the structs involved.
For example, in this patch you present structs videomode, display_timing
and display_timings without giving any hint what they represent.

I'm not asking for you to write a long documentation, but perhaps the
header files could include a few lines of comments above the structs,
explaining the idea.

> +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);

Perhaps this will become clearer after reading the following patches,
but it feels a bit odd to add a release function, without anything in
this patch that would actually allocate the timings.

> diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
> new file mode 100644
> index 0000000..e24f879
> --- /dev/null
> +++ b/drivers/video/videomode.c
> @@ -0,0 +1,46 @@
> +/*
> + * 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(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, 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);

Shouldn't all these calls get the typical value, with index 1?

> +
> +	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..d5bf03f
> --- /dev/null
> +++ b/include/linux/display_timing.h
> @@ -0,0 +1,70 @@
> +/*
> + * 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>

What is this needed for? u32? I don't see it defined in 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(const struct timing_entry *te,
> +					   unsigned int index)
> +{
> +	return te->typ;
> +}

Why did you opt for a placeholder here? It feels trivial to me to have
support to get the min/typ/max value properly.

> +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..5d3e796
> --- /dev/null
> +++ b/include/linux/videomode.h
> @@ -0,0 +1,40 @@
> +/*
> + * 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>

This is not needed, just add:

struct display_timings;

> +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(const struct display_timings *disp,
> +			  struct videomode *vm, unsigned int index);
> +

Are this and the few other functions above meant to be used from
drivers? If so, some explanation of the parameters here would be nice.
If they are just framework internal, they don't probably need that.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]