Re: [PATCH net-next 1/2] dpll: add phase-adjust-gran pin attribute

[Jiri Pirko]

Wed, Oct 29, 2025 at 08:44:52AM +0100, ivecera@xxxxxxxxxx wrote:
>Hi Kuba,
>
>On 10/29/25 2:39 AM, Jakub Kicinski wrote:
>> On Fri, 24 Oct 2025 16:49:26 +0200 Ivan Vecera wrote:
>> > +      -
>> > +        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?
>> 
>To have it unsigned brings a need to use explicit type casting in the core
>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.


>
><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
>