[PATCH RFC] i2c: rk3x: handle dynamic clock rate changes correctly

[Max Schwarz]

Hi Addy,

Am Montag, 15. September 2014, 11:11:24 schrieb addy ke:
> On 2014/9/13 19:26, Max Schwarz wrote:
> > On Wednesday 10 September 2014 at 16:16:10, Addy wrote:
> >>> @@ -724,16 +816,28 @@ static int rk3x_i2c_probe(struct platform_device
> >>> *pdev)
> >>> 
> >>>        return ret;
> >>>    
> >>>    }
> >>> 
> >>> +    i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb;
> >>> +    ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb);
> >>> +    if (ret != 0) {
> >>> +        dev_err(&pdev->dev, "Unable to register clock notifier\n");
> >>> +        goto err_clk;
> >>> +    }
> >>> +
> >>> +    i2c->clk_freq = clk_get_rate(i2c->clk);
> >>> +    rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> >> 
> >> I have tested on rk3288-pinky board:
> >> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate()
> >> to write div value to CLKDIV register. If not, system will crash.
> >> 
> >> So we need call clk_enable() before rk3x_i2c_set_scl_rate().
> > 
> > Interesting. It works without problems on RK3188. Do you know if the clock
> > has to be kept enabled for some time? Or is it okay to do it like this?
> > 
> > clk_enable(i2c->clk);
> > rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> > clk_disable(i2c->clk);
> > 
> > Cheers,
> > 
> >   Max
> 
> Before write value to i2c register, it is necessary to enable i2c's clock
> and parent clock!

The parent clock is enabled by the clk framework when we enable the child 
clock. No need to do it explicitly.

My question was whether we can immediately *disable* the clock after writing 
the divider register. Could you test the above snippet in the i2c probe 
function? I don't have access to a RK3288 board.

> In the linux-rockchip branch code, the i2c's clock and parent clock are
> enabled by default. So there are no problems on it.

Yes, I saw that. I still think it's nice that we disable the clock when we 
don't need it. I'd like to keep that if possible.

> But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed.
> (Maybe its reference count drops to 0 and it is disabled by clk driver)

Yes, that's probably because no one is using childs of the pclk_peri clock 
yet.

Cheers,
  Max