Re: [PATCH net-next v7 3/3] dpll: add SiTime SiT9531x DPLL clock driver

[Ivan Vecera <ivecera@redhat.com>]

On 8/16/26 12:19 AM, Ali Rouhi wrote:
> Add a DPLL subsystem driver for the SiTime SiT95316 and SiT95317
> clock generators. These devices provide low-jitter clock outputs
> commonly used in telecom, networking, and data center timing
> applications.
> 
> The driver exposes all inputs and outputs through the Linux DPLL
> subsystem, supporting:
>   - Lock status monitoring via register polling or optional INTRB IRQ
>   - Input priority management for automatic reference switchover
>   - Per-output frequency readback from hardware state
>   - Phase offset measurement via TDC (time-to-digital converter)
>   - Phase adjustment for fine output alignment
>   - Embedded sync (esync) pulse control on outputs
>   - Fractional frequency offset of the selected reference
>   - Optional reset-gpios for hardware reset
> 
> The driver reads all configuration from the device's on-chip NVM
> at probe time -- no firmware loading is required.
> 
> Co-developed-by: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>
> Signed-off-by: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>
> Assisted-by: Claude:claude-4-opus [chat]
> Signed-off-by: Ali Rouhi <arouhi@sitime.com>
> ---
>   MAINTAINERS                    |    7 +
>   drivers/dpll/Kconfig           |    1 +
>   drivers/dpll/Makefile          |    1 +
>   drivers/dpll/sit9531x/Kconfig  |   17 +
>   drivers/dpll/sit9531x/Makefile |    4 +
>   drivers/dpll/sit9531x/core.c   | 3111 ++++++++++++++++++++++++++++++++
>   drivers/dpll/sit9531x/core.h   |  372 ++++
>   drivers/dpll/sit9531x/dpll.c   | 1232 +++++++++++++
>   drivers/dpll/sit9531x/dpll.h   |   69 +
>   drivers/dpll/sit9531x/prop.c   |  397 ++++
>   drivers/dpll/sit9531x/prop.h   |   39 +
>   drivers/dpll/sit9531x/regs.h   |  371 ++++
>   12 files changed, 5621 insertions(+)
>   create mode 100644 drivers/dpll/sit9531x/Kconfig
>   create mode 100644 drivers/dpll/sit9531x/Makefile
>   create mode 100644 drivers/dpll/sit9531x/core.c
>   create mode 100644 drivers/dpll/sit9531x/core.h
>   create mode 100644 drivers/dpll/sit9531x/dpll.c
>   create mode 100644 drivers/dpll/sit9531x/dpll.h
>   create mode 100644 drivers/dpll/sit9531x/prop.c
>   create mode 100644 drivers/dpll/sit9531x/prop.h
>   create mode 100644 drivers/dpll/sit9531x/regs.h
> 
> ...
> +	/*
> +	 * Convert to unsigned absolute delay.  Negative phase (advance)
> +	 * is rendered as T_out - |phase|, modulo the output period.
> +	 */
> +	if (phase_ps == 0) {
> +		abs_ps = 0;
> +	} else if (phase_ps > 0) {
> +		abs_ps = (u64)phase_ps;
> +	} else {
> +		u64 t_out_ps = div64_u64(1000000000000ULL, freq);
> +		u64 advance = (u64)(-(s64)phase_ps);
> +
> +		if (t_out_ps == 0)
> +			return -EINVAL;
> +		advance %= t_out_ps;
                        ^^^^
This causes build failure on 32bit systems... You could use something
like this:

    div64_u64_rem(advance, t_out_ps, &advance);

> +		abs_ps = (advance == 0) ? 0 : (t_out_ps - advance);
> +	}
> +
> ...
> diff --git a/drivers/dpll/sit9531x/regs.h b/drivers/dpll/sit9531x/regs.h
> new file mode 100644
> index 000000000000..5c3dbeefd86b
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/regs.h
> @@ -0,0 +1,371 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SiTime SiT9531x register definitions
> + *
> + * Copyright (C) 2026 SiTime Corp.
> + * Author: Ali Rouhi <arouhi@sitime.com>
> + * Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>
> + */
> +
> +#ifndef _SIT9531X_REGS_H
> +#define _SIT9531X_REGS_H
> +
> +/*
> + * I2C register model:
> + *   - Page select register at offset 0x01
> + *   - Each page has 128 registers (0x00-0x7F)
> + *   - Some pages are paired (e.g. 0x0A/0x1A for PLLA)
> + */
> +#define SIT9531X_PAGE_SEL		0xFF
> +#define SIT9531X_PAGE_SIZE		0x100
> +#define SIT9531X_NUM_PAGES		32

The comment at the top of regs.h says the page select register is at
offset 0x01 and each page has 128 registers (0x00-0x7F) but the code 
defines it differently (selector at 0xff and page size 256).

One of them is wrong.

Thanks,
Ivan