Re: [PATCH net 1/2] dpll: zl3073x: make embedded sync follow the output carrier frequency

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/dpll/zl3073x/dpll.c,
  zl3073x_dpll_output_pin_esync_get() no longer works out the reported…

--

dpll: zl3073x: make embedded sync follow the output carrier frequency

This commit moves the output pin eSync handling into new
zl3073x_out_esync_{is_enabled,enable,disable}() helpers. When the output
divider changes in zl3073x_dpll_output_pin_frequency_set(), it recomputes
the eSync period and width, and it disables eSync when the new carrier is
1 Hz.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index f2e137475b40f..9eec27331fe3d 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -864,38 +864,28 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,

[ ... ]

>  	esync->range = esync_freq_ranges;
>  	esync->range_num = ARRAY_SIZE(esync_freq_ranges);
>  
> -	if (zl3073x_out_clock_type_get(out) != ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC) {
> -		/* No need to read esync data if it is not enabled */
> +	if (zl3073x_out_esync_is_enabled(out)) {
> +		esync->freq = 1;
> +		esync->pulse = 25;
> +	} else {
>  		esync->freq = 0;
>  		esync->pulse = 0;
> -
> -		return 0;
>  	}
>  
> -	/* Compute esync frequency */
> -	esync->freq = out_freq / out->esync_n_period;
> -
> -	/* By comparing the esync_pulse_width to the half of the pulse width
> -	 * the esync pulse percentage can be determined.
> -	 * Note that half pulse width is in units of half synth cycles, which
> -	 * is why it reduces down to be output_div.
> -	 */
> -	esync->pulse = (50 * out->esync_n_width) / out->div;
> -

[Severity: Medium]
Is it intended that zl3073x_dpll_output_pin_esync_get() now reports fixed
values instead of what is actually programmed in esync_n_period and
esync_n_width?

When the clock type is ESYNC, the getter now always reports freq=1 and
pulse=25, whatever the cached period and width registers contain. The
values can differ from the real signal in at least two cases.

First, zl3073x_out_state_fetch() in out.c loads ESYNC_PERIOD and
ESYNC_WIDTH from the chip at probe and only rejects a zero period:

	rc = zl3073x_read_u32(zldev, ZL_REG_OUTPUT_ESYNC_PERIOD,
			      &out->esync_n_period);
	...
	if (!out->esync_n_period) {

Stale values can carry over. They can come from NVM, or from an older
kernel that changed div without updating esync_n_period, which is the bug
this patch fixes. Take a 1 MHz carrier with esync_n_period = 10000000.
That is really a 0.1 Hz eSync. The old getter reported 0, and the new one
reports 1.

Second, zl3073x_out_esync_enable() in out.h sets:

	out->esync_n_width = out->div / 2;

Its comment notes that resolution is lost when div is odd. With div = 5,
the programmed pulse is 50 * 2 / 5 = 20%. The old getter reported 20, but
the new one reports 25.

The fixed readback also seems to block reprogramming through netlink.
dpll_pin_esync_set() in dpll_netlink.c does:

	if (freq == esync.freq)
		return 0;

So a request for 1 Hz meant to fix a stale period returns success without
calling esync_set(). The registers would only be rewritten by setting 0
and then 1.

In dpll.yaml, esync-frequency and esync-pulse describe the actual embedded
SYNC signal. The input pin getter in the same file only reports 1 after
checking the divider:

	esync->freq = ref->esync_n_div == ZL_REF_ESYNC_DIV_1HZ ? 1 : 0;

Should the output getter keep computing freq and pulse from the cached
esync_n_period, esync_n_width and div? If the change is intended, could
the commit message mention it? Right now it only describes moving code
into helpers, and the values userspace reads back have changed.

>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001080648.1424172-1-ivecera%40redhat.com