Re: [PATCH v3 09/14] drm/solomon: ssd16xx: Expose color mode as plane property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/solomon: ssd16xx: commit message contradicts v3 color_mode behavior
- [Low] drm/solomon: ssd16xx: dead red_supported check in atomic_set_property()
--

commit 52cffe8373e1c5de5f3a1dd257828d014890e4a4
Author: Devarsh Thakkar <devarsht@ti.com>

drm/solomon: ssd16xx: Expose color mode as plane property

This commit exposes a color_mode plane property on ssd16xx panels that
support 3-color display modes, allowing runtime switching between
black-white and 3-color operation while rejecting partial refresh when
3-color mode is selected.

>     drm/solomon: ssd16xx: Expose color mode as plane property
>     
>     Add a DRM plane property "color_mode" for panels that support
>     3-color (black/white/red) display modes.
>     
>     The property allows runtime selection between:
>       - bw (black/white): R1 format represents black (0) and white (1)
>       - 3color: R1 format represents red ink (1) on white background
>     
>     The property is created on all panels with "black-white" selectable;
>     panels with red_supported = true also expose the "3-color" option.
>     The default is set from the panel configuration's default_color_mode
>     field (typically BW for monochrome panels).
>     
>     Signed-off-by: Devarsh Thakkar <devarsht@ti.com>

[Severity: Medium]
Does the commit message still match the v3 implementation?

In drivers/gpu/drm/solomon/ssd16xx.c, ssd16xx_plane_create_properties()
returns early when !device->device_cfg->red_supported, so monochrome panels do
not get the color_mode property at all, whereas the commit message states that
the property is created on all panels.

In addition, ssd16xx_color_mode_enum defines the enum names as "black-white"
and "3-color", while the list in the commit message refers to them as "bw" and
"3color".

> diff --git a/drivers/gpu/drm/solomon/ssd16xx.c b/drivers/gpu/drm/solomon/ssd16xx.c
> index f53c67ae7d317..6037717635a48 100644
> --- a/drivers/gpu/drm/solomon/ssd16xx.c
> +++ b/drivers/gpu/drm/solomon/ssd16xx.c

[ ... ]

> @@ -1516,6 +1523,18 @@ static int ssd16xx_plane_atomic_set_property(struct drm_plane *plane,
>  		ssd_state->refresh_mode = val;
>  		return 0;
>  	}
> +	if (property == device->color_mode_property) {
> +		if (val > SSD16XX_COLOR_MODE_3COLOR)
> +			return -EINVAL;
> +		if (val == SSD16XX_COLOR_MODE_3COLOR &&
> +		    !device->device_cfg->red_supported) {
> +			drm_dbg(&device->drm,
> +				"3-color not supported by this panel\n");
> +			return -EINVAL;
> +		}
> +		ssd_state->color_mode = val;
> +		return 0;
> +	}
>  	return -EINVAL;
>  }

[Severity: Low]
Can this !device->device_cfg->red_supported check in
ssd16xx_plane_atomic_set_property() ever be reached?

In ssd16xx_plane_create_properties(), if !device->device_cfg->red_supported is
true, the function returns early and leaves device->color_mode_property as
NULL:

	if (!device->device_cfg->red_supported)
		return 0;

	device->color_mode_property =
		drm_property_create_enum(drm, 0, "color_mode",
					 ssd16xx_color_mode_enum,
					 ARRAY_SIZE(ssd16xx_color_mode_enum));

Since drm_atomic_plane_set_property() only invokes
ssd16xx_plane_atomic_set_property() for an attached, non-NULL property,
entering the property == device->color_mode_property branch already guarantees
that device->device_cfg->red_supported is true.

[ ... ]

> @@ -1869,6 +1903,23 @@ static int ssd16xx_plane_create_properties(struct ssd16xx_device *device)
>  	drm_object_attach_property(plane_obj,
>  				   device->refresh_mode_property,
>  				   device->refresh_mode);
> +
> +	/*
> +	 * Color mode: only meaningful on panels with a physical red plane.
> +	 * BW-only panels get no property at all rather than a one-value knob.
> +	 */
> +	if (!device->device_cfg->red_supported)
> +		return 0;
> +
> +	device->color_mode_property =
> +		drm_property_create_enum(drm, 0, "color_mode",
> +					 ssd16xx_color_mode_enum,
> +					 ARRAY_SIZE(ssd16xx_color_mode_enum));
> +	if (!device->color_mode_property)
> +		return -ENOMEM;
> +	drm_object_attach_property(plane_obj,
> +				   device->color_mode_property,
> +				   device->color_mode);
>  	return 0;
>  }

 base-commit: d0de3a73194a250a2e5ffbf1dc604c0bcbb16e8a
-- 
2.52.0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019213726.4145532-1-devarsht@ti.com?part=9