[PATCH 3/3] mtd: gpmi: change the code for clocks

[Dong Aisheng]

On Fri, Jun 29, 2012 at 05:29:26PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Fri, Jun 29, 2012 at 10:06:52AM +0800, Shawn Guo wrote:
> > > On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:
> > > > From: Huang Shijie <b32955 at freescale.com>
> > > > 
> > > > The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
> > > > 
> > > > In the old clock framework, all these clocks are chained together,
> > > > all you need is to manipulate the first clock.
> > > > 
> > > > But the kernel uses the common clk framework now, which forces us to
> > > > get the clocks one by one. When we use them, we have to enable them
> > > > one by one too.
> > > > 
> [...]
> > > > +static char *extra_clks_for_mx6q[] = {
> > > > +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
> > > > +};
> > > > +
> > > > +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
> > > > +{
> > > > +	struct resources *r = &this->resources;
> > > > +	char **extra_clks = NULL;
> > > > +	struct clk *clk;
> > > > +	int i;
> > > > +
> > > > +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
> > > > +	if (IS_ERR(r->clock[0]))
> > > > +		goto err_clock;
> > > > +
> > > > +	/* Get extra clocks */
> > > > +	if (GPMI_IS_MX6Q(this))
> > > > +		extra_clks = extra_clks_for_mx6q;
> > > 
> > > We probably do not need this tweaking.  We can have the driver always
> > > take all those 5 clocks, and I think the current imx28 clock driver
> > > can just work with it, because the gpmi-nand clkdev lookup has NULL
> > > con_id and all those 5 clocks can match the same one on imx28.
> > > 
> > Will mx28 fail if doing like that?
> > clk_get will fail if no clock found.
> > struct clk *clk_get_sys(const char *dev_id, const char *con_id)
> > {
> >         struct clk_lookup *cl;
> > 
> >         mutex_lock(&clocks_mutex);
> >         cl = clk_find(dev_id, con_id);
> >         if (cl && !__clk_get(cl->clk))
> >                 cl = NULL;
> >         mutex_unlock(&clocks_mutex);
> > 
> >         return cl ? cl->clk : ERR_PTR(-ENOENT);
> > }
> > EXPORT_SYMBOL(clk_get_sys);
> > 
> > Furthermore, find unnecessary clock for mx28 seems not a good choice.
> > Probably a better way is to define each SoC required clocks and get them
> > respectively. It's explicit and clear.
> > 
> No, that's silly. You would have to change the driver for each
> new platform that the driver can support.
> 
If the new platform is fully compatible with exist platforms, then no.
If need more clocks, then in either way, we have to add support in driver.

> Instead the driver should request the maximum number of clocks that
> the existing set of platforms provide and all platforms with fewer
> clocks should provide an appropriate number of dummy clocks.
> 
I wish we can not use dummy since it's easy causing misleading until
we have no other better way to go.

> This way new platforms can be supported without any change to the
> driver and only if a platform requires even more clocks (like in this
> particular case) would some code outside that platform have to be
> changed at all.

Regards
Dong Aisheng