Thu, May 08, 2025 at 02:21:27PM +0200, [email protected] wrote:
>Add new callback operations for a dpll device:
>- phase_offset_monitor_get(..) - to obtain current state of phase offset
> monitor feature from dpll device,
>- phase_offset_monitor_set(..) - to allow feature configuration.
>
>Obtain the feature state value using the get callback and provide it to
>the user if the device driver implements callbacks.
>
>Execute the set callback upon user requests.
>
>Reviewed-by: Milena Olech <[email protected]>
>Signed-off-by: Arkadiusz Kubalewski <[email protected]>
>---
>v3:
>- remove feature flags and capabilities,
>- add separated (per feature) callback ops,
>- use callback ops to determine feature availability.
>---
> drivers/dpll/dpll_netlink.c | 76 ++++++++++++++++++++++++++++++++++++-
> include/linux/dpll.h | 8 ++++
> 2 files changed, 82 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index c130f87147fa..6d2980455a46 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -126,6 +126,26 @@ dpll_msg_add_mode_supported(struct sk_buff *msg, struct
>dpll_device *dpll,
> return 0;
> }
>
>+static int
>+dpll_msg_add_phase_offset_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->phase_offset_monitor_set && ops->phase_offset_monitor_get) {
>+ ret = ops->phase_offset_monitor_get(dpll, dpll_priv(dpll),
>+ &state, extack);
>+ if (ret)
>+ return -EINVAL;
Why you don't propagate "ret"?
>+ if (nla_put_u32(msg, DPLL_A_PHASE_OFFSET_MONITOR, state))
>+ return -EMSGSIZE;
>+ }
>+
>+ return 0;
>+}
>+
> static int
> dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
> struct netlink_ext_ack *extack)
>@@ -591,6 +611,9 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>sk_buff *msg,
> return ret;
> 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;
>
> return 0;
> }
>@@ -746,6 +769,31 @@ int dpll_pin_change_ntf(struct dpll_pin *pin)
> }
> EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
>
>+static int
>+dpll_phase_offset_monitor_set(struct dpll_device *dpll, struct nlattr *a,
>+ struct netlink_ext_ack *extack)
>+{
>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>+ enum dpll_feature_state state = nla_get_u32(a), old_state;
>+ int ret;
>+
>+ if (!(ops->phase_offset_monitor_set && ops->phase_offset_monitor_get)) {
>+ NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of
>phase offset monitor");
>+ return -EOPNOTSUPP;
>+ }
>+ ret = ops->phase_offset_monitor_get(dpll, dpll_priv(dpll), &old_state,
>+ extack);
>+ if (ret) {
>+ NL_SET_ERR_MSG(extack, "unable to get current state of phase
>offset monitor");
>+ return -EINVAL;
>+ }
>+ if (state == old_state)
>+ return 0;
>+
>+ return ops->phase_offset_monitor_set(dpll, dpll_priv(dpll), state,
>+ extack);
Why you need to do this get/set dance? I mean, just call the driver
set() op and let it do what is needed there, no?
>+}
>+
> static int
> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
> struct netlink_ext_ack *extack)
>@@ -1533,10 +1581,34 @@ int dpll_nl_device_get_doit(struct sk_buff *skb,
>struct genl_info *info)
> return genlmsg_reply(msg, info);
> }
>
>+static int
>+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
>+{
>+ struct nlattr *a;
>+ int rem, ret;
>+
>+ nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>+ genlmsg_len(info->genlhdr), rem) {
Hmm, why you iterate? Why you just don't parse to attr array, as it is
usually done?
>+ switch (nla_type(a)) {
>+ case DPLL_A_PHASE_OFFSET_MONITOR:
>+ ret = dpll_phase_offset_monitor_set(dpll, a,
>+ info->extack);
>+ if (ret)
>+ return ret;
>+ break;
>+ default:
>+ break;
>+ }
>+ }
>+
>+ return 0;
>+}
>+
> int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
>- /* placeholder for set command */
>- return 0;
>+ struct dpll_device *dpll = info->user_ptr[0];
>+
>+ return dpll_set_from_nlattr(dpll, info);
> }
>
> int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback
> *cb)
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index 5e4f9ab1cf75..6ad6c2968a28 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -30,6 +30,14 @@ struct dpll_device_ops {
> void *dpll_priv,
> unsigned long *qls,
> struct netlink_ext_ack *extack);
>+ int (*phase_offset_monitor_set)(const struct dpll_device *dpll,
>+ void *dpll_priv,
>+ enum dpll_feature_state state,
>+ struct netlink_ext_ack *extack);
>+ int (*phase_offset_monitor_get)(const struct dpll_device *dpll,
>+ void *dpll_priv,
>+ enum dpll_feature_state *state,
>+ struct netlink_ext_ack *extack);
> };
>
> struct dpll_pin_ops {
>--
>2.38.1
>
