RE: [PATCH net-next 1/5] dpll: zl3073x: clean up esync get/set and use zl3073x_out_is_ndiv()
From: Prathosh.Satish
Date: Fri Mar 27 2026 - 11:41:39 EST
Reviewed-by: prathosh.satish@xxxxxxxxxxxxx
-----Original Message-----
From: Simon Horman <horms@xxxxxxxxxx>
Sent: Saturday, March 21, 2026 9:01 AM
To: Ivan Vecera <ivecera@xxxxxxxxxx>
Cc: devicetree@xxxxxxxxxxxxxxx; vadim.fedorenko@xxxxxxxxx; jiri@xxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; pvaanane@xxxxxxxxxx; arkadiusz.kubalewski@xxxxxxxxx; robh@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; conor+dt@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; Prathosh Satish - M66066 <Prathosh.Satish@xxxxxxxxxxxxx>; mschmidt@xxxxxxxxxx; poros@xxxxxxxxxx
Subject: Re: [PATCH net-next 1/5] dpll: zl3073x: clean up esync get/set and use zl3073x_out_is_ndiv()
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
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; range_num =
> > > + esync->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; range_num =
> > > + esync->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.