Hi Stephen,

Thanks for the review.

On Sat, Sep 7, 2019 at 5:29 PM Stephen Boyd [off-list ref] wrote:

Quoting Dong Aisheng (2019-08-20 04:13:17)

quoted

diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
index 5e2903e..1ad3f2a 100644
--- a/drivers/clk/imx/clk-imx8qxp.c
+++ b/drivers/clk/imx/clk-imx8qxp.c

@@ -134,7 +134,12 @@ static int imx8qxp_clk_probe(struct platform_device *pdev)
                                i, PTR_ERR(clks[i]));
        }

-       return of_clk_add_hw_provider(ccm_node, of_clk_hw_onecell_get, clk_data);
+       if (clock_cells == 2)

Can you just read this from the DT node again instead of having a global
variable called "clock_cells" for this?

I tried thinking about it.
One problem is that we also need this information in the exist clk
registration API to
keep the backwards compatibility:
e.g.
 static inline struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id,
                                         u8 clk_type)
 {
-       return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type);
+       if (clock_cells == 2)
+               return imx_clk_scu_alloc_dev(name, NULL, 0, rsrc_id, clk_type);
+       else
+               return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type);
 }

Parsing it for all clocks seems not good.

In the future, i planned to totally remove the legacy binding support which
is a premature one and missing continued support.
Then we will also remove this unneeded clock_cells.

quoted

+               ret = of_clk_add_hw_provider(ccm_node, imx_scu_of_clk_src_get, imx_scu_clks);
+       else
+               ret = of_clk_add_hw_provider(ccm_node, of_clk_hw_onecell_get, clk_data);
+
+       return ret;
 }

 static const struct of_device_id imx8qxp_match[] = {

diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index fbef740..48bfb08 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c

@@ -16,6 +19,21 @@
 #define IMX_SIP_SET_CPUFREQ            0x00

 static struct imx_sc_ipc *ccm_ipc_handle;
+struct device_node *pd_np;
+u32 clock_cells;
+
+struct imx_scu_clk_node {
+       const char *name;
+       u32 rsrc;
+       u8 clk_type;
+       const char * const *parents;
+       int num_parents;
+
+       struct clk_hw *hw;
+       struct list_head node;
+};
+
+struct list_head imx_scu_clks[IMX_SC_R_LAST];

 /*
  * struct clk_scu - Description of one SCU clock

@@ -128,9 +146,29 @@ static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
        return container_of(hw, struct clk_scu, hw);
 }

-int imx_clk_scu_init(void)
+int imx_clk_scu_init(struct device_node *np)
 {
-       return imx_scu_get_handle(&ccm_ipc_handle);
+       struct platform_device *pd_dev;
+       int ret, i;
+
+       ret = imx_scu_get_handle(&ccm_ipc_handle);
+       if (ret)
+               return ret;
+
+       if (of_property_read_u32(np, "#clock-cells", &clock_cells))
+               return -EINVAL;
+
+       if (clock_cells == 2) {
+               for (i = 0; i < IMX_SC_R_LAST; i++)
+                       INIT_LIST_HEAD(&imx_scu_clks[i]);
+
+               pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
+               pd_dev = of_find_device_by_node(pd_np);
+               if (!pd_dev || !device_is_bound(&pd_dev->dev))
+                       return -EPROBE_DEFER;

Do you need to put some nodes here with of_node_put() one failure or
when they're done being used?

Good catch.
We should put the node for of_find_compatible_node().

quoted

+       }
+
+       return 0;
 }

 /*

@@ -387,3 +425,99 @@ struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents,

[...]

quoted

+
+struct clk_hw *imx_clk_scu_alloc_dev(const char *name,
+                                    const char * const *parents,
+                                    int num_parents, u32 rsrc_id, u8 clk_type)
+{
+       struct imx_scu_clk_node clk = {
+               .name = name,
+               .rsrc = rsrc_id,
+               .clk_type = clk_type,
+               .parents = parents,
+               .num_parents = num_parents,
+       };
+       struct platform_device *pdev;
+       int ret;
+
+       pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
+       if (!pdev) {
+               pr_err("%s: failed to allocate scu clk dev rsrc %d type %d\n",
+                      name, rsrc_id, clk_type);
+               return ERR_PTR(-ENOMEM);
+       }
+
+       ret = platform_device_add_data(pdev, &clk, sizeof(clk));
+       if (ret) {
+               platform_device_put(pdev);
+               return ERR_PTR(-ENOMEM);

Why not ERR_PTR(ret)?

Good catch. Will fix.

Regards
Aisheng

quoted

+       }
+
+       pdev->driver_override = "imx-scu-clk";
+
+       ret = imx_clk_scu_attach_pd(&pdev->dev, rsrc_id);
+       if (ret)
+               pr_warn("%s: failed to attached the power domain %d\n",
+                       name, ret);
+
+       platform_device_add(pdev);
+
+       /* For API backwards compatiblilty, simply return NULL for success */
+       return NULL;
+}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel