Re: [PATCH net-next v2 05/14] dpll: dpllsim: Add support to set input pin state

[Ivan Vecera <ivecera@redhat.com>]

On 11/5/26 16:18, Petr Oros wrote:
> Add support for setting the state of input pins on the device.
> Implement the state_on_dpll_set callback for input pins and advertise
> the state-can-change capability for them, so that userspace can move a
> pin between connected, disconnected and selectable. The driver stores
> the requested state and reports it back unchanged, it does not act on it
> yet. Output pins keep reporting their state only.
> 
> Signed-off-by: Petr Oros <poros@redhat.com>
> ---
>   drivers/dpll/dpllsim/dpll.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/dpll/dpllsim/dpll.c b/drivers/dpll/dpllsim/dpll.c
> index a1d5921fb8d017..2b0c053a7b6087 100644
> --- a/drivers/dpll/dpllsim/dpll.c
> +++ b/drivers/dpll/dpllsim/dpll.c
> @@ -104,10 +104,23 @@ static int dpllsim_pin_state_get(const struct dpll_pin *pin, void *pin_priv,
>   	return 0;
>   }
>   
> +static int dpllsim_pin_state_set(const struct dpll_pin *pin, void *pin_priv,
> +				 const struct dpll_device *dpll,
> +				 void *dpll_priv, enum dpll_pin_state state,
> +				 struct netlink_ext_ack *extack)
> +{
> +	struct dpllsim_pin *sim_pin = pin_priv;
> +
> +	guard(mutex)(&sim_pin->dev->lock);
> +	sim_pin->state = state;
> +	return 0;
> +}

Should DPLL_PIN_STATE_CONNECTED be accepted here?

The device always reports automatic mode, which dpllsim_make_group() sets
and nothing changes:

	sim->mode = DPLL_MODE_AUTOMATIC;

Documentation/driver-api/dpll.rst limits what a user can request in that
mode:

     In automatic mode (``DPLL_MODE_AUTOMATIC``) the user can request one of
     following pin states:

     - ``DPLL_PIN_STATE_SELECTABLE`` - the pin shall be considered as valid
       input for automatic selection algorithm
     - ``DPLL_PIN_STATE_DISCONNECTED`` - the pin shall be not considered as
       a valid input for automatic selection algorithm

     Pins that have the ``DPLL_PIN_CAPABILITIES_STATE_CONNECTED_OVERRIDE``
     capability can additionally be set to ``DPLL_PIN_STATE_CONNECTED`` in
     automatic mode, overriding the active input selection.

dpll_pin_state_set() in the core only checks state-can-change, so the
driver has to refuse the state. zl3073x does that for its automatic mode
in zl3073x_dpll_input_pin_state_on_dpll_set():

	case ZL_DPLL_MODE_REFSEL_MODE_AUTO:
		if (state == DPLL_PIN_STATE_SELECTABLE) {
			...
		} else if (state == DPLL_PIN_STATE_DISCONNECTED) {
			...
		} else {
			goto invalid_state;
		}

[ ... ]
> @@ -149,6 +162,12 @@ static void dpllsim_pin_props(struct dpllsim_pin *pin,
>   		prop->freq_supported_num = ARRAY_SIZE(dpllsim_freq_ext);
>   		break;
>   	}
> +
> +	/* Capabilities follow the ops the pin is served by */
> +	if (dpllsim_pin_select_ops(pin) == &dpllsim_input_pin_ops)
> +		prop->capabilities = DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
> +	else
> +		prop->capabilities = 0;
>   }
The input pins advertise state-can-change only. If connected is meant to
be settable on this automatic-only device, should
DPLL_PIN_CAPABILITIES_STATE_CONNECTED_OVERRIDE be advertised here as well?

If not, should dpllsim_pin_state_set() return -EINVAL with an extack
message for it?

The commit message says "so that userspace can move a pin between
connected, disconnected and selectable", and the stress test added by the
next patch, 70a4dcf52b9c ("selftests: dpll: add pin state tests"), relies
on it. test_pin_state_flooding() cycles through

	local states=(connected disconnected selectable)

and fails when one of the sets is refused.

Thanks,
Ivan