Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/bridge: ps8622: ps8622_send_config() checks bl instead of luminance ops - [Medium] drm/bridge: ps8622: Kconfig and remove() break link without backlight class - [Medium] drm/bridge: ps8622: ps8622_set_luminance() writes I2C when bridge disabled --
commit 1f704624e2ea918bbeb4f431e2e749044116d3f0 Author: Mario Limonciello <[email protected]> drm/bridge: add PS8622 connector luminance support This commit adds bridge luminance support to the Parade PS8622 driver when CONFIG_DRM_LUMINANCE is enabled, while retaining the backlight class device registration otherwise. > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index c7da6b02d7b6b..d27e9b2485fd6 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -278,7 +278,7 @@ config DRM_PARADE_PS8622 > depends on OF > select DRM_PANEL_BRIDGE > select DRM_KMS_HELPER > - select BACKLIGHT_CLASS_DEVICE > + depends on DRM_LUMINANCE || BACKLIGHT_CLASS_DEVICE = y || > !BACKLIGHT_CLASS_DEVICE > help > Parade eDP-LVDS bridge chip driver. [Severity: Medium] Can this Kconfig dependency cause linker failures when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled or built as a module? When CONFIG_DRM_LUMINANCE=n and CONFIG_BACKLIGHT_CLASS_DEVICE=n, the !BACKLIGHT_CLASS_DEVICE term evaluates to true, allowing CONFIG_DRM_PARADE_PS8622 to be enabled. In that configuration, ps8622_probe() and ps8622_remove() call backlight_device_register() and backlight_device_unregister(), which do not have inline stubs in include/linux/backlight.h when CONFIG_BACKLIGHT_CLASS_DEVICE=n. Furthermore, when CONFIG_DRM_LUMINANCE=y and CONFIG_BACKLIGHT_CLASS_DEVICE=n (or CONFIG_DRM_PARADE_PS8622=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m), ps8622_remove() only guards backlight_device_unregister(ps8622->bl) with a runtime check on ps8622->bl rather than a compile-time !IS_ENABLED(CONFIG_DRM_LUMINANCE) check: drivers/gpu/drm/bridge/parade-ps8622.c:ps8622_remove() { ... if (ps8622->bl) backlight_device_unregister(ps8622->bl); drm_bridge_remove(&ps8622->bridge); } Because ps8622->bl is not a compile-time constant, the call to backlight_device_unregister() is not eliminated and fails to link. Also, when CONFIG_DRM_LUMINANCE=n and CONFIG_BACKLIGHT_CLASS_DEVICE=m, this expression evaluates to n and prevents building DRM_PARADE_PS8622=m. > diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c > b/drivers/gpu/drm/bridge/parade-ps8622.c > index 3a2ec482037df..229d9d7218c4e 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8622.c > +++ b/drivers/gpu/drm/bridge/parade-ps8622.c [ ... ] > @@ -276,7 +277,7 @@ static int ps8622_send_config(struct ps8622_bridge > *ps8622) > > /* FFh for 100% brightness, 0h for 0% brightness */ > err = ps8622_set(cl, 0x01, 0xa7, > - ps8622->bl->props.brightness); > + ps8622->brightness); > if (err) > goto error; > } else { [Severity: High] When CONFIG_DRM_LUMINANCE is enabled, will ps8622_send_config() ever reach this branch? In ps8622_probe(), when use-external-pwm is not set and CONFIG_DRM_LUMINANCE is enabled, the driver sets DRM_BRIDGE_OP_LUMINANCE on ps8622->bridge.ops instead of calling backlight_device_register(), leaving ps8622->bl as NULL. However, ps8622_pre_enable() -> ps8622_send_config() still checks ps8622->bl to choose between internal and external PWM: drivers/gpu/drm/bridge/parade-ps8622.c:ps8622_send_config() { ... if (ps8622->bl) { /* DPCD720, internal PWM */ err = ps8622_set(cl, 0x01, 0xa5, 0xa0); if (err) goto error; /* FFh for 100% brightness, 0h for 0% brightness */ err = ps8622_set(cl, 0x01, 0xa7, ps8622->brightness); if (err) goto error; } else { /* DPCD720, external PWM */ err = ps8622_set(cl, 0x01, 0xa5, 0x80); if (err) goto error; } ... } Because ps8622->bl is NULL, won't ps8622_send_config() always take the else branch, configure register 0x01:0xa5 for external PWM (0x80) instead of internal PWM (0xa0), and skip programming the initial brightness into register 0x01:0xa7? > @@ -324,6 +325,8 @@ static int ps8622_backlight_update(struct > backlight_device *bl) > struct ps8622_bridge *ps8622 = dev_get_drvdata(&bl->dev); > int ret, brightness = backlight_get_brightness(bl); > > + ps8622->brightness = brightness; > + > if (!ps8622->enabled) > return -EINVAL; > > @@ -332,6 +335,15 @@ static int ps8622_backlight_update(struct > backlight_device *bl) > return ret; > } > > +static void ps8622_set_luminance(struct drm_bridge *bridge, > + unsigned int luminance) > +{ > + struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge); > + > + ps8622->brightness = luminance; > + ps8622_set(ps8622->client, 0x01, 0xa7, luminance); > +} [Severity: Medium] Should ps8622_set_luminance() check ps8622->enabled before calling ps8622_set(), just like ps8622_backlight_update() does right above it? If ps8622_send_config() fails during ps8622_pre_enable(), ps8622->enabled remains false while drm_atomic_helper_commit_modeset_enables() still calls: drm_atomic_helper_commit_luminance_enables() -> drm_bridge_connector_set_luminance() -> ps8622_set_luminance() Similarly, if a CRTC transitions from self-refresh (where ps8622_post_disable() has already set ps8622->enabled = false, disabled v12, and asserted gpio_slp and gpio_rst) to disabled, disable_outputs() calls: drm_atomic_helper_commit_luminance_disables() -> drm_bridge_connector_disable_luminance() -> ps8622_set_luminance() Wouldn't calling ps8622_set(ps8622->client, 0x01, 0xa7, luminance) when !ps8622->enabled attempt an I2C write while the bridge is powered down and in reset? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f2413...@kernel.org?part=14
