Re: [PATCH net-next v9 10/15] dpll: sit9531x: add support to adjust output phase

[Ali Rouhi <arouhi@sitime.com>]

On 9/17/26 11:55 AM, Ivan Vecera wrote:
>> +	if (phase_ps == 0) {
>> +		abs_ps = 0;
>> +	} else if (phase_ps > 0) {
>> +		abs_ps = (u64)phase_ps;
>> +		div64_u64_rem(abs_ps, t_out_ps, &abs_ps);
>> +		phase_norm_ps = abs_ps;
>> +	} else {
>> +		u64 advance = (u64)(-(s64)phase_ps);
>> +
>> +		div64_u64_rem(advance, t_out_ps, &advance);
>> +		phase_norm_ps = -(s64)advance;
>> +		abs_ps = (advance == 0) ? 0 : (t_out_ps - advance);
>> +	}
> 
> This if-else branches could be reduced to:
> <snip>
> abs_ps = abs(phase_ps); /* Safe. INT_MIN is not possible here*/
> div64_u64_rem(abs_ps, t_out_ps, &abs_ps);
> phase_norm_ps = phase_ps < 0 ? -(s64)abs_ps : (s64)abs_ps;
> abs_ps = phase_ps < 0 && abs_ps ? t_out_ps - abs_ps : abs_ps;
> </snip>

Taken as written. v10 has those four lines in place of the three-way
branch, with only a pair of parentheses added on the last one.

Your INT_MIN note holds, and for a reason I wanted to confirm rather
than take on trust: the core rejects a phase-adjust request outside the
advertised phase range before the driver is called, and that range is
+/-1 ms expressed in ps. The value that reaches this function is
therefore bounded well inside s32, and abs() cannot be handed INT_MIN.
The comment above the block now says so, so the next reader does not
have to re-derive it.

>> +	coarse = mul_u64_u64_div_u64(abs_ps, fvco, 1000000000000ULL);
>> +	if (coarse >= (1ULL << SIT9531X_OUT_PRG_COARSE_BITS))
>> +		return -ERANGE;
>> +
>> +	/* Fine delay = round((abs_ps - coarse * vco_period_ps) / 30 ps) */
>> +	coarse_ps = mul_u64_u64_div_u64(coarse, 1000000000000ULL, fvco);
>> +	rem_ps = (abs_ps > coarse_ps) ? (abs_ps - coarse_ps) : 0;
>> +	if (rem_ps) {
>> +		u64 steps;
>> +
>> +		steps = div64_u64(rem_ps + SIT9531X_OUT_PRG_FINE_STEP_PS / 2,
>> +				  SIT9531X_OUT_PRG_FINE_STEP_PS);
>> +		if (steps > SIT9531X_OUT_PRG_FINE_MAX)
>> +			steps = SIT9531X_OUT_PRG_FINE_MAX;
>> +		fine = (u8)steps;
>> +	}
> 
> Also this whole block can be skipped (and set coarse to 0) in case of 
> abs_ps == 0 to avoid expensive divisions.

Also taken. The coarse and fine encoding now sits inside an
if (abs_ps) block, and coarse is initialized to 0 at its declaration,
so a zero offset does no division at all. rem_ps moved inside the block
with it, since nothing outside needs it.

Patch 10 in v10 carries

  Suggested-by: Ivan Vecera <ivecera@redhat.com>

v10 is posted:
https://lore.kernel.org/netdev/20260921201108.42676-1-arouhi@sitime.com/

Thanks,
Ali