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

[Ivan Vecera <ivecera@redhat.com>]

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:

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

Thanks,
Ivan