[PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[Kurt Kanzenbach]

Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs
from user space using the netdev trigger. The LEDs are named as
igc-<bus><device>-<led> to be easily identified.

Offloading activity and link speed is supported. Tested on Intel i225.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/Kconfig        |   8 +
 drivers/net/ethernet/intel/igc/Makefile   |   1 +
 drivers/net/ethernet/intel/igc/igc.h      |   5 +
 drivers/net/ethernet/intel/igc/igc_leds.c | 241 ++++++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_main.c |   6 +
 drivers/net/ethernet/intel/igc/igc_regs.h |   1 +
 6 files changed, 262 insertions(+)
 create mode 100644 drivers/net/ethernet/intel/igc/igc_leds.c

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index d55638ad8704..767358b60507 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -368,6 +368,14 @@ config IGC
 
 	  To compile this driver as a module, choose M here. The module
 	  will be called igc.
+
+config IGC_LEDS
+	def_bool LEDS_TRIGGER_NETDEV
+	depends on IGC && LEDS_CLASS
+	help
+	  Optional support for controlling the NIC LED's with the netdev
+	  LED trigger.
+
 config IDPF
 	tristate "Intel(R) Infrastructure Data Path Function Support"
 	depends on PCI_MSI
diff --git a/drivers/net/ethernet/intel/igc/Makefile b/drivers/net/ethernet/intel/igc/Makefile
index 95d1e8c490a4..ebffd3054285 100644
--- a/drivers/net/ethernet/intel/igc/Makefile
+++ b/drivers/net/ethernet/intel/igc/Makefile
@@ -6,6 +6,7 @@
 #
 
 obj-$(CONFIG_IGC) += igc.o
+igc-$(CONFIG_IGC_LEDS) += igc_leds.o
 
 igc-objs := igc_main.o igc_mac.o igc_i225.o igc_base.o igc_nvm.o igc_phy.o \
 igc_diag.o igc_ethtool.o igc_ptp.o igc_dump.o igc_tsn.o igc_xdp.o
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 45430e246e9c..914d5189b001 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -295,6 +295,9 @@ struct igc_adapter {
 		struct timespec64 start;
 		struct timespec64 period;
 	} perout[IGC_N_PEROUT];
+
+	/* LEDs */
+	struct mutex led_mutex;
 };
 
 void igc_up(struct igc_adapter *adapter);
@@ -720,6 +723,8 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter);
 void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts);
 void igc_ptp_tx_tstamp_event(struct igc_adapter *adapter);
 
+int igc_led_setup(struct igc_adapter *adapter);
+
 #define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring))
 
 #define IGC_TXD_DCMD	(IGC_ADVTXD_DCMD_EOP | IGC_ADVTXD_DCMD_RS)
diff --git a/drivers/net/ethernet/intel/igc/igc_leds.c b/drivers/net/ethernet/intel/igc/igc_leds.c
new file mode 100644
index 000000000000..7ca02b2903eb
--- /dev/null
+++ b/drivers/net/ethernet/intel/igc/igc_leds.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2024 Linutronix GmbH */
+
+#include <linux/bits.h>
+#include <linux/leds.h>
+#include <linux/netdevice.h>
+#include <linux/pm_runtime.h>
+#include <uapi/linux/uleds.h>
+
+#include "igc.h"
+
+#define IGC_NUM_LEDS			3
+
+#define IGC_LEDCTL_LED0_MODE_SHIFT	0
+#define IGC_LEDCTL_LED0_MODE_MASK	GENMASK(3, 0)
+#define IGC_LEDCTL_LED0_BLINK		BIT(7)
+#define IGC_LEDCTL_LED1_MODE_SHIFT	8
+#define IGC_LEDCTL_LED1_MODE_MASK	GENMASK(11, 8)
+#define IGC_LEDCTL_LED1_BLINK		BIT(15)
+#define IGC_LEDCTL_LED2_MODE_SHIFT	16
+#define IGC_LEDCTL_LED2_MODE_MASK	GENMASK(19, 16)
+#define IGC_LEDCTL_LED2_BLINK		BIT(23)
+
+#define IGC_LEDCTL_MODE_LINK_10		0x05
+#define IGC_LEDCTL_MODE_LINK_100	0x06
+#define IGC_LEDCTL_MODE_LINK_1000	0x07
+#define IGC_LEDCTL_MODE_LINK_2500	0x08
+#define IGC_LEDCTL_MODE_ACTIVITY	0x0b
+
+#define IGC_SUPPORTED_MODES						 \
+	(BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK_1000) | \
+	 BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK_10) |	 \
+	 BIT(TRIGGER_NETDEV_RX) | BIT(TRIGGER_NETDEV_TX))
+
+struct igc_led_classdev {
+	struct net_device *netdev;
+	struct led_classdev led;
+	int index;
+};
+
+#define lcdev_to_igc_ldev(lcdev)				\
+	container_of(lcdev, struct igc_led_classdev, led)
+
+static void igc_led_select(struct igc_adapter *adapter, int led,
+			   u32 *mask, u32 *shift, u32 *blink)
+{
+	switch (led) {
+	case 0:
+		*mask  = IGC_LEDCTL_LED0_MODE_MASK;
+		*shift = IGC_LEDCTL_LED0_MODE_SHIFT;
+		*blink = IGC_LEDCTL_LED0_BLINK;
+		break;
+	case 1:
+		*mask  = IGC_LEDCTL_LED1_MODE_MASK;
+		*shift = IGC_LEDCTL_LED1_MODE_SHIFT;
+		*blink = IGC_LEDCTL_LED1_BLINK;
+		break;
+	case 2:
+		*mask  = IGC_LEDCTL_LED2_MODE_MASK;
+		*shift = IGC_LEDCTL_LED2_MODE_SHIFT;
+		*blink = IGC_LEDCTL_LED2_BLINK;
+		break;
+	default:
+		*mask = *shift = *blink = 0;
+		netdev_err(adapter->netdev, "Unknown LED %d selected!\n", led);
+	}
+}
+
+static void igc_led_set(struct igc_adapter *adapter, int led, u32 mode,
+			bool blink)
+{
+	u32 shift, mask, blink_bit, ledctl;
+	struct igc_hw *hw = &adapter->hw;
+
+	igc_led_select(adapter, led, &mask, &shift, &blink_bit);
+
+	pm_runtime_get_sync(&adapter->pdev->dev);
+	mutex_lock(&adapter->led_mutex);
+
+	/* Set mode */
+	ledctl = rd32(IGC_LEDCTL);
+	ledctl &= ~mask;
+	ledctl |= mode << shift;
+
+	/* Configure blinking */
+	if (blink)
+		ledctl |= blink_bit;
+	else
+		ledctl &= ~blink_bit;
+	wr32(IGC_LEDCTL, ledctl);
+
+	mutex_unlock(&adapter->led_mutex);
+	pm_runtime_put(&adapter->pdev->dev);
+}
+
+static u32 igc_led_get(struct igc_adapter *adapter, int led)
+{
+	u32 shift, mask, blink_bit, ledctl;
+	struct igc_hw *hw = &adapter->hw;
+
+	igc_led_select(adapter, led, &mask, &shift, &blink_bit);
+
+	pm_runtime_get_sync(&adapter->pdev->dev);
+	mutex_lock(&adapter->led_mutex);
+	ledctl = rd32(IGC_LEDCTL);
+	mutex_unlock(&adapter->led_mutex);
+	pm_runtime_put(&adapter->pdev->dev);
+
+	return (ledctl & mask) >> shift;
+}
+
+static int igc_led_hw_control_is_supported(struct led_classdev *led_cdev,
+					   unsigned long flags)
+{
+	bool rx, tx;
+
+	if (flags & ~IGC_SUPPORTED_MODES)
+		return -EOPNOTSUPP;
+
+	rx = flags & BIT(TRIGGER_NETDEV_RX);
+	tx = flags & BIT(TRIGGER_NETDEV_TX);
+	if (rx != tx)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static int igc_led_hw_control_set(struct led_classdev *led_cdev,
+				  unsigned long flags)
+{
+	struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev);
+	struct igc_adapter *adapter = netdev_priv(ldev->netdev);
+	bool blink = false;
+	u32 mode;
+
+	if (flags & BIT(TRIGGER_NETDEV_LINK_10))
+		mode = IGC_LEDCTL_MODE_LINK_10;
+	if (flags & BIT(TRIGGER_NETDEV_LINK_100))
+		mode = IGC_LEDCTL_MODE_LINK_100;
+	if (flags & BIT(TRIGGER_NETDEV_LINK_1000))
+		mode = IGC_LEDCTL_MODE_LINK_1000;
+	if (flags & BIT(TRIGGER_NETDEV_LINK_2500))
+		mode = IGC_LEDCTL_MODE_LINK_2500;
+	if ((flags & BIT(TRIGGER_NETDEV_TX)) ||
+	    (flags & BIT(TRIGGER_NETDEV_RX)))
+		mode = IGC_LEDCTL_MODE_ACTIVITY;
+
+	/* blink is recommended for activity */
+	if (mode == IGC_LEDCTL_MODE_ACTIVITY)
+		blink = true;
+
+	igc_led_set(adapter, ldev->index, mode, blink);
+
+	return 0;
+}
+
+static int igc_led_hw_control_get(struct led_classdev *led_cdev,
+				  unsigned long *flags)
+{
+	struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev);
+	struct igc_adapter *adapter = netdev_priv(ldev->netdev);
+	u32 mode;
+
+	mode = igc_led_get(adapter, ldev->index);
+
+	switch (mode) {
+	case IGC_LEDCTL_MODE_ACTIVITY:
+		*flags = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
+		break;
+	case IGC_LEDCTL_MODE_LINK_10:
+		*flags = BIT(TRIGGER_NETDEV_LINK_10);
+		break;
+	case IGC_LEDCTL_MODE_LINK_100:
+		*flags = BIT(TRIGGER_NETDEV_LINK_100);
+		break;
+	case IGC_LEDCTL_MODE_LINK_1000:
+		*flags = BIT(TRIGGER_NETDEV_LINK_1000);
+		break;
+	case IGC_LEDCTL_MODE_LINK_2500:
+		*flags = BIT(TRIGGER_NETDEV_LINK_2500);
+		break;
+	}
+
+	return 0;
+}
+
+static struct device *igc_led_hw_control_get_device(struct led_classdev *led_cdev)
+{
+	struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev);
+
+	return &ldev->netdev->dev;
+}
+
+static void igc_led_get_name(struct igc_adapter *adapter, int index, char *buf,
+			     size_t buf_len)
+{
+	snprintf(buf, buf_len, "igc-%x%x-led%d",
+		 pci_domain_nr(adapter->pdev->bus),
+		 pci_dev_id(adapter->pdev), index);
+}
+
+static void igc_setup_ldev(struct igc_led_classdev *ldev,
+			   struct net_device *netdev, int index)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	struct led_classdev *led_cdev = &ldev->led;
+	char led_name[LED_MAX_NAME_SIZE];
+
+	ldev->netdev = netdev;
+	ldev->index = index;
+
+	igc_led_get_name(adapter, index, led_name, LED_MAX_NAME_SIZE);
+	led_cdev->name = led_name;
+	led_cdev->hw_control_trigger = "netdev";
+	led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
+	led_cdev->hw_control_is_supported = igc_led_hw_control_is_supported;
+	led_cdev->hw_control_set = igc_led_hw_control_set;
+	led_cdev->hw_control_get = igc_led_hw_control_get;
+	led_cdev->hw_control_get_device = igc_led_hw_control_get_device;
+
+	devm_led_classdev_register(&netdev->dev, led_cdev);
+}
+
+int igc_led_setup(struct igc_adapter *adapter)
+{
+	struct net_device *netdev = adapter->netdev;
+	struct device *dev = &netdev->dev;
+	struct igc_led_classdev *leds;
+	int i;
+
+	mutex_init(&adapter->led_mutex);
+
+	leds = devm_kcalloc(dev, IGC_NUM_LEDS, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	for (i = 0; i < IGC_NUM_LEDS; i++)
+		igc_setup_ldev(leds + i, netdev, i);
+
+	return 0;
+}
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index ba8d3fe186ae..5ee26def75a7 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6977,6 +6977,12 @@ static int igc_probe(struct pci_dev *pdev,
 
 	pm_runtime_put_noidle(&pdev->dev);
 
+	if (IS_ENABLED(CONFIG_IGC_LEDS)) {
+		err = igc_led_setup(adapter);
+		if (err)
+			goto err_register;
+	}
+
 	return 0;
 
 err_register:
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index d38c87d7e5e8..e5b893fc5b66 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -12,6 +12,7 @@
 #define IGC_MDIC		0x00020  /* MDI Control - RW */
 #define IGC_CONNSW		0x00034  /* Copper/Fiber switch control - RW */
 #define IGC_VET			0x00038  /* VLAN Ether Type - RW */
+#define IGC_LEDCTL		0x00E00	 /* LED Control - RW */
 #define IGC_I225_PHPM		0x00E14  /* I225 PHY Power Management */
 #define IGC_GPHY_VERSION	0x0001E  /* I225 gPHY Firmware Version */
 
-- 
2.39.2

Re: [PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[Simon Horman]

On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote:

...

> +static int igc_led_hw_control_set(struct led_classdev *led_cdev,
> +				  unsigned long flags)
> +{
> +	struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev);
> +	struct igc_adapter *adapter = netdev_priv(ldev->netdev);
> +	bool blink = false;
> +	u32 mode;
> +
> +	if (flags & BIT(TRIGGER_NETDEV_LINK_10))
> +		mode = IGC_LEDCTL_MODE_LINK_10;
> +	if (flags & BIT(TRIGGER_NETDEV_LINK_100))
> +		mode = IGC_LEDCTL_MODE_LINK_100;
> +	if (flags & BIT(TRIGGER_NETDEV_LINK_1000))
> +		mode = IGC_LEDCTL_MODE_LINK_1000;
> +	if (flags & BIT(TRIGGER_NETDEV_LINK_2500))
> +		mode = IGC_LEDCTL_MODE_LINK_2500;
> +	if ((flags & BIT(TRIGGER_NETDEV_TX)) ||
> +	    (flags & BIT(TRIGGER_NETDEV_RX)))
> +		mode = IGC_LEDCTL_MODE_ACTIVITY;

Hi Kurt,

I guess this can't happen in practice,
but if none of the conditions above are met,
then mode is used uninitialised below.

Flagged by Smatch.

> +
> +	/* blink is recommended for activity */
> +	if (mode == IGC_LEDCTL_MODE_ACTIVITY)
> +		blink = true;
> +
> +	igc_led_set(adapter, ldev->index, mode, blink);
> +
> +	return 0;
> +}

...

Re: [PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[Andrew Lunn]

On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote:
> Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs
> from user space using the netdev trigger. The LEDs are named as
> igc-<bus><device>-<led> to be easily identified.
> 
> Offloading activity and link speed is supported. Tested on Intel i225.

Nice to see something not driver by phylib/DSA making use of LEDs.

Is there no plain on/off support? Ideally we want that for software
blinking for when a mode is not supported.

	 Andrew

Re: [PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]

On Wed Jan 24 2024, Simon Horman wrote:
> On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote:
>
> ...
>
>> +static int igc_led_hw_control_set(struct led_classdev *led_cdev,
>> +				  unsigned long flags)
>> +{
>> +	struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev);
>> +	struct igc_adapter *adapter = netdev_priv(ldev->netdev);
>> +	bool blink = false;
>> +	u32 mode;
>> +
>> +	if (flags & BIT(TRIGGER_NETDEV_LINK_10))
>> +		mode = IGC_LEDCTL_MODE_LINK_10;
>> +	if (flags & BIT(TRIGGER_NETDEV_LINK_100))
>> +		mode = IGC_LEDCTL_MODE_LINK_100;
>> +	if (flags & BIT(TRIGGER_NETDEV_LINK_1000))
>> +		mode = IGC_LEDCTL_MODE_LINK_1000;
>> +	if (flags & BIT(TRIGGER_NETDEV_LINK_2500))
>> +		mode = IGC_LEDCTL_MODE_LINK_2500;
>> +	if ((flags & BIT(TRIGGER_NETDEV_TX)) ||
>> +	    (flags & BIT(TRIGGER_NETDEV_RX)))
>> +		mode = IGC_LEDCTL_MODE_ACTIVITY;
>
> Hi Kurt,
>
> I guess this can't happen in practice,
> but if none of the conditions above are met,
> then mode is used uninitialised below.

Yes, it shouldn't happen, because the supported modes are
checked. However, mode can be initialized to off to avoid the warning.

>
> Flagged by Smatch.

Out of curiosity how did you get the warning? I usually run `make W=1 C=1
M=...` before sending patches.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Wed Jan 24 2024, Andrew Lunn wrote:
> On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote:
>> Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs
>> from user space using the netdev trigger. The LEDs are named as
>> igc-<bus><device>-<led> to be easily identified.
>> 
>> Offloading activity and link speed is supported. Tested on Intel i225.
>
> Nice to see something not driver by phylib/DSA making use of LEDs.
>
> Is there no plain on/off support? Ideally we want that for software
> blinking for when a mode is not supported.

Plain on and off is supported is supported, too. Should be possible to
implement brightness_set().

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[Simon Horman]

+ Dan Carpenter <carpenter@linaro.org>

On Thu, Jan 25, 2024 at 08:20:40AM +0100, Kurt Kanzenbach wrote:
> On Wed Jan 24 2024, Simon Horman wrote:
> > On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote:
> >
> > ...
> >
> >> +static int igc_led_hw_control_set(struct led_classdev *led_cdev,
> >> +				  unsigned long flags)
> >> +{
> >> +	struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev);
> >> +	struct igc_adapter *adapter = netdev_priv(ldev->netdev);
> >> +	bool blink = false;
> >> +	u32 mode;
> >> +
> >> +	if (flags & BIT(TRIGGER_NETDEV_LINK_10))
> >> +		mode = IGC_LEDCTL_MODE_LINK_10;
> >> +	if (flags & BIT(TRIGGER_NETDEV_LINK_100))
> >> +		mode = IGC_LEDCTL_MODE_LINK_100;
> >> +	if (flags & BIT(TRIGGER_NETDEV_LINK_1000))
> >> +		mode = IGC_LEDCTL_MODE_LINK_1000;
> >> +	if (flags & BIT(TRIGGER_NETDEV_LINK_2500))
> >> +		mode = IGC_LEDCTL_MODE_LINK_2500;
> >> +	if ((flags & BIT(TRIGGER_NETDEV_TX)) ||
> >> +	    (flags & BIT(TRIGGER_NETDEV_RX)))
> >> +		mode = IGC_LEDCTL_MODE_ACTIVITY;
> >
> > Hi Kurt,
> >
> > I guess this can't happen in practice,
> > but if none of the conditions above are met,
> > then mode is used uninitialised below.
> 
> Yes, it shouldn't happen, because the supported modes are
> checked. However, mode can be initialized to off to avoid the warning.

Thanks.

> > Flagged by Smatch.
> 
> Out of curiosity how did you get the warning? I usually run `make W=1 C=1
> M=...` before sending patches.

Probably there is a better way, but I run Smatch like this:

 .../smatch/smatch_scripts/kchecker a.c

Smatch can be found here:

  https://github.com/error27/smatch

Re: [PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[Andrew Lunn]

On Thu, Jan 25, 2024 at 08:31:54AM +0100, Kurt Kanzenbach wrote:
> On Wed Jan 24 2024, Andrew Lunn wrote:
> > On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote:
> >> Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs
> >> from user space using the netdev trigger. The LEDs are named as
> >> igc-<bus><device>-<led> to be easily identified.
> >> 
> >> Offloading activity and link speed is supported. Tested on Intel i225.
> >
> > Nice to see something not driver by phylib/DSA making use of LEDs.
> >
> > Is there no plain on/off support? Ideally we want that for software
> > blinking for when a mode is not supported.
> 
> Plain on and off is supported is supported, too. Should be possible to
> implement brightness_set().

Great.

Its actually better to first implement brightness_set(). That gives
you full support for everything the netdev trigger has. Then add
offload, which is optional, and will fall back to software for modes
which cannot be offloaded.

      Andrew

Re: [PATCH v1 iwl-next] igc: Add support for LEDs on i225/i226

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

On Thu Jan 25 2024, Andrew Lunn wrote:
> On Thu, Jan 25, 2024 at 08:31:54AM +0100, Kurt Kanzenbach wrote:
>> On Wed Jan 24 2024, Andrew Lunn wrote:
>> > On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote:
>> >> Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs
>> >> from user space using the netdev trigger. The LEDs are named as
>> >> igc-<bus><device>-<led> to be easily identified.
>> >> 
>> >> Offloading activity and link speed is supported. Tested on Intel i225.
>> >
>> > Nice to see something not driver by phylib/DSA making use of LEDs.
>> >
>> > Is there no plain on/off support? Ideally we want that for software
>> > blinking for when a mode is not supported.
>> 
>> Plain on and off is supported is supported, too. Should be possible to
>> implement brightness_set().
>
> Great.
>
> Its actually better to first implement brightness_set(). That gives
> you full support for everything the netdev trigger has. Then add
> offload, which is optional, and will fall back to software for modes
> which cannot be offloaded.

Understood. I'll do that.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]