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

[Ivan Vecera <ivecera@redhat.com>]

On 10/29/25 3:20 PM, Jiri Pirko wrote:
> Wed, Oct 29, 2025 at 08:44:52AM +0100, ivecera@redhat.com 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.
> 
> 
I will use u32 for phase-adjust-gran and dpll_pin_properties.phase_gran.

OK?
>> <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?
Depends... if the driver will use s32 (sse patch 2) then no castings are 
necessary.

Thanks,
Ivan