Re: [RFC] Common clock framework for external clock generators

[Sebastian Hesselbarh <sebastian.hesselbarth@googlemail.com>]

On 05/13/2012 02:29 PM, Mark Brown wrote:
> Always CC maintainers on mails, things get lost on high volume lists.

Yeah, right after posting I thought of this but didn't want to bother
you again. I read a lot of kernel code since then and some things are
much more clearer to me now. Besides the driver is almost done now and
works as expected. Will have to clean up things and will post it as
a patch then.

> I'd suggest using regmap for the cache support (which helps with
> read/modify/write cycles) and because...

I will have a look at regmap before I post a new driver patch for
sure.

>> static const struct clk_ops si5351_xtal_ops = {
>> 	.enable = si5351_xtal_enable,
>> 	.disable = si5351_xtal_disable,
>
> These should be prepare() and unprepare() - enable() and disable() are
> called from atomic context so you can't do I2C I/O.

You are right. The kernel code states this correctly and I should have
read it more carefully.

>> static u8 si5351_clkout_get_parent(struct clk_hw *hw)
>> {
>> 	struct si5351_driver_data *sidata = si5351_clkout_get_data(hw);
>> 	return 0;
>> }
>
> If we need empty functions like this we should fix the framework.

Of course, this function is not empty at all but wasn't done at the
time I posted the RFC.

One question that remains from my original RFC is the missing
clk_unregister() as i2c can be removed there also should be an
function to unregister previously registered clocks?

Sebastian