From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: backlight: add max25014atg backlight
Date: Sun, 12 Apr 2026 13:25:21 +1000
Message-ID: <review-patch2-20260407-max25014-v8-2-14eac7ed673a@gocontroll.com>
In-Reply-To: <20260407-max25014-v8-2-14eac7ed673a@gocontroll.com>
References: <20260407-max25014-v8-0-14eac7ed673a@gocontroll.com> <20260407-max25014-v8-2-14eac7ed673a@gocontroll.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the main driver patch. Several issues:

**Missing include (bug):**

The driver calls `devm_regulator_get_enable()` (line 290) but does not include `<linux/regulator/consumer.h>`. This works today only because the header is pulled in transitively through other headers, but that is fragile and will break if those transitive includes change. Add:
```c
#include <linux/regulator/consumer.h>
```

**`of_match_ptr()` usage (style/correctness):**

```c
.of_match_table = of_match_ptr(max25014_dt_ids),
```

Using `of_match_ptr()` is discouraged in new drivers. When `CONFIG_OF` is disabled, it evaluates to `NULL` and the `max25014_dt_ids` table becomes unused, generating a compiler warning. Just use the table directly:
```c
.of_match_table = max25014_dt_ids,
```

**Indentation bug in `max25014_parse_dt`:**

```c
for (i = 0; i < 4; i++) {
    if (strings[i] == 0)
        maxim->strings_mask |= 1 << i;
}
```

The closing `}` of the `for` loop appears to be indented at the level of the outer `if`, not at the `for` level. This is a whitespace-only issue but indicates sloppy formatting. (Visible at line 247 of the driver.)

**Kernel type conventions:**

The driver uses `uint32_t` and `uint8_t` throughout. Kernel convention strongly prefers `u32` and `u8`. Examples:

```c
uint32_t val;    /* should be: unsigned int val; (for regmap) or u32 */
uint8_t i;       /* should be: int i; or u8 */
uint32_t iset;   /* should be: u32 iset; */
uint8_t strings_mask; /* should be: u8 strings_mask; */
```

Note: `regmap_read()` takes `unsigned int *`, so `val` should ideally be `unsigned int` rather than `uint32_t`/`u32` to match the API precisely.

**Binary literals:**

```c
ret = regmap_write(maxim->regmap, MAX25014_TON_1_4_LSB, reg & 0b00000011);
...
ret = regmap_write(maxim->regmap, MAX25014_TON1L, (reg >> 2) & 0b11111111);
...
return regmap_write(maxim->regmap, MAX25014_TON1H, (reg >> 10) & 0b11111111);
```

Binary literals (`0b...`) are a GCC extension. Kernel code conventionally uses hex: `0x03` and `0xFF`. Also, `& 0xFF` after writing to an 8-bit register via regmap is somewhat redundant since regmap will truncate to `val_bits`, but documenting the field widths explicitly is fine.

**Unnecessary double negation:**

```c
if (!((val & MAX25014_DISABLE_DIS_MASK) == maxim->strings_mask)) {
```

This is more clearly written as:
```c
if ((val & MAX25014_DISABLE_DIS_MASK) != maxim->strings_mask) {
```

**`remove` function doesn't fully disable the hardware:**

```c
static void max25014_remove(struct i2c_client *cl)
{
    struct max25014 *maxim = i2c_get_clientdata(cl);
    backlight_device_set_brightness(maxim->bl, 0);
    gpiod_set_value_cansleep(maxim->enable, 0);
}
```

Setting brightness to 0 only writes the TON registers. The ISET_ENA bit is left set, so the boost converter continues running after the driver is removed. You should also clear the ENA bit (e.g., write 0 to `MAX25014_ISET`) before pulling the enable GPIO low.

**Unchecked return value in `max25014_parse_dt`:**

```c
of_property_read_u32_array(node, "maxim,strings", strings, 4);
```

The return value of `of_property_read_u32_array()` is not checked. If the property exists but has the wrong format, this could silently leave `strings[]` uninitialized. Since you already checked `of_property_count_u32_elems` returned 4, this is low risk, but checking the return value would be more robust.

**Unused defines:**

`MIN_BRIGHTNESS` and `TON_MIN` are defined but never used. Remove them.

**Unnecessary `#include <linux/slab.h>`:**

`devm_kzalloc` is declared via `<linux/device.h>` which is pulled in transitively. The `slab.h` include is not strictly needed, though it's common practice and harmless.

**`ret` reuse pattern in probe:**

```c
ret = max25014_initial_power_state(maxim);
if (ret < 0)
    return dev_err_probe(...);
props.power = ret;
ret = max25014_configure(maxim, ret);
```

This works because `BACKLIGHT_POWER_ON` is 0 and `BACKLIGHT_POWER_OFF` is 4, so both are non-negative. But the dual use of `ret` as both error indicator and power state value is fragile and hard to read. Consider using a separate variable like `initial_state` for clarity.

**Operator precedence in `max25014_check_errors`:**

```c
if (val & 1 << i)
```

This relies on `<<` having higher precedence than `&`, which is correct in C, but adding parentheses `(val & (1 << i))` or using `BIT(i)` would improve readability and match the BIT() style used elsewhere in the file.

---

---
Generated by Claude Code Patch Reviewer