Re: [PATCH v12 5/7] leds: Add driver for ASUS Transformer LEDs

[Svyatoslav Ryhel]

вт, 3 лист. 2026 р. о 16:03 Lee Jones <lee@xxxxxxxxxx> пише:
>
> On Mon, 19 Oct 2026, Svyatoslav Ryhel wrote:
> > From: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>
> >
> > ASUS Transformer tablets have a green and an amber LED on both the Pad
> > and the Dock. If both LEDs are enabled simultaneously, the emitted light
> > will be yellow.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > Signed-off-by: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>
> > ---
> >  drivers/leds/Kconfig                    |  11 +++
> >  drivers/leds/Makefile                   |   1 +
> >  drivers/leds/leds-asus-transformer-ec.c | 125 ++++++++++++++++++++++++
> >  3 files changed, 137 insertions(+)
> >  create mode 100644 drivers/leds/leds-asus-transformer-ec.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 7ec2c8d785426..788e689fe875c 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -120,6 +120,17 @@ config LEDS_OSRAM_AMS_AS3668
> >         To compile this driver as a module, choose M here: the module
> >         will be called leds-as3668.
> >
> > +config LEDS_ASUS_TRANSFORMER_EC
> > +     tristate "LED Support for Asus Transformer charging LED"
> > +     depends on LEDS_CLASS
> > +     depends on MFD_ASUS_TRANSFORMER_EC
> > +     help
> > +       This option enables support for charging indicator on
> > +       Asus Transformer's Pad and its Dock.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called leds-asus-transformer-ec.
> > +
> >  config LEDS_AW200XX
> >       tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
> >       depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 4d4b089156e24..3da746279bb81 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_ADP5520)          += leds-adp5520.o
> >  obj-$(CONFIG_LEDS_AN30259A)          += leds-an30259a.o
> >  obj-$(CONFIG_LEDS_APU)                       += leds-apu.o
> >  obj-$(CONFIG_LEDS_ARIEL)             += leds-ariel.o
> > +obj-$(CONFIG_LEDS_ASUS_TRANSFORMER_EC)       += leds-asus-transformer-ec.o
> >  obj-$(CONFIG_LEDS_OSRAM_AMS_AS3668)  += leds-as3668.o
> >  obj-$(CONFIG_LEDS_AW200XX)           += leds-aw200xx.o
> >  obj-$(CONFIG_LEDS_AW2013)            += leds-aw2013.o
> > diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> > new file mode 100644
> > index 0000000000000..1b129247ea6c6
> > --- /dev/null
> > +++ b/drivers/leds/leds-asus-transformer-ec.c
> > @@ -0,0 +1,125 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include <linux/err.h>
> > +#include <linux/leds.h>
> > +#include <linux/mfd/asus-transformer-ec.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +enum {
> > +     ASUSEC_LED_AMBER,
> > +     ASUSEC_LED_GREEN,
> > +     ASUSEC_LED_MAX
> > +};
> > +
> > +struct asus_ec_led_config {
> > +     const char *name;
> > +     unsigned int color;
> > +     u64 ctrl_bit;
> > +};
> > +
> > +struct asus_ec_led {
> > +     struct asus_ec_leds_data *ddata;
>
>
> Why do we need 'struct ' at all?
>
> If we put 'const struct asusec_core *ec;' directly into 'struct
> asus_ec_led', we can drop the outer wrapper struct and the back-pointer
> completely.
>

You propose to allocate asus_ec_led for each led in a loop? I have not
seen this done by other leds. The asus_ec_leds_data is driver ddata,
asus_ec_led is each led individual structure.

> > +     struct led_classdev cdev;
> > +     u64 ctrl_bit;
> > +};
> > +
> > +struct asus_ec_leds_data {
> > +     const struct asusec_core *ec;
> > +     struct asus_ec_led leds[ASUSEC_LED_MAX];
> > +};
> > +
> > +static const struct asus_ec_led_config asus_ec_leds[] = {
> > +     [ASUSEC_LED_AMBER] = {
> > +             .name = "amber",
> > +             .color = LED_COLOR_ID_AMBER,
> > +             .ctrl_bit = ASUSEC_CTL_LED_AMBER,
> > +     },
> > +     [ASUSEC_LED_GREEN] = {
> > +             .name = "green",
> > +             .color = LED_COLOR_ID_GREEN,
> > +             .ctrl_bit = ASUSEC_CTL_LED_GREEN,
> > +     },
> > +};
> > +
> > +static enum led_brightness asus_ec_led_get_brightness(struct led_classdev *cdev)
> > +{
> > +     struct asus_ec_led *led = container_of(cdev, struct asus_ec_led, cdev);
> > +     const struct asusec_core *ec = led->ddata->ec;
> > +     u64 ctl;
> > +     int ret;
> > +
> > +     ret = asus_dockram_access_ctl(ec->dockram, &ctl, 0, 0);
>
> As requested on v6 and v8, why are we still using this bespoke accessor instead
> of Regmap?
>

https://lore.kernel.org/lkml/CAPVz0n0imF_ESoGMJSrEX7NOnBShFj8wehDtQKXZV_DMcGqq=A@xxxxxxxxxxxxxx/

> > +     if (ret)
> > +             return LED_OFF;
> > +
> > +     return ctl & led->ctrl_bit ? LED_ON : LED_OFF;
> > +}
> > +
> > +static int asus_ec_led_set_brightness(struct led_classdev *cdev,
> > +                                   enum led_brightness brightness)
> > +{
> > +     struct asus_ec_led *led = container_of(cdev, struct asus_ec_led, cdev);
> > +     const struct asusec_core *ec = led->ddata->ec;
> > +
> > +     if (brightness)
> > +             return asus_dockram_access_ctl(ec->dockram, NULL,
> > +                                            led->ctrl_bit, led->ctrl_bit);
>
> Use 100-chars throughout to prevent early wrapping.
>
> > +     return asus_dockram_access_ctl(ec->dockram, NULL, led->ctrl_bit, 0);
> > +}
> > +
> > +static int asus_ec_led_probe(struct platform_device *pdev)
> > +{
> > +     const struct asusec_core *ec = dev_get_drvdata(pdev->dev.parent);
> > +     struct asus_ec_leds_data *ddata;
> > +     struct device *dev = &pdev->dev;
> > +     int ret;
> > +
> > +     ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > +     if (!ddata)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, ddata);
>
> What consumes this?
>

Seems to be an overlook, must have forgotten to remove it during 12
iterations over last 10 month

> > +     ddata->ec = ec;
> > +
> > +     for (int i = 0; i < ASUSEC_LED_MAX; i++) {
>
> `i` isn't just a random iterator here - maybe call it `led`?
>

This is just a disposable iterator.

> > +             const struct asus_ec_led_config *cfg = &asus_ec_leds[i];
> > +             struct asus_ec_led *led = &ddata->leds[i];
> > +
> > +             led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s",
> > +                                             ddata->ec->name, cfg->name);
> > +             if (!led->cdev.name)
> > +                     return -ENOMEM;
> > +
> > +             led->cdev.max_brightness = 1;
> > +             led->cdev.color = cfg->color;
> > +             led->cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> > +             led->cdev.brightness_get = asus_ec_led_get_brightness;
> > +             led->cdev.brightness_set_blocking = asus_ec_led_set_brightness;
> > +
> > +             led->ddata = ddata;
> > +             led->ctrl_bit = cfg->ctrl_bit;
> > +
> > +             ret = devm_led_classdev_register(dev, &led->cdev);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "Failed to register %s LED\n",
> > +                                          cfg->name);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static struct platform_driver asus_ec_led_driver = {
> > +     .driver.name = "asus-transformer-ec-led",
>
> Clever!  But, I'd prefer we still to the usual format to be honest.
>
> > +     .probe = asus_ec_led_probe,
> > +};
> > +module_platform_driver(asus_ec_led_driver);
> > +
> > +MODULE_ALIAS("platform:asus-transformer-ec-led");
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@xxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("ASUS Transformer's charging LED driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.53.0
> >
>
> --
> Lee Jones

v7 seem fine https://lore.kernel.org/lkml/20260702162425.GD2108533@xxxxxxxxxx/
v9 as well https://lore.kernel.org/lkml/20260702162425.GD2108533@xxxxxxxxxx/

Yet again complains and nits