linux-kernel - Re: [RFC,PATCH 1/2] Add a common struct clk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201006031121.21896.jeremy.kerr@canonical.com>
Date:	Thu, 3 Jun 2010 11:21:19 +0800
From:	Jeremy Kerr <jeremy.kerr@...onical.com>
To:	Ben Dooks <ben-linux@...ff.org>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Ben Herrenchmidt <benh@...nel.crashing.org>
Subject: Re: [RFC,PATCH 1/2] Add a common struct clk

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/