Re: [PATCH v7 net-next 4/8] dpll: zl3073x: allow SyncE_Ref pin state change

[Jakub Kicinski]

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?
-- 
pw-bot: cr