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

Vadim Fedorenko
2026-04-01T16:37:43Z

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
>