Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20211212054706.80343-1-gnurou@gmail.com> <20211228220727.GA17003@duo.ucw.cz>
In-Reply-To: <20211228220727.GA17003@duo.ucw.cz>
From:   Alexandre Courbot <gnurou@gmail.com>
Date:   Mon, 14 Feb 2022 11:23:53 +0900
Message-ID: <CAAVeFuJm1zO_Nfozj7uVXKVSNa2sduH5zvoFMP_ERnMLc1DzCw@mail.gmail.com>
Subject: Re: [PATCH v2] leds: add NCT6795D driver
To:     Pavel Machek <pavel@ucw.cz>, Guenter Roeck <linux@roeck-us.net>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-leds@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Pavel, (+Guenter for comments about NCT6775 as he maintains the hwmon driver)

On Wed, Dec 29, 2021 at 7:07 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> 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.

Sure. Should I create a sub-menu for multicolor leds in the Kconfig as well?

>
> > +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?

The description of the other LED modules mention the module name
without the .ko suffix so I did the same for consistency, but let me
know if you prefer to have it anyway.

>
> > 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?

Removed the 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.

Removed 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.

Ack, converted this to a regular int.

>
> > +     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...

Indeed.

>
> > +/*
> > + * 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.

This is just what other superio-based drivers do as the bus can be
shared between several drivers, each controlling a different function
(see for instance gpio-f7188x or the hwmon NCT6775 driver).
request_muxed_region() is used as a way to make sure they do not get
in the way of one another - they all acquire the region for a short
time and release it as soon as they are done. I agree it would be
better if we could arbitrate access in a more centralized way, but
that goes beyond the scope of this patch. There are mentions of moving
the superio functions to a separate file in the hwmon driver though,
so maybe this will happen at some point?

>
> > +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?

I looked at the output of dmidecode and could not find anything
relevant to help probing this unfortunately. Again other superio-based
drivers are doing the same thing for detecting the presence of the
chip.

>
>
> > +     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.

Switched these to -ENODEV.

>
> > +
> > +     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.

Agreed, but that's also what the hwmon and f7188 GPIO drivers do. The
more I repeat this, the more it sounds like we should wait until there
is a more centralized way to manage the superio bus before thinking
about merging this. :) Let's see what Guenter thinks.

Cheers,
Alex.