Re: [PATCH v2 7/9] leds: Add driver for Asus Transformer LEDs

[Svyatoslav Ryhel]

пт, 6 бер. 2026 р. о 12:04 Lee Jones <lee@xxxxxxxxxx> пише:
>
> On Mon, 09 Feb 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-ec.c | 104 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 116 insertions(+)
> >  create mode 100644 drivers/leds/leds-asus-ec.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 597d7a79c988..96dab210f6ca 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_ASUSEC
> > +     tristate "LED Support for Asus Transformer charging LED"
> > +     depends on LEDS_CLASS
> > +     depends on MFD_ASUSEC
> > +     help
> > +       This option enables support for charging indicator on
> > +       Asus Transformer's Pad and it's Dock.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called leds-asus-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 8fdb45d5b439..1117304dfdf4 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_AN30259A)         += leds-an30259a.o
> >  obj-$(CONFIG_LEDS_APU)                       += leds-apu.o
> >  obj-$(CONFIG_LEDS_ARIEL)             += leds-ariel.o
> >  obj-$(CONFIG_LEDS_AS3668)            += leds-as3668.o
> > +obj-$(CONFIG_LEDS_ASUSEC)            += leds-asus-ec.o
> >  obj-$(CONFIG_LEDS_AW200XX)           += leds-aw200xx.o
> >  obj-$(CONFIG_LEDS_AW2013)            += leds-aw2013.o
> >  obj-$(CONFIG_LEDS_BCM6328)           += leds-bcm6328.o
> > diff --git a/drivers/leds/leds-asus-ec.c b/drivers/leds/leds-asus-ec.c
> > new file mode 100644
> > index 000000000000..5dd76c9247ee
> > --- /dev/null
> > +++ b/drivers/leds/leds-asus-ec.c
> > @@ -0,0 +1,104 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ASUS EC driver - battery LED
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/leds.h>
> > +#include <linux/mfd/asus-ec.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +/*
> > + * F[5] & 0x07
> > + *  auto: brightness == 0
> > + *  bit 0: blink / charger on
> > + *  bit 1: amber on
> > + *  bit 2: green on
> > + */
> > +
> > +#define ASUSEC_CTL_LED_BLINK         BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_AMBER         BIT_ULL(41)
> > +#define ASUSEC_CTL_LED_GREEN         BIT_ULL(42)
> > +
> > +static void asus_ec_led_set_brightness_amber(struct led_classdev *led,
> > +                                          enum led_brightness brightness)
> > +{
> > +     const struct asusec_info *ec = dev_get_drvdata(led->dev->parent);
> > +
> > +     if (brightness)
> > +             asus_ec_set_ctl_bits(ec, ASUSEC_CTL_LED_AMBER);
> > +     else
> > +             asus_ec_clear_ctl_bits(ec, ASUSEC_CTL_LED_AMBER);
> > +}
> > +
> > +static void asus_ec_led_set_brightness_green(struct led_classdev *led,
> > +                                          enum led_brightness brightness)
> > +{
> > +     const struct asusec_info *ec = dev_get_drvdata(led->dev->parent);
> > +
> > +     if (brightness)
> > +             asus_ec_set_ctl_bits(ec, ASUSEC_CTL_LED_GREEN);
> > +     else
> > +             asus_ec_clear_ctl_bits(ec, ASUSEC_CTL_LED_GREEN);
> > +}
> > +
> > +static int asus_ec_led_probe(struct platform_device *pdev)
> > +{
> > +     struct asusec_info *ec = cell_to_ec(pdev);
>
> Please remove all of your abstraction layers.  They serve little purpose
> other than to complicate things.  Just use dev_get_drvdata() here.
>
> Remove the "_info" part and change "ec" to "ddata".
>

Controller exposes only required stuff, exposing controllers internal
parts is not desired. Is there any written convention that driver
private data pointer MUST be named "ddata"? "priv" is pretty well
fitting to. You are just nitpicking.

> > +     struct device *dev = &pdev->dev;
> > +     struct led_classdev *amber_led, *green_led;
> > +     int ret;
> > +
> > +     platform_set_drvdata(pdev, ec);
>
> Wait, what?
>
> Why are you doing that?
>

This is redundant, yes.

> > +     amber_led = devm_kzalloc(dev, sizeof(*amber_led), GFP_KERNEL);
> > +     if (!amber_led)
> > +             return -ENOMEM;
> > +
> > +     amber_led->name = devm_kasprintf(dev, GFP_KERNEL, "%s::amber", ec->name);
> > +     amber_led->max_brightness = 1;
> > +     amber_led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> > +     amber_led->brightness_set = asus_ec_led_set_brightness_amber;
> > +
> > +     ret = devm_led_classdev_register(dev, amber_led);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to register amber LED\n");
> > +
> > +     green_led = devm_kzalloc(dev, sizeof(*green_led), GFP_KERNEL);
> > +     if (!green_led)
> > +             return -ENOMEM;
> > +
> > +     green_led->name = devm_kasprintf(dev, GFP_KERNEL, "%s::green", ec->name);
> > +     green_led->max_brightness = 1;
> > +     green_led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> > +     green_led->brightness_set = asus_ec_led_set_brightness_green;
> > +
> > +     ret = devm_led_classdev_register(dev, green_led);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to register green LED\n");
>
> Lots of repetition here.
>
> I'd make a sub-function that takes the differences.
>
> Same with the set brightness functions.
>
> Think to yourself - what if I had to support 16 different LEDs?
>

That is not of my concern what you would do. I have 2 LEDs and I see
no point in "abstraction for he sake of abstraction".

> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id asus_ec_led_match[] = {
> > +     { .compatible = "asus,ec-led" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, asus_ec_led_match);
> > +
> > +static struct platform_driver asus_ec_led_driver = {
> > +     .driver = {
> > +             .name = "asus-ec-led",
> > +             .of_match_table = asus_ec_led_match,
> > +     },
> > +     .probe = asus_ec_led_probe,
> > +};
> > +module_platform_driver(asus_ec_led_driver);
> > +
> > +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.51.0
> >
> >
>
> --
> Lee Jones [李琼斯]