Re: [PATCH net-next 2/3] dpll: add phase_offset_avg_factor_get/set callback ops

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

On 26/09/2025 15:21, Ivan Vecera wrote:
> Add new callback operations for a dpll device:
> - phase_offset_avg_factor_get(...) - to obtain current phase offset
>    averaging factor from dpll device,
> - phase_offset_avg_factor_set(...) - to set phase offset averaging factor
> 
> Obtain the factor value using the get callback and provide it to the user
> if the device driver implements callbacks. Execute the set callback upon
> user requests.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/dpll_netlink.c | 76 +++++++++++++++++++++++++++++++++----
>   include/linux/dpll.h        |  6 +++
>   2 files changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> index 0a852011653c4..55b3ffe08024b 100644
> --- a/drivers/dpll/dpll_netlink.c
> +++ b/drivers/dpll/dpll_netlink.c
> @@ -164,6 +164,28 @@ dpll_msg_add_phase_offset_monitor(struct sk_buff *msg, struct dpll_device *dpll,
>   	return 0;
>   }
>   
> +static int
> +dpll_msg_add_phase_offset_avg_factor(struct sk_buff *msg,
> +				     struct dpll_device *dpll,
> +				     struct netlink_ext_ack *extack)
> +{
> +	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
> +	u32 factor;
> +	int ret;
> +
> +	if (ops->phase_offset_avg_factor_set &&
> +	    ops->phase_offset_avg_factor_get) {
> +		ret = ops->phase_offset_avg_factor_get(dpll, dpll_priv(dpll),
> +						       &factor, extack);

Well, correct me if I'm wrong, but the device can have offset average
factor as a constant, and it can technically report it. I would make
set/get callback optional and don't require both of them to be
implemented.


> +		if (ret)
> +			return ret;
> +		if (nla_put_u32(msg, DPLL_A_PHASE_OFFSET_AVG_FACTOR, factor))
> +			return -EMSGSIZE;
> +	}
> +
> +	return 0;
> +}
> +
>   static int
>   dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
>   			 struct netlink_ext_ack *extack)
> @@ -675,6 +697,9 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
>   	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>   		return -EMSGSIZE;
>   	ret = dpll_msg_add_phase_offset_monitor(msg, dpll, extack);
> +	if (ret)
> +		return ret;
> +	ret = dpll_msg_add_phase_offset_avg_factor(msg, dpll, extack);
>   	if (ret)
>   		return ret;
>   
> @@ -839,6 +864,32 @@ dpll_phase_offset_monitor_set(struct dpll_device *dpll, struct nlattr *a,
>   					     extack);
>   }
>   
> +static int
> +dpll_phase_offset_avg_factor_set(struct dpll_device *dpll, struct nlattr *a,
> +				 struct netlink_ext_ack *extack)
> +{
> +	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
> +	u32 factor = nla_get_u32(a), old_factor;
> +	int ret;
> +
> +	if (!(ops->phase_offset_avg_factor_set &&
> +	      ops->phase_offset_avg_factor_get)) {
> +		NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of phase offset averaging");
> +		return -EOPNOTSUPP;
> +	}
> +	ret = ops->phase_offset_avg_factor_get(dpll, dpll_priv(dpll),
> +					       &old_factor, extack);
> +	if (ret) {
> +		NL_SET_ERR_MSG(extack, "unable to get current phase offset averaging factor");
> +		return ret;
> +	}
> +	if (factor == old_factor)
> +		return 0;

I don't think the core should do any checks here. If the user for some
reason wants to re-install the same value - give them a chance. Some
drivers may implement check logic if it's relevant to the hardware, but
not in general.

> +
> +	return ops->phase_offset_avg_factor_set(dpll, dpll_priv(dpll), factor,
> +						extack);
> +}
> +

[...]