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