[PATCH v2] leds: Add driver for MAX8907 LED cell

[PATCH v2] leds: Add driver for MAX8907 LED cell

[Svyatoslav Ryhel]

Add a small driver to control the LED cell provided by the MAX8907 PMIC.
The LED does not require any specific configuration and is folded into the
PMIC's node.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
Changes in v2:
- rebased so patch is now applicable to linux/next
---
 drivers/leds/Kconfig        |  7 ++++
 drivers/leds/Makefile       |  1 +
 drivers/leds/leds-max8907.c | 81 +++++++++++++++++++++++++++++++++++++
 drivers/mfd/max8907.c       |  1 +
 include/linux/mfd/max8907.h |  2 +-
 5 files changed, 91 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-max8907.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 788e689fe875c..3796c562919fe 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -813,6 +813,13 @@ config LEDS_MAX77705
 	help
 	  LED driver for MAX77705 PMIC from Maxim Integrated.
 
+config LEDS_MAX8907
+	tristate "LED support for MAX8907 PMIC"
+	depends on LEDS_CLASS && MFD_MAX8907
+	help
+	  This option enables support for on-chip LED drivers on
+	  MAXIM MAX8907 PMIC.
+
 config LEDS_MAX8997
 	tristate "LED support for MAX8997 PMIC"
 	depends on LEDS_CLASS && MFD_MAX8997
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3da746279bb81..24efda3fbaa44 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_LTC3220)		+= leds-ltc3220.o
 obj-$(CONFIG_LEDS_MAX5970)		+= leds-max5970.o
 obj-$(CONFIG_LEDS_MAX77650)		+= leds-max77650.o
 obj-$(CONFIG_LEDS_MAX77705)		+= leds-max77705.o
+obj-$(CONFIG_LEDS_MAX8907)		+= leds-max8907.o
 obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
 obj-$(CONFIG_LEDS_MC13783)		+= leds-mc13783.o
 obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
diff --git a/drivers/leds/leds-max8907.c b/drivers/leds/leds-max8907.c
new file mode 100644
index 0000000000000..358505a1c799f
--- /dev/null
+++ b/drivers/leds/leds-max8907.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/err.h>
+#include <linux/leds.h>
+#include <linux/mfd/max8907.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct max8907_led_data {
+	struct max8907 *max8907;
+	struct regmap *regmap;
+	struct led_classdev cdev;
+};
+
+static enum led_brightness max8907_led_get_brightness(struct led_classdev *cdev)
+{
+	struct max8907_led_data *ddata = container_of(cdev, struct max8907_led_data, cdev);
+	unsigned int brightness_lvl;
+	int ret;
+
+	ret = regmap_read(ddata->regmap, MAX8907_REG_WLED_CNTL, &brightness_lvl);
+	if (ret)
+		return LED_OFF;
+
+	return brightness_lvl;
+}
+
+static int max8907_led_set_brightness(struct led_classdev *cdev,
+				      enum led_brightness brightness)
+{
+	struct max8907_led_data *ddata = container_of(cdev, struct max8907_led_data, cdev);
+	int brightness_lvl, ret;
+
+	brightness_lvl = clamp(brightness, LED_OFF, LED_FULL);
+
+	if (brightness_lvl) {
+		ret = regmap_write(ddata->regmap, MAX8907_REG_WLED_CNTL, brightness_lvl);
+		if (ret)
+			return ret;
+	}
+
+	return regmap_update_bits(ddata->regmap, MAX8907_REG_WLED_MODE_CNTL,
+				  LED_ON, brightness_lvl ? LED_ON : LED_OFF);
+}
+
+static int max8907_led_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct max8907 *max8907 = dev_get_drvdata(dev->parent);
+	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);
+
+	ddata->cdev.brightness = LED_OFF;
+	ddata->cdev.flags = LED_CORE_SUSPENDRESUME;
+	ddata->cdev.max_brightness = LED_FULL;
+	ddata->cdev.name = dev_name(dev);
+
+	ddata->cdev.brightness_get = max8907_led_get_brightness;
+	ddata->cdev.brightness_set_blocking = max8907_led_set_brightness;
+
+	return devm_led_classdev_register(dev->parent, &ddata->cdev);
+}
+
+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");
diff --git a/drivers/mfd/max8907.c b/drivers/mfd/max8907.c
index 6d5e6a1556fd5..9f737ad7eb398 100644
--- a/drivers/mfd/max8907.c
+++ b/drivers/mfd/max8907.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 
 static const struct mfd_cell max8907_cells[] = {
+	{ .name = "max8907-leds", },
 	{ .name = "max8907-regulator", },
 	{ .name = "max8907-rtc", },
 };
diff --git a/include/linux/mfd/max8907.h b/include/linux/mfd/max8907.h
index e222250f0cc1e..35b2ede54f856 100644
--- a/include/linux/mfd/max8907.h
+++ b/include/linux/mfd/max8907.h
@@ -112,7 +112,7 @@
 #define MAX8907_REG_CHG_IRQ2_MASK	0x81
 #define MAX8907_REG_CHG_STAT		0x82
 #define MAX8907_REG_WLED_MODE_CNTL	0x84
-#define MAX8907_REG_ILED_CNTL		0x84
+#define MAX8907_REG_WLED_CNTL		0x85
 #define MAX8907_REG_II1RR		0x8E
 #define MAX8907_REG_II2RR		0x8F
 #define MAX8907_REG_LDOCTL20		0x9C
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] leds: use-after-free on driver unbind due to mismatched devm lifecycles
- [Medium] leds: software enum LED_ON is used as a hardware register bitmask
- [Medium] leds: stale non-zero brightness returned when LED is disabled
- [Low] leds: hardcoded device name prevents standard DT property configuration
- [Low] leds: commit message prefix missing driver name
--

commit 18d3fac6772516707b8183897b99b767fe8a81f1
Author: Svyatoslav Ryhel <clamor95@gmail.com>

leds: Add driver for MAX8907 LED cell

Add a small driver to control the LED cell provided by the MAX8907 PMIC.
The LED does not require any specific configuration and is folded into the
PMIC's node.

> leds: Add driver for MAX8907 LED cell

[Severity: Low]
This isn't a bug, but does the commit message prefix follow the standard
subsystem guidelines? The LED subsystem conventions typically expect the
driver name in the prefix, such as "leds: max8907: Add driver ...".

> diff --git a/drivers/leds/leds-max8907.c b/drivers/leds/leds-max8907.c
> new file mode 100644
> index 0000000000000..358505a1c799f
> --- /dev/null
> +++ b/drivers/leds/leds-max8907.c

[ ... ]

> +static enum led_brightness max8907_led_get_brightness(struct led_classdev *cdev)
> +{
> +	struct max8907_led_data *ddata = container_of(cdev, struct max8907_led_data, cdev);
> +	unsigned int brightness_lvl;
> +	int ret;
> +
> +	ret = regmap_read(ddata->regmap, MAX8907_REG_WLED_CNTL, &brightness_lvl);
> +	if (ret)
> +		return LED_OFF;
> +
> +	return brightness_lvl;
> +}

[Severity: Medium]
Will this return a stale non-zero value if the LED was previously turned off?

If max8907_led_set_brightness() is called with LED_OFF, it clears the
enable bit in MAX8907_REG_WLED_MODE_CNTL but skips updating the
MAX8907_REG_WLED_CNTL register. Since max8907_led_get_brightness() does not
verify the mode control enable state, it seems it will incorrectly return the
old brightness value even when the LED is disabled.

[ ... ]

> +static int max8907_led_set_brightness(struct led_classdev *cdev,
> +				      enum led_brightness brightness)
> +{
> +	struct max8907_led_data *ddata = container_of(cdev, struct max8907_led_data, cdev);
> +	int brightness_lvl, ret;
> +
> +	brightness_lvl = clamp(brightness, LED_OFF, LED_FULL);
> +
> +	if (brightness_lvl) {
> +		ret = regmap_write(ddata->regmap, MAX8907_REG_WLED_CNTL, brightness_lvl);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_update_bits(ddata->regmap, MAX8907_REG_WLED_MODE_CNTL,
> +				  LED_ON, brightness_lvl ? LED_ON : LED_OFF);
> +}

[Severity: Medium]
Is the software enum LED_ON intentionally used as a hardware register bitmask
here?

LED_ON is defined in the LED subsystem as 1. Using it as the mask argument
for regmap_update_bits() on MAX8907_REG_WLED_MODE_CNTL mixes a software
state enum with hardware layout. If the PMIC's enable bit changes or differs
from BIT(0), this could fail to enable the LED and potentially corrupt other
configuration bits.

[ ... ]

> +static int max8907_led_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct max8907 *max8907 = dev_get_drvdata(dev->parent);
> +	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);
> +
> +	ddata->cdev.brightness = LED_OFF;
> +	ddata->cdev.flags = LED_CORE_SUSPENDRESUME;
> +	ddata->cdev.max_brightness = LED_FULL;
> +	ddata->cdev.name = dev_name(dev);

[Severity: Low]
Does hardcoding the device name prevent standard Device Tree property
configuration?

By directly assigning dev_name(dev), it bypasses parsing standard DT
properties like color and function, which means board designers cannot
configure standard sysfs directory names for this LED.

> +
> +	ddata->cdev.brightness_get = max8907_led_get_brightness;
> +	ddata->cdev.brightness_set_blocking = max8907_led_set_brightness;
> +
> +	return devm_led_classdev_register(dev->parent, &ddata->cdev);
> +}

[Severity: High]
Can this lead to a use-after-free during driver unbind?

The private data ddata is allocated using devm_kzalloc(dev, ...) tying
its lifecycle to the child platform device. However, the LED classdev is
registered using devm_led_classdev_register(dev->parent, &ddata->cdev),
tying the unregistration action to the parent MFD core device.

If the child platform driver is unbound or removed, ddata will be freed,
but the LED classdev unregistration won't trigger because it's attached
to the parent device. Any subsequent access via sysfs or subsystem triggers
could dereference the freed memory.

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