Re: [PATCH] leds: qnap-mcu: add support for the red and green status leds

["Heiko Stübner" <heiko@sntech.de>]

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