Message-ID: <8a9a59b8-d5c0-46b3-8f86-a4cd910b7af3@gocontroll.com>
Date: Fri, 5 Dec 2025 16:20:55 +0100
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>, 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 v6 2/4] backlight: add max25014atg backlight

Thanks for the review.

On 12/4/25 17:17, Daniel Thompson wrote:
> On Mon, Dec 01, 2025 at 12:53:21PM +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>
> 
>> <snip>
> 
>> +static int max25014_check_errors(struct max25014 *maxim)
>> +{
>> +	uint32_t val;
>> +	uint8_t i;
>> +	int ret;
>> +
>> +	ret = regmap_read(maxim->regmap, MAX25014_OPEN, &val);
>> +	if (ret)
>> +		return ret;
>> +	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)
>> +		return ret;
>> +	if (val) {
>> +		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);
> 
> Shouldn't this be MAX25014_SHORT_LED?

yep you are absolutely right

> 
>> +	if (ret)
>> +		return ret;
>> +	if (val) {
>> +		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)
>> +		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 && val != MAX25014_DIAG_HW_RST) {
>> +		if (val & MAX25014_DIAG_OT)
>> +			dev_err(&maxim->client->dev,
>> +				"Overtemperature shutdown\n");
>> +		if (val & MAX25014_DIAG_OTW)
>> +			dev_err(&maxim->client->dev,
>> +				 "Chip is getting too hot (>125C)\n");
>> +		if (val & MAX25014_DIAG_BSTOV)
>> +			dev_err(&maxim->client->dev,
>> +				"Boost converter overvoltage\n");
>> +		if (val & MAX25014_DIAG_BSTUV)
>> +			dev_err(&maxim->client->dev,
>> +				"Boost converter undervoltage\n");
>> +		if (val & MAX25014_DIAG_IREFOOR)
>> +			dev_err(&maxim->client->dev, "IREF out of range\n");
>> +		return -EIO;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/*
>> + * 1. disable unused strings
>> + * 2. set dim mode
>> + * 3. set setting register
>> + * 4. enable the backlight
>> + */
>> +static int max25014_configure(struct max25014 *maxim, int initial_state)
>> +{
>> +	uint32_t val;
>> +	int ret;
>> +
>> +	/*
>> +	 * Strings can only be disabled when MAX25014_ISET_ENA == 0, check if
>> +	 * it needs to be changed at all to prevent the backlight flashing when
>> +	 * it is configured correctly by the bootloader
>> +	 */
> 
> Attach the comment to the if statement rather than the read.

will do

> 
>> +	ret = regmap_read(maxim->regmap, MAX25014_DISABLE, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!((val & MAX25014_DISABLE_DIS_MASK) == maxim->strings_mask)) {
>> +		if (initial_state == BACKLIGHT_POWER_ON) {
>> +			ret = regmap_write(maxim->regmap, MAX25014_ISET, 0);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +		ret = regmap_write(maxim->regmap, MAX25014_DISABLE, maxim->strings_mask);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = regmap_write(maxim->regmap, MAX25014_IMODE, MAX25014_IMODE_HDIM);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_read(maxim->regmap, MAX25014_SETTING, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(maxim->regmap, MAX25014_SETTING,
>> +			   val & ~MAX25014_SETTING_FPWM);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(maxim->regmap, MAX25014_ISET,
>> +			   maxim->iset | MAX25014_ISET_ENA |
>> +			   MAX25014_ISET_PSEN);
>> +	return ret;
>> +}
>> +
>> +static int max25014_update_status(struct backlight_device *bl_dev)
>> +{
>> +	struct max25014 *maxim = bl_get_data(bl_dev);
>> +	uint32_t reg;
>> +	int ret;
>> +
>> +	if (backlight_is_blank(maxim->bl))
>> +		bl_dev->props.brightness = 0;
> 
> This isn't right. Why would you change the backlight level just because
> it is currently blanked (and sorry I missed this one last time).

so just remove this bit then jeah?

>> +
>> +	reg  = TON_STEP * bl_dev->props.brightness;
> 
> The correct way to honour blanking is just go call
> backlight_get_brightness() instead of reading the property directly.

will do.

> 
>> +
>> +	/*
>> +	 * 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(maxim->regmap, MAX25014_TON_1_4_LSB, reg & 0b00000011);
>> +	if (ret != 0)
>> +		return ret;
>> +	ret = regmap_write(maxim->regmap, MAX25014_TON1L, (reg >> 2) & 0b11111111);
>> +	if (ret != 0)
>> +		return ret;
>> +	return regmap_write(maxim->regmap, MAX25014_TON1H, (reg >> 10) & 0b11111111);
>> +}
>> +
>> +static const struct backlight_ops max25014_bl_ops = {
>> +	.options = BL_CORE_SUSPENDRESUME,
>> +	.update_status = max25014_update_status,
>> +};
>> +
>> +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)
>> +		return dev_err_probe(dev, -EINVAL, "no platform data\n");
>> +
>> +	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 > 15)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Invalid iset, should be a value from 0-15, entered was %d\n",
>> +				     maxim->iset);
>> +
>> +	if (*initial_brightness > 100)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Invalid initial brightness, should be a value from 0-100, entered was %d\n",
>> +				     *initial_brightness);
>> +
>> +	return 0;
>> +}
>> +
>> +static int max25014_probe(struct i2c_client *cl)
>> +{
>> +	const struct i2c_device_id *id = i2c_client_get_device_id(cl);
>> +	struct backlight_properties props;
>> +	uint32_t initial_brightness = 50;
>> +	struct backlight_device *bl;
>> +	struct max25014 *maxim;
>> +	int ret;
>> +
>> +	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)
>> +		return ret;
>> +
>> +	maxim->vin = devm_regulator_get(&maxim->client->dev, "power");
>> +	if (IS_ERR(maxim->vin)) {
>> +		return dev_err_probe(&maxim->client->dev, PTR_ERR(maxim->vin),
>> +				     "failed to get power-supply");
>> +	}
>> +
>> +	ret = regulator_enable(maxim->vin);
>> +	if (ret)
>> +		return dev_err_probe(&maxim->client->dev, ret,
>> +			"failed to enable power-supply\n");
> 
> Can this use devm_regulator_get_enable()?

Yeah guess I'll just switch to that for now, if ever power management 
gets implemented it can be figured out if regulator control is desired.

> 
>> +
>> +	maxim->enable = devm_gpiod_get_optional(&maxim->client->dev, "enable",
>> +						GPIOD_OUT_HIGH);
>> +	if (IS_ERR(maxim->enable)) {
>> +		ret = dev_err_probe(&maxim->client->dev, PTR_ERR(maxim->enable),
>> +				    "failed to get enable gpio\n");
>> +		goto disable_vin;
>> +	}
>> +
>> +	/* Datasheet Electrical Characteristics tSTARTUP 2ms */
>> +	fsleep(2000);
>> +
>> +	maxim->regmap = devm_regmap_init_i2c(cl, &max25014_regmap_config);
>> +	if (IS_ERR(maxim->regmap)) {
>> +		ret = dev_err_probe(&maxim->client->dev, PTR_ERR(maxim->regmap),
>> +			"failed to initialize the i2c regmap\n");
>> +		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_initial_power_state(maxim);
>> +	if (ret < 0) {
>> +		dev_err_probe(&maxim->client->dev, ret, "Could not get enabled state\n");
>> +		goto disable_full;
>> +	}
>> +
>> +	memset(&props, 0, sizeof(struct backlight_properties));
>> +	props.type = BACKLIGHT_PLATFORM;
>> +	props.max_brightness = MAX_BRIGHTNESS;
>> +	props.brightness = initial_brightness;
>> +	props.scale = BACKLIGHT_SCALE_LINEAR;
>> +	props.power = ret;
>> +
>> +	ret = max25014_configure(maxim, ret);
>> +	if (ret) {
>> +		dev_err_probe(&maxim->client->dev, ret, "device config error");
>> +		goto disable_full;
>> +	}
>> +
>> +	bl = devm_backlight_device_register(&maxim->client->dev, id->name,
>> +					    &maxim->client->dev, maxim,
>> +					    &max25014_bl_ops, &props);
>> +	if (IS_ERR(bl)) {
>> +		ret = dev_err_probe(&maxim->client->dev, PTR_ERR(bl),
>> +				    "failed to register backlight\n");
>> +		goto disable_full;
>> +	}
>> +
>> +	maxim->bl = bl;
>> +
>> +	backlight_update_status(maxim->bl);
>> +
>> +	return 0;
>> +
>> +disable_full:
>> +	gpiod_set_value_cansleep(maxim->enable, 0);
> 
> Why is this needed? It was only ever set by devm_gpiod_get_optional().

oops thats a leftover from before that change, good spot.

>> +disable_vin:
>> +	regulator_disable(maxim->vin);
> 
> This is also not needed if you use devm_regulator_get_enable().

jeah I'll drop this then too

kind regards,
Maud

Powered by blists - more mailing lists