diff mbox series

[RFC,2/7] net: Add helpers for netdev LEDs

Message ID 20240317-v6-8-0-net-next-mv88e6xxx-leds-v4-v1-2-80a4e6c6293e@lunn.ch (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: Add generic support for netdev LEDs | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 954 this patch: 956
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: dsahern@kernel.org
netdev/build_clang fail Errors and warnings before: 957 this patch: 963
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 969 this patch: 971
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Andrew Lunn March 17, 2024, 9:45 p.m. UTC
Add a set of helpers for parsing the standard device tree properties
for LEDs are part of an ethernet device, and registering them with the
LED subsystem. This code can be used by any sort of netdev driver, DSA
switch or pure switchdev switch driver.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/netdev_leds.h |  44 +++++++++++
 net/Kconfig               |  10 +++
 net/core/Makefile         |   1 +
 net/core/netdev-leds.c    | 197 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 252 insertions(+)

Comments

Jakub Kicinski March 21, 2024, 3:01 p.m. UTC | #1
On Sun, 17 Mar 2024 16:45:15 -0500 Andrew Lunn wrote:
> +	struct device *dev = &ndev->dev;
> +	struct netdev_led *netdev_led;
> +	struct led_classdev *cdev;
> +	u32 index;
> +	int err;
> +
> +	netdev_led = devm_kzalloc(dev, sizeof(*netdev_led), GFP_KERNEL);
> +	if (!netdev_led)
> +		return -ENOMEM;

Are we guaranteed to have a real bus device under ndev->dev ?
I'm not aware of any use of devres in netdev core today.
Andrew Lunn March 21, 2024, 3:35 p.m. UTC | #2
On Thu, Mar 21, 2024 at 08:01:55AM -0700, Jakub Kicinski wrote:
> On Sun, 17 Mar 2024 16:45:15 -0500 Andrew Lunn wrote:
> > +	struct device *dev = &ndev->dev;
> > +	struct netdev_led *netdev_led;
> > +	struct led_classdev *cdev;
> > +	u32 index;
> > +	int err;
> > +
> > +	netdev_led = devm_kzalloc(dev, sizeof(*netdev_led), GFP_KERNEL);
> > +	if (!netdev_led)
> > +		return -ENOMEM;
> 
> Are we guaranteed to have a real bus device under ndev->dev ?
> I'm not aware of any use of devres in netdev core today.

devm_ does not require a real bus device. It just needs a struct
device. It does not care if it is physical or virtual. All it needs is
that something destroys the struct device using the usual device model
methods.

The struct device is actually part of struct net_device. It is not a
pointer to a bus device. We have register_netdevice() ->
netdev_register_kobject() -> device_initialize(). So the struct device
in struct net_device to registered to the driver core.

unregister_netdevice_many_notify() -> netdev_unregister_kobject() ->
device_del() -> devres_release_all().

So it also gets deleted from the driver core, at which point the
driver core will release all the resources.

So i don't see a reason why this should not work.

	Andrew
Jakub Kicinski March 21, 2024, 3:43 p.m. UTC | #3
On Thu, 21 Mar 2024 16:35:20 +0100 Andrew Lunn wrote:
> The struct device is actually part of struct net_device. It is not a
> pointer to a bus device. We have register_netdevice() ->
> netdev_register_kobject() -> device_initialize(). So the struct device
> in struct net_device to registered to the driver core.
> 
> unregister_netdevice_many_notify() -> netdev_unregister_kobject() ->
> device_del() -> devres_release_all().

Ah, I thought it's only triggered on bus remove. I guess it must be
triggered on both.
Josua Mayer March 22, 2024, 6:33 p.m. UTC | #4
Am 17.03.24 um 22:45 schrieb Andrew Lunn:
> Add a set of helpers for parsing the standard device tree properties
> for LEDs are part of an ethernet device, and registering them with the
> LED subsystem. This code can be used by any sort of netdev driver, DSA
> switch or pure switchdev switch driver.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> ...
>
> +struct netdev_leds_ops {
> +	int (*brightness_set)(struct net_device *ndev, u8 led,
> +			      enum led_brightness brightness);
> +	int (*blink_set)(struct net_device *ndev, u8 led,
> +			 unsigned long *delay_on,  unsigned long *delay_off);
> +	int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
> +				       unsigned long flags);
> +	int (*hw_control_set)(struct net_device *ndev, u8 led,
> +			      unsigned long flags);
> +	int (*hw_control_get)(struct net_device *ndev, u8 led,
> +			      unsigned long *flags);
> +};
I noticed phy.h calls the "flags" argument "rules" instead,
perhaps that is more suitable.
Andrew Lunn March 22, 2024, 6:56 p.m. UTC | #5
On Fri, Mar 22, 2024 at 06:33:28PM +0000, Josua Mayer wrote:
> Am 17.03.24 um 22:45 schrieb Andrew Lunn:
> > Add a set of helpers for parsing the standard device tree properties
> > for LEDs are part of an ethernet device, and registering them with the
> > LED subsystem. This code can be used by any sort of netdev driver, DSA
> > switch or pure switchdev switch driver.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > ...
> >
> > +struct netdev_leds_ops {
> > +	int (*brightness_set)(struct net_device *ndev, u8 led,
> > +			      enum led_brightness brightness);
> > +	int (*blink_set)(struct net_device *ndev, u8 led,
> > +			 unsigned long *delay_on,  unsigned long *delay_off);
> > +	int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
> > +				       unsigned long flags);
> > +	int (*hw_control_set)(struct net_device *ndev, u8 led,
> > +			      unsigned long flags);
> > +	int (*hw_control_get)(struct net_device *ndev, u8 led,
> > +			      unsigned long *flags);
> > +};
> I noticed phy.h calls the "flags" argument "rules" instead,
> perhaps that is more suitable.

The naming is a bit inconsistent. include/linux/leds.h uses:

        /*
         * Check if the LED driver supports the requested mode provided by the
         * defined supported trigger to setup the LED to hw control mode.
         *
         * Return 0 on success. Return -EOPNOTSUPP when the passed flags are not
         * supported and software fallback needs to be used.
         * Return a negative error number on any other case  for check fail due
         * to various reason like device not ready or timeouts.
         */
        int                     (*hw_control_is_supported)(struct led_classdev *led_cdev,
                                                           unsigned long flags);
        /*
         * Activate hardware control, LED driver will use the provided flags
         * from the supported trigger and setup the LED to be driven by hardware
         * following the requested mode from the trigger flags.
         * Deactivate hardware blink control by setting brightness to LED_OFF via
         * the brightness_set() callback.
         *
         * Return 0 on success, a negative error number on flags apply fail.
         */
        int                     (*hw_control_set)(struct led_classdev *led_cdev,
                                                  unsigned long flags);
        /*
         * Get from the LED driver the current mode that the LED is set in hw
         * control mode and put them in flags.
         * Trigger can use this to get the initial state of a LED already set in
         * hardware blink control.
         *
         * Return 0 on success, a negative error number on failing parsing the
         * initial mode. Error from this function is NOT FATAL as the device
         * may be in a not supported initial state by the attached LED trigger.
         */
        int                     (*hw_control_get)(struct led_classdev *led_cdev,
                                                  unsigned long *flags);

So if anything, phy.h should really change to use flags.

   Andrew
diff mbox series

Patch

diff --git a/include/net/netdev_leds.h b/include/net/netdev_leds.h
new file mode 100644
index 000000000000..647ff1666467
--- /dev/null
+++ b/include/net/netdev_leds.h
@@ -0,0 +1,44 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Helpers used for creating and managing LEDs on a netdev MAC
+ * driver.
+ */
+
+#ifndef _NET_NETDEV_LEDS_H
+#define _NET_NETDEV_LEDS_H
+
+struct netdev_leds_ops {
+	int (*brightness_set)(struct net_device *ndev, u8 led,
+			      enum led_brightness brightness);
+	int (*blink_set)(struct net_device *ndev, u8 led,
+			 unsigned long *delay_on,  unsigned long *delay_off);
+	int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
+				       unsigned long flags);
+	int (*hw_control_set)(struct net_device *ndev, u8 led,
+			      unsigned long flags);
+	int (*hw_control_get)(struct net_device *ndev, u8 led,
+			      unsigned long *flags);
+};
+
+#ifdef CONFIG_NETDEV_LEDS
+int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
+		      struct list_head *list, struct netdev_leds_ops *ops);
+
+void netdev_leds_teardown(struct list_head *list, struct net_device *ndev);
+
+#else
+static inline int netdev_leds_setup(struct net_device *ndev,
+				    struct device_node *np,
+				    struct list_head *list,
+				    struct netdev_leds_ops *ops)
+{
+	return 0;
+}
+
+static inline void netdev_leds_teardown(struct list_head *list,
+					struct net_device *ndev)
+{
+}
+#endif /* CONFIG_NETDEV_LEDS */
+
+#endif /* _NET_PORT_LEDS_H */
diff --git a/net/Kconfig b/net/Kconfig
index 3e57ccf0da27..fa12aaea3ed6 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -516,4 +516,14 @@  config NET_TEST
 
 	  If unsure, say N.
 
+config NETDEV_LEDS
+	bool "NETDEV helper code for MAC LEDs"
+	depends on LEDS_CLASS
+	select LEDS_TRIGGER_NETDEV
+	help
+	  NICs and Switches often contain LED controllers. When the LEDs
+	  are part of the MAC, the MAC driver, aka netdev driver, should
+	  make the LEDs available. NETDEV_LEDS offers a small library
+	  of code to help MAC drivers do this.
+
 endif   # if NET
diff --git a/net/core/Makefile b/net/core/Makefile
index 6e6548011fae..9d887af68837 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -42,3 +42,4 @@  obj-$(CONFIG_BPF_SYSCALL) += sock_map.o
 obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
 obj-$(CONFIG_OF)	+= of_net.o
 obj-$(CONFIG_NET_TEST) += gso_test.o
+obj-$(CONFIG_NETDEV_LEDS) += netdev-leds.o
diff --git a/net/core/netdev-leds.c b/net/core/netdev-leds.c
new file mode 100644
index 000000000000..cc87ab77be39
--- /dev/null
+++ b/net/core/netdev-leds.c
@@ -0,0 +1,197 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/leds.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <net/netdev_leds.h>
+
+struct netdev_led {
+	struct list_head led_list;
+	struct led_classdev led_cdev;
+	struct netdev_leds_ops *ops;
+	struct net_device *ndev;
+	u8 index;
+};
+
+#define to_netdev_led(d) container_of(d, struct netdev_led, led_cdev)
+
+static int netdev_brightness_set(struct led_classdev *led_cdev,
+				 enum led_brightness value)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->brightness_set(netdev_led->ndev,
+					       netdev_led->index,
+					       value);
+}
+
+static int netdev_blink_set(struct led_classdev *led_cdev,
+			    unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->blink_set(netdev_led->ndev,
+					  netdev_led->index,
+					  delay_on, delay_off);
+}
+
+static __maybe_unused int
+netdev_hw_control_is_supported(struct led_classdev *led_cdev,
+			       unsigned long flags)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->hw_control_is_supported(netdev_led->ndev,
+							netdev_led->index,
+							flags);
+}
+
+static __maybe_unused int netdev_hw_control_set(struct led_classdev *led_cdev,
+						unsigned long flags)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->hw_control_set(netdev_led->ndev,
+					       netdev_led->index,
+					       flags);
+}
+
+static __maybe_unused int netdev_hw_control_get(struct led_classdev *led_cdev,
+						unsigned long *flags)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->hw_control_get(netdev_led->ndev,
+					       netdev_led->index,
+					       flags);
+}
+
+static struct device *
+netdev_hw_control_get_device(struct led_classdev *led_cdev)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return &netdev_led->ndev->dev;
+}
+
+static int netdev_led_setup(struct net_device *ndev, struct device_node *led,
+			    struct list_head *list, struct netdev_leds_ops *ops)
+{
+	struct led_init_data init_data = {};
+	struct device *dev = &ndev->dev;
+	struct netdev_led *netdev_led;
+	struct led_classdev *cdev;
+	u32 index;
+	int err;
+
+	netdev_led = devm_kzalloc(dev, sizeof(*netdev_led), GFP_KERNEL);
+	if (!netdev_led)
+		return -ENOMEM;
+
+	netdev_led->ndev = ndev;
+	netdev_led->ops = ops;
+	cdev = &netdev_led->led_cdev;
+
+	err = of_property_read_u32(led, "reg", &index);
+	if (err)
+		return err;
+
+	if (index > 255)
+		return -EINVAL;
+
+	netdev_led->index = index;
+
+	if (ops->brightness_set)
+		cdev->brightness_set_blocking = netdev_brightness_set;
+	if (ops->blink_set)
+		cdev->blink_set = netdev_blink_set;
+#ifdef CONFIG_LEDS_TRIGGERS
+	if (ops->hw_control_is_supported)
+		cdev->hw_control_is_supported = netdev_hw_control_is_supported;
+	if (ops->hw_control_set)
+		cdev->hw_control_set = netdev_hw_control_set;
+	if (ops->hw_control_get)
+		cdev->hw_control_get = netdev_hw_control_get;
+	cdev->hw_control_trigger = "netdev";
+#endif
+	cdev->hw_control_get_device = netdev_hw_control_get_device;
+	cdev->max_brightness = 1;
+	init_data.fwnode = of_fwnode_handle(led);
+	init_data.devname_mandatory = true;
+
+	init_data.devicename = dev_name(dev);
+	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
+	if (err)
+		return err;
+
+	INIT_LIST_HEAD(&netdev_led->led_list);
+	list_add(&netdev_led->led_list, list);
+
+	return 0;
+}
+
+/**
+ * netdev_leds_setup - Parse DT node and create LEDs for netdev
+ *
+ * @ndev: struct netdev for the MAC
+ * @np: ethernet-node in device tree
+ * @list: list to add LEDs to
+ * @ops: structure of ops to manipulate the LED.
+ *
+ * Parse the device tree node, as described in ethernet-controller.yaml,
+ * and find any LEDs. For each LED found, create an LED and register
+ * it with the LED subsystem. The LED will be added to the list, which can
+ * be shared by all netdevs of the device. The ops structure contains the
+ * callbacks needed to control the LEDs.
+ *
+ * Return 0 in success, otherwise an negative error code.
+ */
+int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
+		      struct list_head *list, struct netdev_leds_ops *ops)
+{
+	struct device_node *leds, *led;
+	int err;
+
+	leds = of_get_child_by_name(np, "leds");
+	if (!leds)
+		return 0;
+
+	for_each_available_child_of_node(leds, led) {
+		err = netdev_led_setup(ndev, led, list, ops);
+		if (err) {
+			of_node_put(led);
+			return err;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(netdev_leds_setup);
+
+/**
+ * netdev_leds_teardown - Remove LEDs for a netdev
+ *
+ * @list: list to add LEDs to teardown
+ * @ndev: The netdev for which LEDs should be removed
+ *
+ * Unregister all LEDs for a given netdev, freeing up any allocated
+ * memory.
+ */
+void netdev_leds_teardown(struct list_head *list, struct net_device *ndev)
+{
+	struct netdev_led *netdev_led;
+	struct led_classdev *cdev;
+	struct device *dev;
+
+	list_for_each_entry(netdev_led, list, led_list) {
+		if (netdev_led->ndev != ndev)
+			continue;
+		dev = &netdev_led->ndev->dev;
+		cdev = &netdev_led->led_cdev;
+		devm_led_classdev_unregister(dev, cdev);
+	}
+}
+EXPORT_SYMBOL_GPL(netdev_leds_teardown);