Re: [PATCH 3/3] drm: tiny: st7735r: Add support for Okaya RH128128T

[David Lechner <david@lechnology.com>]

On 1/6/20 3:28 AM, Geert Uytterhoeven wrote:
> Hi Sam,
> 
> On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>> Good to see we add more functionality to the smallest driver in DRM.
>> The patch triggered a few comments - see below.
>> Some comments relates to the original driver - and not your changes.
> 
> Thanks for your comments!
> 
>> On Thu, Jan 02, 2020 at 03:12:46PM +0100, Geert Uytterhoeven wrote:
>>> Add support for the Okaya RH128128T display to the st7735r driver.
>>>
>>> The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix
>>> ST7715R TFT Controller/Driver.  The latter is very similar to the
>>> ST7735R, and can be handled by the existing st7735r driver.
>>
>> As a general comment - it would have eased review if this was split
>> in two patches.
>> One patch to introduce the infrastructure to deal with another set of
>> controller/display and one patch introducing the new combination.
> 
> I had thought about that, but didn't pursue as the new combination is
> just 7 added lines.  If you prefer a split, I can do that.
> 
>>> --- a/drivers/gpu/drm/tiny/st7735r.c
>>> +++ b/drivers/gpu/drm/tiny/st7735r.c
>>> @@ -1,8 +1,9 @@
>>>   // SPDX-License-Identifier: GPL-2.0+
>>>   /*
>>> - * DRM driver for Sitronix ST7735R panels
>>> + * DRM driver for Sitronix ST7715R/ST7735R panels
>>
>> This comment could describe the situation a little better.
>> This is a sitronix st7735r controller with a jianda jd-t18003-t01
>> display.
>> Or a sitronix st7715r controller with a okaya rh128128t display.
> 
> Indeed. It is currently limited to two controller/display combos.
> But I expect more combos to be added over time.
> Hence does it make sense to describe all of that in the top comments?
> 
>>> @@ -37,12 +39,28 @@
>>>   #define ST7735R_MY   BIT(7)
>>>   #define ST7735R_MX   BIT(6)
>>>   #define ST7735R_MV   BIT(5)
>>> +#define ST7735R_RGB  BIT(3)
>>> +
>>> +struct st7735r_cfg {
>>> +     const struct drm_display_mode mode;
>>> +     unsigned int left_offset;
>>> +     unsigned int top_offset;
>>> +     unsigned int write_only:1;
>>> +     unsigned int rgb:1;             /* RGB (vs. BGR) */
>>> +};
>>> +
>>> +struct st7735r_priv {
>>> +     struct mipi_dbi_dev dbidev;     /* Must be first for .release() */
>>> +     unsigned int rgb:1;
>>> +};
>>
>> The structs here uses "st7735r" as the generic prefix.
>> But the rest of this file uses "jd_t18003_t01" as the generic prefix.
>>
>> It would help readability if the same prefix is used for the common
>> stuff everywhere.
> 
> Agreed.
> So I think it makes most sense to rename jd_t18003_t01_pipe_{enable,funcs}
> to sh7735r_pipe_{enable,funcs}?
> If needed, the display-specific parts (e.g. gamma parameters) could be
> factored out in st7735r_cfg later, if neeeded.

IIRC, the original intention here is that functions/structs with the
jd_t18003_t01_ prefix are specific to the panel, not the controller.
E.g. things like power settings and gamma curves.

The idea is that it is much easier to write and understand the init sequence
as a function rather than trying to make a generic function that can parse
a any possible init sequence from a data structure.

This new panel really has all of the same settings as the existing one?

Having a separate pipe enable function for the new panel would also eliminate
the need for the extra private rgb data.