[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <610b2e82-7b84-4ccd-a6da-1f018c7f6568@linux.dev>
Date: Mon, 12 Jun 2023 16:00:40 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Jiri Pirko <jiri@...nulli.us>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Cc: kuba@...nel.org, vadfed@...a.com, jonathan.lemon@...il.com,
pabeni@...hat.com, corbet@....net, davem@...emloft.net, edumazet@...gle.com,
vadfed@...com, jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
saeedm@...dia.com, leon@...nel.org, richardcochran@...il.com, sj@...nel.org,
javierm@...hat.com, ricardo.canuelo@...labora.com, mst@...hat.com,
tzimmermann@...e.de, michal.michalik@...el.com, gregkh@...uxfoundation.org,
jacek.lawrynowicz@...ux.intel.com, airlied@...hat.com, ogabbay@...nel.org,
arnd@...db.de, nipun.gupta@....com, axboe@...nel.dk, linux@...y.sk,
masahiroy@...nel.org, benjamin.tissoires@...hat.com,
geert+renesas@...der.be, milena.olech@...el.com, kuniyu@...zon.com,
liuhangbin@...il.com, hkallweit1@...il.com, andy.ren@...cruise.com,
razor@...ckwall.org, idosch@...dia.com, lucien.xin@...il.com,
nicolas.dichtel@...nd.com, phil@....cc, claudiajkang@...il.com,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
linux-rdma@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
poros@...hat.com, mschmidt@...hat.com, linux-clk@...r.kernel.org
Subject: Re: [RFC PATCH v8 05/10] dpll: api header: Add DPLL framework base
functions
On 10/06/2023 08:25, Jiri Pirko wrote:
> Fri, Jun 09, 2023 at 02:18:48PM CEST, arkadiusz.kubalewski@...el.com wrote:
>> From: Vadim Fedorenko <vadim.fedorenko@...ux.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@...el.com>
>> Signed-off-by: Milena Olech <milena.olech@...el.com>
>> Co-developed-by: Michal Michalik <michal.michalik@...el.com>
>> Signed-off-by: Michal Michalik <michal.michalik@...el.com>
>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>> Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.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@...com>
>> +L: netdev@...r.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@...bit.com>
>> M: Lars Ellenberg <lars.ellenberg@...bit.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
>>
Powered by blists - more mailing lists