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

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri at resnulli.us>
>Sent: Tuesday, March 14, 2023 4:45 PM
>
>[...]
>
>
>>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/swit
>ch-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.
>

I understand suggestion on yaml, but here I am confused.
How do they relate to the supported frequency passed between driver and dpll
subsystem?
This bitfield is not visible to the userspace, and sure probably adding macro
can be useful.

>
>>+				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()?

Yes, makes sense.

>
>
>>+		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
>

Make sense to me.

>
>>+		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

Why would we return here a driver return code, userspace can have this info
from extack. IMHO return values of dpll subsystem shall be not dependent on
what is returned from the driver.

>
>
>>+		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.
>

This is the same as above (return code by driver) explanation.

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

Thanks,
Arkadiusz