Re: [PATCH v2 2/4] backlight: add max25014atg backlight

[Maud Spierings <maudspierings@gocontroll.com>]

Hi Daniel,
Thanks for the review!

On 8/22/25 09:20, Daniel Thompson wrote:
> On Tue, Aug 19, 2025 at 12:59:00PM +0200, Maud Spierings via B4 Relay wrote:
>> From: Maud Spierings <maudspierings@gocontroll.com>
>>
>> The Maxim MAX25014 is a 4-channel automotive grade backlight driver IC
>> with intgrated boost controller.
>>
>> Signed-off-by: Maud Spierings maudspierings@gocontroll.com
> 
> Looking good but still a few small comments (below).
> 
> 
>> diff --git a/drivers/video/backlight/max25014.c b/drivers/video/backlight/max25014.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..fe5e0615cf6d151868b56ebb9544b175b09dfcee
>> --- /dev/null
>> +++ b/drivers/video/backlight/max25014.c
>> @@ -0,0 +1,395 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Backlight driver for Maxim MAX25014
>> + *
>> + * Copyright (C) 2025 GOcontroll B.V.
>> + * Author: Maud Spierings <maudspierings@gocontroll.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)
> 
> There is no need to put raw numbers in brackets.

gone in the next version, I'll also align the defines at the top better.
>> +
>> +#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 max25014_platform_data *pdata;
> 
> This appears to be unused.

oops forgot to remove that, how did it even build with that?

>> +	struct gpio_desc *enable;
>> +	struct regulator *vin; /* regulator for boost converter Vin rail */
>> +	uint32_t initial_brightness;
> 
> It is important to keep the initial_brightness for the lifetime of the
> driver?

I guess not, though I am not sure how to comfortably transport this 
value to the max25014_configure() otherwise. Some temporary value that I 
create in the probe function then pass as reference to 
max25014_parse_dt() and then to max25014_configure()?

>> +	uint32_t iset;
>> +	uint8_t strings_mask;
>> +};
>> +
> 
Kind regards,
Maud