From: Jeremy Kerr <jeremy.kerr@canonical.com>
To: Ben Dooks <ben-linux@fluff.org>
Subject: Re: [RFC,PATCH 1/2] Add a common struct clk
Date: Thu, 3 Jun 2010 11:21:19 +0800
User-Agent: KMail/1.13.2 (Linux/2.6.32-22-generic; KDE/4.4.2; x86_64; ; )
Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
       Ben Herrenchmidt <benh@kernel.crashing.org>
References: <1275479804.137633.565764505843.0.gpush@pororo> <1275479804.138101.137592675542.1.gpush@pororo> <20100602120356.GQ7248@trinity.fluff.org>
In-Reply-To: <20100602120356.GQ7248@trinity.fluff.org>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Message-Id: <201006031121.21896.jeremy.kerr@canonical.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2162
Lines: 87

Hi Ben,

> > And a set of clock operations (defined per type of clock):
> > 
> > struct clk_operations {
> > 
> >        int             (*enable)(struct clk *);
> 
> I'd rather the enable/disable calls where simply a set
> and a bool on/off, very rarelyt is the enable and disable
> operartions different.

I thought about merging these, but decided against it. It does work for the 
simple case where we're setting a bit in a register:

static int clk_foo_set_state(struct clk *_clk, int enable)
{
	struct clk_foo *clk = to_clk_foo(_clk)
	u32 reg;

	reg = raw_readl(foo->some_register);
	if (enable)
		reg |= FOO_ENABLE;
	else
		reg &= ~FOO_ENABLE;
	raw_writel(foo->some_register, reg);

	return 0;
}

However, for anything more complex than this - for example, if there's a 
parent clock - then we start getting pretty messy:

static int clk_foo_set_state(struct clk *_clk, int enable)
{
	struct clk_foo *clk = to_clk_foo(_clk)
	u32 reg;

	if (enable) {
		int ret = clk_enable(clk->parent);
		if (ret)
			return ret;
	}

	reg = raw_readl(foo->some_register);
	if (enable)
		reg |= FOO_ENABLE;
	else
		reg &= ~FOO_ENABLE;

	raw_writel(foo->some_register, reg);

	if (!enable)
		clk_disable(clk->parent);

	return 0;
}

- where most of the function becomes surrounded by "if (enable)" statements. 

I'm aware that we can turn this into a conditional call of clk_foo_enable or 
clk_foo_disable, but then we're back to square 1. I also think that the simple 
case is clearer (if a little more verbose) with separate functions.

Also, enable and disable in the external clock API have different return 
types.

> an aside, you might want to just clal these clk_ops to get into the
> spirit of the original naming.

Either is fine with me - looks like 'ops' is more commonly used:

$ git grep -E '^struct \w*operations\s*\{' include/ | wc -l
30

$ git grep -E '^struct \w*ops\s*{' include/ | wc -l
138

Cheers,


Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/