There is one more set of two LEDs on the qnap devices to indicate status.

One LED is green, the other is red and while they occupy the same space
on the front panel, they cannot be enabled at the same time.

But they can interact via blink functions, the MCU can flash them
alternately, going red -> green -> red -> ... either in 500ms or
1s intervals. They can of course also blink individually.

Add specific led functions for them and register them on probe.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/leds/leds-qnap-mcu.c | 156 +++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/drivers/leds/leds-qnap-mcu.c b/drivers/leds/leds-qnap-mcu.c
index 4e4709456261..b7747b47c604 100644
--- a/drivers/leds/leds-qnap-mcu.c
+++ b/drivers/leds/leds-qnap-mcu.c
@@ -190,6 +190,157 @@ static int qnap_mcu_register_usb_led(struct device *dev, struct qnap_mcu *mcu)
 	return qnap_mcu_usb_led_set(&usb_led->cdev, 0);
 }
 
+enum qnap_mcu_status_led_mode {
+	QNAP_MCU_STATUS_LED_OFF = 0,
+	QNAP_MCU_STATUS_LED_ON = 1,
+	QNAP_MCU_STATUS_LED_BLINK_FAST = 2, /* 500ms / 500ms */
+	QNAP_MCU_STATUS_LED_BLINK_SLOW = 3, /* 1s / 1s */
+};
+
+struct qnap_mcu_status;
+struct qnap_mcu_status_led {
+	struct qnap_mcu_status *base;
+	struct led_classdev cdev;
+	u8 mode;
+};
+
+struct qnap_mcu_status {
+	struct qnap_mcu *mcu;
+	struct qnap_mcu_status_led red;
+	struct qnap_mcu_status_led green;
+};
+
+static inline struct qnap_mcu_status_led *
+		cdev_to_qnap_mcu_status_led(struct led_classdev *led_cdev)
+{
+	return container_of(led_cdev, struct qnap_mcu_status_led, cdev);
+}
+
+static u8 qnap_mcu_status_led_encode(struct qnap_mcu_status *status)
+{
+	if (status->red.mode == QNAP_MCU_STATUS_LED_OFF) {
+		switch (status->green.mode) {
+		case QNAP_MCU_STATUS_LED_OFF:
+			return '9';
+		case QNAP_MCU_STATUS_LED_ON:
+			return '6';
+		case QNAP_MCU_STATUS_LED_BLINK_FAST:
+			return '5';
+		case QNAP_MCU_STATUS_LED_BLINK_SLOW:
+			return 'A';
+		}
+	} else if (status->green.mode == QNAP_MCU_STATUS_LED_OFF) {
+		switch (status->red.mode) {
+		case QNAP_MCU_STATUS_LED_OFF:
+			return '9';
+		case QNAP_MCU_STATUS_LED_ON:
+			return '7';
+		case QNAP_MCU_STATUS_LED_BLINK_FAST:
+			return '4';
+		case QNAP_MCU_STATUS_LED_BLINK_SLOW:
+			return 'B';
+		}
+	} else if (status->green.mode == QNAP_MCU_STATUS_LED_BLINK_SLOW &&
+		   status->red.mode == QNAP_MCU_STATUS_LED_BLINK_SLOW) {
+		return 'C';
+	}
+
+	/*
+	 * At this point, both LEDs are on in some fashion, but both
+	 * cannot be on at the same time, so just use the fast blink
+	 */
+	return '8';
+}
+
+static int qnap_mcu_status_led_update(struct qnap_mcu *mcu,
+				      struct qnap_mcu_status *status)
+{
+	u8 cmd[] = { '@', 'C', 0 };
+
+	cmd[2] = qnap_mcu_status_led_encode(status);
+
+	return qnap_mcu_exec_with_ack(mcu, cmd, sizeof(cmd));
+}
+
+static int qnap_mcu_status_led_set(struct led_classdev *led_cdev,
+				   enum led_brightness brightness)
+{
+	struct qnap_mcu_status_led *status_led = cdev_to_qnap_mcu_status_led(led_cdev);
+
+	/* Don't disturb a possible set blink-mode if LED stays on */
+	if (brightness != 0 &&
+	    status_led->mode >= QNAP_MCU_STATUS_LED_BLINK_FAST)
+		return 0;
+
+	status_led->mode = brightness ? QNAP_MCU_STATUS_LED_ON :
+					QNAP_MCU_STATUS_LED_OFF;
+
+	return qnap_mcu_status_led_update(status_led->base->mcu,
+					  status_led->base);
+}
+
+static int qnap_mcu_status_led_blink_set(struct led_classdev *led_cdev,
+					 unsigned long *delay_on,
+					 unsigned long *delay_off)
+{
+	struct qnap_mcu_status_led *status_led = cdev_to_qnap_mcu_status_led(led_cdev);
+
+	/* LED is off, nothing to do */
+	if (status_led->mode == QNAP_MCU_STATUS_LED_OFF)
+		return 0;
+
+	if (*delay_on <= 500) {
+		*delay_on = 500;
+		*delay_off = 500;
+		status_led->mode = QNAP_MCU_STATUS_LED_BLINK_FAST;
+	} else {
+		*delay_on = 1000;
+		*delay_off = 1000;
+		status_led->mode = QNAP_MCU_STATUS_LED_BLINK_SLOW;
+	}
+
+	return qnap_mcu_status_led_update(status_led->base->mcu,
+					  status_led->base);
+}
+
+static int qnap_mcu_register_status_leds(struct device *dev, struct qnap_mcu *mcu)
+{
+	struct qnap_mcu_status *status;
+	int ret;
+
+	status = devm_kzalloc(dev, sizeof(*status), GFP_KERNEL);
+	if (!status)
+		return -ENOMEM;
+
+	status->mcu = mcu;
+	status->red.base = status;
+	status->green.base = status;
+
+	status->red.mode = QNAP_MCU_STATUS_LED_OFF;
+	status->red.cdev.name = "red:status";
+	status->red.cdev.brightness_set_blocking = qnap_mcu_status_led_set;
+	status->red.cdev.blink_set = qnap_mcu_status_led_blink_set;
+	status->red.cdev.brightness = 0;
+	status->red.cdev.max_brightness = 1;
+
+	status->green.mode = QNAP_MCU_STATUS_LED_OFF;
+	status->green.cdev.name = "green:status";
+	status->green.cdev.brightness_set_blocking = qnap_mcu_status_led_set;
+	status->green.cdev.blink_set = qnap_mcu_status_led_blink_set;
+	status->green.cdev.brightness = 0;
+	status->green.cdev.max_brightness = 1;
+
+	ret = devm_led_classdev_register(dev, &status->red.cdev);
+	if (ret)
+		return ret;
+
+	ret = devm_led_classdev_register(dev, &status->green.cdev);
+	if (ret)
+		return ret;
+
+	return qnap_mcu_status_led_update(status->mcu, status);
+}
+
 static int qnap_mcu_leds_probe(struct platform_device *pdev)
 {
 	struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
@@ -210,6 +361,11 @@ static int qnap_mcu_leds_probe(struct platform_device *pdev)
 					"failed to register USB LED\n");
 	}
 
+	ret = qnap_mcu_register_status_leds(&pdev->dev, mcu);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to register status LED\n");
+
 	return 0;
 }
 
-- 
2.47.2

Subject: s/led/LED/

> There is one more set of two LEDs on the qnap devices to indicate status.
> 
> One LED is green, the other is red and while they occupy the same space
> on the front panel, they cannot be enabled at the same time.
> 
> But they can interact via blink functions, the MCU can flash them
> alternately, going red -> green -> red -> ... either in 500ms or
> 1s intervals. They can of course also blink individually.
> 
> Add specific led functions for them and register them on probe.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/leds/leds-qnap-mcu.c | 156 +++++++++++++++++++++++++++++++++++
>  1 file changed, 156 insertions(+)
> 
> diff --git a/drivers/leds/leds-qnap-mcu.c b/drivers/leds/leds-qnap-mcu.c
> index 4e4709456261..b7747b47c604 100644
> --- a/drivers/leds/leds-qnap-mcu.c
> +++ b/drivers/leds/leds-qnap-mcu.c
> @@ -190,6 +190,157 @@ static int qnap_mcu_register_usb_led(struct device *dev, struct qnap_mcu *mcu)
>  	return qnap_mcu_usb_led_set(&usb_led->cdev, 0);
>  }
>  
> +enum qnap_mcu_status_led_mode {
> +	QNAP_MCU_STATUS_LED_OFF = 0,
> +	QNAP_MCU_STATUS_LED_ON = 1,
> +	QNAP_MCU_STATUS_LED_BLINK_FAST = 2, /* 500ms / 500ms */
> +	QNAP_MCU_STATUS_LED_BLINK_SLOW = 3, /* 1s / 1s */
> +};
> +
> +struct qnap_mcu_status;

Forward declarations are a warning flag.

How do all of the other drivers handle this?

> +struct qnap_mcu_status_led {
> +	struct qnap_mcu_status *base;
> +	struct led_classdev cdev;
> +	u8 mode;
> +};
> +
> +struct qnap_mcu_status {
> +	struct qnap_mcu *mcu;
> +	struct qnap_mcu_status_led red;
> +	struct qnap_mcu_status_led green;
> +};
> 
> +static inline struct qnap_mcu_status_led *
> +		cdev_to_qnap_mcu_status_led(struct led_classdev *led_cdev)

This is a strange place to break.

> +{
> +	return container_of(led_cdev, struct qnap_mcu_status_led, cdev);
> +}
> +
> +static u8 qnap_mcu_status_led_encode(struct qnap_mcu_status *status)
> +{
> +	if (status->red.mode == QNAP_MCU_STATUS_LED_OFF) {
> +		switch (status->green.mode) {
> +		case QNAP_MCU_STATUS_LED_OFF:
> +			return '9';
> +		case QNAP_MCU_STATUS_LED_ON:
> +			return '6';
> +		case QNAP_MCU_STATUS_LED_BLINK_FAST:
> +			return '5';
> +		case QNAP_MCU_STATUS_LED_BLINK_SLOW:
> +			return 'A';
> +		}
> +	} else if (status->green.mode == QNAP_MCU_STATUS_LED_OFF) {
> +		switch (status->red.mode) {
> +		case QNAP_MCU_STATUS_LED_OFF:
> +			return '9';
> +		case QNAP_MCU_STATUS_LED_ON:
> +			return '7';
> +		case QNAP_MCU_STATUS_LED_BLINK_FAST:
> +			return '4';
> +		case QNAP_MCU_STATUS_LED_BLINK_SLOW:
> +			return 'B';
> +		}
> +	} else if (status->green.mode == QNAP_MCU_STATUS_LED_BLINK_SLOW &&
> +		   status->red.mode == QNAP_MCU_STATUS_LED_BLINK_SLOW) {
> +		return 'C';
> +	}
> +
> +	/*
> +	 * At this point, both LEDs are on in some fashion, but both
> +	 * cannot be on at the same time, so just use the fast blink
> +	 */
> +	return '8';
> +}
> +
> +static int qnap_mcu_status_led_update(struct qnap_mcu *mcu,
> +				      struct qnap_mcu_status *status)
> +{
> +	u8 cmd[] = { '@', 'C', 0 };
> +
> +	cmd[2] = qnap_mcu_status_led_encode(status);
> +
> +	return qnap_mcu_exec_with_ack(mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_status_led_set(struct led_classdev *led_cdev,
> +				   enum led_brightness brightness)
> +{
> +	struct qnap_mcu_status_led *status_led = cdev_to_qnap_mcu_status_led(led_cdev);
> +
> +	/* Don't disturb a possible set blink-mode if LED stays on */
> +	if (brightness != 0 &&

Use up to 100-chars to make these more readable.

> +	    status_led->mode >= QNAP_MCU_STATUS_LED_BLINK_FAST)
> +		return 0;
> +
> +	status_led->mode = brightness ? QNAP_MCU_STATUS_LED_ON :
> +					QNAP_MCU_STATUS_LED_OFF;
> +
> +	return qnap_mcu_status_led_update(status_led->base->mcu,
> +					  status_led->base);
> +}
> +
> +static int qnap_mcu_status_led_blink_set(struct led_classdev *led_cdev,
> +					 unsigned long *delay_on,
> +					 unsigned long *delay_off)
> +{
> +	struct qnap_mcu_status_led *status_led = cdev_to_qnap_mcu_status_led(led_cdev);
> +
> +	/* LED is off, nothing to do */

I think this is implied by the quality nomenclature.

> +	if (status_led->mode == QNAP_MCU_STATUS_LED_OFF)
> +		return 0;
> +
> +	if (*delay_on <= 500) {
> +		*delay_on = 500;
> +		*delay_off = 500;
> +		status_led->mode = QNAP_MCU_STATUS_LED_BLINK_FAST;
> +	} else {
> +		*delay_on = 1000;
> +		*delay_off = 1000;
> +		status_led->mode = QNAP_MCU_STATUS_LED_BLINK_SLOW;
> +	}
> +
> +	return qnap_mcu_status_led_update(status_led->base->mcu,
> +					  status_led->base);
> +}
> +
> +static int qnap_mcu_register_status_leds(struct device *dev, struct qnap_mcu *mcu)
> +{
> +	struct qnap_mcu_status *status;
> +	int ret;
> +
> +	status = devm_kzalloc(dev, sizeof(*status), GFP_KERNEL);
> +	if (!status)
> +		return -ENOMEM;
> +
> +	status->mcu = mcu;
> +	status->red.base = status;
> +	status->green.base = status;
> +
> +	status->red.mode = QNAP_MCU_STATUS_LED_OFF;
> +	status->red.cdev.name = "red:status";
> +	status->red.cdev.brightness_set_blocking = qnap_mcu_status_led_set;
> +	status->red.cdev.blink_set = qnap_mcu_status_led_blink_set;
> +	status->red.cdev.brightness = 0;
> +	status->red.cdev.max_brightness = 1;
> +
> +	status->green.mode = QNAP_MCU_STATUS_LED_OFF;
> +	status->green.cdev.name = "green:status";
> +	status->green.cdev.brightness_set_blocking = qnap_mcu_status_led_set;
> +	status->green.cdev.blink_set = qnap_mcu_status_led_blink_set;
> +	status->green.cdev.brightness = 0;
> +	status->green.cdev.max_brightness = 1;
> +
> +	ret = devm_led_classdev_register(dev, &status->red.cdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_led_classdev_register(dev, &status->green.cdev);
> +	if (ret)
> +		return ret;
> +
> +	return qnap_mcu_status_led_update(status->mcu, status);
> +}
> +
>  static int qnap_mcu_leds_probe(struct platform_device *pdev)
>  {
>  	struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
> @@ -210,6 +361,11 @@ static int qnap_mcu_leds_probe(struct platform_device *pdev)
>  					"failed to register USB LED\n");
>  	}
>  
> +	ret = qnap_mcu_register_status_leds(&pdev->dev, mcu);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to register status LED\n");
> +
>  	return 0;
>  }
>  
> -- 
> 2.47.2
> 

-- 
Lee Jones [李琼斯]

Am Donnerstag, 31. Juli 2025, 15:39:13 Mitteleuropäische Sommerzeit schrieb Lee Jones:
> Subject: s/led/LED/

note to myself, looking at git log, this applies to the prose part of the
subject, the subsystem indicator seems to stay lower case, so make that

  leds: qnap-mcu: add support for the red and green status LEDs


> > There is one more set of two LEDs on the qnap devices to indicate status.
> > 
> > One LED is green, the other is red and while they occupy the same space
> > on the front panel, they cannot be enabled at the same time.
> > 
> > But they can interact via blink functions, the MCU can flash them
> > alternately, going red -> green -> red -> ... either in 500ms or
> > 1s intervals. They can of course also blink individually.
> > 
> > Add specific led functions for them and register them on probe.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  drivers/leds/leds-qnap-mcu.c | 156 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 156 insertions(+)
> > 
> > diff --git a/drivers/leds/leds-qnap-mcu.c b/drivers/leds/leds-qnap-mcu.c
> > index 4e4709456261..b7747b47c604 100644
> > --- a/drivers/leds/leds-qnap-mcu.c
> > +++ b/drivers/leds/leds-qnap-mcu.c
> > @@ -190,6 +190,157 @@ static int qnap_mcu_register_usb_led(struct device *dev, struct qnap_mcu *mcu)
> >  	return qnap_mcu_usb_led_set(&usb_led->cdev, 0);
> >  }
> >  
> > +enum qnap_mcu_status_led_mode {
> > +	QNAP_MCU_STATUS_LED_OFF = 0,
> > +	QNAP_MCU_STATUS_LED_ON = 1,
> > +	QNAP_MCU_STATUS_LED_BLINK_FAST = 2, /* 500ms / 500ms */
> > +	QNAP_MCU_STATUS_LED_BLINK_SLOW = 3, /* 1s / 1s */
> > +};
> > +
> > +struct qnap_mcu_status;
> 
> Forward declarations are a warning flag.
> 
> How do all of the other drivers handle this?

I guess the question I debated a lot with is, is this one multi-color LED
or two single color LEDs, occupying the same area.

It is either the red _or_ green LED running, never both at the same time.

Similarly the HDD LEDs also occupy the same spaces with the amber
error-LED and the green activity LED. But those are completely distinct,
with the amber LED being controlled via the qnap-mcu and the green LED
getting controlled via a GPIO.

So for simplicity and consistency I opted for the two-LED approach, but
as you see in the resolver, need the target state of the "other" LED to
find a state to set.


But I think I found a different way to get from one LED to the other one
without a forward-declaration :-) .


> > +struct qnap_mcu_status_led {
> > +	struct qnap_mcu_status *base;
> > +	struct led_classdev cdev;
> > +	u8 mode;
> > +};
> > +
> > +struct qnap_mcu_status {
> > +	struct qnap_mcu *mcu;
> > +	struct qnap_mcu_status_led red;
> > +	struct qnap_mcu_status_led green;
> > +};
> > 
> > +static inline struct qnap_mcu_status_led *
> > +		cdev_to_qnap_mcu_status_led(struct led_classdev *led_cdev)
> 
> This is a strange place to break.

and with your 100 char request below, I realized that the break isn't
necessary at all, as it fits into exactly 100 chars :-)


Heiko