linux-kernel - Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure

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]

Date:	Sat, 24 Sep 2011 22:26:51 -0700
From:	"Turquette, Mike" <mturquette@...com>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	jeremy.kerr@...onical.com, broonie@...nsource.wolfsonmicro.com,
	tglx@...utronix.de, linus.walleij@...ricsson.com,
	amit.kucheria@...aro.org, dsaxena@...aro.org, patches@...aro.org,
	linaro-dev@...ts.linaro.org, paul@...an.com, sboyd@...inc.com,
	shawn.guo@...escale.com, skannan@...cinc.com,
	magnus.damm@...il.com, arnd.bergmann@...aro.org,
	linux@....linux.org.uk, eric.miao@...aro.org,
	richard.zhao@...aro.org
Subject: Re: [PATCH v2 1/7] clk: Add a generic clock infrastructure

On Sat, Sep 24, 2011 at 8:55 PM, Grant Likely <grant.likely@...retlab.ca> wrote:
> On Thu, Sep 22, 2011 at 03:26:56PM -0700, Mike Turquette wrote:
>> From: Jeremy Kerr <jeremy.kerr@...onical.com>
>>
>> We currently have ~21 definitions of struct clk in the ARM architecture,
>> each defined on a per-platform basis. This makes it difficult to define
>> platform- (or architecture-) independent clock sources without making
>> assumptions about struct clk, and impossible to compile two
>> platforms with different struct clks into a single image.
>>
>> This change is an effort to unify struct clk where possible, by defining
>> a common struct clk, and a set of clock operations. Different clock
>> implementations can set their own operations, and have a standard
>> interface for generic code. The callback interface is exposed to the
>> kernel proper, while the clock implementations only need to be seen by
>> the platform internals.
>>
>> The interface is split into two halves:
>>
>>  * struct clk, which is the generic-device-driver interface. This
>>    provides a set of functions which drivers may use to request
>>    enable/disable, query or manipulate in a hardware-independent manner.
>>
>>  * struct clk_hw and struct clk_hw_ops, which is the hardware-specific
>>    interface. Clock drivers implement the ops, which allow the core
>>    clock code to implement the generic 'struct clk' API.
>>
>> This allows us to share clock code among platforms, and makes it
>> possible to dynamically create clock devices in platform-independent
>> code.
>>
>> Platforms can enable the generic struct clock through
>> CONFIG_GENERIC_CLK. In this case, the clock infrastructure consists of a
>> common, opaque struct clk, and a set of clock operations (defined per
>> type of clock):
>>
>>   struct clk_hw_ops {
>>       int             (*prepare)(struct clk_hw *);
>>       void            (*unprepare)(struct clk_hw *);
>>       int             (*enable)(struct clk_hw *);
>>       void            (*disable)(struct clk_hw *);
>>       unsigned long   (*recalc_rate)(struct clk_hw *);
>>       int             (*set_rate)(struct clk_hw *,
>>                                       unsigned long, unsigned long *);
>>       long            (*round_rate)(struct clk_hw *, unsigned long);
>>       int             (*set_parent)(struct clk_hw *, struct clk *);
>>       struct clk *    (*get_parent)(struct clk_hw *);
>>   };
>>
>> Platform clock code can register a clock through clk_register, passing a
>> set of operations, and a pointer to hardware-specific data:
>>
>>   struct clk_hw_foo {
>>       struct clk_hw clk;
>>       void __iomem *enable_reg;
>>   };
>>
>>   #define to_clk_foo(c) offsetof(c, clk_hw_foo, clk)
>>
>>   static int clk_foo_enable(struct clk_hw *clk)
>>   {
>>       struct clk_foo *foo = to_clk_foo(clk);
>>       raw_writeb(foo->enable_reg, 1);
>>       return 0;
>>   }
>>
>>   struct clk_hw_ops clk_foo_ops = {
>>       .enable = clk_foo_enable,
>>   };
>>
>> And in the platform initialisation code:
>>
>>   struct clk_foo my_clk_foo;
>>
>>   void init_clocks(void)
>>   {
>>       my_clk_foo.enable_reg = ioremap(...);
>>
>>       clk_register(&clk_foo_ops, &my_clk_foo, NULL);
>
> Shouldn't this be:
>
>        clk_register(&clk_foo_ops, &my_clk_foo->clk, NULL);
>
> ?
>
> Also, this documentation would be good to have in the Documentation
> directory instead of lost in a commit header.

Thanks for your review Grant.  Will fix the changelog and add proper
Documentation/ in the next round.

Regards,
Mike

> Otherwise looks okay to me.
>
> Reviewed-by: Grant Likely <grant.likely@...retlab.ca>
>
>
--
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/