Message-ID: <5bb6c1aa-4462-4459-9198-5a85a7b439b0@gocontroll.com>
Date: Mon, 10 Nov 2025 09:25:57 +0100
From: Maud Spierings <maudspierings@...ontroll.com>
To: Frank Li <Frank.li@....com>
Cc: Lee Jones <lee@...nel.org>, Daniel Thompson <danielt@...nel.org>,
 Jingoo Han <jingoohan1@...il.com>, Pavel Machek <pavel@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Helge Deller <deller@....de>,
 Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>, dri-devel@...ts.freedesktop.org,
 linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 2/4] backlight: add max25014atg backlighty

On 11/7/25 16:51, Frank Li wrote:
> On Fri, Nov 07, 2025 at 01:49:59PM +0100, Maud Spierings via B4 Relay wrote:
>> From: Maud Spierings <maudspierings@...ontroll.com>
>>
>> The Maxim MAX25014 is a 4-channel automotive grade backlight driver IC
>> with integrated boost controller.
>>
>> Signed-off-by: Maud Spierings <maudspierings@...ontroll.com>
>> ---
>>   MAINTAINERS                        |   1 +
>>   drivers/video/backlight/Kconfig    |   7 +
>>   drivers/video/backlight/Makefile   |   1 +
>>   drivers/video/backlight/max25014.c | 409 +++++++++++++++++++++++++++++++++++++
>>   4 files changed, 418 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 606ce086f758..d082d3f8cfae 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15265,6 +15265,7 @@ MAX25014 BACKLIGHT DRIVER
>>   M:	Maud Spierings <maudspierings@...ontroll.com>
>>   S:	Maintained
>>   F:	Documentation/devicetree/bindings/leds/backlight/maxim,max25014.yaml
>> +F:	drivers/video/backlight/max25014.c
>>
>>   MAX31335 RTC DRIVER
>>   M:	Antoniu Miclaus <antoniu.miclaus@...log.com>
>> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
>> index d9374d208cee..d3bb6ccd4185 100644
>> --- a/drivers/video/backlight/Kconfig
>> +++ b/drivers/video/backlight/Kconfig
>> @@ -262,6 +262,13 @@ config BACKLIGHT_DA9052
>>   	help
>>   	  Enable the Backlight Driver for DA9052-BC and DA9053-AA/Bx PMICs.
>>
>> +config BACKLIGHT_MAX25014
>> +	tristate "Backlight driver for the Maxim MAX25014 chip"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	help
>> +	  If you are using a MAX25014 chip as a backlight driver say Y to enable it.
>> +
>>   config BACKLIGHT_MAX8925
>>   	tristate "Backlight driver for MAX8925"
>>   	depends on MFD_MAX8925
>> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
>> index dfbb169bf6ea..1170d9ec40b8 100644
>> --- a/drivers/video/backlight/Makefile
>> +++ b/drivers/video/backlight/Makefile
>> @@ -45,6 +45,7 @@ obj-$(CONFIG_BACKLIGHT_LOCOMO)		+= locomolcd.o
>>   obj-$(CONFIG_BACKLIGHT_LP855X)		+= lp855x_bl.o
>>   obj-$(CONFIG_BACKLIGHT_LP8788)		+= lp8788_bl.o
>>   obj-$(CONFIG_BACKLIGHT_LV5207LP)	+= lv5207lp.o
>> +obj-$(CONFIG_BACKLIGHT_MAX25014)	+= max25014.o
>>   obj-$(CONFIG_BACKLIGHT_MAX8925)		+= max8925_bl.o
>>   obj-$(CONFIG_BACKLIGHT_MP3309C)		+= mp3309c.o
>>   obj-$(CONFIG_BACKLIGHT_MT6370)		+= mt6370-backlight.o
>> diff --git a/drivers/video/backlight/max25014.c b/drivers/video/backlight/max25014.c
>> new file mode 100644
>> index 000000000000..36bae508697e
>> --- /dev/null
> ...
>> +
>> +struct max25014 {
>> +	struct i2c_client *client;
>> +	struct backlight_device *bl;
>> +	struct regmap *regmap;
>> +	struct gpio_desc *enable;
>> +	struct regulator *vin; /* regulator for boost converter Vin rail */
>> +	uint32_t iset;
>> +	uint8_t strings_mask;
>> +};
>> +
>> +static const struct regmap_config max25014_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.max_register = MAX25014_DIAG,
>> +};
>> +
>> +/**
>> + * @brief control the brightness with i2c registers
>> + *
>> + * @param regmap trivial
>> + * @param brt brightness
>> + * @return int
>> + */
>> +static int max25014_register_control(struct regmap *regmap, uint32_t brt)
>> +{
>> +	uint32_t reg = TON_STEP * brt;
>> +	int ret;
>> +	/*
>> +	 * 18 bit number lowest, 2 bits in first register,
>> +	 * next lowest 8 in the L register, next 8 in the H register
>> +	 * Seemingly setting the strength of only one string controls all of
>> +	 * them, individual settings don't affect the outcome.
>> +	 */
>> +
>> +	ret = regmap_write(regmap, MAX25014_TON_1_4_LSB, reg & 0b00000011);
>> +	if (ret != 0)
> 
> if (ret), check others regmap_*()
> 
>> +		return ret;
>> +	ret = regmap_write(regmap, MAX25014_TON1L, (reg >> 2) & 0b11111111);
>> +	if (ret != 0)
>> +		return ret;
>> +	return regmap_write(regmap, MAX25014_TON1H, (reg >> 10) & 0b11111111);
>> +}
>> +
>> +static int max25014_check_errors(struct max25014 *maxim)
>> +{
>> +	uint8_t i;
>> +	int ret;
>> +	uint32_t val;
>> +
>> +	ret = regmap_read(maxim->regmap, MAX25014_OPEN, &val);
>> +	if (ret != 0)
>> +		return ret;
>> +	if (val > 0) {
> 
> uint32 always >= 0
> 
> So
> 	if (val)
> 
>> +		dev_err(&maxim->client->dev, "Open led strings detected on:\n");
>> +		for (i = 0; i < 4; i++) {
>> +			if (val & 1 << i)
>> +				dev_err(&maxim->client->dev, "string %d\n", i + 1);
>> +		}
>> +		return -EIO;
>> +	}
>> +
>> +	ret = regmap_read(maxim->regmap, MAX25014_SHORT_GND, &val);
>> +	if (ret != 0)
>> +		return ret;
>> +	if (val > 0) {
>> +		dev_err(&maxim->client->dev, "Short to ground detected on:\n");
>> +		for (i = 0; i < 4; i++) {
>> +			if (val & 1 << i)
>> +				dev_err(&maxim->client->dev, "string %d\n", i + 1);
>> +		}
>> +		return -EIO;
>> +	}
>> +
>> +	ret = regmap_read(maxim->regmap, MAX25014_SHORT_GND, &val);
>> +	if (ret != 0)
>> +		return ret;
>> +	if (val > 0) {
>> +		dev_err(&maxim->client->dev, "Shorted led detected on:\n");
>> +		for (i = 0; i < 4; i++) {
>> +			if (val & 1 << i)
>> +				dev_err(&maxim->client->dev, "string %d\n", i + 1);
>> +		}
>> +		return -EIO;
>> +	}
>> +
>> +	ret = regmap_read(maxim->regmap, MAX25014_DIAG, &val);
>> +	if (ret != 0)
>> +		return ret;
>> +	/*
>> +	 * The HW_RST bit always starts at 1 after power up.
>> +	 * It is reset on first read, does not indicate an error.
>> +	 */
>> +	if (val > 0 && val != MAX25014_DIAG_HW_RST) {
>> +		if (val & 0b1)
> 
> BIT(0)
> 
>> +			dev_err(&maxim->client->dev,
>> +				"Overtemperature shutdown\n");
>> +		if (val & 0b10)
>> +			dev_err(&maxim->client->dev,
>> +				 "Chip is getting too hot (>125C)\n");
>> +		if (val & 0b1000)
>> +			dev_err(&maxim->client->dev,
>> +				"Boost converter overvoltage\n");
>> +		if (val & 0b10000)
>> +			dev_err(&maxim->client->dev,
>> +				"Boost converter undervoltage\n");
>> +		if (val & 0b100000)
>> +			dev_err(&maxim->client->dev, "IREF out of range\n");
>> +		return -EIO;
>> +	}
>> +	return 0;
>> +}
>> +
> ...
>> +static int max25014_parse_dt(struct max25014 *maxim,
>> +			     uint32_t *initial_brightness)
>> +{
>> +	struct device *dev = &maxim->client->dev;
>> +	struct device_node *node = dev->of_node;
>> +	struct fwnode_handle *child;
>> +	uint32_t strings[4];
>> +	int res, i;
>> +
>> +	if (!node) {
>> +		dev_err(dev, "no platform data\n");
>> +		return -EINVAL;
>> +	}
> 
> call from probe, check other place
> 
> 	return dev_err_probe()
> 
>> +
>> +	child = device_get_next_child_node(dev, NULL);
>> +	if (child) {
>> +		res = fwnode_property_count_u32(child, "led-sources");
>> +		if (res > 0) {
>> +			fwnode_property_read_u32_array(child, "led-sources",
>> +						       strings, res);
>> +
>> +			/* set all strings as disabled, then enable those in led-sources*/
>> +			maxim->strings_mask = 0xf;
>> +			for (i = 0; i < res; i++) {
>> +				if (strings[i] <= 4)
>> +					maxim->strings_mask &= ~BIT(strings[i]);
>> +			}
>> +		}
>> +
>> +		fwnode_property_read_u32(child, "default-brightness",
>> +					 initial_brightness);
>> +
>> +		fwnode_handle_put(child);
>> +	}
>> +
>> +	maxim->iset = MAX25014_ISET_DEFAULT_100;
>> +	of_property_read_u32(node, "maxim,iset", &maxim->iset);
>> +
>> +	if (maxim->iset < 0 || maxim->iset > 15) {
>> +		dev_err(dev,
>> +			"Invalid iset, should be a value from 0-15, entered was %d\n",
>> +			maxim->iset);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (*initial_brightness < 0 || *initial_brightness > 100) {
>> +		dev_err(dev,
>> +			"Invalid initial brightness, should be a value from 0-100, entered was %d\n",
>> +			*initial_brightness);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int max25014_probe(struct i2c_client *cl)
>> +{
>> +	struct backlight_device *bl;
>> +	const struct i2c_device_id *id = i2c_client_get_device_id(cl);
>> +	struct max25014 *maxim;
>> +	struct backlight_properties props;
>> +	int ret;
>> +	uint32_t initial_brightness = 50;
> 
> try keep reverise christmas order
> 
>> +
>> +	maxim = devm_kzalloc(&cl->dev, sizeof(struct max25014), GFP_KERNEL);
>> +	if (!maxim)
>> +		return -ENOMEM;
>> +
>> +	maxim->client = cl;
>> +
>> +	ret = max25014_parse_dt(maxim, &initial_brightness);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	maxim->vin = devm_regulator_get_optional(&maxim->client->dev, "power");
>> +	if (IS_ERR(maxim->vin)) {
>> +		if (PTR_ERR(maxim->vin) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;
>> +		maxim->vin = NULL;
>> +	}
>> +
>> +	if (maxim->vin) {
>> +		ret = regulator_enable(maxim->vin);
>> +		if (ret < 0) {
>> +			dev_err(&maxim->client->dev,
>> +				"failed to enable Vin: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
> 
> use devm_regulator_get_enable_optional() to combine devm_regulator_get_optional()
> and regulator_enable() to one call.

This however does not return the regulator and will not allow further 
potential power management, I did look into using that one but decided 
against it. There is no runtime PM implemented right now so it wouldn't 
really matter at this point. If it is desirable I will switch it, and it 
will have to be switched back when PM gets implemented.

>> +
>> +	maxim->enable = devm_gpiod_get_optional(&maxim->client->dev, "enable",
>> +						GPIOD_ASIS);
>> +	if (IS_ERR(maxim->enable)) {
>> +		ret = PTR_ERR(maxim->enable);
>> +		dev_err(&maxim->client->dev, "failed to get enable gpio: %d\n",
>> +			ret);
>> +		goto disable_vin;
>> +	}
>> +
>> +	if (maxim->enable)
>> +		gpiod_set_value_cansleep(maxim->enable, 1);
> 
> gpiod_set_value_cansleep() tolerate NULL, so needn't if check here
> 
> and if you pass GPIOD_OUT_HIGH at devm_gpiod_get_optional(), needn't call
> this function.

got it, changed.

>> +
>> +	/* Enable can be tied to vin rail wait if either is available */
>> +	if (maxim->enable || maxim->vin) {
>> +		/* Datasheet Electrical Characteristics tSTARTUP 2ms */
>> +		usleep_range(2000, 2500);
> 
> now perfer use fsleep()

Ah didn't know of that, thanks!

Other small remarks have also been resolved.

Kind regards,
Maud

>> +	}
>> +
>> +	maxim->regmap = devm_regmap_init_i2c(cl, &max25014_regmap_config);
>> +	if (IS_ERR(maxim->regmap)) {
>> +		ret = PTR_ERR(maxim->regmap);
>> +		dev_err(&maxim->client->dev,
>> +			"failed to initialize the i2c regmap: %d\n", ret);
>> +		goto disable_full;
>> +	}
>> +
>> +	i2c_set_clientdata(cl, maxim);
>> +
>> +	ret = max25014_check_errors(maxim);
>> +	if (ret) { /* error is already reported in the above function */
>> +		goto disable_full;
>> +	}
>> +
>> +	ret = max25014_configure(maxim);
>> +	if (ret) {
>> +		dev_err(&maxim->client->dev, "device config err: %d", ret);
>> +		goto disable_full;
>> +	}
>> +
>> +	memset(&props, 0, sizeof(props));
>> +	props.type = BACKLIGHT_PLATFORM;
>> +	props.max_brightness = MAX_BRIGHTNESS;
>> +	props.brightness = initial_brightness;
>> +	props.scale = BACKLIGHT_SCALE_LINEAR;
>> +
>> +	bl = devm_backlight_device_register(&maxim->client->dev, id->name,
>> +					    &maxim->client->dev, maxim,
>> +					    &max25014_bl_ops, &props);
>> +	if (IS_ERR(bl))
>> +		return PTR_ERR(bl);
>> +
>> +	maxim->bl = bl;
>> +
>> +	return 0;
>> +
>> +disable_full:
>> +	if (maxim->enable)
>> +		gpiod_set_value_cansleep(maxim->enable, 0);
>> +disable_vin:
>> +	if (maxim->vin)
>> +		regulator_disable(maxim->vin);
>> +	return ret;
>> +}
>> +
>> +static void max25014_remove(struct i2c_client *cl)
>> +{
>> +	struct max25014 *maxim = i2c_get_clientdata(cl);
>> +
>> +	maxim->bl->props.brightness = 0;
>> +	max25014_update_status(maxim->bl);
>> +	if (maxim->enable)
>> +		gpiod_set_value_cansleep(maxim->enable, 0);
>> +	if (maxim->vin)
>> +		regulator_disable(maxim->vin);
>> +}
>> +
>> +static const struct of_device_id max25014_dt_ids[] = {
>> +	{ .compatible = "maxim,max25014", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, max25014_dt_ids);
>> +
>> +static const struct i2c_device_id max25014_ids[] = {
>> +	{ "max25014" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, max25014_ids);
>> +
>> +static struct i2c_driver max25014_driver = {
>> +	.driver = {
>> +		.name = KBUILD_MODNAME,
>> +		.of_match_table = of_match_ptr(max25014_dt_ids),
>> +	},
>> +	.probe = max25014_probe,
>> +	.remove = max25014_remove,
>> +	.id_table = max25014_ids,
>> +};
>> +module_i2c_driver(max25014_driver);
>> +
>> +MODULE_DESCRIPTION("Maxim MAX25014 backlight driver");
>> +MODULE_AUTHOR("Maud Spierings <maudspierings@...ontroll.com>");
>> +MODULE_LICENSE("GPL");
>>
>> --
>> 2.51.2
>>
>>

Powered by blists - more mailing lists