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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
