| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f4e52cc1-9c5f-4069-9079-83be794ab2b3@gocontroll.com>
Date: Mon, 10 Nov 2025 09:40:07 +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 v5 2/4] backlight: add max25014atg backlight
On 11/7/25 17:14, Daniel Thompson 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>
>> diff --git a/drivers/video/backlight/max25014.c b/drivers/video/backlight/max25014.c
>> new file mode 100644
>> index 000000000000..36bae508697e
>> --- /dev/null
>> +++ b/drivers/video/backlight/max25014.c
>> @@ -0,0 +1,409 @@
>> +// 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/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#define MAX25014_ISET_DEFAULT_100 11
>> +#define MAX_BRIGHTNESS 100
>> +#define MIN_BRIGHTNESS 0
>> +#define TON_MAX 130720 /* @153Hz */
>> +#define TON_STEP 1307 /* @153Hz */
>> +#define TON_MIN 0
>> +
>> +#define MAX25014_DEV_ID 0x00
>> +#define MAX25014_REV_ID 0x01
>> +#define MAX25014_ISET 0x02
>> +#define MAX25014_IMODE 0x03
>> +#define MAX25014_TON1H 0x04
>> +#define MAX25014_TON1L 0x05
>> +#define MAX25014_TON2H 0x06
>> +#define MAX25014_TON2L 0x07
>> +#define MAX25014_TON3H 0x08
>> +#define MAX25014_TON3L 0x09
>> +#define MAX25014_TON4H 0x0A
>> +#define MAX25014_TON4L 0x0B
>> +#define MAX25014_TON_1_4_LSB 0x0C
>> +#define MAX25014_SETTING 0x12
>> +#define MAX25014_DISABLE 0x13
>> +#define MAX25014_BSTMON 0x14
>> +#define MAX25014_IOUT1 0x15
>> +#define MAX25014_IOUT2 0x16
>> +#define MAX25014_IOUT3 0x17
>> +#define MAX25014_IOUT4 0x18
>> +#define MAX25014_OPEN 0x1B
>> +#define MAX25014_SHORT_GND 0x1C
>> +#define MAX25014_SHORT_LED 0x1D
>> +#define MAX25014_MASK 0x1E
>> +#define MAX25014_DIAG 0x1F
>> +
>> +#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 {
>> + 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)
>
> This isn't a good name for a function. It doesn't really say what it
> does. Please find a more descriptive name.
Having a lot of difficulties find a succinct name that fits better,
max25014_register_brightness_control()?
max25014_i2c_brightness_control()?
>
>> +{
>> + 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->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)
>> + 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;
>> +}
>> +
>> +/*
>> + * 1. disable unused strings
>> + * 2. set dim mode
>> + * 3. set initial brightness
>
> How does this code set the initial brightness? It doens't set the
> MAX25014_TON* registers.
Yep forgot to remove that, I discovered the backlight core takes care of
the default brightness, so I removed it from here.
>
>> + * 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,
>> + maxim->strings_mask);
>> + if (ret != 0)
>> + return ret;
>> +
>> + ret = regmap_write(maxim->regmap, MAX25014_IMODE, MAX25014_IMODE_HDIM);
>> + if (ret != 0)
>> + return ret;
>> +
>> + 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->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 (backlight_is_blank(maxim->bl))
>> + 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_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;
>> + }
>> +
>> + 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;
>> +
>> + 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");
>
> I would have expected to see devm_regulator_get() here. Why do you care
> whether you get a real regulator or a dummy if you just NULL check
> maxim->vin everywhere?
>
>
>> + if (IS_ERR(maxim->vin)) {
>> + if (PTR_ERR(maxim->vin) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> + maxim->vin = NULL;
>> + }
>> +
>> + if (maxim->vin) {
>
> If you had called devm_regulator_get() there would be no need for a NULL
> check here.
>
>
>> + ret = regulator_enable(maxim->vin);
>> + if (ret < 0) {
>> + dev_err(&maxim->client->dev,
>> + "failed to enable Vin: %d\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> + 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);
>
> No need for NULL pointer check here (see
> https://elixir.bootlin.com/linux/v6.18-rc4/source/drivers/gpio/gpiolib.c#L358-L363 ).
>
>
>> +
>> + /* 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);
>> + }
>
> If you really want to keep the devm_regulator_get_optional() I guess
> maybe you could persuade me it's need to avoid this sleep... although
> I'd be fairly happy to remove the NULL checks here too!
Just wait unconditionally?
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);
>
> Again, NULL check isn't needed.
>
>> +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);
>
> Lose the NULL check.
>
>> + 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");
>
>
> Daniel.
Powered by blists - more mailing lists