Re: [PATCH v3] leds: Add driver for MAX8907 LED cell

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] leds: max8907: missing MODULE_ALIAS() prevents module autoloading
- [Medium] leds: max8907: invalid fwnode and missing default_label in max8907_led_probe()
- [Low] leds: max8907: unused max8907 field and platform_set_drvdata() in probe
--

commit 585d0f44a3d2f49e9a4916f8fc771037b7486e53
Author: Svyatoslav Ryhel <clamor95@gmail.com>

leds: Add driver for MAX8907 LED cell

This commit adds a platform driver for the LED cell on the MAX8907 PMIC and
registers the max8907-leds cell in the parent MFD driver.

> diff --git a/drivers/leds/leds-max8907.c b/drivers/leds/leds-max8907.c
> new file mode 100644
> index 0000000000000..c3bd131800a8c
> --- /dev/null
> +++ b/drivers/leds/leds-max8907.c
> @@ -0,0 +1,92 @@
[ ... ]
> +struct max8907_led_data {
> +	struct max8907 *max8907;
> +	struct regmap *regmap;
> +	struct led_classdev cdev;
> +};
[ ... ]
> +static int max8907_led_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct max8907 *max8907 = dev_get_drvdata(dev->parent);
> +	struct led_init_data init_data = { };
> +	struct max8907_led_data *ddata;
> +
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ddata->max8907 = max8907;
> +	ddata->regmap = max8907->regmap_gen;
> +
> +	platform_set_drvdata(pdev, ddata);

[Severity: Low]
Are ddata->max8907 and the platform_set_drvdata(pdev, ddata) call needed
here?

Neither ddata->max8907 nor the platform device driver data appears to be read
anywhere after max8907_led_probe(), since register accesses go through
ddata->regmap and the LED callbacks retrieve ddata via container_of() on
ddata->cdev.

> +
> +	init_data.fwnode = dev_fwnode(dev->parent);
> +	init_data.devicename = dev_name(dev);
> +
> +	ddata->cdev.brightness = LED_OFF;
> +	ddata->cdev.flags = LED_CORE_SUSPENDRESUME;
> +	ddata->cdev.max_brightness = LED_FULL;
> +
> +	ddata->cdev.brightness_get = max8907_led_get_brightness;
> +	ddata->cdev.brightness_set_blocking = max8907_led_set_brightness;
> +
> +	return devm_led_classdev_register_ext(dev, &ddata->cdev, &init_data);
> +}

[Severity: Medium]
What happens in max8907_led_probe() -> devm_led_classdev_register_ext() ->
led_classdev_register_ext() -> led_compose_name() when init_data.fwnode is
set to dev_fwnode(dev->parent) and init_data.default_label is left NULL?

In drivers/leds/led-core.c:led_compose_name(), when no LED properties are
present on the fwnode:

	} else if (init_data->default_label) {
		if (!devicename) {
			dev_err(dev, "Legacy LED naming requires devicename segment");
			return -EINVAL;
		}
		n = snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
			     devicename, init_data->default_label);
	} else if (is_of_node(fwnode)) {
		n = snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s",
			     to_of_node(fwnode)->name);
	} else if (is_software_node(fwnode)) {
		n = snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s",
			     fwnode_get_name(fwnode));
	} else {
		return -EINVAL;
	}

On non-DT platforms using struct max8907_platform_data,
dev_fwnode(dev->parent) is NULL, and because init_data.default_label is also
NULL, led_compose_name() reaches the final else branch and fails with
-EINVAL.

On Device Tree platforms where the parent maxim,max8907 PMIC node does not
define LED properties (such as color, function, or label), led_compose_name()
falls through to the is_of_node(fwnode) branch and names the LED after
to_of_node(fwnode)->name (the parent MFD node name, such as pmic) while
ignoring init_data.devicename and binding the LED class device directly to
the parent MFD fwnode.

Should max8907_led_probe() set init_data.default_label (or ddata->cdev.name)
instead of passing the parent MFD fwnode without a default_label?

> +
> +static struct platform_driver max8907_led_driver = {
> +	.driver.name = "max8907-leds",
> +	.probe = max8907_led_probe,
> +};
> +module_platform_driver(max8907_led_driver);
> +
> +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("MAX8907 LED driver");
> +MODULE_LICENSE("GPL");

[Severity: Medium]
When CONFIG_LEDS_MAX8907=m, will this module autoload when mfd_add_devices()
registers the max8907-leds cell from max8907_cells[] in
drivers/mfd/max8907.c?

Because max8907_cells[] instantiates the device by name without an
of_compatible string, the platform bus emits MODALIAS=platform:max8907-leds.
Without MODULE_ALIAS("platform:max8907-leds") or a platform_device_id table
with MODULE_DEVICE_TABLE(platform, ...), leds-max8907.ko will have no
modalias entry for udev/kmod to match.

Also, there is a typo in MODULE_AUTHOR ("Svyatolsav").

[ ... ]

> diff --git a/drivers/mfd/max8907.c b/drivers/mfd/max8907.c
> index 7bac1d6517717..0f25aedb32df1 100644
> --- a/drivers/mfd/max8907.c
> +++ b/drivers/mfd/max8907.c
> @@ -19,6 +19,7 @@
>  #include <linux/slab.h>
>  
>  static const struct mfd_cell max8907_cells[] = {
> +	{ .name = "max8907-leds", },
>  	{ .name = "max8907-regulator", },
>  	{ .name = "max8907-rtc", },
>  };
[ ... ]

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