[PATCH] OMAP: DSS2: FEATURES: DSI PLL parameter cleanup

[PATCH] OMAP: DSS2: FEATURES: DSI PLL parameter cleanup

[Archit Taneja]

The DSI PLL parameters (regm, regn, regm_dispc, regm_dsi, fint) have different
fields and also different Max values on OMAP3 and OMAP4. Use dss features to
calculate the register fields and Max values based on current OMAP revision

Also, introduce a new function in dss_features "dss_feat_get_param_range" which
returns the maximum and minimum values supported by the parameter for
the current OMAP revision.

Signed-off-by: Archit Taneja <archit@ti.com>
---
Note: Applies over:

http://gitorious.org/linux-omap-dss2/linux/commits/master

and the patch submitted on l-o:
OMAP4: DSS2: Clock source changes for OMAP4

 drivers/video/omap2/dss/dsi.c          |   75 +++++++++++++++++++++++---------
 drivers/video/omap2/dss/dss_features.c |   40 +++++++++++++++++
 drivers/video/omap2/dss/dss_features.h |    9 ++++
 3 files changed, 103 insertions(+), 21 deletions(-)

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index be7694f..e7d6169 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -185,13 +185,8 @@ struct dsi_reg { u16 idx; };
 #define DSI_DT_RX_SHORT_READ_1		0x21
 #define DSI_DT_RX_SHORT_READ_2		0x22
 
-#define FINT_MAX 2100000
-#define FINT_MIN 750000
-#define REGN_MAX (1 << 7)
-#define REGM_MAX ((1 << 11) - 1)
-#define REGM_DISPC_MAX (1 << 4)
-#define REGM_DSI_MAX (1 << 4)
 #define LP_DIV_MAX ((1 << 13) - 1)
+#define DSI_PARAM_MAX(start, end)	((1 << (start - end + 1)) - 1)
 
 enum fifo_size {
 	DSI_FIFO_SIZE_0		= 0,
@@ -277,6 +272,10 @@ static struct
 	spinlock_t irq_stats_lock;
 	struct dsi_irq_stats irq_stats;
 #endif
+	/* DSI PLL Parameter Ranges */
+	int regm_max, regn_max;
+	int regm_dispc_max, regm_dsi_max;
+	int fint_min, fint_max;
 } dsi;
 
 #ifdef DEBUG
@@ -801,16 +800,16 @@ static int dsi_pll_power(enum dsi_pll_power_state state)
 static int dsi_calc_clock_rates(struct omap_dss_device *dssdev,
 		struct dsi_clock_info *cinfo)
 {
-	if (cinfo->regn == 0 || cinfo->regn > REGN_MAX)
+	if (cinfo->regn == 0 || cinfo->regn > dsi.regn_max)
 		return -EINVAL;
 
-	if (cinfo->regm == 0 || cinfo->regm > REGM_MAX)
+	if (cinfo->regm == 0 || cinfo->regm > dsi.regm_max)
 		return -EINVAL;
 
-	if (cinfo->regm_dispc > REGM_DISPC_MAX)
+	if (cinfo->regm_dispc > dsi.regm_dispc_max)
 		return -EINVAL;
 
-	if (cinfo->regm_dsi > REGM_DSI_MAX)
+	if (cinfo->regm_dsi > dsi.regm_dsi_max)
 		return -EINVAL;
 
 	if (cinfo->use_sys_clk) {
@@ -829,7 +828,7 @@ static int dsi_calc_clock_rates(struct omap_dss_device *dssdev,
 
 	cinfo->fint = cinfo->clkin / (cinfo->regn * (cinfo->highfreq ? 2 : 1));
 
-	if (cinfo->fint > FINT_MAX || cinfo->fint < FINT_MIN)
+	if (cinfo->fint > dsi.fint_max || cinfo->fint < dsi.fint_min)
 		return -EINVAL;
 
 	cinfo->clkin4ddr = 2 * cinfo->regm * cinfo->fint;
@@ -899,17 +898,17 @@ retry:
 	/* no highfreq: 0.75MHz < Fint = clkin / regn < 2.1MHz */
 	/* highfreq: 0.75MHz < Fint = clkin / (2*regn) < 2.1MHz */
 	/* To reduce PLL lock time, keep Fint high (around 2 MHz) */
-	for (cur.regn = 1; cur.regn < REGN_MAX; ++cur.regn) {
+	for (cur.regn = 1; cur.regn < dsi.regn_max; ++cur.regn) {
 		if (cur.highfreq == 0)
 			cur.fint = cur.clkin / cur.regn;
 		else
 			cur.fint = cur.clkin / (2 * cur.regn);
 
-		if (cur.fint > FINT_MAX || cur.fint < FINT_MIN)
+		if (cur.fint > dsi.fint_max || cur.fint < dsi.fint_min)
 			continue;
 
 		/* DSIPHY(MHz) = (2 * regm / regn) * (clkin / (highfreq + 1)) */
-		for (cur.regm = 1; cur.regm < REGM_MAX; ++cur.regm) {
+		for (cur.regm = 1; cur.regm < dsi.regm_max; ++cur.regm) {
 			unsigned long a, b;
 
 			a = 2 * cur.regm * (cur.clkin/1000);
@@ -921,8 +920,8 @@ retry:
 
 			/* dsi_pll_hsdiv_dispc_clk(MHz) =
 			 * DSIPHY(MHz) / regm_dispc  < 173MHz/186Mhz */
-			for (cur.regm_dispc = 1; cur.regm_dispc < REGM_DISPC_MAX;
-					++cur.regm_dispc) {
+			for (cur.regm_dispc = 1; cur.regm_dispc <
+					dsi.regm_dispc_max; ++cur.regm_dispc) {
 				struct dispc_clock_info cur_dispc;
 				cur.dsi_pll_hsdiv_dispc_clk =
 					cur.clkin4ddr / cur.regm_dispc;
@@ -994,6 +993,8 @@ int dsi_pll_set_clock_div(struct dsi_clock_info *cinfo)
 	int r = 0;
 	u32 l;
 	int f;
+	u8 regn_start, regn_end, regm_start, regm_end;
+	u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end;
 
 	DSSDBGF();
 
@@ -1038,19 +1039,30 @@ int dsi_pll_set_clock_div(struct dsi_clock_info *cinfo)
 		dss_feat_get_clk_source_name(DSS_CLK_SRC_DSI_PLL_HSDIV_DSI),
 		cinfo->dsi_pll_hsdiv_dsi_clk);
 
+	dss_feat_get_reg_field(FEAT_REG_DSIPLL_REGN, &regn_start, &regn_end);
+	dss_feat_get_reg_field(FEAT_REG_DSIPLL_REGM, &regm_start, &regm_end);
+	dss_feat_get_reg_field(FEAT_REG_DSIPLL_REGM_DISPC, &regm_dispc_start,
+			&regm_dispc_end);
+	dss_feat_get_reg_field(FEAT_REG_DSIPLL_REGM_DSI, &regm_dsi_start,
+			&regm_dsi_end);
+
 	REG_FLD_MOD(DSI_PLL_CONTROL, 0, 0, 0); /* DSI_PLL_AUTOMODE = manual */
 
 	l = dsi_read_reg(DSI_PLL_CONFIGURATION1);
 	l = FLD_MOD(l, 1, 0, 0);		/* DSI_PLL_STOPMODE */
-	l = FLD_MOD(l, cinfo->regn - 1, 7, 1);	/* DSI_PLL_REGN */
-	l = FLD_MOD(l, cinfo->regm, 18, 8);	/* DSI_PLL_REGM */
+	/* DSI_PLL_REGN */
+	l = FLD_MOD(l, cinfo->regn - 1, regn_start, regn_end);
+	/* DSI_PLL_REGM */
+	l = FLD_MOD(l, cinfo->regm, regm_start, regm_end);
+	/* DSI_CLOCK_DIV */
 	l = FLD_MOD(l, cinfo->regm_dispc > 0 ? cinfo->regm_dispc - 1 : 0,
-			22, 19);		/* DSI_CLOCK_DIV */
+			regm_dispc_start, regm_dispc_end);
+	/* DSIPROTO_CLOCK_DIV */
 	l = FLD_MOD(l, cinfo->regm_dsi > 0 ? cinfo->regm_dsi - 1 : 0,
-			26, 23);		/* DSIPROTO_CLOCK_DIV */
+			regm_dsi_start, regm_dsi_end);
 	dsi_write_reg(DSI_PLL_CONFIGURATION1, l);
 
-	BUG_ON(cinfo->fint < 750000 || cinfo->fint > 2100000);
+	BUG_ON(cinfo->fint < dsi.fint_min || cinfo->fint > dsi.fint_max);
 	if (cinfo->fint < 1000000)
 		f = 0x3;
 	else if (cinfo->fint < 1250000)
@@ -3333,6 +3345,25 @@ void dsi_wait_pll_hsdiv_dsi_active(void)
 			dss_feat_get_clk_source_name(DSS_CLK_SRC_DSI_PLL_HSDIV_DSI));
 }
 
+static void dsi_calc_clock_param_ranges(void)
+{
+	u8 start, end;
+
+	dss_feat_get_reg_field(FEAT_REG_DSIPLL_REGM, &start, &end);
+	dsi.regm_max = DSI_PARAM_MAX(start, end);
+
+	dss_feat_get_reg_field(FEAT_REG_DSIPLL_REGN, &start, &end);
+	dsi.regn_max = DSI_PARAM_MAX(start, end);
+
+	dss_feat_get_reg_field(FEAT_REG_DSIPLL_REGM_DISPC, &start, &end);
+	dsi.regm_dispc_max = DSI_PARAM_MAX(start, end);
+
+	dss_feat_get_reg_field(FEAT_REG_DSIPLL_REGM_DSI, &start, &end);
+	dsi.regm_dsi_max = DSI_PARAM_MAX(start, end);
+
+	dss_feat_get_param_range(FEAT_DSI_FINT, &dsi.fint_min, &dsi.fint_max);
+}
+
 static int dsi_init(struct platform_device *pdev)
 {
 	u32 rev;
@@ -3397,6 +3428,8 @@ static int dsi_init(struct platform_device *pdev)
 		dsi.vc[i].vc_id = 0;
 	}
 
+	dsi_calc_clock_param_ranges();
+
 	enable_clocks(1);
 
 	rev = dsi_read_reg(DSI_REVISION);
diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index 6eb6ec6..d458da0 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -34,6 +34,11 @@ struct dss_reg_field {
 	u8 start, end;
 };
 
+struct dss_param_range {
+	enum dss_range_param param;
+	int min, max;
+};
+
 struct omap_dss_features {
 	const struct dss_reg_field *reg_fields;
 	const int num_reg_fields;
@@ -46,6 +51,7 @@ struct omap_dss_features {
 	const enum omap_display_type *supported_displays;
 	const enum omap_color_mode *supported_color_modes;
 	const struct dss_clk_source_name *clksrc_names;
+	const struct dss_param_range *dss_params;
 };
 
 /* This struct is assigned to one of the below during initialization */
@@ -60,6 +66,10 @@ static const struct dss_reg_field omap2_dss_reg_fields[] = {
 	{ FEAT_REG_HORIZONTALACCU, 9, 0 },
 	{ FEAT_REG_VERTICALACCU, 25, 16 },
 	{ FEAT_REG_DISPC_CLK_SWITCH, 0, 0 },
+	{ FEAT_REG_DSIPLL_REGM, 0, 0 },
+	{ FEAT_REG_DSIPLL_REGN, 0, 0 },
+	{ FEAT_REG_DSIPLL_REGM_DISPC, 0, 0 },
+	{ FEAT_REG_DSIPLL_REGM_DSI, 0, 0 },
 };
 
 static const struct dss_reg_field omap3_dss_reg_fields[] = {
@@ -71,6 +81,10 @@ static const struct dss_reg_field omap3_dss_reg_fields[] = {
 	{ FEAT_REG_HORIZONTALACCU, 9, 0 },
 	{ FEAT_REG_VERTICALACCU, 25, 16 },
 	{ FEAT_REG_DISPC_CLK_SWITCH, 0, 0 },
+	{ FEAT_REG_DSIPLL_REGM, 18, 8 },
+	{ FEAT_REG_DSIPLL_REGN, 7, 1 },
+	{ FEAT_REG_DSIPLL_REGM_DISPC, 22, 19 },
+	{ FEAT_REG_DSIPLL_REGM_DSI, 26, 23 },
 };
 
 static const struct dss_reg_field omap4_dss_reg_fields[] = {
@@ -82,6 +96,10 @@ static const struct dss_reg_field omap4_dss_reg_fields[] = {
 	{ FEAT_REG_HORIZONTALACCU, 10, 0 },
 	{ FEAT_REG_VERTICALACCU, 26, 16 },
 	{ FEAT_REG_DISPC_CLK_SWITCH, 9, 8 },
+	{ FEAT_REG_DSIPLL_REGM, 20, 9 },
+	{ FEAT_REG_DSIPLL_REGN, 8, 1 },
+	{ FEAT_REG_DSIPLL_REGM_DISPC, 25, 21 },
+	{ FEAT_REG_DSIPLL_REGM_DSI, 30, 26 },
 };
 
 static const enum omap_display_type omap2_dss_supported_displays[] = {
@@ -180,6 +198,18 @@ static const struct dss_clk_source_name omap4_dss_clk_source_names[] = {
 	{ DSS_CLK_SRC_FCK, "DSS_FCLK" },
 };
 
+static const struct dss_param_range omap2_dss_param_range[] = {
+	{ FEAT_DSI_FINT, 0, 0 },
+};
+
+static const struct dss_param_range omap3_dss_param_range[] = {
+	{ FEAT_DSI_FINT, 750000, 2100000 },
+};
+
+static const struct dss_param_range omap4_dss_param_range[] = {
+	{ FEAT_DSI_FINT, 500000, 2500000 },
+};
+
 /* OMAP2 DSS Features */
 static struct omap_dss_features omap2_dss_features = {
 	.reg_fields = omap2_dss_reg_fields,
@@ -196,6 +226,7 @@ static struct omap_dss_features omap2_dss_features = {
 	.supported_displays = omap2_dss_supported_displays,
 	.supported_color_modes = omap2_dss_supported_color_modes,
 	.clksrc_names = omap2_dss_clk_source_names,
+	.dss_params = omap2_dss_param_range,
 };
 
 /* OMAP3 DSS Features */
@@ -215,6 +246,7 @@ static struct omap_dss_features omap3430_dss_features = {
 	.supported_displays = omap3430_dss_supported_displays,
 	.supported_color_modes = omap3_dss_supported_color_modes,
 	.clksrc_names = omap3_dss_clk_source_names,
+	.dss_params = omap3_dss_param_range,
 };
 
 static struct omap_dss_features omap3630_dss_features = {
@@ -234,6 +266,7 @@ static struct omap_dss_features omap3630_dss_features = {
 	.supported_displays = omap3630_dss_supported_displays,
 	.supported_color_modes = omap3_dss_supported_color_modes,
 	.clksrc_names = omap3_dss_clk_source_names,
+	.dss_params = omap3_dss_param_range,
 };
 
 /* OMAP4 DSS Features */
@@ -252,6 +285,7 @@ static struct omap_dss_features omap4_dss_features = {
 	.supported_displays = omap4_dss_supported_displays,
 	.supported_color_modes = omap3_dss_supported_color_modes,
 	.clksrc_names = omap4_dss_clk_source_names,
+	.dss_params = omap4_dss_param_range,
 };
 
 /* Functions returning values related to a DSS feature */
@@ -293,6 +327,12 @@ const char *dss_feat_get_clk_source_name(enum dss_clk_source id)
 	return omap_current_dss_features->clksrc_names[id].clksrc_name;
 }
 
+void dss_feat_get_param_range(enum dss_range_param param, int *min, int *max)
+{
+	*min = omap_current_dss_features->dss_params[param].min;
+	*max = omap_current_dss_features->dss_params[param].max;
+}
+
 /* DSS has_feature check */
 bool dss_has_feature(enum dss_feat_id id)
 {
diff --git a/drivers/video/omap2/dss/dss_features.h b/drivers/video/omap2/dss/dss_features.h
index 3302247..78b51a9 100644
--- a/drivers/video/omap2/dss/dss_features.h
+++ b/drivers/video/omap2/dss/dss_features.h
@@ -52,6 +52,14 @@ enum dss_feat_reg_field {
 	FEAT_REG_HORIZONTALACCU,
 	FEAT_REG_VERTICALACCU,
 	FEAT_REG_DISPC_CLK_SWITCH,
+	FEAT_REG_DSIPLL_REGM,
+	FEAT_REG_DSIPLL_REGN,
+	FEAT_REG_DSIPLL_REGM_DISPC,
+	FEAT_REG_DSIPLL_REGM_DSI,
+};
+
+enum dss_range_param {
+	FEAT_DSI_FINT,
 };
 
 /* DSS Feature Functions */
@@ -63,6 +71,7 @@ enum omap_color_mode dss_feat_get_supported_color_modes(enum omap_plane plane);
 bool dss_feat_color_mode_supported(enum omap_plane plane,
 		enum omap_color_mode color_mode);
 const char *dss_feat_get_clk_source_name(enum dss_clk_source id);
+void dss_feat_get_param_range(enum dss_range_param param, int *min, int *max);
 
 bool dss_has_feature(enum dss_feat_id id);
 void dss_feat_get_reg_field(enum dss_feat_reg_field id, u8 *start, u8 *end);
-- 
1.7.1

Re: [PATCH] OMAP: DSS2: FEATURES: DSI PLL parameter cleanup

[Tomi Valkeinen]

On Fri, 2011-03-04 at 05:03 -0600, Taneja, Archit wrote:
> The DSI PLL parameters (regm, regn, regm_dispc, regm_dsi, fint) have different
> fields and also different Max values on OMAP3 and OMAP4. Use dss features to
> calculate the register fields and Max values based on current OMAP revision
> 
> Also, introduce a new function in dss_features "dss_feat_get_param_range" which
> returns the maximum and minimum values supported by the parameter for
> the current OMAP revision.

See comment about "also" in the last mail =). You are introducing a new
kind of dss feature as a sidenote. I think it should clearly be a
separate patch.

<snip>

> +void dss_feat_get_param_range(enum dss_range_param param, int *min, int *max)
> +{
> +	*min = omap_current_dss_features->dss_params[param].min;
> +	*max = omap_current_dss_features->dss_params[param].max;

This isn't right. Here you're using the param as index, but in the param
array itself the param is just part of the struct. So it's just luck
that the index is correct.

This is actually wrong with the reg fields and clock sources also...

I believe there was a way to give entries in an array by the index.
Something like:

static struct foo bar[] = {
	[0] = { stuff },
	[1] = { stuff },
};

In that solution it's not necessary to have the index in the struct
itself, like now.

Alternatively, with the current struct, we could iterate through the
array to find a matching id.

> +}
> +
>  /* DSS has_feature check */
>  bool dss_has_feature(enum dss_feat_id id)
>  {
> diff --git a/drivers/video/omap2/dss/dss_features.h b/drivers/video/omap2/dss/dss_features.h
> index 3302247..78b51a9 100644
> --- a/drivers/video/omap2/dss/dss_features.h
> +++ b/drivers/video/omap2/dss/dss_features.h
> @@ -52,6 +52,14 @@ enum dss_feat_reg_field {
>  	FEAT_REG_HORIZONTALACCU,
>  	FEAT_REG_VERTICALACCU,
>  	FEAT_REG_DISPC_CLK_SWITCH,
> +	FEAT_REG_DSIPLL_REGM,
> +	FEAT_REG_DSIPLL_REGN,
> +	FEAT_REG_DSIPLL_REGM_DISPC,
> +	FEAT_REG_DSIPLL_REGM_DSI,
> +};
> +
> +enum dss_range_param {
> +	FEAT_DSI_FINT,
>  };
>  
>  /* DSS Feature Functions */
> @@ -63,6 +71,7 @@ enum omap_color_mode dss_feat_get_supported_color_modes(enum omap_plane plane);
>  bool dss_feat_color_mode_supported(enum omap_plane plane,
>  		enum omap_color_mode color_mode);
>  const char *dss_feat_get_clk_source_name(enum dss_clk_source id);
> +void dss_feat_get_param_range(enum dss_range_param param, int *min, int *max);

Currently used only for fint, but if it's supposed to handle clock
rates, unsigned long instead of int could be better.

I was also thinking that we now have dss_feat_get_max_dss_fck(), which
is somewhat similar to this.

Could something like this work:

unsigned long dss_feat_get_param_min(enum dss_range_param param);
unsigned long dss_feat_get_param_max(enum dss_range_param param);

That would make the usage cleaner for cases where you're only interested
in the max.

 Tomi

Re: [PATCH] OMAP: DSS2: FEATURES: DSI PLL parameter cleanup

[archit taneja]

Hi,

On Monday 07 March 2011 01:24 PM, Valkeinen, Tomi wrote:
> On Fri, 2011-03-04 at 05:03 -0600, Taneja, Archit wrote:
>> The DSI PLL parameters (regm, regn, regm_dispc, regm_dsi, fint) have different
>> fields and also different Max values on OMAP3 and OMAP4. Use dss features to
>> calculate the register fields and Max values based on current OMAP revision
>>
>> Also, introduce a new function in dss_features "dss_feat_get_param_range" which
>> returns the maximum and minimum values supported by the parameter for
>> the current OMAP revision.
>
> See comment about "also" in the last mail =). You are introducing a new
> kind of dss feature as a sidenote. I think it should clearly be a
> separate patch.
>
> <snip>
>
>> +void dss_feat_get_param_range(enum dss_range_param param, int *min, int *max)
>> +{
>> +	*min = omap_current_dss_features->dss_params[param].min;
>> +	*max = omap_current_dss_features->dss_params[param].max;
>
> This isn't right. Here you're using the param as index, but in the param
> array itself the param is just part of the struct. So it's just luck
> that the index is correct.

The param part of the struct is just for readability, and possibly more 
thorough checking for later on.

The index isn't correct by luck, its mainly because during defining the 
array, we fill it up in the sequence in which the enum is defined.

>
> This is actually wrong with the reg fields and clock sources also...
>
> I believe there was a way to give entries in an array by the index.
> Something like:
>
> static struct foo bar[] = {
> 	[0] = { stuff },
> 	[1] = { stuff },
> };

I agree this is better , I didn't know we could use this.

>
> In that solution it's not necessary to have the index in the struct
> itself, like now.
>
> Alternatively, with the current struct, we could iterate through the
> array to find a matching id.

Yes, my earlier patch for dss features used a list and we iterated to 
get the matching id, but that was bit of an overkill.

>
>> +}
>> +
>>   /* DSS has_feature check */
>>   bool dss_has_feature(enum dss_feat_id id)
>>   {
>> diff --git a/drivers/video/omap2/dss/dss_features.h b/drivers/video/omap2/dss/dss_features.h
>> index 3302247..78b51a9 100644
>> --- a/drivers/video/omap2/dss/dss_features.h
>> +++ b/drivers/video/omap2/dss/dss_features.h
>> @@ -52,6 +52,14 @@ enum dss_feat_reg_field {
>>   	FEAT_REG_HORIZONTALACCU,
>>   	FEAT_REG_VERTICALACCU,
>>   	FEAT_REG_DISPC_CLK_SWITCH,
>> +	FEAT_REG_DSIPLL_REGM,
>> +	FEAT_REG_DSIPLL_REGN,
>> +	FEAT_REG_DSIPLL_REGM_DISPC,
>> +	FEAT_REG_DSIPLL_REGM_DSI,
>> +};
>> +
>> +enum dss_range_param {
>> +	FEAT_DSI_FINT,
>>   };
>>
>>   /* DSS Feature Functions */
>> @@ -63,6 +71,7 @@ enum omap_color_mode dss_feat_get_supported_color_modes(enum omap_plane plane);
>>   bool dss_feat_color_mode_supported(enum omap_plane plane,
>>   		enum omap_color_mode color_mode);
>>   const char *dss_feat_get_clk_source_name(enum dss_clk_source id);
>> +void dss_feat_get_param_range(enum dss_range_param param, int *min, int *max);
>
> Currently used only for fint, but if it's supposed to handle clock
> rates, unsigned long instead of int could be better.
>
> I was also thinking that we now have dss_feat_get_max_dss_fck(), which
> is somewhat similar to this.
>
> Could something like this work:
>
> unsigned long dss_feat_get_param_min(enum dss_range_param param);
> unsigned long dss_feat_get_param_max(enum dss_range_param param);
>

I agree. Should we remove the max_dss_fck patch since we are moving to 
this approach?

Archit

Re: [PATCH] OMAP: DSS2: FEATURES: DSI PLL parameter cleanup

[Tomi Valkeinen]

On Mon, 2011-03-07 at 03:17 -0600, Taneja, Archit wrote:
> Hi,
> 
> On Monday 07 March 2011 01:24 PM, Valkeinen, Tomi wrote:
> > On Fri, 2011-03-04 at 05:03 -0600, Taneja, Archit wrote:
> >> The DSI PLL parameters (regm, regn, regm_dispc, regm_dsi, fint) have different
> >> fields and also different Max values on OMAP3 and OMAP4. Use dss features to
> >> calculate the register fields and Max values based on current OMAP revision
> >>
> >> Also, introduce a new function in dss_features "dss_feat_get_param_range" which
> >> returns the maximum and minimum values supported by the parameter for
> >> the current OMAP revision.
> >
> > See comment about "also" in the last mail =). You are introducing a new
> > kind of dss feature as a sidenote. I think it should clearly be a
> > separate patch.
> >
> > <snip>
> >
> >> +void dss_feat_get_param_range(enum dss_range_param param, int *min, int *max)
> >> +{
> >> +	*min = omap_current_dss_features->dss_params[param].min;
> >> +	*max = omap_current_dss_features->dss_params[param].max;
> >
> > This isn't right. Here you're using the param as index, but in the param
> > array itself the param is just part of the struct. So it's just luck
> > that the index is correct.
> 
> The param part of the struct is just for readability, and possibly more 
> thorough checking for later on.
> 
> The index isn't correct by luck, its mainly because during defining the 
> array, we fill it up in the sequence in which the enum is defined.

Well, by reading the code nothing tells the reader that the ordering is
important, he has to guess it. One could easily add a new item in wrong
way, and it would be very difficult to notice.

<snip>

> >
> > Could something like this work:
> >
> > unsigned long dss_feat_get_param_min(enum dss_range_param param);
> > unsigned long dss_feat_get_param_max(enum dss_range_param param);
> >
> 
> I agree. Should we remove the max_dss_fck patch since we are moving to 
> this approach?

Hmm. I guess it would cause a bit too many conflicts. I can check it
later if it's easy to remove, but for now let's just build on top of it.

 Tomi