Re: [PATCH v2 1/2] clk: qcom: rcg2: Add support for display port clock ops

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Stephen,

Thanks for your review.

On 7/16/2019 4:13 AM, Stephen Boyd wrote:
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.


Sure, would take care in the next patch.


  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.


Would remove the pr_err.

+                                                       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.


My bad would fix in the next patch.

+
+       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.


I would check for (num != den).

+       } 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?


I guess it is required as the RCG SRC would be 0x0 by default.

+
+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.

Would it be good to leave this function :).

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux