Re: [PATCH net-next 2/3] dpll: add phase_offset_avg_factor_get/set callback ops
From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Date: 2025-09-26 15:26:12
Also in:
linux-doc, lkml
On 26/09/2025 15:21, Ivan Vecera wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+ 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); +} +
[...]