Re: [PATCH v5 8/9] clk: nuvoton: ma35d1: get HXT/LXT from DT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: nuvoton: missing IS_ERR check causes permanent probe deferral
- [Medium] clk: nuvoton: hardcoded parent strings cause orphaned gate clocks
--

commit 6ea21fa12153954a64270097e3f0cfbc24e8a162
Author: Miquel Raynal <miquel.raynal@bootlin.com>

clk: nuvoton: ma35d1: get HXT/LXT from DT

This commit updates the driver to get the HXT and LXT crystal oscillator
inputs from the device tree using devm_clk_get_optional(). It points the
internal clock table to the discovered clk_hw instances instead of always
registering fixed rate clocks. It falls back to fixed rate clocks for
older device trees.

> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
> index ceebcbd8c18b3..81a412b1706e4 100644
> --- a/drivers/clk/nuvoton/clk-ma35d1.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1.c

[ ... ]

> @@ -240,10 +256,10 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	hws[HXT] = ma35d1_clk_fixed("hxt", 24000000);
> +	hws[HXT] = ma35d1_clk_xtal(dev, "hxt", 24000000);

[Severity: High]
Does this code silently ignore probe deferrals?

The ma35d1_clk_xtal() function in drivers/clk/nuvoton/clk-ma35d1.c calls
devm_clk_get_optional(), which can return ERR_PTR(-EPROBE_DEFER) if the
external clock provider hasn't probed yet.

Because ma35d1_clocks_probe() assigns this directly to hws[HXT] without an
IS_ERR() check, the probe function will complete successfully and return 0
instead of deferring.

The error pointer then permanently resides in the provider's onecell data,
and downstream consumers requesting these clocks will face infinite probe
deferrals.

>  	hws[HXT_GATE] = ma35d1_clk_gate(dev, "hxt_gate", "hxt",
>  					clk_base + REG_CLK_PWRCTL, 0);

[Severity: Medium]
Will this hardcoded parent string lead to an orphaned gate clock?

Since the driver dynamically fetches external clocks from the DT in
ma35d1_clocks_probe(), the Common Clock Framework will resolve the string
"hxt" against the provider's global clock name.

If a device tree supplies an external clock with a different global name
such as "clk_hxt", the framework will fail to match it and the gate clock
will be orphaned.

Should this pass the resolved clk_hw_get_name(hws[HXT]) instead of relying
on exact string matches against DT nodes to ensure reliable linkage?

> -	hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
> +	hws[LXT] = ma35d1_clk_xtal(dev, "lxt", 32768);

[Severity: High]
Does this face the same probe deferral regression as HXT?

In ma35d1_clocks_probe(), without an IS_ERR() check, an ERR_PTR from
ma35d1_clk_xtal() allows the function to silently succeed despite missing
dependencies, permanently storing the error pointer in hws[LXT].

>  	hws[LXT_GATE] = ma35d1_clk_gate(dev, "lxt_gate", "lxt",
>  					clk_base + REG_CLK_PWRCTL, 1);

[Severity: Medium]
Can this string cause the same orphan regression as HXT_GATE?

Passing the literal string "lxt" to ma35d1_clk_gate() as the parent name
could fail to match if the external DT clock has a different global name.

>  	hws[HIRC] = ma35d1_clk_fixed("hirc", 12000000);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-perso-ma35d1-upstream-clk-v5-0-68533e935ee4@bootlin.com?part=8