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

[Doug Anderson]

Max,

On Tue, Sep 23, 2014 at 1:27 AM, Max Schwarz <max.schwarz at online.de> wrote:
> The i2c input clock can change dynamically, e.g. on the RK3066 where
> pclk_i2c0 and pclk_i2c1 are connected to the armclk, which changes
> rate on cpu frequency scaling.
>
> Until now, we incorrectly called clk_get_rate() while holding the
> i2c->lock in rk3x_i2c_xfer() to adapt to clock rate changes.
> Thanks to Huang Tao for reporting this issue.
>
> Do it properly now using the clk notifier framework. The callback
> logic was taken from i2c-cadence.c.
>
> Signed-off-by: Max Schwarz <max.schwarz at online.de>
> Tested-by: Max Schwarz <max.schwarz at online.de> on RK3188
> Tested-by: Doug Anderson <dianders at chromium.org> on RK3288
> (dynamic rate changes are untested!)
> ---
>
> This is based on Wolframs' i2c/for-current branch, since that
> includes the recent divisor fix by Addy Ke (b4a7bd7a38).
>
> Doug, I'm keeping your Tested-By since the changes should not touch
> the static clock case, but you might want to confirm that the
> Reviewed-By still stands.

No problem.  I just re-tested your v5 and it still boots.  You can add
my Reviewed-by back though I have one nit below.


> Changes since v4:
>  - fixed a bug in rk3x_i2c_clk_notifier_cb() which would feed
>    the input clock frequency as the desired SCL frequency into
>    rk3x_i2c_set_scl_frequency(i2c, scl_rate). Rename & change to
>    rk3x_i2c_adapt_div(i2c, clk_rate) to make things clear.

Whew, glad you caught this!  Since the notifier block wasn't used on
rk3288 which I'm using, I didn't spend nearly enough time check that
part of the code...


> +static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
> +                                   event, void *data)
> +{
> +       struct clk_notifier_data *ndata = data;
> +       struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, clk_rate_nb);
> +       unsigned int div;
> +
> +       switch (event) {
> +       case PRE_RATE_CHANGE:
> +       {

Now that you're not declaring any variables here, you don't need
braces in this case.


-Doug