Re: [PATCH v1 08/12] clk: imx: Add initial support for i.MXRT1170 clock driver

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

 





On 4/22/22 23:03, Stephen Boyd wrote:
Quoting Jesse Taube (2022-03-26 07:43:09)
diff --git a/drivers/clk/imx/clk-imxrt1170.c b/drivers/clk/imx/clk-imxrt1170.c
new file mode 100644
index 000000000000..041aea3d4b02
--- /dev/null
+++ b/drivers/clk/imx/clk-imxrt1170.c
@@ -0,0 +1,391 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/*
+ * Copyright (C) 2022
+ * Author(s):
+ * Jesse Taube <Mr.Bossman075@xxxxxxxxx>
+ */
+#include <linux/clk.h>
+#include <linux/of_address.h>

Is this include used?

Yes `of_iomap`
+#include <linux/of_irq.h>

Is this include used?

No, but includes
#include <linux/slab.h>
for `kzalloc`
+#include <linux/platform_device.h>

Need to include clk-provider.h

+#include <dt-bindings/clock/imxrt1170-clock.h>
+
+#include "clk.h"
+
+#define CLOCK_MUX_DEFAULT "rcosc48M_div2", "osc", "rcosc400M", "rcosc16M"
+
+#define LPCG_GATE(gate) (0x6000 + (gate * 0x20))
+
+#define DEF_CLOCK(flags, macro, name) \
+do { \
+       hws[macro##_SEL] = imx_clk_hw_mux(#name"_sel", ccm_base + (name * 0x80), \
+               8, 3, root_clocks[name], 8); \
+       hws[macro##_GATE] = imx_clk_hw_gate_dis_flags(#name"_gate", #name"_sel", \
+               ccm_base + (name * 0x80), 24, flags); \
+       hws[macro] = imx_clk_hw_divider(#name, #name"_gate", ccm_base + (name * 0x80), 0, 8); \
+} while (0)
+
+enum root_clock_names {
+       m7,             /* root clock m7. */

Is the comment adding any value? It has the enum name after "root clock"
and the enum is "root_clock_names" so it looks very obvious.

+       m4,             /* root clock m4. */
+       bus,            /* root clock bus. */
+       bus_lpsr,       /* root clock bus lpsr. */
[...]
+       end,            /* root clock end. */
+};
+
+static const char * const root_clocks[79][8] = {
+       {CLOCK_MUX_DEFAULT, "pll_arm", "pll1_sys", "pll3_sys", "pll_video"},

Space after { and before }

+       {CLOCK_MUX_DEFAULT, "pll3_pfd3", "pll3_sys", "pll2_sys", "pll1_div5"},
+       {CLOCK_MUX_DEFAULT, "pll3_sys", "pll1_div5", "pll2_sys", "pll2_pfd3"},
[...]
+       {CLOCK_MUX_DEFAULT, "pll2_pfd3", "rcosc48M", "pll3_pfd1", "pll_audio"}
+};
+
+static const char * const pll_arm_mux[] = {"pll_arm_pre", "osc"};
+static const char * const pll3_mux[] = {"pll3_pre", "osc"};
+static const char * const pll2_mux[] = {"pll2_pre", "osc"};
+
+static const struct clk_div_table post_div_table[] = {
+       { .val = 3, .div = 1, },
+       { .val = 2, .div = 8, },
+       { .val = 1, .div = 4, },
+       { .val = 0, .div = 2, },
+       { }
+};
+
+static struct clk_hw **hws;
+static struct clk_hw_onecell_data *clk_hw_data;

Do either of these need to be static global variables? They could be
local function pointers allocated on the heap (like they already are).

+
+static int imxrt1170_clocks_probe(struct platform_device *pdev)
+{
[...]
+       hws[IMXRT1170_CLK_PLL2_PFD3] = imx_clk_hw_pfd("pll2_pfd3", "pll2_sys", pll_base + 0x270, 3);
+
+       /* CCM clocks */
+       ccm_base = devm_platform_ioremap_resource(pdev, 0);
+       if (WARN_ON(IS_ERR(ccm_base)))
+               return PTR_ERR(ccm_base);
+
+       DEF_CLOCK(CLK_IS_CRITICAL, IMXRT1170_CLK_M7, m7);

Don't have macros do things to variables that are in global scope. It
makes things very non-obvious. Instead, pass hw to the macro, or better
yet make a static inline function and let the compiler decide to inline
it or not.

+       DEF_CLOCK(CLK_IS_CRITICAL, IMXRT1170_CLK_M4, m4);
[...]
+       DEF_CLOCK(0, IMXRT1170_CLK_CSI2_UI, csi2_ui);
+       DEF_CLOCK(0, IMXRT1170_CLK_CSI, csi);
+       DEF_CLOCK(0, IMXRT1170_CLK_CKO1, cko1);
+       DEF_CLOCK(0, IMXRT1170_CLK_CKO2, cko2);
+
+       hws[IMXRT1170_CLK_USB] = imx_clk_hw_gate("usb", "bus", ccm_base + LPCG_GATE(115), 0);
+
+       clk_set_rate(hws[IMXRT1170_CLK_PLL_ARM]->clk, 90000000);

Use assigned-clock-rates?

+
+       imx_check_clk_hws(hws, IMXRT1170_CLK_END);



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux