Re: [PATCH net-next v3 2/3] dpll: add frequency monitoring callback ops

[Vadim Fedorenko]

On 01/04/2026 17:29, Ivan Vecera wrote:
> Hi Vadim,
>
> 1. dubna 2026 16:47:21 SELČ, Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx> napsal:
> > On 01/04/2026 10:12, Ivan Vecera wrote:
> > > Add new callback operations for a dpll device:
> > > - freq_monitor_get(..) - to obtain current state of frequency monitor
> > >     feature from dpll device,
> > > - freq_monitor_set(..) - to allow feature configuration.
> > >
> > > Add new callback operation for a dpll pin:
> > > - measured_freq_get(..) - to obtain the measured frequency in mHz.
> > >
> > > Obtain the feature state value using the get callback and provide it to
> > > the user if the device driver implements callbacks. The measured_freq_get
> > > pin callback is only invoked when the frequency monitor is enabled.
> > > The freq_monitor_get device callback is required when measured_freq_get
> > > is provided by the driver.
> > >
> > > Execute the set callback upon user requests.
> > >
> > > Reviewed-by: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
> > > Signed-off-by: Ivan Vecera <ivecera@xxxxxxxxxx>
> > > ---
> > > Changes v2 -> v3:
> > > - Made freq_monitor_get required when measured_freq_get is present (Jakub)
> > >
> > > Changes v1 -> v2:
> > > - Renamed actual-frequency to measured-frequency (Vadim)
> > > ---
> > >    drivers/dpll/dpll_netlink.c | 92 +++++++++++++++++++++++++++++++++++++
> > >    include/linux/dpll.h        | 10 ++++
> > >    2 files changed, 102 insertions(+)
> > >
> > > diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> > > index 83cbd64abf5a4..576d0cd074bd4 100644
> > > --- a/drivers/dpll/dpll_netlink.c
> > > +++ b/drivers/dpll/dpll_netlink.c
> > > @@ -175,6 +175,26 @@ dpll_msg_add_phase_offset_monitor(struct sk_buff *msg, struct dpll_device *dpll,
> > >    	return 0;
> > >    }
> > >    +static int
> > > +dpll_msg_add_freq_monitor(struct sk_buff *msg, struct dpll_device *dpll,
> > > +			  struct netlink_ext_ack *extack)
> > > +{
> > > +	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
> > > +	enum dpll_feature_state state;
> > > +	int ret;
> > > +
> > > +	if (ops->freq_monitor_set && ops->freq_monitor_get) {
> > > +		ret = ops->freq_monitor_get(dpll, dpll_priv(dpll),
> > > +					    &state, extack);
> > > +		if (ret)
> > > +			return ret;
> > > +		if (nla_put_u32(msg, DPLL_A_FREQUENCY_MONITOR, state))
> > > +			return -EMSGSIZE;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >    static int
> > >    dpll_msg_add_phase_offset_avg_factor(struct sk_buff *msg,
> > >    				     struct dpll_device *dpll,
> > > @@ -400,6 +420,40 @@ static int dpll_msg_add_ffo(struct sk_buff *msg, struct dpll_pin *pin,
> > >    			    ffo);
> > >    }
> > >    +static int dpll_msg_add_measured_freq(struct sk_buff *msg, struct dpll_pin *pin,
> > > +				      struct dpll_pin_ref *ref,
> > > +				      struct netlink_ext_ack *extack)
> > > +{
> > > +	const struct dpll_device_ops *dev_ops = dpll_device_ops(ref->dpll);
> > > +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> > > +	struct dpll_device *dpll = ref->dpll;
> > > +	enum dpll_feature_state state;
> > > +	u64 measured_freq;
> > > +	int ret;
> > > +
> > > +	if (!ops->measured_freq_get)
> > > +		return 0;
> > > +	if (WARN_ON(!dev_ops->freq_monitor_get))
> > > +		return -EINVAL;
> >
> > I think pin registration function has to be adjusted to not allow
> > measured_freq_get() callback if device doesn't have freq_monitor_get()
> > callback (or both freq_monitor_{s,g}et). Then this defensive part can
> > be completely removed.
>
> Ok, make sense... Will move such check to pin registration function...
>
> Q: with WARN_ON or without?

Well, we have to provide reason for blocking device registration
somehow, and the only way to this is via "WARN_ON"...

> Thanks
> Ivan