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

[Ivan Vecera <ivecera@redhat.com>]

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?

Thanks 
Ivan