Re: [PATCH v2 4/5] clk: renesas: rzv2h-cpg: Extract PLL calculation math into a library

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The new file rzv2h-cpg-lib.c uses EXPORT_SYMBOL_NS_GPL but fails to include <linux/export.h> or <linux/module.h>, which will cause compilation errors.

Pre-existing issues:
- [High] Multiplication of a signed 16-bit integer by an unsigned long results in incorrect calculations on 32-bit architectures, breaking the PLL fractional calculation for negative values.
--

commit 160d4676b08e19a075d3da03680cc8c66148bc74
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

clk: renesas: rzv2h-cpg: Extract PLL calculation math into a library

Move the common PLL and divider parameter calculation logic from the core
rzv2h-cpg driver into a standalone library file. Introduce the
CLK_RZV2H_CPG_LIB Kconfig configuration symbol. The original driver retains
wrappers that call into the new library helpers.

> diff --git a/drivers/clk/renesas/rzv2h-cpg-lib.c b/drivers/clk/renesas/rzv2h-cpg-lib.c
> new file mode 100644
> index 0000000000000..98e789df5d7f1
> --- /dev/null
> +++ b/drivers/clk/renesas/rzv2h-cpg-lib.c

[ ... ]

> +				/* Compute (Ffvco * 65536) */
> +				fvco = mul_u32_u32(p.m * 65536 + p.k, fref);
> +				if (fvco < mul_u32_u32(limits->fvco.min, 65536) ||
> +				    fvco > mul_u32_u32(limits->fvco.max, 65536))
> +					continue;
> +
> +				/* PLL_M component of (output * 65536 * PLL_P) */
> +				output = mul_u32_u32(p.m * 65536, input_fref);
> +				/* PLL_K component of (output * 65536 * PLL_P) */
> +				output += p.k * input_fref;

[Severity: High]
This is a pre-existing issue, but since the commit message mentions this
library will be shared with the upcoming RZ/T2H platforms (which are 32-bit),
could this calculation corrupt the output frequency when p.k is negative?

On 32-bit architectures, input_fref (unsigned long) is 32-bit. When p.k
(s16) is negative, standard C promotion rules convert it to a 32-bit
unsigned integer. The multiplication evaluates as a 32-bit unsigned product,
which is then zero-extended to 64 bits before being added to output (u64).

Instead of subtracting from output, won't this add a large positive number?

> +				/* Make it in mHz */
> +				output *= MILLI;
> +				output = DIV_U64_ROUND_CLOSEST(output, 65536 * p.p * divider);

[ ... ]

> +			}
> +		}
> +	}
> +
> +	/* If no valid parameters were found, return false */
> +	if (best.error_millihz == S64_MAX)
> +		return false;
> +
> +	*pars = best;
> +	return true;
> +}
> +EXPORT_SYMBOL_NS_GPL(rzv2h_cpg_get_pll_pars, "RZV2H_CPG");

[Severity: Low]
Does this file need to include <linux/export.h> or <linux/module.h>?

The EXPORT_SYMBOL_NS_GPL macro is used here, but the necessary headers
are missing from the includes at the top of the file. Could this cause
compilation errors on some architectures due to implicit declarations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609105924.962573-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=4