-----Original Message-----
From: Stephen Boyd <sboyd@kernel.org>
Sent: Thursday, 10 August, 2023 5:27 AM
To: Rabara, Niravkumar L <redacted>
Cc: Ng, Adrian Ho Yin <redacted>; andrew@lunn.ch;
conor+dt@kernel.org; devicetree@vger.kernel.org; dinguyen@kernel.org;
krzysztof.kozlowski+dt@linaro.org; linux-clk@vger.kernel.org; linux-
kernel@vger.kernel.org; Turquette, Mike [off-list ref];
netdev@vger.kernel.org; p.zabel@pengutronix.de; richardcochran@gmail.com;
robh+dt@kernel.org; wen.ping.teh@intel.com
Subject: Re: [PATCH v2 4/5] clk: socfpga: agilex: add clock driver for the Agilex5

Quoting niravkumar.l.rabara@intel.com (2023-07-31 18:02:33)

quoted

diff --git a/drivers/clk/socfpga/clk-agilex.c

b/drivers/clk/socfpga/clk-agilex.c
index 74d21bd82710..3dcd0f233c17 100644

--- a/drivers/clk/socfpga/clk-agilex.c
+++ b/drivers/clk/socfpga/clk-agilex.c

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (C) 2019, Intel Corporation
+ * Copyright (C) 2019-2023, Intel Corporation
  */
 #include <linux/slab.h>
 #include <linux/clk-provider.h>

@@ -9,6 +9,7 @@
 #include <linux/platform_device.h>

 #include <dt-bindings/clock/agilex-clock.h>
+#include <dt-bindings/clock/intel,agilex5-clkmgr.h>

 #include "stratix10-clk.h"

@@ -41,6 +42,67 @@ static const struct clk_parent_data mpu_free_mux[] = {
          .name = "f2s-free-clk", },
 };

+static const struct clk_parent_data core0_free_mux[] = {
+       { .fw_name = "main_pll_c1",
+         .name = "main_pll_c1", },

We're adding support for something new? Only set .fw_name in that case, as
.name will never be used. To do even better, set only .index so that we don't do
any string comparisons.

Yes we are adding Agilex5 SoCFPGA platform specific clocks. 
I will remove .name and only keep .fw_name in next version of this patch. 
 

quoted

+       { .fw_name = "peri_pll_c0",
+         .name = "peri_pll_c0", },
+       { .fw_name = "osc1",
+         .name = "osc1", },
+       { .fw_name = "cb-intosc-hs-div2-clk",
+         .name = "cb-intosc-hs-div2-clk", },
+       { .fw_name = "f2s-free-clk",
+         .name = "f2s-free-clk", },
+};
+

[...]

quoted

+
 static int n5x_clk_register_c_perip(const struct n5x_perip_c_clock *clks,
                                       int nums, struct
stratix10_clock_data *data)  { @@ -535,6 +917,51 @@ static int
n5x_clkmgr_init(struct platform_device *pdev)
        return 0;
 }

+static int agilex5_clkmgr_init(struct platform_device *pdev) {
+       struct device_node *np = pdev->dev.of_node;
+       struct device *dev = &pdev->dev;
+       struct stratix10_clock_data *clk_data;

Maybe call this stratix_data so that clk_data.clk_data is stratix_data.clk_data.

Will fix this in next version. 

quoted

+       struct resource *res;
+       void __iomem *base;
+       int i, num_clks;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap_resource(dev, res);

This is a single function call, devm_platform_ioremap_resource().i

Noted. Will fix in next version. 

quoted

+       if (IS_ERR(base))
+               return PTR_ERR(base);
+
+       num_clks = AGILEX5_NUM_CLKS;
+
+       clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
+                                                num_clks), GFP_KERNEL);
+       if (!clk_data)
+               return -ENOMEM;
+
+       for (i = 0; i < num_clks; i++)
+               clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+
+       clk_data->base = base;
+       clk_data->clk_data.num = num_clks;
+
+       agilex_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks),
+                               clk_data);
+
+       agilex_clk_register_c_perip(agilex5_main_perip_c_clks,
+                                   ARRAY_SIZE(agilex5_main_perip_c_clks),
+                                   clk_data);
+
+       agilex_clk_register_cnt_perip(agilex5_main_perip_cnt_clks,
+                                     ARRAY_SIZE(agilex5_main_perip_cnt_clks),
+                                     clk_data);
+
+       agilex_clk_register_gate(agilex5_gate_clks,
+                                ARRAY_SIZE(agilex5_gate_clks),
+ clk_data);
+
+       of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+ &clk_data->clk_data);

devm? Or when is this provider removed?

Will fix in next version. 

quoted

+       return 0;
+}
+
 static int agilex_clkmgr_probe(struct platform_device *pdev)  {
        int (*probe_func)(struct platform_device *init_func);