Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions

[Jiri Pirko <jiri@resnulli.us>]

Sun, Mar 12, 2023 at 03:28:03AM CET, vadfed@meta.com wrote:

[...]


>diff --git a/MAINTAINERS b/MAINTAINERS
>index edd3d562beee..0222b19af545 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -6289,6 +6289,15 @@ F:	Documentation/networking/device_drivers/ethernet/freescale/dpaa2/switch-drive
> F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-switch*
> F:	drivers/net/ethernet/freescale/dpaa2/dpsw*
> 
>+DPLL CLOCK SUBSYSTEM

Why "clock"? You don't mention "clock" anywhere else.

[...]


>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>new file mode 100644
>index 000000000000..3fc151e16751
>--- /dev/null
>+++ b/drivers/dpll/dpll_core.c
>@@ -0,0 +1,835 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ *  dpll_core.c - Generic DPLL Management class support.

Why "class" ?

[...]


>+static int
>+dpll_msg_add_pin_freq(struct sk_buff *msg, const struct dpll_pin *pin,
>+		      struct netlink_ext_ack *extack, bool dump_any_freq)
>+{
>+	enum dpll_pin_freq_supp fs;
>+	struct dpll_pin_ref *ref;
>+	unsigned long i;
>+	u32 freq;
>+
>+	xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) {
>+		if (ref && ref->ops && ref->dpll)
>+			break;
>+	}
>+	if (!ref || !ref->ops || !ref->dpll)
>+		return -ENODEV;
>+	if (!ref->ops->frequency_get)
>+		return -EOPNOTSUPP;
>+	if (ref->ops->frequency_get(pin, ref->dpll, &freq, extack))
>+		return -EFAULT;
>+	if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY, freq))
>+		return -EMSGSIZE;
>+	if (!dump_any_freq)
>+		return 0;
>+	for (fs = DPLL_PIN_FREQ_SUPP_UNSPEC + 1;
>+	     fs <= DPLL_PIN_FREQ_SUPP_MAX; fs++) {
>+		if (test_bit(fs, &pin->prop.freq_supported)) {
>+			if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED,
>+			    dpll_pin_freq_value[fs]))

This is odd. As I suggested in the yaml patch, better to treat all
supported frequencies the same, no matter if it is range or not. The you
don't need this weird bitfield.

You can have a macro to help driver to assemble array of supported
frequencies and ranges.


>+				return -EMSGSIZE;
>+		}
>+	}
>+	if (pin->prop.any_freq_min && pin->prop.any_freq_max) {
>+		if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MIN,
>+				pin->prop.any_freq_min))
>+			return -EMSGSIZE;
>+		if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MAX,
>+				pin->prop.any_freq_max))
>+			return -EMSGSIZE;
>+	}
>+
>+	return 0;
>+}
>+

[...]


>+static int
>+dpll_cmd_pin_on_dpll_get(struct sk_buff *msg, struct dpll_pin *pin,
>+			 struct dpll_device *dpll,
>+			 struct netlink_ext_ack *extack)
>+{
>+	struct dpll_pin_ref *ref;
>+	int ret;
>+
>+	if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id))
>+		return -EMSGSIZE;
>+	if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description))
>+		return -EMSGSIZE;
>+	if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type))
>+		return -EMSGSIZE;
>+	if (nla_put_u32(msg, DPLL_A_PIN_DPLL_CAPS, pin->prop.capabilities))
>+		return -EMSGSIZE;
>+	ret = dpll_msg_add_pin_direction(msg, pin, extack);
>+	if (ret)
>+		return ret;
>+	ret = dpll_msg_add_pin_freq(msg, pin, extack, true);
>+	if (ret && ret != -EOPNOTSUPP)
>+		return ret;
>+	ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>+	if (!ref)

How exactly this can happen? Looks to me like only in case of a bug.
WARN_ON() perhaps (put directly into dpll_xa_ref_dpll_find()?


>+		return -EFAULT;
>+	ret = dpll_msg_add_pin_prio(msg, pin, ref, extack);
>+	if (ret && ret != -EOPNOTSUPP)
>+		return ret;
>+	ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, extack);
>+	if (ret && ret != -EOPNOTSUPP)
>+		return ret;
>+	ret = dpll_msg_add_pin_parents(msg, pin, extack);
>+	if (ret)
>+		return ret;
>+	if (pin->rclk_dev_name)

Use && and single if


>+		if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE,
>+				   pin->rclk_dev_name))
>+			return -EMSGSIZE;
>+
>+	return 0;
>+}
>+

[...]


>+static int
>+dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>+		  struct netlink_ext_ack *extack)
>+{
>+	u32 freq = nla_get_u32(a);
>+	struct dpll_pin_ref *ref;
>+	unsigned long i;
>+	int ret;
>+
>+	if (!dpll_pin_is_freq_supported(pin, freq))
>+		return -EINVAL;
>+
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		ret = ref->ops->frequency_set(pin, ref->dpll, freq, extack);
>+		if (ret)
>+			return -EFAULT;

return what the op returns: ret


>+		dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_FREQUENCY);
>+	}
>+
>+	return 0;
>+}
>+

[...]


>+static int
>+dpll_pin_direction_set(struct dpll_pin *pin, struct nlattr *a,
>+		       struct netlink_ext_ack *extack)
>+{
>+	enum dpll_pin_direction direction = nla_get_u8(a);
>+	struct dpll_pin_ref *ref;
>+	unsigned long i;
>+
>+	if (!(DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE & pin->prop.capabilities))
>+		return -EOPNOTSUPP;
>+
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		if (ref->ops->direction_set(pin, ref->dpll, direction, extack))

ret = ..
if (ret)
	return ret;

Please use this pattern in other ops call code as well.


>+			return -EFAULT;
>+		dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_DIRECTION);
>+	}
>+
>+	return 0;

[...]