On Tue, Mar 20, 2012 at 12:54:55AM -0700, Saravana Kannan wrote:

On Tue, March 20, 2012 12:20 am, Shawn Guo wrote:

quoted

On Mon, Mar 19, 2012 at 08:38:26PM -0700, Saravana Kannan wrote:

quoted

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

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.

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.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |