Re: [PATCH net-next 1/5] dpll: zl3073x: clean up esync get/set and use zl3073x_out_is_ndiv()

[Simon Horman <horms@kernel.org>]

On Fri, Mar 20, 2026 at 06:31:36PM +0100, Ivan Vecera wrote:
> Hi Simon,
> 
> On 3/20/26 6:17 PM, Simon Horman wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > 
> > For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> > ---
> > dpll: zl3073x: clean up esync get/set and use zl3073x_out_is_ndiv()
> > 
> > Return -EOPNOTSUPP early in esync_get callbacks when esync is not
> > supported instead of conditionally populating the range at the end.
> > This simplifies the control flow by removing the finish label/goto
> > in the output variant and the conditional range assignment in both
> > input and output variants.
> > 
> > Replace open-coded N-div signal format switch statements with
> > zl3073x_out_is_ndiv() helper in esync_get, esync_set and
> > frequency_set callbacks.
> > 
> > > diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> > > index a29f606318f6..79ef62d69a32 100644
> > > --- a/drivers/dpll/zl3073x/dpll.c
> > > +++ b/drivers/dpll/zl3073x/dpll.c
> > > @@ -131,6 +131,12 @@ zl3073x_dpll_input_pin_esync_get(const struct dpll_pin *dpll_pin,
> > >   	ref_id = zl3073x_input_pin_ref_get(pin->id);
> > >   	ref = zl3073x_ref_state_get(zldev, ref_id);
> > > 
> > > +	if (!pin->esync_control || zl3073x_ref_freq_get(ref) == 1)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	esync->range = esync_freq_ranges;
> > > +	esync->range_num = ARRAY_SIZE(esync_freq_ranges);
> > > +
> > >   	switch (FIELD_GET(ZL_REF_SYNC_CTRL_MODE, ref->sync_ctrl)) {
> > >   	case ZL_REF_SYNC_CTRL_MODE_50_50_ESYNC_25_75:
> > >   		esync->freq = ref->esync_n_div == ZL_REF_ESYNC_DIV_1HZ ? 1 : 0;
> > > @@ -142,17 +148,6 @@ zl3073x_dpll_input_pin_esync_get(const struct dpll_pin *dpll_pin,
> > >   		break;
> > >   	}
> > > 
> > > -	/* If the pin supports esync control expose its range but only
> > > -	 * if the current reference frequency is > 1 Hz.
> > > -	 */
> > > -	if (pin->esync_control && zl3073x_ref_freq_get(ref) > 1) {
> > > -		esync->range = esync_freq_ranges;
> > > -		esync->range_num = ARRAY_SIZE(esync_freq_ranges);
> > > -	} else {
> > > -		esync->range = NULL;
> > > -		esync->range_num = 0;
> > > -	}
> > > -
> > >   	return 0;
> > >   }
> > 
> > The refactoring changes the behavior when the reference frequency is 0 Hz.
> > 
> > The old code checked 'zl3073x_ref_freq_get(ref) > 1' which excluded both
> > freq==0 and freq==1 from having the esync range exposed. The new code checks
> > 'zl3073x_ref_freq_get(ref) == 1' which only excludes freq==1.
> > 
> > When freq==0 (which can happen when freq_base, freq_mult, or freq_ratio_m
> > is 0), the old code would return 0 with esync->range=NULL and
> > esync->range_num=0, while the new code returns 0 with
> > esync->range=esync_freq_ranges and esync->range_num=1.
> > 
> > The removed comment documented the original intent: "if the current reference
> > frequency is > 1 Hz". Should the new check be 'zl3073x_ref_freq_get(ref) <= 1'
> > to preserve the original behavior for 0 Hz frequencies?
> > 
> > > 
> > > @@ -582,8 +577,8 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
> > >   	struct zl3073x_dpll_pin *pin = pin_priv;
> > >   	const struct zl3073x_synth *synth;
> > >   	const struct zl3073x_out *out;
> > > +	u32 synth_freq, out_freq;
> > >   	u8 clock_type, out_id;
> > > -	u32 synth_freq;
> > > 
> > >   	out_id = zl3073x_output_pin_out_get(pin->id);
> > >   	out = zl3073x_out_state_get(zldev, out_id);
> > > @@ -592,17 +587,19 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
> > >   	 * for N-division is also used for the esync divider so both cannot
> > >   	 * be used.
> > >   	 */
> > > -	switch (zl3073x_out_signal_format_get(out)) {
> > > -	case ZL_OUTPUT_MODE_SIGNAL_FORMAT_2_NDIV:
> > > -	case ZL_OUTPUT_MODE_SIGNAL_FORMAT_2_NDIV_INV:
> > > +	if (zl3073x_out_is_ndiv(out))
> > >   		return -EOPNOTSUPP;
> > > -	default:
> > > -		break;
> > > -	}
> > > 
> > >   	/* Get attached synth frequency */
> > >   	synth = zl3073x_synth_state_get(zldev, zl3073x_out_synth_get(out));
> > >   	synth_freq = zl3073x_synth_freq_get(synth);
> > > +	out_freq = synth_freq / out->div;
> > > +
> > > +	if (!pin->esync_control || out_freq == 1)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	esync->range = esync_freq_ranges;
> > > +	esync->range_num = ARRAY_SIZE(esync_freq_ranges);
> > > 
> > >   	clock_type = FIELD_GET(ZL_OUTPUT_MODE_CLOCK_TYPE, out->mode);
> > >   	if (clock_type != ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC) {
> > > @@ -610,11 +607,11 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
> > >   		esync->freq = 0;
> > >   		esync->pulse = 0;
> > > 
> > > -		goto finish;
> > > +		return 0;
> > >   	}
> > > 
> > >   	/* Compute esync frequency */
> > > -	esync->freq = synth_freq / out->div / out->esync_n_period;
> > > +	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.
> > > @@ -623,18 +620,6 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
> > >   	 */
> > >   	esync->pulse = (50 * out->esync_n_width) / out->div;
> > > 
> > > -finish:
> > > -	/* Set supported esync ranges if the pin supports esync control and
> > > -	 * if the output frequency is > 1 Hz.
> > > -	 */
> > > -	if (pin->esync_control && (synth_freq / out->div) > 1) {
> > > -		esync->range = esync_freq_ranges;
> > > -		esync->range_num = ARRAY_SIZE(esync_freq_ranges);
> > > -	} else {
> > > -		esync->range = NULL;
> > > -		esync->range_num = 0;
> > > -	}
> > > -
> > >   	return 0;
> > >   }
> > 
> > Similar to the input variant, this changes the behavior for 0 Hz output
> > frequencies.
> > 
> > The old code used '(synth_freq / out->div) > 1' to decide whether to expose
> > the esync range, which excluded both out_freq==0 and out_freq==1. The new
> > code uses 'out_freq == 1' for the early -EOPNOTSUPP return, only excluding
> > out_freq==1.
> > 
> > When out_freq==0 (which can happen when synth_freq is 0 or synth_freq <
> > out->div), the old code returned 0 with esync->range=NULL and
> > esync->range_num=0, while the new code returns 0 with
> > esync->range=esync_freq_ranges and esync->range_num=1.
> > 
> > The removed comment documented the intent: "if the output frequency is > 1
> > Hz". Should this also use 'out_freq <= 1' to maintain consistency with the
> > original behavior?
> > 
> > [ ... ]
> > 
> 
> The frequency for both input and output pins cannot be zero... Cannot be
> set to 0 and device never returns configured frequency to be 0.
> 
> I can modify the lines to be:
> 
> if (!pin->esync_control || out_freq <= 1)
> 	return -EOPNOTSUPP;
> 
> but it is needless - but let me know.

Thanks Ivan,

If it is needless then there is no need to handle it in the code.
Sorry for not understanding that before forwarding on the AI generated review.