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

[Lee Jones <lee@kernel.org>]

--- checkpatch.pl (1 issue) ---
Line 38: WARNING: please write a help paragraph that fully describes the config symbol with at least 4 lines
----------------------------------

On Tue, 27 Oct 2026, Svyatoslav Ryhel wrote:

> 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
>
> Changes in v3:
> - added check if led is off for get callback
> - added dedicated MAX8907_REG_WLED_MODE_ON mask
> - switched to devm_led_classdev_register_ext
>
> Changes in v4:
> - platform_set_drvdata dropped
> - init_data.devicename dropped
> - added MODULE_ALIAS
> ---
>  drivers/leds/Kconfig        |  7 +++
>  drivers/leds/Makefile       |  1 +
>  drivers/leds/leds-max8907.c | 90 +++++++++++++++++++++++++++++++++++++
>  drivers/mfd/max8907.c       |  1 +
>  include/linux/mfd/max8907.h |  2 +-
>  5 files changed, 100 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.

As requested on v3, please expand this help text to at least 4 lines to satisfy
'checkpatch.pl'.

> +
>  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..a5c445f75d1b1
> --- /dev/null
> +++ b/drivers/leds/leds-max8907.c
> @@ -0,0 +1,90 @@
> +// 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/property.h>
> +#include <linux/regmap.h>
> +
> +#define MAX8907_REG_WLED_MODE_ON	BIT(0)
> +
> +struct max8907_led_data {
> +	struct max8907 *max8907;

As asked on v3, what is 'max8907' used for?

> +	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_MODE_CNTL, &brightness_lvl);
> +	if (ret || !(brightness_lvl & MAX8907_REG_WLED_MODE_ON))
> +		return LED_OFF;
> +
> +	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,
> +				  MAX8907_REG_WLED_MODE_ON,
> +				  brightness_lvl ? MAX8907_REG_WLED_MODE_ON : 0);
> +}
> +
> +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;
> +
> +	init_data.fwnode = dev_fwnode(dev->parent);

This will evaluate to 'pmic'.  The sysfs attribute will be incorrect.

Since there is no child node, why not set 'ddata->cdev.name' directly
and use 'devm_led_classdev_register()'?

> +
> +	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);
> +}
> +
> +static struct platform_driver max8907_led_driver = {
> +	.driver.name = "max8907-leds",

Cute.  But please expand.

> +	.probe = max8907_led_probe,
> +};
> +module_platform_driver(max8907_led_driver);
> +
> +MODULE_ALIAS("platform:max8907-leds");
> +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");

Your name is misspelt here.

> +MODULE_DESCRIPTION("MAX8907 LED driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/max8907.c b/drivers/mfd/max8907.c
> index e282dd82ece69..826d7768fd8d0 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", },

Separate patch.

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

-- 
Lee Jones