Re: [PATCH v5 09/12] clk: actions: Add fixed factor clock support

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

Hi Stephen,

On Mon, Mar 19, 2018 at 06:10:03PM -0700, Stephen Boyd wrote:
> Quoting Manivannan Sadhasivam (2018-03-17 03:09:49)
> > Add support for Actions Semi fixed factor clock together with
> > helper functions to be used in composite clock.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/clk/actions/Makefile           |  1 +
> >  drivers/clk/actions/owl-fixed-factor.c | 81 ++++++++++++++++++++++++++++++++++
> >  drivers/clk/actions/owl-fixed-factor.h | 62 ++++++++++++++++++++++++++
> >  3 files changed, 144 insertions(+)
> >  create mode 100644 drivers/clk/actions/owl-fixed-factor.c
> >  create mode 100644 drivers/clk/actions/owl-fixed-factor.h
> > 
> > diff --git a/drivers/clk/actions/Makefile b/drivers/clk/actions/Makefile
> > index 994357fa560b..b618696ba54e 100644
> > --- a/drivers/clk/actions/Makefile
> > +++ b/drivers/clk/actions/Makefile
> > @@ -5,3 +5,4 @@ clk-owl-y                       += owl-gate.o
> >  clk-owl-y                      += owl-mux.o
> >  clk-owl-y                      += owl-divider.o
> >  clk-owl-y                      += owl-factor.o
> > +clk-owl-y                      += owl-fixed-factor.o
> > diff --git a/drivers/clk/actions/owl-fixed-factor.c b/drivers/clk/actions/owl-fixed-factor.c
> > new file mode 100644
> > index 000000000000..f1281565129c
> > --- /dev/null
> > +++ b/drivers/clk/actions/owl-fixed-factor.c
> > @@ -0,0 +1,81 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +// OWL fixed factor clock driver
> > +//
> > +// Copyright (c) 2014 Actions Semi Inc.
> > +// Author: David Liu <liuwei@actions-semi.com>
> > +//
> > +// Copyright (c) 2018 Linaro Ltd.
> > +// Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#include "owl-fixed-factor.h"
> > +
> > +long owl_fix_fact_helper_round_rate(struct owl_clk_common *common,
> > +                               const struct owl_fix_fact_hw *fix_fact_hw,
> > +                               unsigned long rate,
> > +                               unsigned long *parent_rate)
> > +{
> > +       if (clk_hw_get_flags(&common->hw) & CLK_SET_RATE_PARENT) {
> > +               unsigned long best_parent;
> > +
> > +               best_parent = (rate / fix_fact_hw->mul) * fix_fact_hw->div;
> > +               *parent_rate = clk_hw_round_rate(clk_hw_get_parent(&common->hw),
> > +                                                       best_parent);
> > +       }
> > +
> > +       return (*parent_rate / fix_fact_hw->div) * fix_fact_hw->mul;
> > +}
> > +
> > +static long owl_fix_fact_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                       unsigned long *parent_rate)
> > +{
> > +       struct owl_fix_fact *fix_fact = hw_to_owl_fix_fact(hw);
> > +       struct owl_fix_fact_hw *fix_fact_hw = &fix_fact->fix_fact_hw;
> > +
> > +       return owl_fix_fact_helper_round_rate(&fix_fact->common, fix_fact_hw,
> > +                                                       rate, parent_rate);
> > +}
> > +
> > +unsigned long owl_fix_fact_helper_recalc_rate(struct owl_clk_common *common,
> > +                       const struct owl_fix_fact_hw *fix_fact_hw,
> > +                       unsigned long parent_rate)
> > +{
> > +       unsigned long long int rate;
> > +
> > +       rate = (unsigned long long int)parent_rate * fix_fact_hw->mul;
> > +       do_div(rate, fix_fact_hw->div);
> > +
> > +       return (unsigned long)rate;
> 
> You can drop the cast.
> 

Okay.

> > +}
> > +
> > +static unsigned long owl_fix_fact_recalc_rate(struct clk_hw *hw,
> > +                       unsigned long parent_rate)
> > +{
> > +       struct owl_fix_fact *fix_fact = hw_to_owl_fix_fact(hw);
> > +       struct owl_fix_fact_hw *fix_fact_hw = &fix_fact->fix_fact_hw;
> > +
> > +       return owl_fix_fact_helper_recalc_rate(&fix_fact->common, fix_fact_hw,
> > +                                                       parent_rate);
> > +}
> > +
> > +static int owl_fix_fact_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                              unsigned long parent_rate)
> > +{
> > +       /*
> > +        * We must report success but we can do so unconditionally because
> > +        * clk_fix_fact_round_rate returns values that ensure this call is a
> 
> What function is that?
>

It should be owl_fix_fact_round_rate.

> > +        * nop.
> > +        */
> > +
> > +       return 0;
> > +}
> > +
> > +const struct clk_ops owl_fix_fact_ops = {
> > +       .round_rate     = owl_fix_fact_round_rate,
> > +       .recalc_rate    = owl_fix_fact_recalc_rate,
> > +       .set_rate       = owl_fix_fact_set_rate,
> > +};
> 
> Why can't you use the regular fixed factor clk code and ops?
>

That's going to be really messy. Since I'm having the clk_hw embedded inside
owl_clk_common and using it for registering all the clocks, using generic
fixed factor functions will be a different approach _only_ for this clock and
it won't look good I guess. Also, it may become complicated with composite
clocks.

If you still want to use the generic fixed factor code, I can do that in
next revision.

Your views?

Thanks,
Mani