lkml.org 
[lkml]   [2021]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] leds: add NCT6795D driver
Hi!

> Add support for the LED feature of the NCT6795D chip found on some
> motherboards, notably MSI ones. The LEDs are typically used using a
> RGB connector so this driver takes advantage of the multicolor
> framework.

Ok.

> Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> ---
> Changes since v1 [1]:
> - Use the multicolor framework
>
> [1] https://lkml.org/lkml/2020/7/13/674 (sorry, took me some time to
> come back to this patch)
>
> drivers/leds/Kconfig | 10 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-nct6795d.c | 442 +++++++++++++++++++++++++++++++++++
> 3 files changed, 453 insertions(+)
> create mode 100644 drivers/leds/leds-nct6795d.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ed800f5da7d8..0db5986ca967 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -871,6 +871,16 @@ config LEDS_ACER_A500
> This option enables support for the Power Button LED of
> Acer Iconia Tab A500.

Can we put it into drivers/leds/multi/? Lets group multicolor stuff there.

> +config LEDS_NCT6795D
> + tristate "LED support for NCT6795D chipsets"
> + depends on LEDS_CLASS_MULTICOLOR
> + help
> + Enables support for the RGB LED feature of the NCT6795D chips found
> + on some MSI motherboards.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called leds-nct6795d.

.ko?

> diff --git a/drivers/leds/leds-nct6795d.c b/drivers/leds/leds-nct6795d.c
> new file mode 100644
> index 000000000000..90d5d2a67cfa
> --- /dev/null
> +++ b/drivers/leds/leds-nct6795d.c
> @@ -0,0 +1,442 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NCT6795D/NCT6797D LED driver
> + *
> + * Copyright (c) 2021 Alexandre Courbot <gnurou@gmail.com>
> + *
> + * Driver to control the RGB interfaces found on some MSI motherboards.
> + * This is for the most part a port of the MSI-RGB user-space program
> + * by Simonas Kazlauskas (https://github.com/nagisa/msi-rgb.git) to the Linux
> + * kernel LED interface.
> + *
> + * It is more limited than the original program due to limitations in the LED
> + * interface. For now, only static colors are supported.

Ok. We do have pattern trigger and hardware-accelerated blinking, if
it helps. But that may be a lot of fun with multicolor.

> +static inline int superio_enter(int ioreg)
> +{
> + if (!request_muxed_region(ioreg, 2, "NCT6795D LED"))
> + return -EBUSY;
> +
> + outb(0x87, ioreg);
> + outb(0x87, ioreg);
> +
> + return 0;
> +}
> +
> +static inline void superio_exit(int ioreg)
> +{
> + outb(0xaa, ioreg);
> + outb(0x02, ioreg);
> + outb(0x02, ioreg + 1);
> + release_region(ioreg, 2);
> +}

Are these two too big for inline?

> +static u8 init_vals[NUM_COLORS];
> +module_param_named(r, init_vals[RED], byte, 0);
> +MODULE_PARM_DESC(r, "Initial red intensity (default 0)");
> +module_param_named(g, init_vals[GREEN], byte, 0);
> +MODULE_PARM_DESC(g, "Initial green intensity (default 0)");
> +module_param_named(b, init_vals[BLUE], byte, 0);
> +MODULE_PARM_DESC(b, "Initial blue intensity (default 0)");

Lets... not add parameters for this.

> +/*
> + * Return the detected chip or an error code. If no chip was detected, -ENXIO
> + * is returned.
> + */
> +static enum nct679x_chip nct6795d_led_detect(u16 base_port)
> +{

"enum" return type is confusing here, as you also return errors.

> + val = superio_inb(led->base_port, 0x2c);
> + if ((val & 0x10) != 0x10)
> + superio_outb(led->base_port, 0x2c, val | 0x10);
> +
> + superio_select(led->base_port, NCT6795D_RGB_BANK);
> +
> + /* Check if RGB control enabled */
> + val = superio_inb(led->base_port, 0xe0);
> + if ((val & 0xe0) != 0xe0)
> + superio_outb(led->base_port, 0xe0, val | 0xe0);

I'd simply do outbs unconditionally...

> +/*
> + * Commit all colors to the hardware.
> + */
> +static int nct6795d_led_commit(const struct nct6795d_led *led)
> +{
> + const struct mc_subled *subled = led->subled;
> + int ret;
> +
> + dev_dbg(led->dev, "setting values: R=%d G=%d B=%d\n",
> + subled[RED].brightness, subled[GREEN].brightness,
> + subled[BLUE].brightness);
> +
> + ret = superio_enter(led->base_port);
> + if (ret)
> + return ret;

Are you sure you want to do superio_enter() each time LED values are
updated? That sounds... expensive, wrong. You have
request_muxed_region() call in there.

> +static int __init nct6795d_led_init(void)
> +{
> + static const u16 io_bases[] = { 0x4e, 0x2e };
> + struct resource io_res = {
> + .name = "io_base",
> + .flags = IORESOURCE_REG,
> + };
> + enum nct679x_chip detected_chip;
> + int ret;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(io_bases); i++) {
> + detected_chip = nct6795d_led_detect(io_bases[i]);
> + if (detected_chip >= 0)
> + break;
> + }

Are you sure this won't cause problems somewhere? Could compatible
mainboards be detected using DMI or something like that?


> + if (i == ARRAY_SIZE(io_bases)) {
> + pr_err(KBUILD_MODNAME ": no supported chip detected\n");
> + return -ENXIO;
> + }

I don't think ENXIO is normally used like this. -ENODEV? You have this
elsewhere, too.

> +
> + pr_info(KBUILD_MODNAME ": found %s chip at address 0x%x\n",
> + chip_names[detected_chip], io_bases[i]);
> +
> + ret = platform_driver_register(&nct6795d_led_driver);
> + if (ret)
> + return ret;
> +
> + nct6795d_led_pdev =
> + platform_device_alloc(NCT6795D_DEVICE_NAME "_led", 0);
> + if (!nct6795d_led_pdev) {
> + ret = -ENOMEM;
> + goto error_pdev_alloc;
> + }

Are you sure you are using platform devices in reasonable way? You
probe, first, then register. That's highly unusual.

Best regards,
Pavel

--
http://www.livejournal.com/~pavelmachek
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-12-28 23:08    [W:1.888 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site