Message-ID: <3887eb47-0e30-4adb-8698-e964fe4d22d9@gocontroll.com>
Date: Tue, 19 Aug 2025 12:33:05 +0200
From: Maud Spierings <maudspierings@...ontroll.com>
To: Daniel Thompson <daniel@...cstar.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>, 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,
 MaudSpieringsmaudspierings@...ontroll.com
Subject: Re: [PATCH 2/4] backlight: add max25014atg backlight

Hi Daniel,

Thank you very much for your review, for some reason it ended in my spam 
box, so I only saw it just now.

On 8/11/25 16:15, Daniel Thompson wrote:
> On Fri, Jul 25, 2025 at 01:09:24PM +0200, 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 intgrated boost controller.
>>
>> Signed-off-by: Maud Spierings maudspierings@...ontroll.com
>> ---
>>   MAINTAINERS                            |   2 +
>>   drivers/video/backlight/Kconfig        |   7 +
>>   drivers/video/backlight/Makefile       |   1 +
>>   drivers/video/backlight/max25014.c     | 449 +++++++++++++++++++++++++++++++++
>>   include/linux/platform_data/max25014.h |  24 ++
> 
> Who else included this header file? Can the code here simply be included
> in the C file?

That was my instinct too, I was following a clearly incorrect pattern 
from another driver, merged the fields from that struct into the main 
max25014 struct.

> 
>> diff --git a/drivers/video/backlight/max25014.c b/drivers/video/backlight/max25014.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..371b6017953ae5955f4dfef921980dfdedd65d85
>> --- /dev/null
>> +++ b/drivers/video/backlight/max25014.c
>> @@ -0,0 +1,449 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Backlight driver for Maxim MAX25014
>> + *
>> + * Copyright (C) 2025 GOcontroll B.V.
>> + * Author: Maud Spierings <maudspierings@...ontroll.com>
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/platform_data/max25014.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#define MAX25014_ISET_DEFAULT_100 11U
>> +#define MAX_BRIGHTNESS (100U)
>> +#define MIN_BRIGHTNESS (0U)
>> +#define TON_MAX (130720U) /* @153Hz */
>> +#define TON_STEP (1307U) /* @153Hz */
>> +#define TON_MIN (0U)
>> +
>> +#define MAX25014_DEV_ID         (0x00U)
>> +#define MAX25014_REV_ID         (0x01U)
>> +#define MAX25014_ISET           (0x02U)
>> +#define MAX25014_IMODE          (0x03U)
>> +#define MAX25014_TON1H          (0x04U)
>> +#define MAX25014_TON1L          (0x05U)
>> +#define MAX25014_TON2H          (0x06U)
>> +#define MAX25014_TON2L          (0x07U)
>> +#define MAX25014_TON3H          (0x08U)
>> +#define MAX25014_TON3L          (0x09U)
>> +#define MAX25014_TON4H          (0x0AU)
>> +#define MAX25014_TON4L          (0x0BU)
>> +#define MAX25014_TON_1_4_LSB    (0x0CU)
>> +#define MAX25014_SETTING        (0x12U)
>> +#define MAX25014_DISABLE        (0x13U)
>> +#define MAX25014_BSTMON         (0x14U)
>> +#define MAX25014_IOUT1          (0x15U)
>> +#define MAX25014_IOUT2          (0x16U)
>> +#define MAX25014_IOUT3          (0x17U)
>> +#define MAX25014_IOUT4          (0x18U)
>> +#define MAX25014_OPEN           (0x1BU)
>> +#define MAX25014_SHORT_GND      (0x1CU)
>> +#define MAX25014_SHORT_LED      (0x1DU)
>> +#define MAX25014_MASK           (0x1EU)
>> +#define MAX25014_DIAG           (0x1FU)
> 
> Forcing all these constants to be unsigned is unusual. Is it really
> needed?

Removed all the U's

> 
>> +#define MAX25014_IMODE_HDIM     BIT(2)
>> +#define MAX25014_ISET_ENABLE    BIT(5)
>> +#define MAX25014_ISET_PSEN      BIT(4)
>> +#define MAX25014_DIAG_HW_RST    BIT(2)
>> +#define MAX25014_SETTING_FPWM   GENMASK(6, 4)
>> +
>> +struct max25014;
> 
> This is redundant. Remove.

Thats an interesting leftover, removed.

>> +
>> +struct max25014 {
>> +	const char *chipname;
> 
> Why keep this value around? It is only used during the probe.
> 
>> +	struct i2c_client *client;
>> +	struct backlight_device *bl;
>> +	struct device *dev;
> 
> It is necessary to cache this, is it just a copy of client->dev)?

yep completely unnecessary, removed.

> 
>> +	struct regmap *regmap;
>> +	struct max25014_platform_data *pdata;
>> +	struct gpio_desc *enable;
>> +	struct regulator *vin; /* regulator for boost converter Vin rail */
>> +};
>> +
>> +static const struct regmap_config max25014_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.max_register = MAX25014_DIAG,
>> +};
>> +
>> +/**
>> + * @brief get the bit mask for the DISABLE register.
>> + *
>> + * @param strings the led string configuration array.
>> + * @return uint8_t bits to set in the register.
>> + */
>> +static uint8_t strings_mask(struct max25014 *maxim)
>> +{
>> +	uint8_t res, i;
>> +
>> +	for (i = 0; i < 4; i++) {
>> +		if (maxim->pdata->strings[i] == 0)
>> +			res |= 1 << i;
>> +	}
>> +	return res;
> 
> Could this converison have happened during DT parsing?

inlined it, changed the strings field in to strings_mask and only store 
the mask it calculates.

>> +}
>> +
>> +/**
>> + * @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)
>> +		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) {
>> +		dev_err(maxim->dev, "Open led strings detected on:\n");
>> +		for (i = 0; i < 4; i++) {
>> +			if (val & 1 << i)
>> +				dev_err(maxim->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->dev, "Short to ground detected on:\n");
>> +		for (i = 0; i < 4; i++) {
>> +			if (val & 1 << i)
>> +				dev_err(maxim->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->dev, "Shorted led detected on:\n");
>> +		for (i = 0; i < 4; i++) {
>> +			if (val & 1 << i)
>> +				dev_err(maxim->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)
>> +			dev_err(maxim->dev, "Overtemperature shutdown\n");
>> +		if (val & 0b10)
>> +			dev_warn(maxim->dev,
>> +				 "Chip is getting too hot (>125C)\n");
>> +		if (val & 0b1000)
>> +			dev_err(maxim->dev, "Boost converter overvoltage\n");
>> +		if (val & 0b10000)
>> +			dev_err(maxim->dev, "Boost converter undervoltage\n");
>> +		if (val & 0b100000)
>> +			dev_err(maxim->dev, "IREF out of range\n");
>> +		return -EIO;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/*
>> + * 1. disable unused strings
>> + * 2. set dim mode
>> + * 3. set initial brightness
>> + * 4. set setting register
>> + * 5. enable the backlight
>> + */
>> +static int max25014_configure(struct max25014 *maxim)
>> +{
>> +	int ret;
>> +	uint32_t val;
>> +
>> +	ret = regmap_write(maxim->regmap, MAX25014_DISABLE,
>> +			   strings_mask(maxim));
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	ret = regmap_write(maxim->regmap, MAX25014_IMODE, MAX25014_IMODE_HDIM);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	max25014_register_control(maxim->regmap,
>> +				  maxim->pdata->initial_brightness);
>> +
>> +	ret = regmap_read(maxim->regmap, MAX25014_SETTING, &val);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	ret = regmap_write(
>> +		maxim->regmap, MAX25014_SETTING,
>> +		val & ~MAX25014_SETTING_FPWM);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	ret = regmap_write(maxim->regmap, MAX25014_ISET,
>> +			   maxim->pdata->iset | MAX25014_ISET_ENABLE | MAX25014_ISET_PSEN);
>> +	return ret;
>> +}
>> +
>> +static int max25014_update_status(struct backlight_device *bl_dev)
>> +{
>> +	struct max25014 *maxim = bl_get_data(bl_dev);
>> +
>> +	if (bl_dev->props.state & BL_CORE_SUSPENDED)
>> +		bl_dev->props.brightness = 0;
>> +
>> +	return max25014_register_control(maxim->regmap, bl_dev->props.brightness);
>> +}
>> +
>> +static const struct backlight_ops max25014_bl_ops = {
>> +	.options = BL_CORE_SUSPENDRESUME,
>> +	.update_status = max25014_update_status,
>> +};
>> +
>> +static int max25014_backlight_register(struct max25014 *maxim)
>> +{
>> +	struct backlight_device *bl;
>> +	struct backlight_properties props;
>> +	struct max25014_platform_data *pdata = maxim->pdata;
>> +
>> +	memset(&props, 0, sizeof(props));
>> +	props.type = BACKLIGHT_PLATFORM;
>> +	props.max_brightness = MAX_BRIGHTNESS;
>> +
>> +	if (pdata->initial_brightness > props.max_brightness)
>> +		pdata->initial_brightness = props.max_brightness;
> 
> Handle this during DT parsing.

It is already handled there, this is double, so dropped.

>> +
>> +	props.brightness = pdata->initial_brightness;
>> +
>> +	bl = devm_backlight_device_register(maxim->dev, maxim->chipname, maxim->dev,
>> +					    maxim, &max25014_bl_ops, &props);
>> +	if (IS_ERR(bl))
>> +		return PTR_ERR(bl);
>> +
>> +	maxim->bl = bl;
>> +
>> +	return 0;
>> +}
> 
> Can max25014_backlight_register() be moved into the probe function?
> There is no special control flow here so this function doesn't make the
> probe function any simpler.

Done.

>> +
>> +#ifdef CONFIG_OF
>> +static int max25014_parse_dt(struct max25014 *maxim)
>> +{
>> +	struct device *dev = maxim->dev;
>> +	struct device_node *node = dev->of_node;
>> +	struct max25014_platform_data *pdata;
>> +
>> +	int res;
>> +
>> +	if (!node) {
>> +		dev_err(dev, "no platform data\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		return -ENOMEM;
>> +
>> +	res = of_property_count_u32_elems(node, "maxim,strings");
>> +	if (res == 4) {
>> +		of_property_read_u32_array(node, "maxim,strings", pdata->strings, 4);
>> +	} else {
>> +		dev_err(dev, "strings property not correctly defined\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	pdata->initial_brightness = 50U;
>> +	of_property_read_u32(node, "default-brightness", &pdata->initial_brightness);
>> +	pdata->iset = MAX25014_ISET_DEFAULT_100;
>> +	of_property_read_u32(node, "maxim,iset", &pdata->iset);
>> +
>> +	if (pdata->iset < 0 || pdata->iset > 15) {
>> +		dev_err(dev,
>> +			"Invalid iset, should be a value from 0-15, entered was %d\n",
>> +			pdata->iset);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (pdata->initial_brightness < 0 || pdata->initial_brightness > 100) {
>> +		dev_err(dev,
>> +			"Invalid initial brightness, should be a value from 0-100, entered was %d\n",
>> +			pdata->initial_brightness);
>> +		return -EINVAL;
>> +	}
>> +
>> +	maxim->pdata = pdata;
>> +
>> +	return 0;
>> +}
>> +#else
>> +static int max25014_parse_dt(struct max25014 *maxim)
>> +{
>> +	dev_err(maxim->dev,
>> +		"CONFIG_OF not configured, unable to parse devicetree");
>> +	return -EINVAL;
>> +}
> 
> What is the point of this method? New drivers shouldn't support platform
> data so CONFIG_OF is required for this driver to work at all.

I think it is me following a bad pattern again, dropped the ifdef.

>> +#endif
>> +
>> +static int max25014_probe(struct i2c_client *cl)
>> ...

Kind regards,
Maud

Powered by blists - more mailing lists