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

[Lee Jones <lee@kernel.org>]

On Wed, 04 Nov 2026, Svyatoslav Ryhel wrote:

> ср, 4 лист. 2026 р. о 10:28 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.
> >
> 
> Ooops, v7 https://lore.kernel.org/lkml/20260527144619.GA671544@google.com/
> 
> > > Yet again complains and nits
> >
> > That my job.  You're welcome.  =;-)
> >
> > Sometimes new things are found on re-review.
> >
> > Apologies for the disappointment.
> >
> 
> I will incorporate all your requests, but please, may you apply/check
> v13 a bit quicker. That would be awesome.

Sure.  =:-)

-- 
Lee Jones