Received-SPF: pass (google.com: domain of linux-kernel+bounces-9549-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Javier Martinez Canillas <javierm@redhat.com>
To: Jocelyn Falempe <jfalempe@redhat.com>, linux-kernel@vger.kernel.org
Cc: Geert Uytterhoeven <geert@linux-m68k.org>, Maxime Ripard
 <mripard@kernel.org>, Peter Robinson <pbrobinson@gmail.com>, Rob Herring
 <robh@kernel.org>, Conor Dooley <conor@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski@linaro.org>, Thomas Zimmermann <tzimmermann@suse.de>,
 Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@gmail.com>, Maarten
 Lankhorst <maarten.lankhorst@linux.intel.com>,
 dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 4/4] drm/ssd130x: Add support for the SSD133x OLED
 controller family
In-Reply-To: <01cc7bd1-25d8-43ad-a305-05fcc81bfd22@redhat.com>
References: <20231219203416.2299702-1-javierm@redhat.com>
 <20231219203416.2299702-5-javierm@redhat.com>
 <01cc7bd1-25d8-43ad-a305-05fcc81bfd22@redhat.com>
Date: Fri, 22 Dec 2023 10:36:56 +0100
Message-ID: <87y1dm4lzb.fsf@minerva.mail-host-address-is-not-set>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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