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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 01/04/2026 17:29, Ivan Vecera wrote:
> Hi Vadim,
> 
> 1. dubna 2026 16:47:21 SELČ, Vadim Fedorenko <vadim.fedorenko@linux.dev> 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@linux.dev>
>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>>> ---
>>> 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
>