| 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: <20240829162705.GR6858@google.com>
Date: Thu, 29 Aug 2024 17:27:05 +0100
From: Lee Jones <lee@...nel.org>
To: Heiko Stuebner <heiko@...ech.de>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
jdelvare@...e.com, linux@...ck-us.net, dmitry.torokhov@...il.com,
pavel@....cz, ukleinek@...ian.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-input@...r.kernel.org,
linux-leds@...r.kernel.org
Subject: Re: [PATCH v6 3/7] leds: add driver for LEDs from qnap-mcu devices
On Sun, 25 Aug 2024, Heiko Stuebner wrote:
> This adds a driver that connects to the qnap-mcu mfd driver and provides
> access to the LEDs on it.
>
> Signed-off-by: Heiko Stuebner <heiko@...ech.de>
> ---
> MAINTAINERS | 1 +
> drivers/leds/Kconfig | 11 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-qnap-mcu.c | 226 +++++++++++++++++++++++++++++++++++
> 4 files changed, 239 insertions(+)
> create mode 100644 drivers/leds/leds-qnap-mcu.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0fbd2d953da4..4dff0e237f22 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18657,6 +18657,7 @@ F: drivers/media/tuners/qm1d1c0042*
> QNAP MCU DRIVER
> M: Heiko Stuebner <heiko@...ech.de>
> S: Maintained
> +F: drivers/leds/leds-qnap-mcu.c
> F: drivers/mfd/qnap-mcu.c
> F: include/linux/qnap-mcu.h
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 8d9d8da376e4..9a337478dd80 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -580,6 +580,17 @@ config LEDS_PCA995X
> LED driver chips accessed via the I2C bus. Supported
> devices include PCA9955BTW, PCA9952TW and PCA9955TW.
>
> +config LEDS_QNAP_MCU
> + tristate "LED Support for QNAP MCU controllers"
> + depends on LEDS_CLASS
> + depends on MFD_QNAP_MCU
> + help
> + This option enables support for LEDs available on embedded
> + controllers used in QNAP NAS devices.
> +
> + This driver can also be built as a module. If so, the module
> + will be called qnap-mcu-leds.
> +
> config LEDS_WM831X_STATUS
> tristate "LED support for status LEDs on WM831x PMICs"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 18afbb5a23ee..c6f74865d729 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_LEDS_PCA995X) += leds-pca995x.o
> obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o
> obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
> obj-$(CONFIG_LEDS_PWM) += leds-pwm.o
> +obj-$(CONFIG_LEDS_QNAP_MCU) += leds-qnap-mcu.o
> obj-$(CONFIG_LEDS_REGULATOR) += leds-regulator.o
> obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
> obj-$(CONFIG_LEDS_SUN50I_A100) += leds-sun50i-a100.o
> diff --git a/drivers/leds/leds-qnap-mcu.c b/drivers/leds/leds-qnap-mcu.c
> new file mode 100644
> index 000000000000..0723ec52e4c5
> --- /dev/null
> +++ b/drivers/leds/leds-qnap-mcu.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
Superfluous line.
> +/*
> + * Driver for LEDs found on QNAP MCU devices
> + *
> + * Copyright (C) 2024 Heiko Stuebner <heiko@...ech.de>
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/mfd/qnap-mcu.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <uapi/linux/uleds.h>
> +
> +enum qnap_mcu_err_led_mode {
> + QNAP_MCU_ERR_LED_ON = 0,
> + QNAP_MCU_ERR_LED_OFF = 1,
> + QNAP_MCU_ERR_LED_BLINK_FAST = 2,
> + QNAP_MCU_ERR_LED_BLINK_SLOW = 3,
> +};
> +
> +struct qnap_mcu_err_led {
> + struct qnap_mcu *mcu;
> + struct led_classdev cdev;
> + char name[LED_MAX_NAME_SIZE];
> + u8 num;
> + u8 mode;
> +};
> +
> +static inline struct qnap_mcu_err_led *
> + cdev_to_qnap_mcu_err_led(struct led_classdev *led_cdev)
> +{
> + return container_of(led_cdev, struct qnap_mcu_err_led, cdev);
> +}
> +
> +static int qnap_mcu_err_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
'value' is a terrible variable name.
Please describe the data. 'brightness' seems appropriate.
> +{
> + struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
> + u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
Really not fan of these magic values being used raw like this.
> + /* Don't disturb a possible set blink-mode if LED is already on */
Why not save cycles and return if blink mode is already enabled?
> + if (value == 0)
> + err_led->mode = QNAP_MCU_ERR_LED_OFF;
> + else if (err_led->mode == QNAP_MCU_ERR_LED_OFF)
> + err_led->mode = QNAP_MCU_ERR_LED_ON;
Then you can do:
err_led->mode = value ? QNAP_MCU_ERR_LED_ON : QNAP_MCU_ERR_LED_OFF;
> + cmd[3] = 0x30 + err_led->mode;
> +
> + return qnap_mcu_exec_with_ack(err_led->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_err_led_blink_set(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
> + u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
> +
> + /* LED is off, nothing to do */
> + if (err_led->mode == QNAP_MCU_ERR_LED_OFF)
> + return 0;
> +
> + if (*delay_on < 500) {
Setting delay_on based on the current value of delay_on sounds sketchy.
> + *delay_on = 100;
> + *delay_off = 100;
> + err_led->mode = QNAP_MCU_ERR_LED_BLINK_FAST;
> + } else {
> + *delay_on = 500;
> + *delay_off = 500;
> + err_led->mode = QNAP_MCU_ERR_LED_BLINK_SLOW;
> + }
How do you change from a fast to a slow blinking LED and back again?
> + cmd[3] = 0x30 + err_led->mode;
> +
> + return qnap_mcu_exec_with_ack(err_led->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_register_err_led(struct device *dev, struct qnap_mcu *mcu, int num)
What's num? I should be able to answer that by the nomenclature.
> +{
> + struct qnap_mcu_err_led *err_led;
> + int ret;
> +
> + err_led = devm_kzalloc(dev, sizeof(*err_led), GFP_KERNEL);
> + if (!err_led)
> + return -ENOMEM;
> +
> + err_led->mcu = mcu;
> + err_led->num = num;
> + err_led->mode = QNAP_MCU_ERR_LED_OFF;
> +
> + snprintf(err_led->name, LED_MAX_NAME_SIZE, "hdd%d:red:status", num + 1);
scnprintf()?
> + err_led->cdev.name = err_led->name;
> +
> + err_led->cdev.brightness_set_blocking = qnap_mcu_err_led_set;
> + err_led->cdev.blink_set = qnap_mcu_err_led_blink_set;
> + err_led->cdev.brightness = 0;
> + err_led->cdev.max_brightness = 1;
> +
> + ret = devm_led_classdev_register(dev, &err_led->cdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register hdd led %d", num);
"HDD LED"
> + return qnap_mcu_err_led_set(&err_led->cdev, 0);
> +}
> +
> +enum qnap_mcu_usb_led_mode {
> + QNAP_MCU_USB_LED_ON = 1,
> + QNAP_MCU_USB_LED_OFF = 3,
> + QNAP_MCU_USB_LED_BLINK = 2,
> +};
> +
> +struct qnap_mcu_usb_led {
> + struct qnap_mcu *mcu;
> + struct led_classdev cdev;
> + u8 mode;
> +};
> +
> +static inline struct qnap_mcu_usb_led *
> + cdev_to_qnap_mcu_usb_led(struct led_classdev *led_cdev)
> +{
> + return container_of(led_cdev, struct qnap_mcu_usb_led, cdev);
> +}
> +
> +static int qnap_mcu_usb_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct qnap_mcu_usb_led *usb_led = cdev_to_qnap_mcu_usb_led(led_cdev);
> + u8 cmd[] = { 0x40, 0x43, 0 };
> +
> + /*
> + * If the led is off, turn it on. Otherwise don't disturb
"LED"
> + * a possible set blink-mode.
> + */
> + if (value == 0)
> + usb_led->mode = QNAP_MCU_USB_LED_OFF;
> + else if (usb_led->mode == QNAP_MCU_USB_LED_OFF)
> + usb_led->mode = QNAP_MCU_USB_LED_ON;
Same suggestions as above.
> + /* byte 3 is shared between the usb led target and setting the mode */
"Byte" for two reasons. Firstly because it's the correct capitalisation
of Byte and secondly because it's the start of a sentence.
"USB" and "LED" throughout please.
> + cmd[2] = 0x44 | usb_led->mode;
> +
> + return qnap_mcu_exec_with_ack(usb_led->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_usb_led_blink_set(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + struct qnap_mcu_usb_led *usb_led = cdev_to_qnap_mcu_usb_led(led_cdev);
> + u8 cmd[] = { 0x40, 0x43, 0 };
> +
> + /* LED is off, nothing to do */
> + if (usb_led->mode == QNAP_MCU_USB_LED_OFF)
> + return 0;
> +
> + *delay_on = 250;
> + *delay_off = 250;
> + usb_led->mode = QNAP_MCU_USB_LED_BLINK;
> +
> + /* byte 3 is shared between the usb led target and setting the mode */
> + cmd[2] = 0x44 | usb_led->mode;
> +
> + return qnap_mcu_exec_with_ack(usb_led->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_register_usb_led(struct device *dev, struct qnap_mcu *mcu)
> +{
> + struct qnap_mcu_usb_led *usb_led;
> + int ret;
> +
> + usb_led = devm_kzalloc(dev, sizeof(*usb_led), GFP_KERNEL);
> + if (!usb_led)
> + return -ENOMEM;
> +
> + usb_led->mcu = mcu;
> + usb_led->mode = QNAP_MCU_USB_LED_OFF;
> + usb_led->cdev.name = "usb:blue:disk";
> + usb_led->cdev.brightness_set_blocking = qnap_mcu_usb_led_set;
> + usb_led->cdev.blink_set = qnap_mcu_usb_led_blink_set;
> + usb_led->cdev.brightness = 0;
> + usb_led->cdev.max_brightness = 1;
> +
> + ret = devm_led_classdev_register(dev, &usb_led->cdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register usb led");
> +
> + return qnap_mcu_usb_led_set(&usb_led->cdev, 0);
> +}
> +
> +static int qnap_mcu_leds_probe(struct platform_device *pdev)
> +{
> + struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
> + const struct qnap_mcu_variant *variant = qnap_mcu_get_variant_data(mcu);
Grab this from platform_data.
> + int ret, i;
> +
> + for (i = 0; i < variant->num_drives; i++) {
You can use:
for (int i = 0; ..
> + ret = qnap_mcu_register_err_led(&pdev->dev, mcu, i);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register error led %d\n", i);
> + }
> +
> + if (variant->usb_led) {
> + ret = qnap_mcu_register_usb_led(&pdev->dev, mcu);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register usb led %d\n", i);
The 'i' here looks like a copy/paste error.
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver qnap_mcu_leds_driver = {
> + .probe = qnap_mcu_leds_probe,
> + .driver = {
> + .name = "qnap-mcu-leds",
> + },
> +};
> +module_platform_driver(qnap_mcu_leds_driver);
> +
> +MODULE_ALIAS("platform:qnap-mcu-leds");
> +MODULE_AUTHOR("Heiko Stuebner <heiko@...ech.de>");
> +MODULE_DESCRIPTION("QNAP MCU LEDs driver");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists