Re: [PATCH] clk: add si5351 i2c common clock driver

From: Sebastian Hesselbarth
Date: Mon Feb 11 2013 - 04:52:24 EST

On 02/11/2013 06:46 AM, Mike Turquette wrote:
Quoting Sebastian Hesselbarth (2013-02-09 04:59:32)
This patch adds a common clock driver for Silicon Labs Si5351a/b/c
i2c programmable clock generators. Currently, the driver supports
DT kernels only and VXCO feature of si5351b is not implemented. DT
bindings selectively allow to overwrite stored Si5351 configuration
which is very helpful for clock generators with empty eeprom
configuration. Corresponding device tree binding documentation is
also added.

Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@xxxxxxxxx>
- During development I used a debugfs clock consumer that I can also
post if there is interest in it.

Please do. I have a set of patches that implement a fake clock subtree
for testing the core framework. I've been thinking of pushing this to
the list once it is more presentable and your work might fit into that


then I will clean the debugfs driver and post it together with this
patch for 3.9-rc1 as an individual patch.

- With current (3.8-rc6) common clock framework there is two (minor)
* although clocks are registered with devm_clk_register they are not
removed from the clock tree on unloading. That makes reloading of
clk-si5351 as module impossible.

This is a known issue. clk_unregister is a NOP and defining it has
always been deferred until the day that someone needed it. Care to
take a crack at it?

Ok. I can have a look at it and propose a patch but that will take a
while as other stuff came in between. But IMHO, preparing/enabling
clocks by clock consumers should increase reference count so referenced
modules cannot be unloaded.. but that I have never had a look at, yet ;)

* potentially there could be more than one different external si5351
generators but clocks are registered with names that do not refer
to e.g. the device name. Maybe common clock framework should
prepend the device name for each registered clock, i.e. 0-0060.clk0.
That would also avoid name collisions with same clock names from
different drivers (clk0 is likely to be used by others ;))

More unfinished work, just like clk_unregister above. I'm sure you are
aware that clk_register takes struct device *dev as input, but does
nothing with it. It wouldn't take much to concatenate the device name
and clock name if dev is present. However a complication here is that
the registration code takes a parent string name to match parents up for
discrete subtrees; how could statically defined data know about the
device name ahead of time?

I see. Wrt the above comment about spare time, would prepending DT
clocks be sufficient? Or/And use a fallback mechanism that first tries
a full match, full match with own device name, and relaxed match for
clock name as it is now?

The above design decision took place before the big DT push we have
today and was short-sighted. It would be better to change the framework
to rely less on string name lookups and DT is one way out of that.

3.8-rc7 is already out and I don't plan to take anything that hasn't
already been submitted for 3.9 now. Can you resubmit this after 3.9-rc1
comes out?

Sure, but I'll be not available next 2 weeks or so. If 3.8 falls
within that time, I will re-post it later. It is ok for me, if it has
to go in after 3.9 also.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at