Re: [PATCH v10 05/11] clk: realtek: Add support for phase locked loops (PLLs)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asymmetry in enable/disable logic permanently bricks PLLs that lack a readable power status register.
- [Low] The header file `freq_table.h` lacks include guards.
--

--- Patch [5]: [PATCH v10 05/11] clk: realtek: Add support for phase locked loops (PLLs) ---
commit 1e05dd1885eded463b663e611cd68c039c2be096
Author: Cheng-Yu Lee <cylee12@realtek.com>
clk: realtek: Add support for phase locked loops (PLLs)

This commit provides a full set of PLL operations for programmable PLLs
and a read-only variant for fixed or hardware-managed PLLs.

> diff --git a/drivers/clk/realtek/clk-pll.c b/drivers/clk/realtek/clk-pll.c
> new file mode 100644
> index 0000000000000..54c284070e47a
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll.c
[ ... ]
> @@ -173,20 +173,19 @@ static int clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  static int clk_pll_enable(struct clk_hw *hw)
>  {
>  	struct clk_pll *clkp = to_clk_pll(hw);
>  	unsigned long flags;
>  	int ret = 0;
>  
>  	if (!clkp->seq_power_on)
>  		return ret;
>  
>  	spin_lock_irqsave(&clkp->lock, flags);
>  
>  	if (is_power_on(clkp))
>  		goto unlock;

[Severity: High]
If a PLL has power on/off sequences but no readable status register,
is_power_on() unconditionally returns true. Will this cause the enable path
to permanently skip the power-on sequence if the clock was previously
disabled?

When clk_pll_disable() is called, it executes the power-off sequence:

static void clk_pll_disable(struct clk_hw *hw)
{
        ...
        regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_power_off, ...);
        ...
}

But when clk_pll_enable() is subsequently called, is_power_on() returns true
because power_reg is 0. Does this cause the power-on sequence to be skipped,
leaving the PLL permanently disabled despite the clock core believing it is
enabled?

> +
> +	ret = regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_power_on,
> +				     clkp->num_seq_power_on);
> +	if (ret)
> +		goto unlock;
[ ... ]
> diff --git a/drivers/clk/realtek/freq_table.h b/drivers/clk/realtek/freq_table.h
> new file mode 100644
> index 0000000000000..78215aee33007
> --- /dev/null
> +++ b/drivers/clk/realtek/freq_table.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +struct freq_table {

[Severity: Low]
Is it intentional to omit the standard include guards here? If this header
is included multiple times in the future, could it lead to redefinition
errors for struct freq_table?

> +	u32 val;
> +	unsigned long rate;
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713155630.3054636-1-eleanor.lin@realtek.com?part=5