Re: [PATCH 5.10.y-cip 05/24] clk: renesas: Add CPG core wrapper for RZ/G2L SoC

From: Pavel Machek <pavel@denx.de>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Cc: cip-dev@lists.cip-project.org,
	Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>,
	Pavel Machek <pavel@denx.de>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [PATCH 5.10.y-cip 05/24] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
Date: Fri, 17 Dec 2021 11:10:54 +0100	[thread overview]
Message-ID: <20211217101053.GA17079@amd> (raw)
In-Reply-To: <20211216125446.15451-6-prabhakar.mahadev-lad.rj@bp.renesas.com>

[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]

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@renesas.com>.

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().

> +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?

> +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?)

> +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.

> +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.

> +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 /**.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]