Re: [PATCH net-next v14 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] dpll: sit9531x: endianness reversal in DIVN register read
--

commit 1d40f7b3172f27292951c0c38e90a095069d4230
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

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

This commit introduces pin frequency set and get functionality for the
sit9531x DPLL driver. It calculates hardware divider values, reads actual
pin rates, and manages device locking and programming sequences.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index f3dbe949f219b..875a5fe783d5c 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> +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 bytes of the 32-bit numerator?

Because the chip's registers use a Big-Endian layout, the lowest address
(offset + 0) contains the most significant byte. By reading from the highest
address (offset + 3) first and shifting it into the MSB position of the
accumulator, it appears to be parsing the value as Little-Endian. Will this
cause the VCO frequency calculation to produce incorrect values?

> +
> +	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]
Can this identical loop logic cause the denominator bytes to be reversed as
well?

Since both the numerator and denominator are affected, it looks like this
will corrupt the readbacks and cause sit9531x_output_divo_calc() to
miscalculate or incorrectly reject requested output rates. Should the loop
read the lowest address first, or should the accumulator shift be reversed?

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