From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 73EB0388885 for ; Sun, 5 Jul 2026 15:47:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783266475; cv=none; b=l9qxJWpWaHVzXCZf0vB+VglsFnabj6fm1FtRtswfCv+BP/9P9wLL5s1ZYBLDd0VsAAvbtMBESnrAp5yMLmKnnB4kHx5FlW7A6Tq+OxxsN3RW5a4jo7OclXEVhGBcrCQlDywJg7VfaAHxsL2bk8TKQXH8KcshBkpVkJ+RNCQS2kI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783266475; c=relaxed/simple; bh=xp8r2tvHDyAXffHpYkoiEnvzYKDZK5PY0NR1ySjdQMI=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=Tu9YQ1wFe5C/yreGvLkBnyc16x3SEhfGta6gW3fpntkfSoO705ItIAQnTPLil8bpR3wEy5402Oy9WzXRpYYET5tOqIEb0LLPMuIZtkYLNV4GxjoGqqVWDdlxc7BX9Zvz3cWp6Vr6Q7PslZM204XmW9jxmE6GaSxIDK0w3nA2LYw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=RxIU9cS5; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="RxIU9cS5" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0DA5B1F00A3A; Sun, 5 Jul 2026 15:47:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783266474; bh=6clcRuWzyG7PDsah/V8slA2DfijCLDtMK2kx/fwmH3k=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=RxIU9cS5kTNidthC5LnJMiddVWILXiYYdahCzMbMDh7+TxSJPrEUR983HMgTL9pcK iqr3k/ITF5wLAFTvToVkaIQucPPKItE1m1Lk1w7g8MSD+LhkhRo57f1/ZtekDFvn+e wMrxb7kfi+/XxiD0+bJZ4TdZ0kpwx8Gl95Zgn4nDJOwBrQIRoMB40Om1GTjoU70LF+ hbOeBmAEVrgQw1c8oNuWniNdVDV+Ho+AKQD+GTCY4+7s82rnPpL1OB+joHhv57m3dW D8s4EbfZSOQV+f+fjX7UH8z9lpeAaj66zByR/oiV4cFiogFUnFIDtiODwPpq2KzDON iqcNdFOYlQmVA== From: sashiko-bot@kernel.org Subject: Re: [PATCH v2 2/2] drm/panel: Add Novatek NT36536 panel driver Reply-To: sashiko-reviews@lists.linux.dev To: "Pengyu Luo" Cc: robh@kernel.org, conor+dt@kernel.org, devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org In-Reply-To: <20260705153515.46147-3-mitltlatltl@gmail.com> References: <20260705153515.46147-1-mitltlatltl@gmail.com> <20260705153515.46147-3-mitltlatltl@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sun, 05 Jul 2026 15:47:53 +0000 Message-Id: <20260705154754.0DA5B1F00A3A@smtp.kernel.org> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 5 potential issue(= s) to consider: - [Medium] Missing 120ms delay after MIPI_DCS_ENTER_SLEEP_MODE before asser= ting reset and removing power. - [Medium] The driver ignores transmission errors from DSC-related MIPI DSI= multi-context commands in `novatek_prepare()`, masking failures and skippi= ng proper hardware cleanup. - [Medium] The backlight update_status callback races with the panel unprep= are path, allowing DCS commands to be sent to an unpowered panel. - [Low] Missing dependency for DRM DSC helpers in Kconfig, leading to undef= ined references at link time. - [Low] Explicit invocation of `drm_panel_remove()` in the error path for a= panel registered with `devm_drm_panel_add()`. -- commit 51663d35dbaba4a4aee3bdfad1a94da90787d8f6 Author: Pengyu Luo Subject: drm/panel: Add Novatek NT36536 panel driver This commit adds a new driver for Novatek NT36536 display panels. It specifically implements support for the CSOT PP8807HB1-1 dual-link 10-bit panel used in the LENOVO Legion Y700 Gen4. > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -620,6 +620,17 @@ config DRM_PANEL_NOVATEK_NT36523 > around the Novatek NT36523 display controller, such as some > Boe panels used in Xiaomi Mi Pad 5 and 5 Pro tablets. > =20 > +config DRM_PANEL_NOVATEK_NT36536 > + tristate "Novatek NT36536 panel driver" > + depends on OF > + depends on DRM_MIPI_DSI > + depends on BACKLIGHT_CLASS_DEVICE > + select DRM_KMS_HELPER [Severity: Low] Does this Kconfig entry need to select DRM_DISPLAY_DSC_HELPER and=20 DRM_DISPLAY_HELPER? Building this driver on a configuration that does not=20 incidentally enable CONFIG_DRM_DISPLAY_DSC_HELPER could result in a build=20 failure due to an undefined reference to drm_dsc_pps_payload_pack(). > + help > + Say Y here if you want to enable support for Novatek NT36536-based > + display panels, such as the one found in the LENOVO Legion Y700 > + Gen4. > + [ ... ] > diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36536.c b/drivers/gpu/= drm/panel/panel-novatek-nt36536.c > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-novatek-nt36536.c [ ... ] > +static int novatek_prepare(struct drm_panel *panel) > +{ [ ... ] > + drm_dsc_pps_payload_pack(&pps, &ctx->dsc); > + mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps); > + mipi_dsi_compression_mode_multi(&dsi_ctx, true); > + mipi_dsi_msleep(&dsi_ctx, 28); > + > + return backlight_enable(ctx->backlight); [Severity: Medium] Should the accumulated error in dsi_ctx.accum_err be checked before returni= ng? If any of the DSC MIPI DSI commands fail, the error is recorded but never evaluated by novatek_prepare(). Returning the result of backlight_enable()= =20 directly could mask failures, skip proper hardware cleanup, and leave the=20 panel in a corrupted state. > +} > + > +static int novatek_off(struct mipi_dsi_multi_context *dsi_ctx) > +{ > + mipi_dsi_dcs_set_display_off_multi(dsi_ctx); > + mipi_dsi_dcs_enter_sleep_mode_multi(dsi_ctx); > + > + return dsi_ctx->accum_err; > +} > + > +static int novatek_unprepare(struct drm_panel *panel) > +{ [ ... ] > + ret =3D novatek_off(&dsi_ctx); > + if (ret < 0) > + dev_err(dev, "Failed to un-initialize panel: %d\n", ret); > + > + gpiod_set_value_cansleep(ctx->reset_gpio, 1); > + regulator_bulk_disable(ARRAY_SIZE(novatek_supplies), ctx->supplies); [Severity: Medium] Should there be a delay before removing power? novatek_off() sends the MIPI_DCS_ENTER_SLEEP_MODE command, but the regulators are disabled and the= =20 reset GPIO is asserted immediately without waiting. Removing power immediat= ely=20 after sending Sleep In can violate the MIPI DCS specification. > + > + return 0; > +} [ ... ] > +static int novatek_bl_update_status(struct backlight_device *bl) > +{ > + struct novatek *ctx =3D bl_get_data(bl); > + u16 brightness =3D backlight_get_brightness(bl); > + > + return mipi_dsi_dcs_set_display_brightness_large(to_primary_dsi(ctx), > + brightness); [Severity: Medium] Could this DCS command be sent to an unpowered panel? A user-space write to the backlight sysfs interface can trigger novatek_bl_update_status() asynchronously. If this happens after novatek_unprepare() has disabled the= =20 regulators and asserted reset, it could result in DSI host timeouts or=20 display controller lockups. Should there be a check to ensure the panel is= =20 currently prepared? > +} [ ... ] > +static int novatek_probe(struct mipi_dsi_device *dsi) > +{ [ ... ] > + ret =3D devm_drm_panel_add(dev, &ctx->panel); > + if (ret < 0) > + return dev_err_probe(dev, ret, "failed to add panel\n"); > + > + for (i =3D 0; i < num_dsi; i++) { > + ctx->dsi[i]->lanes =3D desc->lanes; > + ctx->dsi[i]->format =3D desc->format; > + ctx->dsi[i]->mode_flags =3D desc->mode_flags; > + ctx->dsi[i]->dsc =3D &ctx->dsc; > + ret =3D devm_mipi_dsi_attach(dev, ctx->dsi[i]); > + if (ret < 0) { > + drm_panel_remove(&ctx->panel); > + return dev_err_probe(dev, ret, > + "Failed to attach to DSI host\n"); [Severity: Low] Is the manual drm_panel_remove() call necessary here? The panel was=20 registered using the managed API devm_drm_panel_add(), which automatically= =20 handles cleanup. Calling drm_panel_remove() explicitly on the error path=20 could cause the devres framework to execute it a second time. > + } > + } [ ... ] --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260705153515.4614= 7-1-mitltlatltl@gmail.com?part=3D2