Re: [RFC PATCH v8 05/10] dpll: api header: Add DPLL framework base functions

[Jiri Pirko]

Fri, Jun 09, 2023 at 02:18:48PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote:
>From: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
>
>DPLL framework is used to represent and configure DPLL devices
>in systems. Each device that has DPLL and can configure sources
>and outputs can use this framework. Netlink interface is used to
>provide configuration data and to receive notification messages
>about changes in the configuration or status of DPLL device.
>Inputs and outputs of the DPLL device are represented as special
>objects which could be dynamically added to and removed from DPLL
>device.
>
>Add kernel api header, make dpll subsystem available to device drivers.
>
>Add/update makefiles/Kconfig to allow compilation of dpll subsystem.
>
>Co-developed-by: Milena Olech <milena.olech@xxxxxxxxx>
>Signed-off-by: Milena Olech <milena.olech@xxxxxxxxx>
>Co-developed-by: Michal Michalik <michal.michalik@xxxxxxxxx>
>Signed-off-by: Michal Michalik <michal.michalik@xxxxxxxxx>
>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
>Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>---
> MAINTAINERS           |   8 +++
> drivers/Kconfig       |   2 +
> drivers/Makefile      |   1 +
> drivers/dpll/Kconfig  |   7 ++
> drivers/dpll/Makefile |   9 +++
> include/linux/dpll.h  | 144 ++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 171 insertions(+)
> create mode 100644 drivers/dpll/Kconfig
> create mode 100644 drivers/dpll/Makefile
> create mode 100644 include/linux/dpll.h
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 288d9a5edb9d..0e69429ecc55 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -6306,6 +6306,14 @@ 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
>+M:	Vadim Fedorenko <vadfed@xxxxxx>
>+L:	netdev@xxxxxxxxxxxxxxx
>+S:	Maintained

I think status should be rather "Supported":
"Supported:   Someone is actually paid to look after this."

Also, I think that it would be good to have Arkadiusz Kubalewski
listed here, as he is the one that knows the subsystem by heart.

Also, if you don't mind, I would be happy as a co-maintainer of the
subsystem to be listed here, as I helped to shape the code and
interfaces and I also know it pretty good.



>+F:	drivers/dpll/*
>+F:	include/net/dpll.h
>+F:	include/uapi/linux/dpll.h
>+
> DRBD DRIVER
> M:	Philipp Reisner <philipp.reisner@xxxxxxxxxx>
> M:	Lars Ellenberg <lars.ellenberg@xxxxxxxxxx>
>diff --git a/drivers/Kconfig b/drivers/Kconfig
>index 514ae6b24cb2..ce5f63918eba 100644
>--- a/drivers/Kconfig
>+++ b/drivers/Kconfig
>@@ -243,4 +243,6 @@ source "drivers/hte/Kconfig"
> 
> source "drivers/cdx/Kconfig"
> 
>+source "drivers/dpll/Kconfig"
>+
> endmenu
>diff --git a/drivers/Makefile b/drivers/Makefile
>index 7241d80a7b29..6fea42a6dd05 100644
>--- a/drivers/Makefile
>+++ b/drivers/Makefile
>@@ -195,3 +195,4 @@ obj-$(CONFIG_PECI)		+= peci/
> obj-$(CONFIG_HTE)		+= hte/
> obj-$(CONFIG_DRM_ACCEL)		+= accel/
> obj-$(CONFIG_CDX_BUS)		+= cdx/
>+obj-$(CONFIG_DPLL)		+= dpll/
>diff --git a/drivers/dpll/Kconfig b/drivers/dpll/Kconfig
>new file mode 100644
>index 000000000000..a4cae73f20d3
>--- /dev/null
>+++ b/drivers/dpll/Kconfig
>@@ -0,0 +1,7 @@
>+# SPDX-License-Identifier: GPL-2.0-only
>+#
>+# Generic DPLL drivers configuration
>+#
>+
>+config DPLL
>+  bool
>diff --git a/drivers/dpll/Makefile b/drivers/dpll/Makefile
>new file mode 100644
>index 000000000000..2e5b27850110
>--- /dev/null
>+++ b/drivers/dpll/Makefile
>@@ -0,0 +1,9 @@
>+# SPDX-License-Identifier: GPL-2.0
>+#
>+# Makefile for DPLL drivers.
>+#
>+
>+obj-$(CONFIG_DPLL)      += dpll.o
>+dpll-y                  += dpll_core.o
>+dpll-y                  += dpll_netlink.o
>+dpll-y                  += dpll_nl.o
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>new file mode 100644
>index 000000000000..a18bcaa13553
>--- /dev/null
>+++ b/include/linux/dpll.h
>@@ -0,0 +1,144 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/*
>+ *  Copyright (c) 2023 Meta Platforms, Inc. and affiliates
>+ *  Copyright (c) 2023 Intel and affiliates
>+ */
>+
>+#ifndef __DPLL_H__
>+#define __DPLL_H__
>+
>+#include <uapi/linux/dpll.h>
>+#include <linux/device.h>
>+#include <linux/netlink.h>
>+
>+struct dpll_device;
>+struct dpll_pin;
>+
>+struct dpll_device_ops {
>+	int (*mode_get)(const struct dpll_device *dpll, void *dpll_priv,
>+			enum dpll_mode *mode, struct netlink_ext_ack *extack);
>+	int (*mode_set)(const struct dpll_device *dpll, void *dpll_priv,
>+			const enum dpll_mode mode,
>+			struct netlink_ext_ack *extack);
>+	bool (*mode_supported)(const struct dpll_device *dpll, void *dpll_priv,
>+			       const enum dpll_mode mode,
>+			       struct netlink_ext_ack *extack);
>+	int (*source_pin_idx_get)(const struct dpll_device *dpll,
>+				  void *dpll_priv,
>+				  u32 *pin_idx,
>+				  struct netlink_ext_ack *extack);
>+	int (*lock_status_get)(const struct dpll_device *dpll, void *dpll_priv,
>+			       enum dpll_lock_status *status,
>+			       struct netlink_ext_ack *extack);
>+	int (*temp_get)(const struct dpll_device *dpll, void *dpll_priv,
>+			s32 *temp, struct netlink_ext_ack *extack);
>+};
>+
>+struct dpll_pin_ops {
>+	int (*frequency_set)(const struct dpll_pin *pin, void *pin_priv,
>+			     const struct dpll_device *dpll, void *dpll_priv,
>+			     const u64 frequency,
>+			     struct netlink_ext_ack *extack);
>+	int (*frequency_get)(const struct dpll_pin *pin, void *pin_priv,
>+			     const struct dpll_device *dpll, void *dpll_priv,
>+			     u64 *frequency, struct netlink_ext_ack *extack);
>+	int (*direction_set)(const struct dpll_pin *pin, void *pin_priv,
>+			     const struct dpll_device *dpll, void *dpll_priv,
>+			     const enum dpll_pin_direction direction,
>+			     struct netlink_ext_ack *extack);
>+	int (*direction_get)(const struct dpll_pin *pin, void *pin_priv,
>+			     const struct dpll_device *dpll, void *dpll_priv,
>+			     enum dpll_pin_direction *direction,
>+			     struct netlink_ext_ack *extack);
>+	int (*state_on_pin_get)(const struct dpll_pin *pin, void *pin_priv,
>+				const struct dpll_pin *parent_pin,
>+				void *parent_pin_priv,
>+				enum dpll_pin_state *state,
>+				struct netlink_ext_ack *extack);
>+	int (*state_on_dpll_get)(const struct dpll_pin *pin, void *pin_priv,
>+				 const struct dpll_device *dpll,
>+				 void *dpll_priv, enum dpll_pin_state *state,
>+				 struct netlink_ext_ack *extack);
>+	int (*state_on_pin_set)(const struct dpll_pin *pin, void *pin_priv,
>+				const struct dpll_pin *parent_pin,
>+				void *parent_pin_priv,
>+				const enum dpll_pin_state state,
>+				struct netlink_ext_ack *extack);
>+	int (*state_on_dpll_set)(const struct dpll_pin *pin, void *pin_priv,
>+				 const struct dpll_device *dpll,
>+				 void *dpll_priv,
>+				 const enum dpll_pin_state state,
>+				 struct netlink_ext_ack *extack);
>+	int (*prio_get)(const struct dpll_pin *pin,  void *pin_priv,
>+			const struct dpll_device *dpll,  void *dpll_priv,
>+			u32 *prio, struct netlink_ext_ack *extack);
>+	int (*prio_set)(const struct dpll_pin *pin, void *pin_priv,
>+			const struct dpll_device *dpll, void *dpll_priv,
>+			const u32 prio, struct netlink_ext_ack *extack);
>+};
>+
>+struct dpll_pin_frequency {
>+	u64 min;
>+	u64 max;
>+};
>+
>+#define DPLL_PIN_FREQUENCY_RANGE(_min, _max)	\
>+	{					\
>+		.min = _min,			\
>+		.max = _max,			\
>+	}
>+
>+#define DPLL_PIN_FREQUENCY(_val) DPLL_PIN_FREQUENCY_RANGE(_val, _val)
>+#define DPLL_PIN_FREQUENCY_1PPS \
>+	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_1_HZ)
>+#define DPLL_PIN_FREQUENCY_10MHZ \
>+	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_10_MHZ)
>+#define DPLL_PIN_FREQUENCY_IRIG_B \
>+	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_10_KHZ)
>+#define DPLL_PIN_FREQUENCY_DCF77 \
>+	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ)
>+
>+struct dpll_pin_properties {
>+	const char *board_label;
>+	const char *panel_label;
>+	const char *package_label;
>+	enum dpll_pin_type type;
>+	unsigned long capabilities;
>+	u32 freq_supported_num;
>+	struct dpll_pin_frequency *freq_supported;
>+};
>+
>+struct dpll_device
>+*dpll_device_get(u64 clock_id, u32 dev_driver_id, struct module *module);
>+
>+void dpll_device_put(struct dpll_device *dpll);
>+
>+int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>+			 const struct dpll_device_ops *ops, void *priv);
>+
>+void dpll_device_unregister(struct dpll_device *dpll,
>+			    const struct dpll_device_ops *ops, void *priv);
>+
>+struct dpll_pin
>+*dpll_pin_get(u64 clock_id, u32 dev_driver_id, struct module *module,
>+	      const struct dpll_pin_properties *prop);
>+
>+int dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
>+		      const struct dpll_pin_ops *ops, void *priv);
>+
>+void dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin,
>+			 const struct dpll_pin_ops *ops, void *priv);
>+
>+void dpll_pin_put(struct dpll_pin *pin);
>+
>+int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
>+			     const struct dpll_pin_ops *ops, void *priv);
>+
>+void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin *pin,
>+				const struct dpll_pin_ops *ops, void *priv);
>+
>+int dpll_device_change_ntf(struct dpll_device *dpll);
>+
>+int dpll_pin_change_ntf(struct dpll_pin *pin);

Why exactly did you split this into a separate patch? To me, it does not
make any sense. Please squash this header addition to the 


>+
>+#endif
>-- 
>2.37.3
>