Message-ID: <20211228220727.GA17003@duo.ucw.cz>
Date:   Tue, 28 Dec 2021 23:07:28 +0100
From:   Pavel Machek <pavel@....cz>
To:     Alexandre Courbot <gnurou@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [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@...il.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@...il.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

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)

Powered by blists - more mailing lists