Re: [PATCH v10 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:
- [Medium] The manually constructed LED sysfs name incorrectly places the color in the function field.
- [Medium] The child driver uses a bespoke custom accessor function exposed by the MFD parent, violating MFD API layering guidelines.
- [Low] The commit message subject prefix does not adhere to the mandatory subsystem prefix format.
- [Low] The Kconfig help text contains a grammatical error, using "it's" instead of "its".
--

--- Patch [5]: [PATCH v10 5/7] leds: Add driver for ASUS Transformer LEDs ---
commit 177b0b82c65e4fc0357f15d45f7c0353ef5cb99a
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.

> leds: Add driver for ASUS Transformer LEDs

[Severity: Low]
This isn't a bug, but does the commit subject prefix follow the LED subsystem
guidelines? The subsystem typically expects the format
leds: <Driver>: <Capitalized description>.

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c8705..f637d23400a89 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 it's Dock.
                                           ^^^^

[Severity: Low]
This isn't a bug, but should "it's" be "its" here?

[ ... ]

> diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> new file mode 100644
> index 0000000000000..4421d629911ed
> --- /dev/null
> +++ b/drivers/leds/leds-asus-transformer-ec.c

[ ... ]

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

[Severity: Medium]
Does using the custom parent accessor asus_dockram_access_ctl() violate
MFD API layering guidelines? 

MFD core drivers should usually expose standard shared resources like regmap
rather than bespoke helper functions for child drivers to use, as it creates
tight coupling between the parent and child.

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

[Severity: Medium]
This is the same custom accessor as above, used again here in
asus_ec_led_set_brightness(). Should this be refactored to use standard
accessors?

[ ... ]

> +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);
> +	ddata->ec = ec;
> +
> +	for (int 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);

[Severity: Medium]
Since cfg->name contains the color (amber or green), does this format string
incorrectly place the color in the function segment of the standard
devicename:color:function naming scheme?

This will generate devicename::color instead of devicename:color:function,
which might confuse userspace LED managers that rely on standard sysfs
directories.

> +		if (!led->cdev.name)
> +			return -ENOMEM;

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