Re: [PATCH v2 2/4] drm/ssd130x: Add RGB565 support to SSD133X family

[Amit Barzilai]

Thanks for the review.

On Tue, 23 Jun 2026 12:03:06 +0300, Andy Shevchenko wrote:
>> + * Each Segment holds one pixel and each Common output has a row
>> + * of pixels. A pixel is 8 bits (one byte) in the 256 color
>> + * (RGB332) format or 16 bits (two bytes) in the 65k color
>> + * (RGB565) format. When using the (default) horizontal address
>> + * increment mode, the pixel data is sent Segment by Segment
>> + * (e.g: SEG0 first).
>>   *
>>   * When using the 256 color depth format, each pixel contains 3
>>   * sub-pixels for color A, B and C. These have 3 bit, 3 bit and
>>   * 2 bits respectively.
>
> Something wrong with the plural. There is a difference between "3-bit" and
> "3 bits", but "3 bit" is odd.

You're right. This is pre-existing context, but since I'm reworking the block
I'll fix it. In v3 it will read:

	 * These have 3, 3 and 2 bits respectively.

>> + *
>> + * When using the 65k color depth format, each pixel contains 3
>> + * sub-pixels for color A, B and C. These have 5 bit, 6 bit and
>> + * 5 bits respectively.
>
> Same mistake is repeated here.

Fixing it the same way in v3:

	 * These have 5, 6 and 5 bits respectively.

>> +/*
>> + * Per-variant output format selector for the SSD133X data path. The
>> + * hardware can drive the panel in RGB332 (1 byte/pixel) or RGB565
>> + * (2 bytes/pixel); this is a policy choice per variant, not a
>
> In other comments it was spelled fully, be consistent "1 byte per pixel",
> "2 bytes per pixel".

Good catch. Rather than spell it out, I'll switch to "bpp" (bits per pixel),
which is more concise and matches the wording already used in the commit
message. For consistency I'll update both this comment and the
ssd133x_format_info() one in ssd130x.c. In v3:

	 * hardware can drive the panel in RGB332 (8bpp) or RGB565 (16bpp);
	 * this is a policy choice per variant, not a



--
Thanks,
Amit