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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 10/06/2023 08:25, Jiri Pirko wrote:
> Fri, Jun 09, 2023 at 02:18:48PM CEST, arkadiusz.kubalewski@intel.com wrote:
>> From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>>
>> 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@intel.com>
>> Signed-off-by: Milena Olech <milena.olech@intel.com>
>> Co-developed-by: Michal Michalik <michal.michalik@intel.com>
>> Signed-off-by: Michal Michalik <michal.michalik@intel.com>
>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>> Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> ---
>> 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@fb.com>
>> +L:	netdev@vger.kernel.org
>> +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.
> 

Yes, sure, I'll update the list of maintainers to add both yourself and
Arkadiusz to the list, and of course the status to Supported.
> 
> 
>> +F:	drivers/dpll/*
>> +F:	include/net/dpll.h
>> +F:	include/uapi/linux/dpll.h
>> +
>> DRBD DRIVER
>> M:	Philipp Reisner <philipp.reisner@linbit.com>
>> M:	Lars Ellenberg <lars.ellenberg@linbit.com>
>> 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
> 

Sounds reasonable.

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