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: Re: [PATCH v3 2/3] drm: panel: Add Samsung S6E8FCO DSI controller for M1906F9 panel
Date: Fri, 13 Mar 2026 14:15:08 +1000
Message-ID: <review-patch2-1029b5b9-c2ab-42be-a0cf-71304e40e6d8@ixit.cz>
In-Reply-To: <1029b5b9-c2ab-42be-a0cf-71304e40e6d8@ixit.cz>
References: <20260312-panel-patches-v3-0-6ed8c006d0be@gmail.com> <1029b5b9-c2ab-42be-a0cf-71304e40e6d8@ixit.cz>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the main driver patch. Several issues:

**Bug: Typo in MODULE_DESCRIPTION (line 654 of mbox / line 302 of the new f=
ile):**

```c
MODULE_DESCRIPTION("DRM driver for Samsubng s6e8fco DSI controller for m190=
6f9 amoled video mode panel");
```

"Samsubng" should be "Samsung". This is a trivially-fixable typo but should=
 be corrected before merge.

**Questionable: Mode name contains "samsungp" (line 494 of mbox):**

```c
static const struct drm_display_mode s6e8fco_m1906f9_samsungp_mode =3D {
```

The trailing "p" in `samsungp` looks like a stray character. It should like=
ly just be `s6e8fco_m1906f9_mode` or `s6e8fco_m1906f9_samsung_mode`.

**Observation: Reset GPIO polarity:**

```c
static void s6e8fco_m1906f9_reset(struct s6e8fco_m1906f9_ctx *ctx)
{
	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
	usleep_range(12000, 13000);
	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
	usleep_range(2000, 3000);
	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
	usleep_range(10000, 11000);
}
```

The GPIO is requested as `GPIOD_OUT_HIGH` (asserting reset initially), then=
 the reset sequence de-asserts (0), pulses assert (1), then de-asserts (0).=
 The DTS specifies `GPIO_ACTIVE_LOW`, so the logical and physical levels sh=
ould be consistent. This looks correct.

**Observation: LPM flag toggling in backlight ops:**

```c
static int s6e8fco_m1906f9_bl_update_status(struct backlight_device *bl)
{
	...
	dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
	ret =3D mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
	...
	dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
	return 0;
}
```

This pattern of toggling LPM around brightness commands is used by several =
existing panel drivers (panel-visionox-rm69299, panel-lg-sw43408, etc.), so=
 it's an accepted pattern, though it isn't thread-safe. This is pre-existin=
g practice, not something to block on.

**Observation: Backlight brightness comment:**

```c
	// The downstream dts claims 2047, but seems to ignore the MSB.
	.max_brightness =3D 1023,
```

This comment is helpful and explains the deviation from the downstream valu=
e. The use of `mipi_dsi_dcs_set_display_brightness_large` (which sends a 16=
-bit value) with max 1023 seems reasonable.

**Observation: `MIPI_DSI_MODE_LPM` not set initially:**

The driver sets:
```c
dsi->mode_flags =3D MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
		  MIPI_DSI_CLOCK_NON_CONTINUOUS;
```

`MIPI_DSI_MODE_LPM` is never set initially, yet the backlight code adds it =
back after brightness commands. This means the initial on-sequence commands=
 are sent in HS mode (no LPM), and then after the first backlight update, L=
PM gets permanently set. This could be intentional but is worth a second lo=
ok =E2=80=94 if LPM should be set from the start, it should be added to the=
 initial mode_flags. If HS mode is desired for all commands, the LPM toggli=
ng in the backlight ops is suspicious.

**No issue: The driver uses `devm_drm_panel_alloc`**, which is the modern p=
attern. Good.

---
Generated by Claude Code Patch Reviewer