Re: [PATCH v10 14/15] drm/bridge: add PS8622 connector luminance support

[sashiko-bot@kernel.org]

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 <mario.limonciello@amd.com>

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-b2a6f24131f5@kernel.org?part=14