Re: [PATCH net-next 1/2] dpll: add phase-adjust-gran pin attribute
From: Ivan Vecera
Date: Wed Oct 29 2025 - 11:00:41 EST
On 10/29/25 3:20 PM, Jiri Pirko wrote:
Wed, Oct 29, 2025 at 08:44:52AM +0100, ivecera@xxxxxxxxxx wrote:I will use u32 for phase-adjust-gran and dpll_pin_properties.phase_gran.
Hi Kuba,
On 10/29/25 2:39 AM, Jakub Kicinski wrote:
On Fri, 24 Oct 2025 16:49:26 +0200 Ivan Vecera wrote:To have it unsigned brings a need to use explicit type casting in the core
+ -
+ name: phase-adjust-gran
+ type: s32
+ doc: |
+ Granularity of phase adjustment, in picoseconds. The value of
+ phase adjustment must be a multiple of this granularity.
Do we need this to be signed?
and driver's code. The phase adjustment can be both positive and
negative it has to be signed. The granularity specifies that adjustment
has to be multiple of granularity value so the core checks for zero
remainder (this patch) and the driver converts the given adjustment
value using division by the granularity.
If we would have phase-adjust-gran and corresponding structure fields
defined as u32 then we have to explicitly cast the granularity to s32
because for:
I prefer cast. The uapi should be clear. There is not point of having
negative granularity.
OK?
Depends... if the driver will use s32 (sse patch 2) then no castings are necessary.<snip>
s32 phase_adjust, remainder;
u32 phase_gran = 1000;
phase_adjust = 5000;
remainder = phase_adjust % phase_gran;
/* remainder = 0 -> OK for positive adjust */
phase_adjust = -5000;
remainder = phase_adjust % phase_gran;
/* remainder = 296
* Wrong for negative adjustment because phase_adjust is casted to u32
* prior division -> 2^32 - 5000 = 4294962296.
* 4294962296 % 1000 = 296
*/
remainder = phase_adjust % (s32)phase_gran;
/* remainder = 0
* Now OK because phase_adjust remains to be s32
*/
</snip>
Similarly for division in the driver code if the granularity would be
u32.
So I have proposed phase adjustment granularity to be s32 to avoid these
explicit type castings and potential bugs in drivers.
Cast in dpll core, no?
Thanks,
Ivan