Re: [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller

[Javier Martinez Canillas <javierm@redhat.com>]

Amit Barzilai <amit.barzilai22@gmail.com> writes:

Hello Amit,

> This series adds support for the Solomon SSD1351, a 128x128 65k-color
> RGB OLED controller, to the ssd130x DRM driver:
>
>   - Patch 1 adds the device tree binding.
>
>   - Patch 2 switches the SSD133X family from RGB332 to RGB565, bringing
>     65k color to the SSD1331.
>
>   - Patch 3 adds the SSD1351 as a new SSD135X_FAMILY, reusing the
>     SSD133X plane/CRTC and blit/clear helpers. The only data-path
>     difference is the explicit Write RAM command (0x5c) the SSD1351
>     needs before pixel data; it also gets its own init sequence.
>

Great, this approach looks correct to me now. I'll review this series but
likely will do it in a few days.

> Testing:
>
>   - The SSD1351 (patches 1 and 3) is tested on hardware.
>   - The SSD1331 RGB565 change (patch 2) is compile-tested only; I do not
>     currently have a working SSD1331 panel. Javier has kindly offered to
>     test it on his SSD1331.
>
> Dependency:
>
>   The SSD1351 reuses ssd133x_update_rect(), which programs the column
>   and row *end* address as a relative offset rather than an absolute
>   coordinate. This breaks partial updates that do not start at (0,0). A
>   separate fix is posted at [2]; until it lands, the SSD1351 shows the
>   same partial-redraw artifacts. This series applies independently of
>   that fix, but the two are best merged together.

I just pushed this series to drm-misc-next, please rebase on top of that
when posting a new version.

>
> Backlight:
>
>   While adding the SSD1351 I noticed that the shared backlight path
>   (ssd130x_update_bl()) is only correct for the SSD130X and SSD132X
>   families, where 0x81 is the contrast command. On the SSD133X, 0x81 is
>   "Set Contrast for Color A", so brightness changes shift the color
>   balance rather than dim the panel. On the SSD1351, 0x81 is not
>   implemented at all and the brightness byte itself would be executed
>   as a command opcode (e.g. 0xae is Display OFF). This series therefore
>   does not register a backlight device for the SSD135X family. I plan a
>   follow-up making the backlight path family-aware (scaling
>   0x81/0x82/0x83 together for SSD133X, 0xc1 contrast A/B/C for
>   SSD135X), which would also fix the existing SSD1331 behavior. Happy
>   to reorder if you would prefer that rework to land first.
>

Oh, interesting. I think your call is the right one here and this
backlight logic fix / rework can be done as a follow-up series.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat