Re: [PATCH net-next v3 09/14] dpll: dpllsim: Add support to set frequency on pins

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch adds dpllsim_pin_frequency_set()
  (drivers/dpll/dpllsim/dpll.c).

--

dpll: dpllsim: Add support to set frequency on pins

This adds a netlink frequency_set callback to the dpllsim input and output
pin ops. The callback stores the value in sim_pin->frequency under
dev->lock. The DPLL core has already checked it against the pin's
supported frequency list.

> diff --git a/drivers/dpll/dpllsim/dpll.c b/drivers/dpll/dpllsim/dpll.c
> index 5e36059609761..8590d2db808e4 100644
> --- a/drivers/dpll/dpllsim/dpll.c
> +++ b/drivers/dpll/dpllsim/dpll.c
> @@ -79,6 +79,19 @@ static int dpllsim_pin_frequency_get(const struct dpll_pin *pin,
>  	return 0;
>  }
>  
> +static int dpllsim_pin_frequency_set(const struct dpll_pin *pin,
> +				     void *pin_priv,
> +				     const struct dpll_device *dpll,
> +				     void *dpll_priv, u64 frequency,
> +				     struct netlink_ext_ack *extack)
> +{
> +	struct dpllsim_pin *sim_pin = pin_priv;
> +
> +	guard(mutex)(&sim_pin->dev->lock);
> +	sim_pin->frequency = frequency;
> +	return 0;
> +}

[Severity: Low]
This isn't a bug, but should Documentation/driver-api/dpllsim.rst be
updated along with this?

dpllsim_pin_frequency_set() writes sim_pin->frequency. That is the same
field the configfs frequency attribute stores and shows through
dpllsim_pin_frequency_store() and dpllsim_pin_frequency_show() in core.c.
dpllsim_pin_frequency_valid() checks it again on the next live=1.

Nothing on the undeploy path resets it, so a frequency set over netlink:

  - shows up when the configfs frequency attribute is read,
  - is still there after live=0,
  - is used on the next live=1 instead of the value written to configfs.

The rst describes pin attributes only as configuration written before
going live:

  Attributes are written while the instance is not live, then ``live`` is
  set to 1 to register the device and its pins with the DPLL subsystem.

  ``frequency``  Frequency in Hz, must be supported by the pin type
                 (default 0, which is rejected)

The struct dpllsim_pin comment in dpllsim.h was updated to say "The netlink
setters change prio and frequency at runtime under dev->lock", but the rst
still doesn't mention runtime changes or that they carry over to the next
deploy. Prio, added by the earlier "dpll: dpllsim: Add support to get/set
priority on input pins" commit, has the same gap.

Could a sentence be added to dpllsim.rst covering both prio and frequency?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109093523.931201-1-poros%40redhat.com