On Mon, 2012-08-13 at 17:29 +0530, Chandrabhanu Mahapatra wrote: > All the cpu_is checks have been moved to dss_init_features function providing a > much more generic and cleaner interface. The OMAP version and revision specific > initializations in various functions are cleaned and the necessary data are > moved to dss_features structure which is local to dss.c. It'd be good to have some version information here. Even just [PATCH v3 3/6] in the subject is good, but of course the best is if there's a short change log > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@xxxxxx> > --- > drivers/video/omap2/dss/dss.c | 115 ++++++++++++++++++++++++++--------------- > 1 file changed, 74 insertions(+), 41 deletions(-) > > diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c > index 7b1c6ac..6ab236e 100644 > --- a/drivers/video/omap2/dss/dss.c > +++ b/drivers/video/omap2/dss/dss.c > @@ -31,6 +31,7 @@ > #include <linux/clk.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > +#include <linux/dma-mapping.h> Hmm, what is this for? > #include <video/omapdss.h> > > @@ -65,6 +66,13 @@ struct dss_reg { > static int dss_runtime_get(void); > static void dss_runtime_put(void); > > +struct dss_features { > + u16 fck_div_max; > + int factor; > + char *clk_name; > + bool (*check_cinfo_fck) (void); > +}; Is the check_cinfo_fck a leftover from previous versions? I think "factor" name is too vague. If I remember right, it's some kind of implicit multiplier for the dss fck. So "dss_fck_multiplier"? You could check the clock trees to verify what the multiplier exactly was, and see if you can come up with a better name =). > + > static struct { > struct platform_device *pdev; > void __iomem *base; > @@ -83,6 +91,8 @@ static struct { > > bool ctx_valid; > u32 ctx[DSS_SZ_REGS / sizeof(u32)]; > + > + const struct dss_features *feat; > } dss; > > static const char * const dss_generic_clk_source_names[] = { > @@ -91,6 +101,30 @@ static const char * const dss_generic_clk_source_names[] = { > [OMAP_DSS_CLK_SRC_FCK] = "DSS_FCK", > }; > > +static const struct __initdata dss_features omap2_dss_features = { > + .fck_div_max = 16, > + .factor = 2, > + .clk_name = NULL, > +}; include/linux/init.h says says __initdata should be used like: static int init_variable __initdata = 0; And there actually seems to be __initconst also, which I think is the correct one to use here. Didn't know about that =): static const char linux_logo[] __initconst = { 0x32, 0x36, ... }; > +static const struct __initdata dss_features omap34_dss_features = { Perhaps the names could be more consistent. "omap34" doesn't sound like any omap we have ;). So maybe just omap2xxx, omap34xx, etc, the same way we have in the cpu_is calls. > + .fck_div_max = 16, > + .factor = 2, > + .clk_name = "dpll4_m4_ck", > +}; > + > +static const struct __initdata dss_features omap36_dss_features = { > + .fck_div_max = 32, > + .factor = 1, > + .clk_name = "dpll4_m4_ck", > +}; > + > +static const struct __initdata dss_features omap4_dss_features = { > + .fck_div_max = 32, > + .factor = 1, > + .clk_name = "dpll_per_m5x2_ck", > +}; > + > static inline void dss_write_reg(const struct dss_reg idx, u32 val) > { > __raw_writel(val, dss.base + idx.idx); > @@ -236,7 +270,6 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src) > return dss_generic_clk_source_names[clk_src]; > } > > - > void dss_dump_clocks(struct seq_file *s) > { > unsigned long dpll4_ck_rate; > @@ -259,18 +292,10 @@ void dss_dump_clocks(struct seq_file *s) > > seq_printf(s, "dpll4_ck %lu\n", dpll4_ck_rate); > > - if (cpu_is_omap3630() || cpu_is_omap44xx()) > - seq_printf(s, "%s (%s) = %lu / %lu = %lu\n", > - fclk_name, fclk_real_name, > - dpll4_ck_rate, > - dpll4_ck_rate / dpll4_m4_ck_rate, > - fclk_rate); > - else > - seq_printf(s, "%s (%s) = %lu / %lu * 2 = %lu\n", > - fclk_name, fclk_real_name, > - dpll4_ck_rate, > - dpll4_ck_rate / dpll4_m4_ck_rate, > - fclk_rate); > + seq_printf(s, "%s (%s) = %lu / %lu * %d = %lu\n", > + fclk_name, fclk_real_name, dpll4_ck_rate, > + dpll4_ck_rate / dpll4_m4_ck_rate, > + dss.feat->factor, fclk_rate); > } else { > seq_printf(s, "%s (%s) = %lu\n", > fclk_name, fclk_real_name, > @@ -470,7 +495,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo, > > unsigned long fck, max_dss_fck; > > - u16 fck_div, fck_div_max = 16; > + u16 fck_div; > > int match = 0; > int min_fck_per_pck; > @@ -480,9 +505,8 @@ 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 && prate == dss.cache_prate && > + dss.cache_dss_cinfo.fck == fck) { > DSSDBG("dispc clock info found from cache.\n"); > *dss_cinfo = dss.cache_dss_cinfo; > *dispc_cinfo = dss.cache_dispc_cinfo; > @@ -519,16 +543,10 @@ retry: > > goto found; > } else { > - if (cpu_is_omap3630() || cpu_is_omap44xx()) > - fck_div_max = 32; > - > - for (fck_div = fck_div_max; fck_div > 0; --fck_div) { > + for (fck_div = dss.feat->fck_div_max; fck_div > 0; --fck_div) { > struct dispc_clock_info cur_dispc; > > - if (fck_div_max == 32) > - fck = prate / fck_div; > - else > - fck = prate / fck_div * 2; > + fck = prate / fck_div * dss.feat->factor; > > if (fck > max_dss_fck) > continue; > @@ -633,22 +651,11 @@ static int dss_get_clocks(void) > > dss.dss_clk = clk; > > - if (cpu_is_omap34xx()) { > - clk = clk_get(NULL, "dpll4_m4_ck"); > - if (IS_ERR(clk)) { > - DSSERR("Failed to get dpll4_m4_ck\n"); > - r = PTR_ERR(clk); > - goto err; > - } > - } else if (cpu_is_omap44xx()) { > - clk = clk_get(NULL, "dpll_per_m5x2_ck"); > - if (IS_ERR(clk)) { > - DSSERR("Failed to get dpll_per_m5x2_ck\n"); > - r = PTR_ERR(clk); > - goto err; > - } > - } else { /* omap24xx */ > - clk = NULL; > + clk = clk_get(NULL, dss.feat->clk_name); > + if (IS_ERR(clk)) { > + DSSERR("Failed to get %s\n", dss.feat->clk_name); > + r = PTR_ERR(clk); > + goto err; > } > > dss.dpll4_m4_ck = clk; > @@ -704,6 +711,26 @@ void dss_debug_dump_clocks(struct seq_file *s) > } > #endif > > +static int __init dss_init_features(struct device *dev) > +{ > + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL); > + if (!dss.feat) { > + dev_err(dev, "Failed to allocate local DSS Features\n"); > + return -ENOMEM; > + } > + > + if (cpu_is_omap24xx()) > + dss.feat = &omap2_dss_features; > + else if (cpu_is_omap34xx()) > + dss.feat = &omap34_dss_features; > + else if (cpu_is_omap3630()) > + dss.feat = &omap36_dss_features; > + else > + dss.feat = &omap4_dss_features; Check for omap4 also, and if the cpu is none of the above, return error. > + > + return 0; > +} > + > /* DSS HW IP initialisation */ > static int __init omap_dsshw_probe(struct platform_device *pdev) > { > @@ -750,6 +777,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev) > dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK; > dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK; > > + r = dss_init_features(&dss.pdev->dev); > + if (r) > + return r; > + > rev = dss_read_reg(DSS_REVISION); > printk(KERN_INFO "OMAP DSS rev %d.%d\n", > FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0)); > @@ -772,6 +803,8 @@ static int __exit omap_dsshw_remove(struct platform_device *pdev) > > dss_put_clocks(); > > + devm_kfree(&dss.pdev->dev, (void *)dss.feat); You don't need to free the memory allocated with devm_kzalloc. The framework will free it automatically on unload (that's the idea of devm_* functions). Otherwise looks good, cleans up nicely the cpu_is_* mess. Tomi
Attachment:
signature.asc
Description: This is a digitally signed message part