On Fri, Jun 29, 2012 at 05:29:26PM +0800, Lothar Wa?mann wrote:

Hi,

Dong Aisheng writes:

quoted

On Fri, Jun 29, 2012 at 10:06:52AM +0800, Shawn Guo wrote:

quoted

On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:

quoted

From: Huang Shijie <redacted>

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.

[...]

quoted

quoted

quoted

+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