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

From: Svyatoslav Ryhel

Date: Thu Nov 05 2026 - 09:32:56 EST

чт, 5 лист. 2026 р. о 16:08 Lee Jones <lee@xxxxxxxxxx> пише:
>
> On Wed, 04 Nov 2026, Svyatoslav Ryhel wrote:
>
> > ср, 4 лист. 2026 р. о 17:16 Lee Jones <lee@xxxxxxxxxx> пише:
> > >
> > > --- 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@xxxxxxxxx>
> > > > ---
> > > > 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()'?
> > >
> >
> > This is required to be able to reference led in the device tree. Since
> > led node is folded into pmic's node, pmic's node is used to bind led.
>
> Right, so you need device_set_node() called in led_classdev_register_ext()
> so fwnode_led_get() can look up the LED via the parent PMIC phandle.
>
> However, passing only 'init_data.fwnode = dev_fwnode(dev->parent)' into
> led_compose_name() when the parent PMIC node has no 'label', 'function'
> or 'color' properties causes it to fall through to:
>
> } else if (is_of_node(fwnode)) {
> n = snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s",
> to_of_node(fwnode)->name);
>
> Which registers the LED in sysfs as '/sys/class/leds/pmic' (and fails
> with -EINVAL if 'fwnode' is NULL on non-DT platforms).
>
> To bind the parent fwnode while avoiding the node-name fallback, please
> also populate 'devicename' and 'default_label' (similar to
> drivers/leds/leds-tps6105x.c):
>
> init_data.fwnode = dev_fwnode(dev->parent);
> init_data.devicename = "max8907";
> init_data.default_label = ":wled"; /* or "white:backlight" */
>

Thank you for your suggestion. I was tinkering a bit and got to
conclusion that adding dedicated LED node will be beneficial.

My idea is to add led node without compatible (since it is redundant,
pmic has only a single led anyway).

Driver will check for this node and return -ENODEV if node is not present.

Then I will use devm_led_classdev_register_ext but will pass led node
and then most of your concerns should be addressed. What do you think
about this?

As a prerequisite, this change would require
https://lore.kernel.org/lkml/20261027114634.79706-1-clamor95@xxxxxxxxx/

I have answered your concerns regarding Sashiko finding.

> --
> Lee Jones