On Wed, 26 Mar 2008 19:52:34 +0300
Dmitry <dbaryshkov@gmail.com> wrote:

quoted text
> Basically mode becomes enable/disable (however it may be better to merge back
> those pointers into one function). And dev and index go to priv data.

I think it would definitely help to combine some hooks into one.
enable/disable is one example, set_rate/round_rate is another.

quoted text
> The documentation will come later.

Hmm. Missing documentation makes it harder to review this stuff...

quoted text
> >  I have quite a few clocks, so the increased memory consumption is quite
> >  significant. What are the advantages of this?  
> 
> At maximum 55, IIUC. I counted 32 or so additional bytes in the struct
> (over avr32-specific one). That would count up to 1.5 K overhead. Is
> that really too much for current kernels?

If the only advantage is less code duplication, I'd say it's too much.
However...

quoted text
> OTOH this would bring unification of platform code, allow
> configurations when a non-platform driver would provide it's own
> clocks (think about multi-function companion chips when there is a
> "core" which manages "clocks" for it's "periferal" devices. Currently
> if one tries to implement such driver, he is forced to either bind it
> to platform code, or to implement non-standard
> my_device_clock_enable()-like functions.

...that is a good argument. External clock generators come to
mind...they can even be parents for other clocks.

quoted text
> Also you aren't forced to use this API. simply don't select
> HAVE_CLOCK_LIB and leave
> all things as they are. E.g. gpiolib is now merged, however not all
> gpio-providing platforms
> are using it.

Sure, but then I won't be able to use the suggested drivers that
depend on this stuff.

How about we try to slim things down a bit? Let's see...

quoted text
> +struct clk {
> +	struct list_head node;
> +	struct clk	*parent;
> +
> +	const char	*name;

I guess these are always required if we're going to do dynamic
registration...

quoted text
> +	struct module	*owner;

This will probably be unused for most platforms, but I guess we need it
to get the advantage you're talking about.

quoted text
> +	int		users;

Reference counting, probably need that too.

quoted text
> +	unsigned long	rate;

This one is redundant. Use getrate() instead.

quoted text
> +	int		delay;

Huh? A delay after enabling the clock? Why can't the enable() hook do
that if it's really necessary?

quoted text
> +	int (*can_get)	(struct clk *, struct device *);

What's this for? I'm assuming it's necessary to couple clocks to
specific devices?

quoted text
> +	int (*set_parent) (struct clk *, struct clk *);

We probably need this.

quoted text
> +	int (*enable)	(struct clk *);
> +	void (*disable)	(struct clk *);

Combine these into "mode" or something (with an extra parameter)

quoted text
> +	unsigned long (*getrate) (struct clk*);

We need this one.

quoted text
> +	int (*setrate)	(struct clk *, unsigned long);
> +	long (*roundrate) (struct clk *, unsigned long);

Combine these into one call with an extra "apply" parameter or
something. The underlying logic is pretty much the same apart from the
"actually write stuff to registers" step.

quoted text
> +	void		*priv;

Remove this and let platforms extend the struct instead. They can use
container_of() to access the extra fields, which is faster too.

quoted text
> +};

The result is something like this:

struct clk {
	struct list_head node;
	struct clk	*parent;

	const char	*name;
	struct module	*owner;

	int		users;

	int (*can_get)	(struct clk *, struct device *);
	int (*set_parent) (struct clk *, struct clk *);
	int (*mode)	(struct clk *, bool enabled);
	unsigned long (*getrate) (struct clk*);
	int (*setrate)	(struct clk *, unsigned long, bool apply);
};

which is 44 bytes, 12 bytes more than the avr32 version. That can
probably be explained by the "node" and "owner" fields, which really
are necessary in order to support dynamic registration of clocks.

Adding the "dev" and "index" fields takes us to 52 bytes, or 20 bytes
more. That's about 1.1K extra in total for 55 clocks, which is still a
bit much, but I can probably live with that.

Haavard
--
unsubscribe notice
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/