Re: [PATCH v9 14/27] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

Adding Stephen to the discussion.
Adding Stephen to the discussion.

On Sat, Jan 5, 2019 at 6:27 PM, Uwe Kleine-König 
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> Hello Paul,
>
> On Sat, Jan 05, 2019 at 06:05:38PM -0300, Paul Cercueil wrote:
> >  On Sat, Jan 5, 2019 at 4:57 PM, Uwe Kleine-König
> >  <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> >  > You are assuming stuff here about the parent clk which isn't 
> > guaranteed
> >  > (AFAICT) by the clk framework: If you call clk_round_rate(clk, 
> > rate - 1)
> >  > this might well return rate even if the clock could run slower 
> > than
> >  > rate.
> >
> >  It may not be guaranteed by the clock framework itself, but it is 
> > guaranteed
> >  to behave like that on this family of SoCs.
>
> You shouldn't rely on that. Experience shows that people will start
> copying code to machines where this is not guaranteed. Even if they
> don't copy and only learn from reading this is bad. Also how do you
> guarantee that this won't change in the future making the pwm code 
> break
> without noticing?
>
> If you use an API better don't assume more things given than are
> guaranteed by the API.
>
> Having said that I would consider it sensible to introduce something
> like clk_roundup_rate() and clk_rounddown_rate() which would allow
> calculations like that.

@Stephen:
Some context: my algorithm makes use of clk_round_rate(clk, rate - 1) 
to get the
next (smaller) clock rate that a clock support.

Is it something safe to assume? If not is there a better way?

> >  > Wouldn't it make sense to start iterating with rate = 0xffff * 
> > 1e9 /
> >  > period? Otherwise you get bad configurations if rate is 
> > considerable
> >  > slower than necessary.
> >
> >  The algorithm will start with 'rate' being the parent clock's rate, 
> > which
> >  will always be the highest rate that the child clock will support.
>
> Ah right, I missed that bit.

Thanks,
-Paul