Re: [PATCH v10 10/14] dpll: sit9531x: add support to adjust output phase

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dpll: sit9531x: missing phase_stale update on adjust failure
--

commit 133246fcb22569dda87bbed0ea13e64009ac8953
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to adjust output phase

Adds support to adjust output phase by shifting the output in time against
others driven by the same PLL. The requested offset is encoded into coarse
and fine fields, with delay wrapped to the output period when needed.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> +	/*
> +	 * Restart the output divider phase so the freshly programmed delay is
> +	 * applied against a known edge instead of the divider's arbitrary
> +	 * running phase.
> +	 */
> +	rc = sit9531x_output_phase_flush(sitdev, pll_idx);
> +	if (rc)
> +		return rc;
[ ... ]
> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> +sit9531x_dpll_output_pin_phase_adjust_get(const struct dpll_pin *pin,
> +					  void *pin_priv,
> +					  const struct dpll_device *dpll,
> +					  void *dpll_priv, s32 *phase_adjust,
> +					  struct netlink_ext_ack *extack)
> +{
[ ... ]
> +	mutex_lock(&sitdev->multiop_lock);
> +	/*
> +	 * A request whose writes reached the device but whose commit or
> +	 * phase flush failed left the cache describing the delay before it.
> +	 * There is no poll of the delay registers to correct that, so read
> +	 * them here rather than report a value the output is not using.
> +	 */
> +	if (sitdev->out[dpin->id].phase_stale) {
[ ... ]
> +sit9531x_dpll_output_pin_phase_adjust_set(const struct dpll_pin *pin,
> +					  void *pin_priv,
> +					  const struct dpll_device *dpll,
> +					  void *dpll_priv, s32 phase_adjust,
> +					  struct netlink_ext_ack *extack)
> +{
> +	struct sit9531x_dpll_pin *dpin = pin_priv;
> +	struct sit9531x_dpll *sitdpll = dpll_priv;
> +	struct sit9531x_dev *sitdev = sitdpll->dev;
> +	int rc;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_output_phase_adjust_set(sitdev, dpin->id, phase_adjust);
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	if (rc) {
> +		NL_SET_ERR_MSG(extack, "Phase adjust failed");
> +		return rc;
> +	}

[Severity: Medium]
Does this error path leave the software cache permanently out of sync
with the hardware state?

When sit9531x_output_phase_adjust_set() is called, it can succeed in writing
the new configuration to the device but fail later on (for example, during
sit9531x_output_phase_flush() as shown above). When this happens, it returns
early without updating the cache.

Since sit9531x_dpll_output_pin_phase_adjust_set() does not set
sitdev->out[dpin->id].phase_stale = true on failure, a subsequent call to
sit9531x_dpll_output_pin_phase_adjust_get() will evaluate phase_stale as
false. This bypasses the mechanism intended to read back the delay registers
and correct the stranded cache values.

Should phase_stale be set to true in this error path, similar to how it is
handled in sit9531x_output_freq_set() during rate changes?

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