Re: [PATCH v2 2/2] drm/panel: Add Novatek NT36532 panel driver

On Wed, 29 Jul 2026 16:28:47 +0000, [email protected] wrote:
> - [High] Missing Kconfig dependency for DRM_DISPLAY_DSC_HELPER leads
>   to linker errors.
> - [High] Improper use of DCS brightness helper inside a vendor
>   sequence corrupts the initialization payload.
> - [Medium] Global static DSC configuration structure is assigned to
>   device context and mutated by host drivers.
> - [Medium] Missing DRM backlight device registration and OF backlight
>   mapping.
> - [Low] Panel physical dimensions are defined but not reported to DRM
>   userspace.

Thanks for the report. What I'll fix in v3 first, then the two claims
I disagree with.

Will fix in v3:

1) Kconfig: add select DRM_DISPLAY_DSC_HELPER + DRM_DISPLAY_HELPER (for
   drm_dsc_pps_payload_pack()) and DRM_KMS_HELPER (for
   drm_connector_helper_get_modes_fixed()), plus depends on
   BACKLIGHT_CLASS_DEVICE, like the other DSC panels. Verified on
   arm64: disable DRM_MSM/amdgpu and the other DSC panels and Kconfig
   happily gives you NT36532=m with the DSC helper off, leaving
   drm_dsc_pps_payload_pack without a provider. Most DSI hosts and all
   the DSI bridges select DRM_MIPI_DSI but not the DSC helper, so this
   is easy to hit. Good catch.

2) The set_display_brightness_multi(0x00d4) call sits in the page 0x23
   vendor dimming table (0x4e..0x52, one byte each); 0x51 there is a
   table entry, not the DCS brightness command. The stock OnePlus
   Pad 2 firmware DT sends it as an 0x15 short write (51 d4), while
   the helper emits a long write with a trailing 0x00. Will switch to
   a plain one-byte write. The "51 07 ff" after switching to page 0x10
   is the real brightness command and stays; I'll write it as
   MIPI_DCS_SET_DISPLAY_BRIGHTNESS with an explicit 07 ff payload
   instead of the byte-swapped 0xff07 (same bytes on the wire), and
   drop the other accidental MIPI_DCS_* macro names in the vendor pages
   (SET_GAMMA_CURVE, SET_ADDRESS_MODE, set_pixel_format, ...).

3) Make the DSC template const and copy it into the instance at probe
   time (ctx->dsc = *panel_info->dsc_cfg), like panel-novatek-nt36536 does,
   so the module-global struct is never written.

4) Call drm_panel_of_backlight(). Brightness on this device comes from
   a separate KTZ8866 backlight controller via the standard
   'backlight' property (already allowed through panel-common-dual.yaml,
   so no binding change). Without the hookup the backlight stays on
   after a panel DPMS off; reproducible on the device here.

5) Move width_mm/height_mm into csot_display_mode; modetest currently
   reports the connector as 0x0 mm.

Won't change:

a) "extra byte could shift subsequent commands out of alignment" - no.
   Each DCS write is its own DSI packet with its own payload length; a
   longer packet can't corrupt the ones after it. The 0x51 change
   above is about matching the vendor sequence, not corruption - the
   panel comes up fine as is.

b) The two DSI devices keep sharing one drm_dsc_config; only its
   storage moves, per 3). Both links of a bonded panel must run
   identical DSC parameters, and the host derives the rc parameters
   deterministically from the same seed values while the pair is
   brought up sequentially in one thread, so there is nothing to race.
   Per-link copies would just be two structs that must never diverge.

Thanks,
Junjie