On 1/6/20 3:28 AM, Geert Uytterhoeven wrote:
Hi Sam,
On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam@xxxxxxxxxxxx> 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.
