> -----Original Message-----
> From: Jakub Kicinski <[email protected]>
> Sent: Saturday, May 2, 2026 7:33 PM
> To: Nitka, Grzegorz <[email protected]>
> Cc: Jakub Kicinski <[email protected]>; [email protected]; Vecera, Ivan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; Kitszel, Przemyslaw <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; Kubalewski, Arkadiusz
> <[email protected]>; [email protected]; intel-wired-
> [email protected]; [email protected]; [email protected];
> Nguyen, Anthony L <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [PATCH v7 net-next 4/8] dpll: zl3073x: allow SyncE_Ref pin state
> change
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> dpll: zl3073x: allow SyncE_Ref pin state change
>
> This patch advertises DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE on the
> output-direction branch of zl3073x_pin_props_get() with the stated intent
> of allowing dynamic enable/disable of the SyncE_Ref pin.
>
> > diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> > index ac9d41d0f978..acd7061a741a 100644
> > --- a/drivers/dpll/zl3073x/prop.c
> > +++ b/drivers/dpll/zl3073x/prop.c
> > @@ -215,6 +215,15 @@ struct zl3073x_pin_props
> *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
> >
> > props->dpll_props.type = DPLL_PIN_TYPE_GNSS;
> >
> > + /*
> > + * The SyncE_Ref pin supports enabling/disabling dynamically.
> > + * Some platforms may choose to expose this through
> firmware
> > + * configuration later. For now, advertise this capability
> > + * universally since the hardware allows state toggling.
> > + */
> > + props->dpll_props.capabilities |=
> > + DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
>
> Is this capability actually supported by the output pin ops?
>
> Looking at drivers/dpll/zl3073x/dpll.c, the output pin ops vector does
> not install a .state_on_dpll_set callback:
>
> static const struct dpll_pin_ops zl3073x_dpll_output_pin_ops = {
> .direction_get = zl3073x_dpll_pin_direction_get,
> .esync_get = zl3073x_dpll_output_pin_esync_get,
> .esync_set = zl3073x_dpll_output_pin_esync_set,
> .frequency_get = zl3073x_dpll_output_pin_frequency_get,
> .frequency_set = zl3073x_dpll_output_pin_frequency_set,
> .phase_adjust_get = zl3073x_dpll_output_pin_phase_adjust_get,
> .phase_adjust_set = zl3073x_dpll_output_pin_phase_adjust_set,
> .state_on_dpll_get = zl3073x_dpll_output_pin_state_on_dpll_get,
> };
>
> and the corresponding getter hardcodes the state to CONNECTED with no
> hardware read:
>
> zl3073x_dpll_output_pin_state_on_dpll_get(...)
> {
> /* If the output pin is registered then it is always connected */
> *state = DPLL_PIN_STATE_CONNECTED;
> return 0;
> }
>
> On the netlink side, dpll_pin_state_set() first checks the capability
> bit and then requires the setter to exist:
>
> if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE & pin-
> >prop.capabilities)) {
> NL_SET_ERR_MSG(extack, "state changing is not allowed");
> return -EOPNOTSUPP;
> }
> ...
> if (!ops->state_on_dpll_set)
> return -EOPNOTSUPP;
>
> With this patch, the capability check now passes for every output pin,
> but the operation then fails with -EOPNOTSUPP because no setter is
> provided, and the reported state will remain CONNECTED regardless of
> what userspace requested.
>
> Should this series also add a .state_on_dpll_set callback (and a
> matching hardware path, for example via zl3073x_out_state_set or the
> output-enable mailbox) before advertising the capability, so the commit
> subject "allow SyncE_Ref pin state change" can actually be honored?
>
> Separately, should the scope of this change match the commit message?
>
> The subject and the first sentence of the new comment describe a change
> scoped to the SyncE_Ref pin:
>
> * The SyncE_Ref pin supports enabling/disabling dynamically.
>
> but the assignment lives in the generic else-branch of
> zl3073x_pin_props_get() that runs for every output pin (SMA outputs,
> 1PPS outputs, internal-oscillator outputs, MUX outputs, etc.), and it
> executes before the fwnode "connection-type" parsing below can classify
> the pin as ext/gnss/int/synce/mux. The last sentence of the same
> comment acknowledges this:
>
> * universally since the hardware allows state toggling.
>
> Would it be clearer to either gate the flag on the SyncE_Ref pin
> specifically (e.g., via firmware-node property, as the comment hints
> at), or to update the subject and leading comment sentence to reflect
> that the capability is being advertised for all output pins?
>
> > + /*
> > + * The SyncE_Ref pin supports enabling/disabling dynamically.
> > + * Some platforms may choose to expose this through
> firmware
> > + * configuration later. For now, advertise this capability
> > + * universally since the hardware allows state toggling.
> > + */
>
> This isn't a bug, but the indentation of the comment block is
> inconsistent. The opening "/*" is prefixed with one tab followed by
> seven spaces, while the " *" continuation lines and the closing " */"
> are prefixed with two tabs, so the opening "/" does not align with the
> continuation asterisks. Adjacent comment blocks in the same function
> (for example "The output pin phase adjustment granularity...") use pure
> tab alignment.
>
> > + props->dpll_props.capabilities |=
> > + DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
>
> This isn't a bug, but props was just obtained from kzalloc_obj() and
> nothing in this else-branch writes to capabilities before this point.
> The symmetric input branch immediately above uses plain "=":
>
> props->dpll_props.capabilities =
> DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE |
> DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
>
> Would "=" be more consistent here, since there are no pre-existing bits
> to merge with?
Agree, it's not ideal solution. After discussion with Ivan Vecera, I'd like to
come up
with a different approach, i.e. relax the capability check in
dpll_pin_state_set() and
dpll_pin_on_pin_state_set(): when a pin has an associated fwnode, bypass
the capability gate and let the ops layer decide.
To be presented in v8
Thanks!
Grzegorz
> --
> pw-bot: cr
