Re: [PATCH net-next v15 12/13] dpll: sit9531x: add support to get phase offset on the connected input pin

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] dpll: sit9531x: track key_pending in
  sit9531x_chan_routed_ref_read()
--

commit 4157ae757748960930773aef1f00d28b5cffb1f4
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to get phase offset on the connected input pin

This commit adds phase_offset_get support for SiT9531x input pins by reading
the phase difference between a PLL and its active reference from the on-chip
time-to-digital converter via the debug registers.

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index e6271ebd47ac1..ba4ac5e7d365d 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -488,6 +488,122 @@ sit9531x_dpll_input_pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
>  	return 0;
>  }
>  
> +/*
> + * Report the loop's residual phase error, sampled with the loop closed. The TDC
> + * compares against a signal the PLL drives, so a PLL driving no output returns
> + * -ENODATA.
> + */
> +static int
> +sit9531x_dpll_input_pin_phase_offset_get(const struct dpll_pin *pin,
> +					 void *pin_priv,
> +					 const struct dpll_device *dpll,
> +					 void *dpll_priv, s64 *phase_offset,
> +					 struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	rc = sit9531x_chan_routed_ref_read(sitdev, sitdpll->id, &routed);
> +	if (!rc && routed == dpin->id) {
> +		rc = sit9531x_phase_offset_read(sitdev, sitdpll->id, &offset);
> +		if (!rc)
> +			rc = sit9531x_chan_routed_ref_read(sitdev, sitdpll->id,
> +							   &routed);
> +		if (!rc && routed != dpin->id)
> +			rc = -ENODATA;
> +	} else if (!rc) {
> +		rc = -ENODATA;
> +	}

[Severity: Low]
Should sit9531x_chan_routed_ref_read() also update chan->key_pending when
re-locking SIT9531X_PLL_REG_DEBUG?

sit9531x_phase_offset_read() updates chan->key_pending when closing the
debug window, but sit9531x_chan_routed_ref_read() unlocks and re-locks the
same register without updating chan->key_pending:

drivers/dpll/sit9531x/core.c:sit9531x_chan_routed_ref_read() {
    ...
relock:
	lock_rc = sit9531x_write_pll_close(sitdev, pll_idx,
					   SIT9531X_PLL_REG_DEBUG,
					   SIT9531X_PLL_DEBUG_LOCK);

	return rc ? rc : lock_rc;
}

If sit9531x_phase_offset_read() closes the debug window (setting
chan->key_pending to false) and the immediately following
sit9531x_chan_routed_ref_read() fails to re-lock SIT9531X_PLL_REG_DEBUG,
the PLL debug window remains open with chan->key_pending left false, so
sit9531x_tap_back_retry() and sit9531x_tap_pending_retry() will skip
closing it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110202607.55744-1-arouhi@sitime.com?part=12