Re: [PATCH 2/5] i2c: rcar: get clock rate only once and simplify calculation

[Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>]

Hi Magnus

Thanks for your comments

On Wed, 11 Sep 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski
> <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> > There is no need to repeatedly query clock frequency, where it is not
> > expected to change. The complete loop can also trivially be replaced with
> > a simple division. A further loop below the one, being simplified, could
> > also be replaced, but that would get more complicated.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Thanks for your efforts. What makes you think that you should assume
> that the clock frequency doesn't change?

Sorry, maybe I didn't explain well enough in the patch description. 
Please, have a look at the patch. It removes repeated clock rate readings 
inside a tight loop. I'm aware that the clock rate can change, but there 
should be a way to make sure, that beginning now and until a certain later 
point of time the rate doesn't change. Presumably this should be done by 
locking the clock. I'm not sure this is currently supported by the clock 
API. Neither clk_get(), nor clk_prepare() nor clk_enable() seem to have 
the semantics of locking the clock's frequency. Maybe the one who actually 
prepared the clock should become its owner, but I don't think this is 
currently the expected behaviour. In either case, I don't think calling 
clk_get_rate() repeatedly inside _that_ loop makes sense - the rate can 
anyway be changed after the loop, so, it doesn't add any security.

Thanks
Guennadi

> As you already know, we want drivers to be reusable across multiple
> SoCs. Assuming it that it would be fixed based on one particular SoC
> doesn't guarantee that that's the case on other SoCs. Which SoCs did
> you take into consideration?

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/