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

[Jiri Pirko]

Sat, Jun 10, 2023 at 09:25:59AM CEST, jiri@xxxxxxxxxxx wrote:
>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 

..Hit send be mistake.

Please squash this header addition to the patch where you actually
introcude the functions. Since you define a lot of structures here, I
believe that without this patch things are not compilable and breat
dissection. Makes me wonder why you did this split...


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