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

[archit taneja <archit@ti.com>]

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