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
--
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
