linux-kernel - Re: [PATCH 2/2] clk: Move init fields from clk to 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]
Date:	Tue, 20 Mar 2012 03:17:10 -0700 (PDT)
From:	"Saravana Kannan" <skannan@...eaurora.org>
To:	"Sascha Hauer" <s.hauer@...gutronix.de>
Cc:	"Saravana Kannan" <skannan@...eaurora.org>,
	"Shawn Guo" <shawn.guo@...aro.org>,
	"Mike Turquette" <mturquette@...aro.org>,
	"Arnd Bergman" <arnd.bergmann@...aro.org>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, "Andrew Lunn" <andrew@...n.ch>,
	"Rob Herring" <rob.herring@...xeda.com>,
	"Russell King" <linux@....linux.org.uk>,
	"Jeremy Kerr" <jeremy.kerr@...onical.com>,
	"Thomas Gleixner" <tglx@...utronix.de>,
	"Paul Walmsley" <paul@...an.com>,
	"Shawn Guo" <shawn.guo@...escale.com>,
	"Jamie Iles" <jamie@...ieiles.com>,
	"Richard Zhao" <richard.zhao@...aro.org>,
	"Magnus Damm" <magnus.damm@...il.com>,
	"Mark Brown" <broonie@...nsource.wolfsonmicro.com>,
	"Linus Walleij" <linus.walleij@...ricsson.com>,
	"Stephen Boyd" <sboyd@...eaurora.org>,
	"Amit Kucheria" <amit.kucheria@...aro.org>,
	"Deepak Saxena" <dsaxena@...aro.org>,
	"Grant Likely" <grant.likely@...retlab.ca>
Subject: Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw


On Tue, March 20, 2012 2:40 am, Sascha Hauer wrote:
> On Tue, Mar 20, 2012 at 12:54:55AM -0700, Saravana Kannan wrote:
>>
>> On Tue, March 20, 2012 12:20 am, Shawn Guo wrote:
>> > On Mon, Mar 19, 2012 at 08:38:26PM -0700, Saravana Kannan wrote:
>> >> This has a couple of advantages:
>> >> * Completely hides struct clk from many clock platform drivers and
>> >> static
>> >>   clock initialization code.
>> >> * Simplifies the generic clk_register() function and allows adding
>> >> optional
>> >>   fields in the future without modifying the function signature.
>> >> * Allows for simpler static initialization of clocks on all platforms
>> by
>> >>   removing the need for forward delcarations.
>> >> * Halves the number of symbols added for each static clock
>> >> initialization.
>> >>
>> >> Signed-off-by: Saravana Kannan <skannan@...eaurora.org>
>> >
>> > I agree this is a reasonable move.  But while you simplify the
>> interface
>> > of clk_register(), why not making a further step to simplify the
>> > following interfaces simple too?
>> >
>> > struct clk *clk_register_fixed_rate(struct device *dev, const char
>> *name,
>> >                 const char *parent_name, unsigned long flags,
>> >                 unsigned long fixed_rate);
>> > struct clk *clk_register_gate(struct device *dev, const char *name,
>> >                 const char *parent_name, unsigned long flags,
>> >                 void __iomem *reg, u8 bit_idx,
>> >                 u8 clk_gate_flags, spinlock_t *lock);
>> > struct clk *clk_register_divider(struct device *dev, const char *name,
>> >                 const char *parent_name, unsigned long flags,
>> >                 void __iomem *reg, u8 shift, u8 width,
>> >                 u8 clk_divider_flags, spinlock_t *lock);
>> > struct clk *clk_register_mux(struct device *dev, const char *name,
>> >                 char **parent_names, u8 num_parents, unsigned long
>> flags,
>> >                 void __iomem *reg, u8 shift, u8 width,
>> >                 u8 clk_mux_flags, spinlock_t *lock);
>>
>> If you simplify those functions further. They would just become
>> clk_register(). I'm not sure I see a value in them in at that point or
>> even in their current form. But if others see (I'm guessing since they
>> acked or didn't nack it), I'm not going to ask to remove them. If
>> everyone
>> agrees that we should just remove them, I would be glad to.
>>
>> It's arguable that these functions for the common hardware types saves
>> the
>> need to deal with the kalloc in every platform driver. But it's not
>> clear
>> to me where they would get these parameters in the first place. Most
>> likely form some sort of static array. At which point, it might as well
>> be
>> a static array of pointers to clk_gated.hw, clk_fixed_rate.hw, etc
>> instead
>> of a platform specific  struct to hold these initializers.
>
> I am using these functions and don't need a static array, I just call
> the functions with the desired parameters.

Sure, then let's leave it in. Curious, where do you get the desired
parameters from? Is it static date in code or is it from DT? You somehow
probe it?

> Overall the clock framework was written in a way that we have to expose
> as little information about the internally used structs as necessary. It
> seems your patches are pulling in the opposite direction now.

I'm not exposing anything that you don't already pass from the platform
driver. Also, you realize that this is very similar to what you suggested
with clk_initializer right? If there is a strong push, we can make a copy
of these inside the struct clk, but for these few init fields I don't see
a point (see earlier email).

-Saravana


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.


--
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/