-----Original Message-----
From: Pavel Machek <pavel@...>
Sent: 17 December 2021 10:11
To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...>
Cc: cip-dev@...; Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...>; Pavel Machek
<pavel@...>; Biju Das <biju.das.jz@...>
Subject: Re: [PATCH 5.10.y-cip 05/24] clk: renesas: Add CPG core wrapper for RZ/G2L SoC

Hi!

commit ef3c613ccd68a78727b817c3dacf4a68d1ffc67f upstream.

Add CPG core wrapper for RZ/G2L family.

Based on a patch in the BSP by Binh Nguyen
<binh.nguyen.jz@...>.

Some comments below.

+static struct clk * __init
+rzg2l_cpg_pll_clk_register(const struct cpg_core_clk *core,

...

+ pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
+ if (!pll_clk) {
+ clk = ERR_PTR(-ENOMEM);
+ return NULL;
+ }

I believe this wanted to return clk? But I'd recommend just directly returning the ERR_PTR().

The later patches do return ERR_PTR(-ENOMEM);

+static struct clk
+*rzg2l_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
+ void *data)

...

+ if (IS_ERR(clk))
+ dev_err(dev, "Cannot get %s clock %u: %ld", type, clkidx,
+ PTR_ERR(clk));

Is "\n" missing?

Will fix that upstream.

+static void __init
+rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
+ const struct rzg2l_cpg_info *info,
+ struct rzg2l_cpg_priv *priv)
+{
+ struct mstp_clock *clock = NULL;

...

+ parent = priv->clks[mod->parent];
+ if (IS_ERR(parent)) {
+ clk = parent;
+ goto fail;
+ }
+
+ clock = devm_kzalloc(dev, sizeof(*clock), GFP_KERNEL);
+ if (!clock) {
+ clk = ERR_PTR(-ENOMEM);
+ goto fail;
+ }

...

+fail:
+ dev_err(dev, "Failed to register %s clock %s: %ld\n", "module",
+ mod->name, PTR_ERR(clk));
+ kfree(clock);

Should this be devm_kfree? (And is devm_kfree(NULL) ok?)

Agreed will fix that upstream and backport later. Yes devm_kfree(NULL) is OK see [0]

+static bool rzg2l_cpg_is_pm_clk(const struct of_phandle_args
+*clkspec) {
+ if (clkspec->args_count != 2)
+ return false;
+
+ switch (clkspec->args[0]) {
+ case CPG_MOD:
+ return true;
+
+ default:
+ return false;
+ }
+}

"return clkspec->args[0] == CPG_MOD" would be simpler way to say this.

Agreed.

+static int __init rzg2l_cpg_probe(struct platform_device *pdev) {

...

+ error = rzg2l_cpg_reset_controller_register(priv);
+ if (error)
+ return error;
+
+ return 0;
+}

You can just return error unconditionally.

Agreed.

+static const struct of_device_id rzg2l_cpg_match[] = {
+ { /* sentinel */ }
+};

It matches nothing? Aha, id is added in next patch.

+/**
+ * Definitions of CPG Core Clocks
+ *
+ * These include:
+ * - Clock outputs exported to DT
+ * - External input clocks
+ * - Internal CPG clocks
+ */

This is not kerneldoc -> should not be marked with /**.

Agreed.

[0] https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/base/devres.c#L1053

Cheers,
Prabhakar

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany