Re: [PATCH v1 7/7] clk: qcom: Add video clock controller driver for SC7180

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

 



Hi Stephen,

Thanks for the review.

On 11/6/2019 6:09 AM, Stephen Boyd wrote:
Quoting Taniya Das (2019-10-31 05:21:13)
diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
new file mode 100644
index 0000000..bef034b
--- /dev/null
+++ b/drivers/clk/qcom/videocc-sc7180.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>

Are these of includes used?


I will clean them up.

+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,videocc-sc7180.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "common.h"
+#include "gdsc.h"
+
+enum {
+       P_BI_TCXO,
+       P_CHIP_SLEEP_CLK,
+       P_CORE_BI_PLL_TEST_SE,
+       P_VIDEO_PLL0_OUT_EVEN,
+       P_VIDEO_PLL0_OUT_MAIN,
+       P_VIDEO_PLL0_OUT_ODD,
+};
+
+static struct pll_vco fabia_vco[] = {

const?


yes, will add it.

+       { 249600000, 2000000000, 0 },
+};
+
[...]
+
+static int video_cc_sc7180_probe(struct platform_device *pdev)
+{
+       struct regmap *regmap;
+       struct alpha_pll_config video_pll0_config = {};
+
+       regmap = qcom_cc_map(pdev, &video_cc_sc7180_desc);
+       if (IS_ERR(regmap))
+               return PTR_ERR(regmap);
+
+       video_pll0_config.l = 0x1F;

lowercase hex please.

+       video_pll0_config.alpha = 0x4000;
+       video_pll0_config.user_ctl_val = 0x00000001;
+       video_pll0_config.user_ctl_hi_val = 0x00004805;

Same question, why on stack?


Will update the same.

+
+       clk_fabia_pll_configure(&video_pll0, regmap, &video_pll0_config);
+
+       /* video_cc_xo_clk */

What are we doing? Enabling it?


yes, enabling it. Did not model and mark it CRITICAL.

+       regmap_update_bits(regmap, 0x984, 0x1, 0x1);
+
+       return qcom_cc_really_probe(pdev, &video_cc_sc7180_desc, regmap);
+}
+
+static struct platform_driver video_cc_sc7180_driver = {
+       .probe = video_cc_sc7180_probe,
+       .driver = {
+               .name = "sc7180-videocc",
+               .of_match_table = video_cc_sc7180_match_table,
+       },
+};
+
+static int __init video_cc_sc7180_init(void)
+{
+       return platform_driver_register(&video_cc_sc7180_driver);
+}
+core_initcall(video_cc_sc7180_init);
+
+static void __exit video_cc_sc7180_exit(void)
+{
+       platform_driver_unregister(&video_cc_sc7180_driver);
+}
+module_exit(video_cc_sc7180_exit);

Same question, module platform driver perhaps?


I will move it to subsys_initcall().

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