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 <[email protected]> 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 <[email protected]> [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/[email protected]?part=9
