Hi Stephen,

Thanks for the quick review! :)

-----Original Message-----
From: Stephen Boyd [mailto:sboyd at kernel.org]
Sent: Thursday, November 15, 2018 7:29 AM

[...]

Quoting A.s. Dong (2018-11-10 07:20:08)

quoted

Add imx8qxp lpcg driver support

Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <redacted>
Cc: Michael Turquette <mturquette@baylibre.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

I like it!

Great to know that.

quoted

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c

b/drivers/clk/imx/clk-imx8qxp-lpcg.c
new file mode 100644
index 0000000..7b318d7

--- /dev/null
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c

@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP
+ *     Dong Aisheng <aisheng.dong@nxp.com>
+ */
+
+#include <dt-bindings/clock/imx8qxp-clock.h>

Include this after <linux/*> and before locals please.

Got it.

quoted

+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "clk-scu.h"
+#include "clk-imx8qxp-lpcg.h"
+
+struct imx8qxp_lpcg_data {
+       int id;
+       char *name;
+       char *parent;
+       unsigned long flags;
+       u32 offset;
+       u8 bit_idx;
+       bool hw_gate;
+};
+
+struct imx8qxp_ss_lpcg {
+       const struct imx8qxp_lpcg_data *lpcg;
+       u8 num_lpcg;
+       u8 num_max;
+};

Any chance we can get kernel-doc on these structs and members of the
structs?

I can add it.

quoted

+
+static const struct imx8qxp_lpcg_data imx8qxp_lpcg_adma[] = {
+       { IMX8QXP_ADMA_LPCG_UART0_IPG_CLK, "uart0_lpcg_ipg_clk",

"dma_ipg_clk_root", 0, ADMA_LPUART_0_LPCG, 16, 0, },

quoted

+       { IMX8QXP_ADMA_LPCG_UART0_BAUD_CLK,

"uart0_lpcg_baud_clk", "uart0_clk", 0, ADMA_LPUART_0_LPCG, 0, 0, },

quoted

+       { IMX8QXP_ADMA_LPCG_UART1_IPG_CLK, "uart1_lpcg_ipg_clk",

"dma_ipg_clk_root", 0, ADMA_LPUART_1_LPCG, 16, 0, },

quoted

+       { IMX8QXP_ADMA_LPCG_UART1_BAUD_CLK,

"uart1_lpcg_baud_clk",

quoted

+"uart1_clk", 0, ADMA_LPUART_1_LPCG, 0, 0, },

[...]

quoted

+
+static int imx8qxp_lpcg_clk_probe(struct platform_device *pdev) {
+       struct device *dev = &pdev->dev;
+       struct device_node *np = dev->of_node;
+       struct clk_hw_onecell_data *clk_data;
+       const struct imx8qxp_ss_lpcg *ss_lpcg;
+       const struct imx8qxp_lpcg_data *lpcg;
+       struct resource *res;
+       struct clk_hw **clks;
+       void __iomem *base;
+       int i;

That's a good amount of local variables!

Yes, seems we need them.
Please let me know if you have a better idea.

quoted

+
+       ss_lpcg = of_device_get_match_data(dev);
+       if (!ss_lpcg)
+               return -ENODEV;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap(dev, res->start, resource_size(res));

Why not devm_ioremap_resource()?

Because LPCGs are distributed in various subsystem memory space which
may be overlapped with some IP modules. So we can't use devm_ioremap_resource
which has a request_mem_region() in it.
It's something like what we've done for drivers/mfd/syscon.c.

quoted

+       if (!base)
+               return -ENOMEM;

And then return the error code out of it?

See above.

quoted

+
+       clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) +
+                               sizeof(*clk_data->hws) *
+ ss_lpcg->num_max,

Can you use struct_size() here?

Got it, thanks for pointing to this new api.

quoted

+                               GFP_KERNEL);
+       if (!clk_data)
+               return -ENOMEM;
+
+       clk_data->num = ss_lpcg->num_max;
+       clks = clk_data->hws;
+
+       for (i = 0; i < ss_lpcg->num_lpcg; i++) {
+               lpcg = ss_lpcg->lpcg + i;
+               clks[lpcg->id] = imx_clk_lpcg_scu(lpcg->name,

lpcg->parent,

quoted

+                                                 lpcg->flags, base +

lpcg->offset,

quoted

+                                                 lpcg->bit_idx,

lpcg->hw_gate);

quoted

+       }
+
+       for (i = 0; i < clk_data->num; i++) {
+               if (IS_ERR(clks[i]))
+                       pr_err("i.MX clk %u: register failed with %ld\n",
+                               i, PTR_ERR(clks[i]));

But we're OK to continue? Alright...

Yes, probably better to use pr_warn()?

quoted

+       }
+
+       return of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+ clk_data);

Why not use devm_? Or can the driver never be unbound from sysfs?

We expect the driver to be never unbound from sysfs.
Probably we'd better add .suppress_bind_attrs.

quoted

+}
+
+static const struct of_device_id imx8qxp_lpcg_match[] = {
+       { .compatible = "fsl,imx8qxp-lpcg-adma", &imx8qxp_ss_adma, },
+       { .compatible = "fsl,imx8qxp-lpcg-conn", &imx8qxp_ss_conn, },
+       { .compatible = "fsl,imx8qxp-lpcg-lsio", &imx8qxp_ss_lsio, },
+       { /* sentinel */ }
+};
+
+static struct platform_driver imx8qxp_lpcg_clk_driver = {
+       .driver = {
+               .name = "imx8qxp-lpcg-clk",
+               .of_match_table = imx8qxp_lpcg_match,
+       },
+       .probe = imx8qxp_lpcg_clk_probe, };
+
+builtin_platform_driver(imx8qxp_lpcg_clk_driver);
+
+MODULE_AUTHOR("Dong Aisheng [off-list ref]");
+MODULE_DESCRIPTION("IMX8QXP LPCG Clock driver");

MODULE_LICENSE("GPL

quoted

+v2");

If the module is builtin all the time because of
builtin_platform_driver() I think we don't need these MODULE_ macros.

Okay, I can remove them.

Regards
Dong Aisheng