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

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:

quoted

On Fri, 24 Oct 2025 16:49:26 +0200 Ivan Vecera wrote:

quoted

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