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

[Lee Jones <lee@kernel.org>]

On Tue, 03 Nov 2026, Svyatoslav Ryhel wrote:

> вт, 3 лист. 2026 р. о 16:03 Lee Jones <lee@kernel.org> пише:
> >
> > On Mon, 19 Oct 2026, Svyatoslav Ryhel wrote:
> > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > >
> > > 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@gmail.com>
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > ---
> > >  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.

For your whole 2 LEDs, yes, I think that will be better.

> > > +     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@mail.gmail.com/

Sorry, I've slept since then, and subsequently forgotten this
conversation.  You can drop this request.

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

No problem, it happens.

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

It's a disposable iterator that is clearly a `led`.

Thus my request for and improvement in the nomenclature remains valid.

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

These are the same link.

> Yet again complains and nits

That my job.  You're welcome.  =;-)

Sometimes new things are found on re-review.

Apologies for the disappointment.

-- 
Lee Jones