Re: [PATCH v2 3/4] OMAPDSS: HDMI: change the timing match logic

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

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

On Mon, 2012-01-02 at 14:14 +0530, mythripk@ti.com wrote:
> From: Mythri P K <mythripk@ti.com>
> 
> Change the timing match logic, Instead of the statically mapped method
> to get the corresponding timings for a given code and mode, move to a
> simpler array indexed method. It  will help to scale up to add more
> timings when needed.
> 
> Signed-off-by: Mythri P K <mythripk@ti.com>
> ---
>  drivers/video/omap2/dss/hdmi.c |  174 +++++++++++++++++-----------------------
>  1 files changed, 75 insertions(+), 99 deletions(-)


> -static int get_timings_index(void)
> +static const struct hdmi_config *hdmi_find_timing(
> +					const struct hdmi_config *timings_arr,
> +					int len)
>  {
> -	int code;
> +	int i;
> +	const struct hdmi_config *timing, timing1 = { {0}, {0} };
>  
> -	if (hdmi.mode == 0)
> -		code = code_vesa[hdmi.code];
> -	else
> -		code = code_cea[hdmi.code];
> +	for (i = 0; i < len; i++) {
> +		if (timings_arr[i].cm.code == hdmi.code) {
> +			timing = &timings_arr[i];
> +			return timing;
> +		}
> +	}
> +	timing = &timing1;
> +	return timing;
> +}

You should return NULL if the find fails, not a timing struct with zero
values. And then there's no need for timing nor timing1 variables.

Furthermore, the code is broken. The timing1 value is allocated from the
stack frame of hdmi_find_timing function. You return a pointer to that,
but the value is no longer valid after the function returns.

>  
> -	if (code == -1)	{
> -		/* HDMI code 4 corresponds to 640 * 480 VGA */
> -		hdmi.code = 4;
> -		/* DVI mode 1 corresponds to HDMI 0 to DVI */
> -		hdmi.mode = HDMI_DVI;
> +static const struct hdmi_config *hdmi_get_timings(void)
> +{
> +	const struct hdmi_config *timing;
>  
> -		code = code_vesa[hdmi.code];
> +	if (hdmi.mode == 0) {
> +		timing = hdmi_find_timing(vesa_timings,
> +					ARRAY_SIZE(vesa_timings));
> +	} else {
> +		timing = hdmi_find_timing(cea_timings,
> +					ARRAY_SIZE(cea_timings));
> +	}
> +	return timing;
> +}

hdmi.mode is enum hdmi_core_hdmi_dvi, isn't it? Then you should use the
enum values when comparing, not 0. Although the use of that enum seems
to be broken elsewhere also, as struct hdmi_cm uses int instead of the
enum. That (and the other similar mode problems) should be fixed in a
separate patch.

And I think this function would be cleaner this way:

static const struct hdmi_config *hdmi_get_timings(void)
{
	const struct hdmi_config *arr;
	int len;

	if (hdmi.mode == HDMI_DVI) {
		arr = vesa_timings;
		len = ARRAY_SIZE(vesa_timings);
	} else {
		arr = cea_timings;
		len = ARRAY_SIZE(cea_timings);
	}

	return hdmi_find_timing(arr, len);
}

> +static bool hdmi_timings_compare(struct omap_video_timings *timing1,
> +				const struct hdmi_video_timings *timing2)
> +{
> +	int timing1_vsync, timing1_hsync, timing2_vsync, timing2_hsync;
> +
> +	if ((timing2->pixel_clock == timing1->pixel_clock) &&
> +		(timing2->x_res == timing1->x_res) &&
> +		(timing2->y_res == timing1->y_res)) {
> +
> +		timing2_hsync = timing2->hfp + timing2->hsw + timing2->hbp;
> +		timing1_hsync = timing1->hfp + timing1->hsw + timing1->hbp;
> +		timing2_vsync = timing2->vfp + timing2->vsw + timing2->vbp;
> +		timing1_vsync = timing2->vfp + timing2->vsw + timing2->vbp;
> +
> +		DSSDBG("timing1_hsync = %d timing1_vsync = %d"\
> +			"timing2_hsync = %d timing2_vsync = %d\n",
> +			timing1_hsync, timing1_vsync,
> +			timing2_hsync, timing2_vsync);
> +
> +		if ((timing1_hsync == timing2_hsync) &&
> +			(timing1_vsync == timing2_vsync)) {
> +			return true;
> +		}
>  	}
> -	return code;
> +	return false;
>  }
>  
>  static struct hdmi_cm hdmi_get_code(struct omap_video_timings *timing)
>  {
> -	int i = 0, code = -1, temp_vsync = 0, temp_hsync = 0;
> -	int timing_vsync = 0, timing_hsync = 0;
> -	struct hdmi_video_timings temp;
> +	int i;
>  	struct hdmi_cm cm = {-1};
>  	DSSDBG("hdmi_get_code\n");
>  
> -	for (i = 0; i < OMAP_HDMI_TIMINGS_NB; i++) {
> -		temp = cea_vesa_timings[i].timings;
> -		if ((temp.pixel_clock == timing->pixel_clock) &&
> -			(temp.x_res == timing->x_res) &&
> -			(temp.y_res == timing->y_res)) {
> -
> -			temp_hsync = temp.hfp + temp.hsw + temp.hbp;
> -			timing_hsync = timing->hfp + timing->hsw + timing->hbp;
> -			temp_vsync = temp.vfp + temp.vsw + temp.vbp;
> -			timing_vsync = timing->vfp + timing->vsw + timing->vbp;
> -
> -			DSSDBG("temp_hsync = %d , temp_vsync = %d"
> -				"timing_hsync = %d, timing_vsync = %d\n",
> -				temp_hsync, temp_hsync,
> -				timing_hsync, timing_vsync);
> -
> -			if ((temp_hsync == timing_hsync) &&
> -					(temp_vsync == timing_vsync)) {
> -				code = i;
> -				cm.code = code_index[i];
> -				if (code < 14)
> -					cm.mode = HDMI_HDMI;
> -				else
> -					cm.mode = HDMI_DVI;
> -				DSSDBG("Hdmi_code = %d mode = %d\n",
> -					 cm.code, cm.mode);
> -				break;
> -			 }
> +	for (i = 0; i < ARRAY_SIZE(cea_timings); i++) {
> +		if (hdmi_timings_compare(timing, &cea_timings[i].timings)) {
> +			cm = cea_timings[i].cm;
> +			goto end;
> +		}
> +	}
> +	for (i = 0; i < ARRAY_SIZE(vesa_timings); i++) {
> +		if (hdmi_timings_compare(timing, &vesa_timings[i].timings)) {
> +			cm = vesa_timings[i].cm;
> +			goto end;
>  		}
>  	}
>  
> -	return cm;
> -}
> +end:	return cm;
>  
> -static void update_hdmi_timings(struct hdmi_config *cfg,
> -		struct omap_video_timings *timings, int code)
> -{
> -	cfg->timings.x_res = timings->x_res;
> -	cfg->timings.y_res = timings->y_res;
> -	cfg->timings.hbp = timings->hbp;
> -	cfg->timings.hfp = timings->hfp;
> -	cfg->timings.hsw = timings->hsw;
> -	cfg->timings.vbp = timings->vbp;
> -	cfg->timings.vfp = timings->vfp;
> -	cfg->timings.vsw = timings->vsw;
> -	cfg->timings.pixel_clock = timings->pixel_clock;
> -	cfg->timings.vsync_pol = cea_vesa_timings[code].timings.vsync_pol;
> -	cfg->timings.hsync_pol = cea_vesa_timings[code].timings.hsync_pol;
>  }
>  
>  unsigned long hdmi_get_pixel_clock(void)
> @@ -325,7 +295,7 @@ static void hdmi_compute_pll(struct omap_dss_device *dssdev, int phy,
>  
>  static int hdmi_power_on(struct omap_dss_device *dssdev)
>  {
> -	int r, code = 0;
> +	int r;
>  	struct omap_video_timings *p;
>  	unsigned long phy;
>  
> @@ -341,8 +311,14 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
>  		dssdev->panel.timings.x_res,
>  		dssdev->panel.timings.y_res);
>  
> -	code = get_timings_index();
> -	update_hdmi_timings(&hdmi.ip_data.cfg, p, code);
> +	hdmi.ip_data.cfg = *(hdmi_get_timings());
> +	if (hdmi.ip_data.cfg.timings.x_res == 0) {

Here you should check if the function returns NULL, and use that to
decide if it failed or not.

> +		/* HDMI code 4 corresponds to 640 * 480 VGA */
> +		hdmi.code = 4;
> +		/* DVI mode 1 corresponds to HDMI 0 to DVI */
> +		hdmi.mode = HDMI_DVI;
> +		hdmi.ip_data.cfg = vesa_timings[0];
> +	}
 
 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]