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@xxxxxxxxx> > --- > 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@xxxxxxxxx> > + * > + * 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
Attachment:
signature.asc
Description: PGP signature
