Re: [PATCH 0/2] drm: Add driver for the Solomon SSD1351 OLED controller

[Javier Martinez Canillas]

Amit Barzilai <amit.barzilai22@xxxxxxxxx> writes:

> Hi Javier,
>
> Thanks you for the detailed review and for the Reviewed-by on the binding.
>

You are welcome.

> I agree with folding the SSD1351 into ssd130x rather than keeping it
> standalone. My plan for the v2 driver series follows your two steps:
>
>   1. Add RGB565 support to the existing SSD133X family (so the SSD1331 gains
>      65k color), gating the format on a per-variant flag in device_info so the
>      existing RGB332 path is untouched.
>   2. Add a new SSD135X family for the SSD1351 on top of that, reusing the
>      ssd133x data path (the update_rect window logic is already
>      format-agnostic) and adding only the SSD1351-specific bits - the 0x5c
>      write-RAM command and its init sequence.
>

These make sense to me.

> I'm deferring step 3 (native 256k color) for now since, as you suspected, 
> there's no matching DRM fourcc, and I'm dropping the 0/180 rotation support
> to keep the series focused; it can come back later.
>

This also makes sense. Having 256k color support is nice to have, but so was
65k color for SSD1331, that didn't prevent me to add support for that family.

> Two things I'd like to confirm before I send the series:
>
>   - I have an SSD1331 panel, but it is currently unsoldered and I don't have
>     the means to solder it myself. I'm trying to arrange testing through
>     someone else - I can't promise it will work out. If it doesn't, the
>     SSD1331 RGB565 change would be compile-tested only (I do have SSD1351
>     hardware to test the new family). Is compile-tested-only acceptable for
>     the SSD1331 part, or would you prefer I hold that piece until it can be
>     verified on hardware?

That's OK. I've a SSD1331 that I used to test the SSD130X support when I was
working on it. So I can test your SSD1331 changes without any problem.

>   - Could you confirm the RGB565 byte order the SSD1331 expects? The
>     standalone SSD1351 code used big-endian (drm_fb_xrgb8888_to_rgb565be);
>     I want the shared conversion helper to match the SSD1331 datasheet.
>

If I read the SSD1331 data sheet correctly I see that it support different 65k
formats but it does support the same 64k color depth format than the SSD1351.

Only supporting that variant is totally fine so that you could reuse it for 
both chip families.

> Separately, I've sent the DT binding as a standalone v2. Since I dropped the
> width/height and rotation properties per Krzysztof's review, I did not carry
> your Reviewed-by forward - please re-review at your convenience.
>

Yes, that looks good to me. I already gave it my r-b again.

BTW, I just added some cleanup patches from Alberto that simplify how the driver
send multiple commands. Please base your v2 on drm-misc-next branch to ensure it 
does not conflict with your changes.

> Thanks,
> Amit
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat