Re: [PATCH 3/6] OMAPDSS: DSS: Remove cpu_is_xxxx checks

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2515 bytes --]

On Tue, 2012-08-07 at 13:58 +0530, Chandrabhanu Mahapatra wrote:
> The cpu_is checks have been removed from dss.c providing it a much generic and
> cleaner interface. The OMAP version and revision specific functions are
> initialized by dss_ops structure in dss features.
> 
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
>  drivers/video/omap2/dss/dss.c          |  114 ++++++++++++++++++++++----------
>  drivers/video/omap2/dss/dss.h          |   23 ++++++-
>  drivers/video/omap2/dss/dss_features.c |   40 +++++++++++
>  drivers/video/omap2/dss/dss_features.h |    1 +
>  4 files changed, 141 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> index 7b1c6ac..c263da7 100644
> --- a/drivers/video/omap2/dss/dss.c
> +++ b/drivers/video/omap2/dss/dss.c
> @@ -83,6 +83,8 @@ static struct {
>  
>  	bool		ctx_valid;
>  	u32		ctx[DSS_SZ_REGS / sizeof(u32)];
> +
> +	const struct dss_ops *ops;
>  } dss;
>  
>  static const char * const dss_generic_clk_source_names[] = {
> @@ -236,6 +238,15 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src)
>  	return dss_generic_clk_source_names[clk_src];
>  }
>  
> +char *set_dump_clk_str_24_34(void)
> +{
> +	return "%s (%s) = %lu / %lu * 2  = %lu\n";
> +}
> +
> +char *set_dump_clk_str(void)
> +{
> +	return "%s (%s) = %lu / %lu  = %lu\n";
> +}

You don't need functions for these, or for the max fck div. Just add the
string or the number to the struct.

And I don't really like the format string being separately. It would
probably be better if you instead had just a flag for the x2 multiplier,
and the dump function would check the flag to see which format string it
uses.

> @@ -479,10 +511,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>  
>  	max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
>  
> -	fck = clk_get_rate(dss.dss_clk);
> -	if (req_pck == dss.cache_req_pck &&
> -			((cpu_is_omap34xx() && prate == dss.cache_prate) ||
> -			 dss.cache_dss_cinfo.fck == fck)) {
> +	if (req_pck == dss.cache_req_pck && dss.ops->set_dss_cinfo()) {
>  		DSSDBG("dispc clock info found from cache.\n");
>  		*dss_cinfo = dss.cache_dss_cinfo;
>  		*dispc_cinfo = dss.cache_dispc_cinfo;

This is quite confusing. "set" function should set something, normally
something that is given as a parameter. Your function seems to be
checking some values.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]