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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e5bdc85b-361c-4909-a10d-fd48077a6c70@gocontroll.com>
Date: Fri, 22 Aug 2025 09:31:26 +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 v2 2/4] backlight: add max25014atg backlight

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@...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
> 
> 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@...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)
> 
> 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