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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect use of `LED_CORE_SUSPENDRESUME` for charging indicators.
- [Medium] Lack of hardware state synchronization (`brightness_get`) at initialization.
- [Medium] Missing `MODULE_ALIAS` prevents automatic module loading.
- [Low] The driver exposes a physical bi-color LED as two independent single-color LED devices instead of utilizing the LED Multicolor class.
--

commit f02de4369b03c2d6785dea5fb4edfab53036130d
Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>

leds: Add driver for ASUS Transformer LEDs

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.

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

Since this describes a physically multi-color LED, shouldn't the driver use
the LED Multicolor class API (CONFIG_LEDS_CLASS_MULTICOLOR) instead of exposing
them as two separate single-color LED devices?

> diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> new file mode 100644
> index 0000000000000..c9bdc46c02e15
> --- /dev/null
> +++ b/drivers/leds/leds-asus-transformer-ec.c
[ ... ]
> +	for (i = 0; i < ASUSEC_LED_MAX; i++) {
> +		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;

Does LED_CORE_SUSPENDRESUME make sense for a charging indicator?

This flag instructs the LED core to explicitly turn off the LED during suspend,
which turns off the charging indicator while the device is sleeping.

Also, could the Embedded Controller have already turned the LED on before the
driver probes?

Since there is no brightness_get callback to synchronize the initial hardware
state, the LED core will initialize the software brightness to 0. If the device
resumes from suspend, the LED core will restore this saved brightness of 0,
permanently turning off the LED even if it was physically on before.

[ ... ]
> +static struct platform_driver asus_ec_led_driver = {
> +	.driver.name = "asus-transformer-ec-led",
> +	.probe = asus_ec_led_probe,
> +};
> +module_platform_driver(asus_ec_led_driver);
> +
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's charging LED driver");
> +MODULE_LICENSE("GPL");

Will this module load automatically when the platform device is registered?

Since there is no MODULE_ALIAS("platform:asus-transformer-ec-led") declaration,
udev might not be able to automatically load the module during boot when the
MFD core registers the corresponding platform device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518095907.36158-1-clamor95@gmail.com?part=5