From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm: panel: Add Samsung S6E8FCO DSI controller for M1906F9 panel Date: Mon, 16 Mar 2026 11:54:46 +1000 Message-ID: In-Reply-To: <20260314-panel-patches-v4-2-1ecbb2c0c3c8@gmail.com> References: <20260314-panel-patches-v4-0-1ecbb2c0c3c8@gmail.com> <20260314-panel-patches-v4-2-1ecbb2c0c3c8@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The panel driver is well-structured and follows modern conventions. **1. Reset GPIO polarity vs DTS mismatch.** The driver requests the GPIO as= `GPIOD_OUT_HIGH` (line 1024 of the mbox / driver line 230): ```c ctx->reset_gpio =3D devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); ``` The reset sequence (driver lines 39-47) does: ```c gpiod_set_value_cansleep(ctx->reset_gpio, 0); // deassert usleep_range(12000, 13000); gpiod_set_value_cansleep(ctx->reset_gpio, 1); // assert usleep_range(2000, 3000); gpiod_set_value_cansleep(ctx->reset_gpio, 0); // deassert ``` And in the DTS (patch 3), the reset GPIO is `GPIO_ACTIVE_LOW`: ``` reset-gpios =3D <&tlmm 90 GPIO_ACTIVE_LOW>; ``` With `GPIO_ACTIVE_LOW`, `gpiod_set_value(1)` drives the pin low (asserting = reset), and `gpiod_set_value(0)` drives it high (deasserting). So initializ= ing with `GPIOD_OUT_HIGH` means the panel starts with reset asserted, the s= equence deasserts=E2=86=92asserts=E2=86=92deasserts, which is a standard re= set pulse. This is consistent and correct. **2. `_samsungp_mode` naming typo.** The mode struct has a stray "p" in the= name (mbox line 925): ```c static const struct drm_display_mode s6e8fco_m1906f9_samsungp_mode =3D { ``` Should probably be `s6e8fco_m1906f9_mode` =E2=80=94 the `_samsungp_` looks = like a leftover from the generator. Not a functional issue but worth cleani= ng up. **3. LPM mode flag toggling in backlight ops.** The backlight update/get fu= nctions (mbox lines 952-984) toggle `MIPI_DSI_MODE_LPM` on `dsi->mode_flags= `: ```c 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; ``` But `MIPI_DSI_MODE_LPM` is never set in the probe function's `mode_flags` i= nitialization (mbox line 1034): ```c dsi->mode_flags =3D MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_CLOCK_NON_CONTINUOUS; ``` So on the first call to `s6e8fco_bl_update_status`, clearing a flag that is= n't set is a no-op, and then it unconditionally sets `MIPI_DSI_MODE_LPM` af= terwards =E2=80=94 which changes the DSI mode for all subsequent DSI traffi= c, not just the brightness command. If the panel truly needs HS mode for br= ightness and LPM for everything else, `MIPI_DSI_MODE_LPM` should be include= d in the initial `mode_flags`. If the panel doesn't need LPM at all, this t= oggling should be removed. Please verify the intended behavior. **4. `drm_probe_helper.h` include appears unused.** The driver includes `` but uses `drm_connector_helper_get_modes_fixed()` w= hich is from `` =E2=80=94 actually this is correct,= ignore this point. **5. Missing `#include ` usage clarity.** The driver include= s `` for `usleep_range()` in the reset function =E2=80=94 th= at's fine. **6. Between-the-dashes comment.** The note between the `---` markers (mbox= lines 718-719): ``` The downstream dts claims a max brightness of 2047, but seems to ignore the MSB. ``` This is useful context for reviewers but won't appear in the final commit m= essage, which is appropriate. Overall this patch is good. The main actionable items are the mode name typ= o and the LPM flag question. --- --- Generated by Claude Code Patch Reviewer