[PATCH 0/3] drm: Add support for Okaya RH128128T

[PATCH 0/3] drm: Add support for Okaya RH128128T

[Geert Uytterhoeven]

	Hi all,

This patch series adds support for the Okaya RH128128T LCD to the
existing ST7735R driver.  This is a 128x128 1.4" TFT display driven by a
Sitronix ST7715R TFT Controller/Driver.  It is used on the "lcd-pmod"
display module that is shipped with Renesas RSK+RZA1 development boards,
and with several other Renesas starter kits, for RX, Synergy, and RZ/T1
MCUs and SoCs.

I'm not 100% sure about the actual Okaya part number, but this is the
only display listed on the Okaya website that matches the
specifications.

Patch 2 depends on "[PATCH] drm/mipi_dbi: Fix off-by-one bugs in
mipi_dbi_blank()"[1], which I sent earlier this week.

This has been tested using the r7s72100-rskrza1-pmod-spi.dtso and
r7s72100-rskrza1-pmod2-lcd.dtso DT overlays[2].
Note that for using this on RSK+RZA1, there is a dependency on RSPI
cs-gpios support[3].  With DT overlays, this also depends on DT
overlays[4] and gpio-hog overlay support[5].

Thanks for your comments!

[1] https://lore.kernel.org/lkml/20191230130604.31006-1-geert+renesas@glider.be/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/renesas-overlays
[3] "[PATCH 0/6] spi: rspi: Add support for multiple native and GPIO
    chip selects"
    https://lore.kernel.org/lkml/20200102133822.29346-1-geert+renesas@glider.be/
[4] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/overlays
[5] "[PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs"
    https://lore.kernel.org/lkml/20191230133852.5890-1-geert+renesas@glider.be/

Geert Uytterhoeven (3):
  dt-bindings: display: sitronix,st7735r: Add Okaya rh128128t
  drm/mipi_dbi: Add support for display offsets
  drm: tiny: st7735r: Add support for Okaya RH128128T

 .../bindings/display/sitronix,st7735r.txt     |  4 +-
 drivers/gpu/drm/drm_mipi_dbi.c                | 30 ++++++---
 drivers/gpu/drm/tiny/st7735r.c                | 65 ++++++++++++++++---
 include/drm/drm_mipi_dbi.h                    | 12 ++++
 4 files changed, 90 insertions(+), 21 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/3] dt-bindings: display: sitronix,st7735r: Add Okaya rh128128t

[Geert Uytterhoeven]

Document support for the Okaya RH128128T display, which is a 128x128
1.44" TFT display driven by a Sitronix ST7715R TFT Controller/Driver.

ST7715R and ST7735R are very similar.  Their major difference is that
the former is restricted to displays of up to 132x132 pixels, while the
latter supports displays up to 132x162 pixels.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 .../devicetree/bindings/display/sitronix,st7735r.txt          | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.txt b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
index cd5c7186890a2be7..87ebdcb294e29798 100644
--- a/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
+++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
@@ -4,7 +4,9 @@ This binding is for display panels using a Sitronix ST7735R controller in SPI
 mode.
 
 Required properties:
-- compatible:	"jianda,jd-t18003-t01", "sitronix,st7735r"
+- compatible:	Must be one of the following combinations:
+		  - "jianda,jd-t18003-t01", "sitronix,st7735r"
+		  - "okaya,rh128128t", "sitronix,st7715r"
 - dc-gpios:	Display data/command selection (D/CX)
 - reset-gpios:	Reset signal (RSTX)
 
-- 
2.17.1

Re: [PATCH 1/3] dt-bindings: display: sitronix, st7735r: Add Okaya rh128128t

[Sam Ravnborg]

Hi Geert.

On Thu, Jan 02, 2020 at 03:12:44PM +0100, Geert Uytterhoeven wrote:
> Document support for the Okaya RH128128T display, which is a 128x128
> 1.44" TFT display driven by a Sitronix ST7715R TFT Controller/Driver.
> 
> ST7715R and ST7735R are very similar.  Their major difference is that
> the former is restricted to displays of up to 132x132 pixels, while the
> latter supports displays up to 132x162 pixels.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  .../devicetree/bindings/display/sitronix,st7735r.txt          | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.txt b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
> index cd5c7186890a2be7..87ebdcb294e29798 100644
> --- a/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
> +++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
While touching the bindings file, can I convince you to convert it to
meta-schema format (.yaml)?


> @@ -4,7 +4,9 @@ This binding is for display panels using a Sitronix ST7735R controller in SPI
>  mode.
>  
>  Required properties:
> -- compatible:	"jianda,jd-t18003-t01", "sitronix,st7735r"
> +- compatible:	Must be one of the following combinations:
> +		  - "jianda,jd-t18003-t01", "sitronix,st7735r"
> +		  - "okaya,rh128128t", "sitronix,st7715r"

It would be nice if there was a "description" for each pair of
compatible that identified the actual panel.
In your case "Okaya RH128128T 1.44" 128x128 TFT display"
It can be looked up in git history - but better to have it in the
binding file.

	Sam

Re: [PATCH 1/3] dt-bindings: display: sitronix, st7735r: Add Okaya rh128128t

[David Lechner]

On 1/2/20 8:46 AM, Sam Ravnborg wrote:
> Hi Geert.
> 
> On Thu, Jan 02, 2020 at 03:12:44PM +0100, Geert Uytterhoeven wrote:
>> Document support for the Okaya RH128128T display, which is a 128x128
>> 1.44" TFT display driven by a Sitronix ST7715R TFT Controller/Driver.
>>
>> ST7715R and ST7735R are very similar.  Their major difference is that
>> the former is restricted to displays of up to 132x132 pixels, while the
>> latter supports displays up to 132x162 pixels.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>   .../devicetree/bindings/display/sitronix,st7735r.txt          | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.txt b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
>> index cd5c7186890a2be7..87ebdcb294e29798 100644
>> --- a/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
>> +++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
> While touching the bindings file, can I convince you to convert it to
> meta-schema format (.yaml)?
> 
> 
>> @@ -4,7 +4,9 @@ This binding is for display panels using a Sitronix ST7735R controller in SPI
>>   mode.
>>   
>>   Required properties:
>> -- compatible:	"jianda,jd-t18003-t01", "sitronix,st7735r"
>> +- compatible:	Must be one of the following combinations:
>> +		  - "jianda,jd-t18003-t01", "sitronix,st7735r"
>> +		  - "okaya,rh128128t", "sitronix,st7715r"
> 
> It would be nice if there was a "description" for each pair of
> compatible that identified the actual panel.
> In your case "Okaya RH128128T 1.44" 128x128 TFT display"
> It can be looked up in git history - but better to have it in the
> binding file.
> 
> 	Sam
> 

It would be nice to have the Adafruit part name in here too while we
are at it. I had to dig really deep to find what the actual display
panel was.

https://www.adafruit.com/product/358
https://www.adafruit.com/product/618

[PATCH 2/3] drm/mipi_dbi: Add support for display offsets

[Geert Uytterhoeven]

If the resolution of the TFT display is smaller than the maximum
resolution supported by the display controller, the display may be
connected to the driver output arrays with a horizontal and/or vertical
offset, leading to a shifted image.

Add support for specifying these offsets.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 30 ++++++++++++++++++++----------
 include/drm/drm_mipi_dbi.h     | 12 ++++++++++++
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 16bff1be4b8ac622..27fe81a53c88e338 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -238,6 +238,23 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(mipi_dbi_buf_copy);
 
+static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
+					unsigned int xs, unsigned int xe,
+					unsigned int ys, unsigned int ye)
+{
+	struct mipi_dbi *dbi = &dbidev->dbi;
+
+	xs += dbidev->left_offset;
+	xe += dbidev->left_offset;
+	ys += dbidev->top_offset;
+	ye += dbidev->top_offset;
+
+	mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, (xs >> 8) & 0xff,
+			 xs & 0xff, (xe >> 8) & 0xff, xe & 0xff);
+	mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, (ys >> 8) & 0xff,
+			 ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
+}
+
 static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 {
 	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
@@ -271,12 +288,8 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 		tr = cma_obj->vaddr;
 	}
 
-	mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS,
-			 (rect->x1 >> 8) & 0xff, rect->x1 & 0xff,
-			 ((rect->x2 - 1) >> 8) & 0xff, (rect->x2 - 1) & 0xff);
-	mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS,
-			 (rect->y1 >> 8) & 0xff, rect->y1 & 0xff,
-			 ((rect->y2 - 1) >> 8) & 0xff, (rect->y2 - 1) & 0xff);
+	mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
+				    rect->y2 - 1);
 
 	ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
 				   width * height * 2);
@@ -366,10 +379,7 @@ static void mipi_dbi_blank(struct mipi_dbi_dev *dbidev)
 
 	memset(dbidev->tx_buf, 0, len);
 
-	mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, 0, 0,
-			 ((width - 1) >> 8) & 0xFF, (width - 1) & 0xFF);
-	mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, 0, 0,
-			 ((height - 1) >> 8) & 0xFF, (height - 1) & 0xFF);
+	mipi_dbi_set_window_address(dbidev, 0, width - 1, 0, height - 1);
 	mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START,
 			     (u8 *)dbidev->tx_buf, len);
 
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 67c66f5ee591e80f..33f325f5af2b921f 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -109,6 +109,18 @@ struct mipi_dbi_dev {
 	 */
 	unsigned int rotation;
 
+	/**
+	 * @left_offset: Horizontal offset of the display relative to the
+	 *               controller's driver array
+	 */
+	unsigned int left_offset;
+
+	/**
+	 * @top_offset: Vertical offset of the display relative to the
+	 *              controller's driver array
+	 */
+	unsigned int top_offset;
+
 	/**
 	 * @backlight: backlight device (optional)
 	 */
-- 
2.17.1

Re: [PATCH 2/3] drm/mipi_dbi: Add support for display offsets

[Sam Ravnborg]

Hi Geert.

On Thu, Jan 02, 2020 at 03:12:45PM +0100, Geert Uytterhoeven wrote:
> If the resolution of the TFT display is smaller than the maximum
> resolution supported by the display controller, the display may be
> connected to the driver output arrays with a horizontal and/or vertical
> offset, leading to a shifted image.
> 
> Add support for specifying these offsets.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Looks good - the helper made the code more readable.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/drm_mipi_dbi.c | 30 ++++++++++++++++++++----------
>  include/drm/drm_mipi_dbi.h     | 12 ++++++++++++
>  2 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index 16bff1be4b8ac622..27fe81a53c88e338 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -238,6 +238,23 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL(mipi_dbi_buf_copy);
>  
> +static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
> +					unsigned int xs, unsigned int xe,
> +					unsigned int ys, unsigned int ye)
> +{
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +
> +	xs += dbidev->left_offset;
> +	xe += dbidev->left_offset;
> +	ys += dbidev->top_offset;
> +	ye += dbidev->top_offset;
> +
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, (xs >> 8) & 0xff,
> +			 xs & 0xff, (xe >> 8) & 0xff, xe & 0xff);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, (ys >> 8) & 0xff,
> +			 ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
> +}
> +
>  static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  {
>  	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
> @@ -271,12 +288,8 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  		tr = cma_obj->vaddr;
>  	}
>  
> -	mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS,
> -			 (rect->x1 >> 8) & 0xff, rect->x1 & 0xff,
> -			 ((rect->x2 - 1) >> 8) & 0xff, (rect->x2 - 1) & 0xff);
> -	mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS,
> -			 (rect->y1 >> 8) & 0xff, rect->y1 & 0xff,
> -			 ((rect->y2 - 1) >> 8) & 0xff, (rect->y2 - 1) & 0xff);
> +	mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
> +				    rect->y2 - 1);
>  
>  	ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
>  				   width * height * 2);
> @@ -366,10 +379,7 @@ static void mipi_dbi_blank(struct mipi_dbi_dev *dbidev)
>  
>  	memset(dbidev->tx_buf, 0, len);
>  
> -	mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, 0, 0,
> -			 ((width - 1) >> 8) & 0xFF, (width - 1) & 0xFF);
> -	mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, 0, 0,
> -			 ((height - 1) >> 8) & 0xFF, (height - 1) & 0xFF);
> +	mipi_dbi_set_window_address(dbidev, 0, width - 1, 0, height - 1);
>  	mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START,
>  			     (u8 *)dbidev->tx_buf, len);
>  
> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
> index 67c66f5ee591e80f..33f325f5af2b921f 100644
> --- a/include/drm/drm_mipi_dbi.h
> +++ b/include/drm/drm_mipi_dbi.h
> @@ -109,6 +109,18 @@ struct mipi_dbi_dev {
>  	 */
>  	unsigned int rotation;
>  
> +	/**
> +	 * @left_offset: Horizontal offset of the display relative to the
> +	 *               controller's driver array
> +	 */
> +	unsigned int left_offset;
> +
> +	/**
> +	 * @top_offset: Vertical offset of the display relative to the
> +	 *              controller's driver array
> +	 */
> +	unsigned int top_offset;
> +
>  	/**
>  	 * @backlight: backlight device (optional)
>  	 */
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Geert Uytterhoeven]

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.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/gpu/drm/tiny/st7735r.c | 65 ++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
index 3f4487c716848cf8..05d162e76d8481e5 100644
--- 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
  *
  * Copyright 2017 David Lechner <david@lechnology.com>
+ * Copyright (C) 2019 Glider bvba
  */
 
 #include <linux/backlight.h>
@@ -10,6 +11,7 @@
 #include <linux/dma-buf.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/property.h>
 #include <linux/spi/spi.h>
 #include <video/mipi_display.h>
@@ -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;
+};
 
 static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
 				      struct drm_crtc_state *crtc_state,
 				      struct drm_plane_state *plane_state)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	struct st7735r_priv *priv = container_of(dbidev, struct st7735r_priv,
+						 dbidev);
 	struct mipi_dbi *dbi = &dbidev->dbi;
 	int ret, idx;
 	u8 addr_mode;
@@ -87,6 +105,10 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
 		addr_mode = ST7735R_MY | ST7735R_MV;
 		break;
 	}
+
+	if (priv->rgb)
+		addr_mode |= ST7735R_RGB;
+
 	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
 	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT,
 			 MIPI_DCS_PIXEL_FMT_16BIT);
@@ -116,8 +138,17 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
 	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
-static const struct drm_display_mode jd_t18003_t01_mode = {
-	DRM_SIMPLE_MODE(128, 160, 28, 35),
+static const struct st7735r_cfg jd_t18003_t01_cfg = {
+	.mode		= { DRM_SIMPLE_MODE(128, 160, 28, 35) },
+	/* Cannot read from Adafruit 1.8" display via SPI */
+	.write_only	= true,
+};
+
+static const struct st7735r_cfg rh128128t_cfg = {
+	.mode		= { DRM_SIMPLE_MODE(128, 128, 25, 26) },
+	.left_offset	= 2,
+	.top_offset	= 3,
+	.rgb		= true,
 };
 
 DEFINE_DRM_GEM_CMA_FOPS(st7735r_fops);
@@ -136,13 +167,14 @@ static struct drm_driver st7735r_driver = {
 };
 
 static const struct of_device_id st7735r_of_match[] = {
-	{ .compatible = "jianda,jd-t18003-t01" },
+	{ .compatible = "jianda,jd-t18003-t01", .data = &jd_t18003_t01_cfg },
+	{ .compatible = "okaya,rh128128t", .data = &rh128128t_cfg },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, st7735r_of_match);
 
 static const struct spi_device_id st7735r_id[] = {
-	{ "jd-t18003-t01", 0 },
+	{ "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg },
 	{ },
 };
 MODULE_DEVICE_TABLE(spi, st7735r_id);
@@ -150,17 +182,26 @@ MODULE_DEVICE_TABLE(spi, st7735r_id);
 static int st7735r_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	const struct st7735r_cfg *cfg;
 	struct mipi_dbi_dev *dbidev;
+	struct st7735r_priv *priv;
 	struct drm_device *drm;
 	struct mipi_dbi *dbi;
 	struct gpio_desc *dc;
 	u32 rotation = 0;
 	int ret;
 
-	dbidev = kzalloc(sizeof(*dbidev), GFP_KERNEL);
-	if (!dbidev)
+	cfg = of_device_get_match_data(&spi->dev);
+	if (!cfg)
+		cfg = (void *)spi_get_device_id(spi)->driver_data;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
 
+	dbidev = &priv->dbidev;
+	priv->rgb = cfg->rgb;
+
 	dbi = &dbidev->dbi;
 	drm = &dbidev->drm;
 	ret = devm_drm_dev_init(dev, drm, &st7735r_driver);
@@ -193,10 +234,14 @@ static int st7735r_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	/* Cannot read from Adafruit 1.8" display via SPI */
-	dbi->read_commands = NULL;
+	if (cfg->write_only)
+		dbi->read_commands = NULL;
+
+	dbidev->left_offset = cfg->left_offset;
+	dbidev->top_offset = cfg->top_offset;
 
-	ret = mipi_dbi_dev_init(dbidev, &jd_t18003_t01_pipe_funcs, &jd_t18003_t01_mode, rotation);
+	ret = mipi_dbi_dev_init(dbidev, &jd_t18003_t01_pipe_funcs, &cfg->mode,
+				rotation);
 	if (ret)
 		return ret;
 
-- 
2.17.1

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

[Sam Ravnborg]

Hi Geert.

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.

	Sam

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.

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/gpu/drm/tiny/st7735r.c | 65 ++++++++++++++++++++++++++++------
>  1 file changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
> index 3f4487c716848cf8..05d162e76d8481e5 100644
> --- 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.


>   *
>   * Copyright 2017 David Lechner <david@lechnology.com>
> + * Copyright (C) 2019 Glider bvba
>   */
>  
>  #include <linux/backlight.h>
> @@ -10,6 +11,7 @@
>  #include <linux/dma-buf.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/property.h>
>  #include <linux/spi/spi.h>
>  #include <video/mipi_display.h>
> @@ -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.

struct st7735r_priv includes "rgb" which is copied from struct
st7735r_cfg.
Maybe just add a const pointer to struct st7735r_cfg,
so when we later add more configuration items we do not need to have two
copies. And then ofc drop st7735r_priv.rgb.

>  
>  static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
>  				      struct drm_crtc_state *crtc_state,
>  				      struct drm_plane_state *plane_state)
>  {
>  	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> +	struct st7735r_priv *priv = container_of(dbidev, struct st7735r_priv,
> +						 dbidev);
>  	struct mipi_dbi *dbi = &dbidev->dbi;
>  	int ret, idx;
>  	u8 addr_mode;
> @@ -87,6 +105,10 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
>  		addr_mode = ST7735R_MY | ST7735R_MV;
>  		break;
>  	}
> +
> +	if (priv->rgb)
> +		addr_mode |= ST7735R_RGB;
> +
>  	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
>  	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT,
>  			 MIPI_DCS_PIXEL_FMT_16BIT);
> @@ -116,8 +138,17 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
>  	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
>  };
>  
> -static const struct drm_display_mode jd_t18003_t01_mode = {
> -	DRM_SIMPLE_MODE(128, 160, 28, 35),
> +static const struct st7735r_cfg jd_t18003_t01_cfg = {
> +	.mode		= { DRM_SIMPLE_MODE(128, 160, 28, 35) },
> +	/* Cannot read from Adafruit 1.8" display via SPI */
> +	.write_only	= true,
> +};
> +
> +static const struct st7735r_cfg rh128128t_cfg = {
> +	.mode		= { DRM_SIMPLE_MODE(128, 128, 25, 26) },
> +	.left_offset	= 2,
> +	.top_offset	= 3,
> +	.rgb		= true,
>  };
>  
>  DEFINE_DRM_GEM_CMA_FOPS(st7735r_fops);
> @@ -136,13 +167,14 @@ static struct drm_driver st7735r_driver = {
>  };
>  
>  static const struct of_device_id st7735r_of_match[] = {
> -	{ .compatible = "jianda,jd-t18003-t01" },
> +	{ .compatible = "jianda,jd-t18003-t01", .data = &jd_t18003_t01_cfg },
> +	{ .compatible = "okaya,rh128128t", .data = &rh128128t_cfg },
>  	{ },
{ /* sentinel },

Also - which is not a new thing - this fails to check that we have the
correct combination of two compatibles.
From the binding:

    Must be one of the following combinations:
    - "jianda,jd-t18003-t01", "sitronix,st7735r"
    - "okaya,rh128128t", "sitronix,st7715r"

>  };
>  MODULE_DEVICE_TABLE(of, st7735r_of_match);
>  
>  static const struct spi_device_id st7735r_id[] = {
> -	{ "jd-t18003-t01", 0 },
> +	{ "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg },
>  	{ },
{ /* sentinel */ },

Do we need an entry for "okaya,rh128128t" here?

Note: I have not fully understood how MODULE_DEVICE_TABLE()
works - so forgive me my ignorance.

>  };
>  MODULE_DEVICE_TABLE(spi, st7735r_id);
> @@ -150,17 +182,26 @@ MODULE_DEVICE_TABLE(spi, st7735r_id);
>  static int st7735r_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> +	const struct st7735r_cfg *cfg;
>  	struct mipi_dbi_dev *dbidev;
> +	struct st7735r_priv *priv;
>  	struct drm_device *drm;
>  	struct mipi_dbi *dbi;
>  	struct gpio_desc *dc;
>  	u32 rotation = 0;
>  	int ret;
>  
> -	dbidev = kzalloc(sizeof(*dbidev), GFP_KERNEL);
> -	if (!dbidev)
> +	cfg = of_device_get_match_data(&spi->dev);
> +	if (!cfg)
> +		cfg = (void *)spi_get_device_id(spi)->driver_data;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
>  
> +	dbidev = &priv->dbidev;
> +	priv->rgb = cfg->rgb;
> +
>  	dbi = &dbidev->dbi;
>  	drm = &dbidev->drm;
>  	ret = devm_drm_dev_init(dev, drm, &st7735r_driver);
> @@ -193,10 +234,14 @@ static int st7735r_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	/* Cannot read from Adafruit 1.8" display via SPI */
> -	dbi->read_commands = NULL;
> +	if (cfg->write_only)
> +		dbi->read_commands = NULL;
> +
> +	dbidev->left_offset = cfg->left_offset;
> +	dbidev->top_offset = cfg->top_offset;
>  
> -	ret = mipi_dbi_dev_init(dbidev, &jd_t18003_t01_pipe_funcs, &jd_t18003_t01_mode, rotation);
> +	ret = mipi_dbi_dev_init(dbidev, &jd_t18003_t01_pipe_funcs, &cfg->mode,
> +				rotation);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Geert Uytterhoeven]

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.

> struct st7735r_priv includes "rgb" which is copied from struct
> st7735r_cfg.
> Maybe just add a const pointer to struct st7735r_cfg,
> so when we later add more configuration items we do not need to have two
> copies. And then ofc drop st7735r_priv.rgb.

I thought about that, but didn't do so, as the rgb field is the only
parameter used outside the probe function.  Can be changed, of course.

> > @@ -136,13 +167,14 @@ static struct drm_driver st7735r_driver = {
> >  };
> >
> >  static const struct of_device_id st7735r_of_match[] = {
> > -     { .compatible = "jianda,jd-t18003-t01" },
> > +     { .compatible = "jianda,jd-t18003-t01", .data = &jd_t18003_t01_cfg },
> > +     { .compatible = "okaya,rh128128t", .data = &rh128128t_cfg },
> >       { },
> { /* sentinel },
>
> Also - which is not a new thing - this fails to check that we have the
> correct combination of two compatibles.
> From the binding:
>
>     Must be one of the following combinations:
>     - "jianda,jd-t18003-t01", "sitronix,st7735r"
>     - "okaya,rh128128t", "sitronix,st7715r"

That will be checked by "make dtbs_check", once I have converted the DT
bindings to yaml ;-)

In reality, there are lots of different ways to communicate with an
ST77[13]5R display controller (3/4-wire SPI, or 6800/8080 bus, ...), and
lots of different ways to wire a display to the controller.  Ideally,
this should be described in DT.  As I don't have schematics for the
display board, doing so is difficult, and will miss important details,
which may lead to DTB ABI compatibility issues later.
I understand using these compatible combinations was a pragmatic solution
to this problem.

> >  };
> >  MODULE_DEVICE_TABLE(of, st7735r_of_match);
> >
> >  static const struct spi_device_id st7735r_id[] = {
> > -     { "jd-t18003-t01", 0 },
> > +     { "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg },
> >       { },
> { /* sentinel */ },
>
> Do we need an entry for "okaya,rh128128t" here?
>
> Note: I have not fully understood how MODULE_DEVICE_TABLE()
> works - so forgive me my ignorance.

st7735r_id[] is used for matching based on platform device name.
Hence an entry is needed only to use the display on non-DT systems.
Can be added later, if ever needed.

Note that there is a separate MODULE_DEVICE_TABLE() for DT-based matching,
so the driver module will be auto-loaded on DT-based systems.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Sam Ravnborg]

Hi Geert.

> 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.

The good thing with a split is that is shows how little is really
required to support a new controller/panel pair.
So it would be good if you did so.

> 
> > > --- 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?

If we do describe it we should do so as kernel-doc and then wire up the
driver somwhere under Documentation/gpu/
But there is no good place to wire it up yet.

> > > @@ -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}?
s/sh/st/
Yeah, or maybe just sitronix_pipe_{enable,funcs} as we have support
for more than one Sitronix controller anyway.

> If needed, the display-specific parts (e.g. gamma parameters) could be
> factored out in st7735r_cfg later, if neeeded.
> 
> In reality, there are lots of different ways to communicate with an
> ST77[13]5R display controller (3/4-wire SPI, or 6800/8080 bus, ...), and
> lots of different ways to wire a display to the controller.  Ideally,
> this should be described in DT.  As I don't have schematics for the
> display board, doing so is difficult, and will miss important details,
> which may lead to DTB ABI compatibility issues later.
> I understand using these compatible combinations was a pragmatic solution
> to this problem.
Agreed, let's bite that when we have a display we care enough about
and maybe then we also can write a proper driver for it.
I have a few displays here using 8080 and I hope someone steps up
to do proper support for such displays.

> > >
> > >  static const struct spi_device_id st7735r_id[] = {
> > > -     { "jd-t18003-t01", 0 },
> > > +     { "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg },
> > >       { },
> > { /* sentinel */ },
> >
> > Do we need an entry for "okaya,rh128128t" here?
> >
> > Note: I have not fully understood how MODULE_DEVICE_TABLE()
> > works - so forgive me my ignorance.
> 
> st7735r_id[] is used for matching based on platform device name.
> Hence an entry is needed only to use the display on non-DT systems.
> Can be added later, if ever needed.
> 
> Note that there is a separate MODULE_DEVICE_TABLE() for DT-based matching,
> so the driver module will be auto-loaded on DT-based systems.

Thanks for the explanation.

	Sam

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

[Geert Uytterhoeven]

Hi Sam,

On Mon, Jan 6, 2020 at 6:08 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> > > 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.
>
> The good thing with a split is that is shows how little is really
> required to support a new controller/panel pair.
> So it would be good if you did so.

OK.

> > > > --- 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?
>
> If we do describe it we should do so as kernel-doc and then wire up the
> driver somwhere under Documentation/gpu/
> But there is no good place to wire it up yet.

Documentation/devicetree/bindings/display/sitronix,st7735r.yaml? ;-)

> > > > @@ -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}?
> s/sh/st/

Oops, seen to much SuperH-derived hardware ;-)

> Yeah, or maybe just sitronix_pipe_{enable,funcs} as we have support
> for more than one Sitronix controller anyway.

Note that there are other drivers for Sitronix controllers, e.g.
drivers/gpu/drm/tiny/st7586.c, which is a different beast.
So st7735r_* sounds better to me ('15 and '35 are very similar).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[David Lechner]

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.

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

[Geert Uytterhoeven]

Hi David,

On Mon, Jan 6, 2020 at 6:12 PM David Lechner <david@lechnology.com> wrote:
> On 1/6/20 3:28 AM, Geert Uytterhoeven wrote:
> > On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> >> 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.

> >>> --- a/drivers/gpu/drm/tiny/st7735r.c
> >>> +++ b/drivers/gpu/drm/tiny/st7735r.c

> >>> @@ -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.

Makes sense.

> 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.

I believe the init sequence is the same.  The init parameters may not be.
What happened to the separation of code and data? ;-)

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

I went through all the ST77[13]5R-specific register settings in the pipe
enable function. All these registers exist on both ST7715R and ST7735R.
Unfortunately the Okaya display documentation doesn't give any clues
w.r.t. the expected values to program.
However, my display seems to work fine.  Even the grayscale bands from
fbtest test006 look good, so the gamma parameters must be correct ;-)

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

At the expense of duplicating the whole function, for one single bit of
difference...

P.S. Note that I'm using this on an RSK+RZA1 board with 32 MiB of SDRAM.
Enabling support for this display increases kernel size by 316 KiB.
And apparently most real world users of the RZ/A1 SoC are not even using
SDRAM, but doing XIP with the builtin 10 MiB of SRAM...

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds