Re: [PATCH net-next v12 08/12] dpll: sit9531x: add support to get and set frequency on pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: sit9531x: endianness bug when reading DIVN_NUM and DIVN_DEN reverses bytes
--

commit 0b96c7bf8e03b2b8b37f84bca16849f39b173d51
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to get and set frequency on pins

Adds support for getting and setting frequencies on sit9531x pins. Input
frequency is reported from firmware, while output frequency is computed from
VCO divided by a divider. Includes logic for programming output dividers and
phase flush.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 79520640dfc88..ea15830b0efae 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> @@ -1342,6 +1481,672 @@ int sit9531x_input_prio_add(struct sit9531x_dev *sitdev, u8 pll_idx,
[ ... ]
> +static int sit9531x_divn_static(struct sit9531x_dev *sitdev, u8 pll_idx,
> +				s64 *divn)
> +{
> +	u32 int_part, fracn_raw = 0, fracd_raw = 0;
> +	u64 fracd;
> +	s64 fracn;
> +	int rc, i;
> +	u8 v;
> +
> +	rc = sit9531x_read_pll_u8(sitdev, pll_idx,
> +				  SIT9531X_PLL_REG_DIVN_INT, &v);
> +	if (rc)
> +		return rc;
> +	int_part = v;
> +
> +	for (i = 3; i >= 0; i--) {
> +		rc = sit9531x_read_pll_u8(sitdev, pll_idx,
> +					  SIT9531X_PLL_REG_DIVN_NUM + i, &v);
> +		if (rc)
> +			return rc;
> +		fracn_raw = (fracn_raw << 8) | v;
> +	}

[Severity: High]
Does this loop reverse the byte order of the fractional numerator?

The device appears to use Big Endian registers, as seen in
sit9531x_output_divo_write() where the MSB is placed at the lowest address.

Here, the loop reads the highest address (SIT9531X_PLL_REG_DIVN_NUM + 3,
the LSB) first and shifts it into the MSB of fracn_raw. Because
sit9531x_output_freq_set() verifies that div64_u64(fvco, divo) equals the
requested frequency, could this scrambled VCO frequency cause valid
frequency set requests to fail with -EINVAL?

> +
> +	for (i = 3; i >= 0; i--) {
> +		rc = sit9531x_read_pll_u8(sitdev, pll_idx,
> +					  SIT9531X_PLL_REG_DIVN_DEN + i, &v);
> +		if (rc)
> +			return rc;
> +		fracd_raw = (fracd_raw << 8) | v;
> +	}

[Severity: High]
Does this apply the same reversed byte order logic to the denominator?

> +
> +	/*
> +	 * NUM/DEN is the fractional part of DIVN, so |NUM| is below DEN by
> +	 * construction.  A pair that says otherwise did not come from a

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009183151.78497-1-arouhi@sitime.com?part=8