Re: [PATCH v3 4/4] drm/ssd130x: Add support for the SSD133x OLED controller family

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

Jocelyn Falempe <jfalempe@redhat.com> writes:

Hello Jocelyn,

Thanks a lot for your review!

> On 19/12/2023 21:34, Javier Martinez Canillas wrote:
>> The Solomon SSD133x controllers (such as the SSD1331) are used by RGB dot
>> matrix OLED panels, add a modesetting pipeline to support the chip family.
>> 
>> The SSD133x controllers support 256 (8-bit) and 65k (16-bit) color depths
>> but only the former is implemented for now. This is because the 256 color
>> depth format matches a fourcc code already present in DRM (RGB8), but the
>> 65k pixel format does not match the existing RG16 fourcc code format.
>> 
>> Instead of a R:G:B 5:6:5, the controller expects the 16-bit pixels to be
>> R:G:B 6:5:6, and so a new fourcc needs to be added to support this format.
>
> small typo here, R:G:B 6:5:6 => that's 17 bits
>

Oh, tanks for pointing that out.

It seems to be a typo in the SSD1331 controller datasheet itself:

https://cdn-shop.adafruit.com/datasheets/SSD1331_1.2.pdf

"Each pixel has 16-bit data. Three sub-pixels for color A, B and C have 6
bits, 5 bits and 6 bits respectively."

I blindly copied what the datasheet said without relizing that it was 17
bits indeed!

So looking again at "Table 9 - Data bus usage under different bus width
and color depth mode" in the datasheet shared above, it seems the format
has the same sub-pixel layout than DRM_FORMAT_RGB565. But I tested with
that format and the colors were off, and the same for DRM_FORMAT_BGR565.

Now, even when only using 256 colors the images are pretty decent as you
can see in https://fosstodon.org/@javierm/111591985174504541

I'll reword the commit message and drop the comment about that RGB format
and explain that only DRM_FORMAT_RGB332 is supported for now.

> other than that, it looks good to me, feel free to add:
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat