| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250731133913.GH1049189@google.com>
Date: Thu, 31 Jul 2025 14:39:13 +0100
From: Lee Jones <lee@...nel.org>
To: Heiko Stuebner <heiko@...ech.de>
Cc: pavel@...nel.org, linux-leds@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] leds: qnap-mcu: add support for the red and green status
leds
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@...ech.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 [李琼斯]
Powered by blists - more mailing lists