Quoting Taniya Das (2019-05-14 21:20:38)
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 18bdf34..0de080f 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -15,6 +15,7 @@ menuconfig COMMON_CLK_QCOM
> depends on ARCH_QCOM || COMPILE_TEST
> select REGMAP_MMIO
> select RESET_CONTROLLER
> + select RATIONAL
Make this an alphabetical list of selects please.
>
> if COMMON_CLK_QCOM
>
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 8c02bff..98071c0 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -1128,3 +1129,81 @@ int qcom_cc_register_rcg_dfs(struct regmap *regmap,
> return 0;
> }
> EXPORT_SYMBOL_GPL(qcom_cc_register_rcg_dfs);
> +
> +static int clk_rcg2_dp_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> + struct freq_tbl f = { 0 };
> + u32 mask = BIT(rcg->hid_width) - 1;
> + u32 hid_div, cfg;
> + int i, num_parents = clk_hw_get_num_parents(hw);
> + unsigned long num, den;
> +
> + rational_best_approximation(parent_rate, rate,
> + GENMASK(rcg->mnd_width - 1, 0),
> + GENMASK(rcg->mnd_width - 1, 0), &den, &num);
> +
> + if (!num || !den) {
> + pr_err("Invalid MN values derived for requested rate %lu\n",
Does this ever happen? I worry that this printk could happen many times
if a driver gets into a bad state and starts selecting invalid
frequencies over and over again for each frame (every 16ms). Maybe just
return -EINVAL instead of printing anything.
> + rate);
> + return -EINVAL;
> + }
> +
> + regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg);
> + hid_div = cfg;
> + cfg &= CFG_SRC_SEL_MASK;
> + cfg >>= CFG_SRC_SEL_SHIFT;
> +
> + for (i = 0; i < num_parents; i++)
> + if (cfg == rcg->parent_map[i].cfg) {
> + f.src = rcg->parent_map[i].src;
> + break;
> + }
Weird indent for this brace. Please fix and put a brace on the for
statement too.
> +
> + f.pre_div = hid_div;
> + f.pre_div >>= CFG_SRC_DIV_SHIFT;
> + f.pre_div &= mask;
> +
> + if (num == den) {
> + f.m = 0;
> + f.n = 0;
Isn't this the default? So just have if (num != den) here.
> + } else {
> + f.m = num;
> + f.n = den;
> + }
> +
> + return clk_rcg2_configure(rcg, &f);
> +}
> +
> +static int clk_rcg2_dp_set_rate_and_parent(struct clk_hw *hw,
> + unsigned long rate, unsigned long parent_rate, u8 index)
> +{
> + return clk_rcg2_dp_set_rate(hw, rate, parent_rate);
> +}
Does this need to be implemented? The parent index isn't passed to
clk_rcg2_dp_set_rate() so I suspect the parent index doesn't matter?
Does the parent change?
> +
> +static int clk_rcg2_dp_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + struct clk_rate_request parent_req = *req;
> + int ret;
> +
> + ret = __clk_determine_rate(clk_hw_get_parent(hw), &parent_req);
> + if (ret)
> + return ret;
> +
> + req->best_parent_rate = parent_req.rate;
> +
> + return 0;
> +}
Do you need this op? It's just calling determine rate on the parent, so
we already do that if the proper flag is set. I'm confused about this
function.
