Jocelyn Falempe <jfalempe@xxxxxxxxxx> 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@xxxxxxxxxx> > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
