[PATCH v2 0/2] DRM driver for Sitronix ST7735R display panels

[PATCH v2 0/2] DRM driver for Sitronix ST7735R display panels

[David Lechner]

This series adds a new DRM/TinyDRM driver for Sitronix ST7735R, specifically
the Adafruit 1.8" TFT.

Nothing fancy here. Just mostly TinyDRM boilerplate with the init sequence
from the fbtft driver for the same panel.

Please see new discussion on device tree bindings in patch 1/2.

David Lechner (2):
  dt-bindings: Add binding for Sitronix ST7735R display panels
  drm/tinydrm: add driver for ST7735R panels

 .../bindings/display/sitronix,st7735r.txt          |  35 ++++
 MAINTAINERS                                        |   6 +
 drivers/gpu/drm/tinydrm/Kconfig                    |  10 +
 drivers/gpu/drm/tinydrm/Makefile                   |   1 +
 drivers/gpu/drm/tinydrm/st7735r.c                  | 219 +++++++++++++++++++++
 5 files changed, 271 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sitronix,st7735r.txt
 create mode 100644 drivers/gpu/drm/tinydrm/st7735r.c

-- 
2.7.4

[PATCH v2 1/2] dt-bindings: Add binding for Sitronix ST7735R display panels

[David Lechner]

This adds a new device tree binding for Sitronix ST7735R display panels,
such as the Adafruit 1.8" TFT.

Signed-off-by: David Lechner <david@lechnology.com>
Acked-by: Rob Herring <robh@kernel.org>
---

v2: changes:
* None, but...

I'm wondering about my choice of compatible here. I chose the name
"sitronix,st7735r-jd-t18003-t01" to mean a Sitronix ST7735R controller
connected to a JD-T18003-T01 LCD screen.

What I am actually using, though, is an Adafruit 1.8" TFT breakout [1]. It
has some additional electronics which just pass through signals to the
controller. The bare display is also available from Adafruit [2].

Limor brought up an interesting point in an off-list discussion. The
same controller can be wired to the same LCD in different ways. This is
evident in the fbftf drivers in staging. There is an "adafruit18" and an
"adafruit18_green" in fbftf_devices.c where apparently, two otherwise
identical displays were wired slightly differently at the factory so that
on one, the on-board GRAM word 0 does not correspond to pixel 0,0 on the
LCD. It requires a special offset to the GRAM starting address in order to
have the image displayed correctly.

Additionally, fbtft supports a SainSmart 1.8" TFT [3] that uses the same
controller, but it appears that these have different gamma curves (perhaps
they use different LCDs?). The available pins are exactly the same as the
Adafruit display though.

As I am writing this, I am looking at the JD-1800 datasheet [4] again for
the Adafruit display and I see that in addition to specifying the size of
the display it says "IC: ST7735B". So, I am starting to think that "jianda,
jd-t18003-t01" would be a better compatible string since it tells you
everything you need to know about this display. Just using the controller
name by itself ("sitronix,st7735r") is not enough because it does not tell
you things like the number of pixels or the physical size of the LCD.

What I am not sure about, though, is how to represent the Adafruit display
that requires the offset in the device tree. Would it be a different
compatible string or should we add an extra property to indicate this
quirk?

On a related note, I recently submitted device tree bindings for a similar
SPI display breakout board [5][6]. After more digging though, I found a
datasheet for that display [7], so I'm thinking a compatible string of
"vot,v220hf01a-t" would be better by the same reasoning above.

And, I know this is getting long, but one more thing...

There was a binding acked recently for the LCD on a D-Link DIR-685 Wireless
N Storage Router [8]. This uses the compound compatible string of "dlink,
dir-685-panel", "ilitek,ili9322". If we want to try to keep things 
consistent, perhaps I should be adopting this pattern as well? And perhaps
it would be better to use the better known vendor name instead of the
obscure vendors from the datasheets that I have found? For example,
"adafruit,618" "sitronix,st7735r" instead of "jianda,jd-t18003-t01"?

[1]: https://www.adafruit.com/product/358
[2]: https://www.adafruit.com/product/618
[3]: https://www.sainsmart.com/products/1-8-tft-spi-lcd-screen-with-microsd-socket
[4]: http://www.adafruit.com/datasheets/JD-T1800.pdf
[5]: https://patchwork.freedesktop.org/patch/187038/
[6]: https://patchwork.freedesktop.org/patch/189233/
[7]: http://www.hotmcu.com/22-176x220-tft-lcd-with-spi-interface-p-316.html
[8]: https://patchwork.freedesktop.org/patch/191462/

 .../bindings/display/sitronix,st7735r.txt          | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sitronix,st7735r.txt

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.txt b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
new file mode 100644
index 0000000..bbb8ba6
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
@@ -0,0 +1,35 @@
+Sitronix ST7735R display panels
+
+This binding is for display panels using a Sitronix ST7735R controller in SPI
+mode.
+
+Required properties:
+- compatible:	"sitronix,st7735r-jd-t18003-t01"
+- dc-gpios:	Display data/command selection (D/CX)
+- reset-gpios:	Reset signal (RSTX)
+
+The node for this driver must be a child node of a SPI controller, hence
+all mandatory properties described in ../spi/spi-bus.txt must be specified.
+
+Optional properties:
+- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
+- backlight:	phandle of the backlight device attached to the panel
+
+Example:
+
+	backlight: backlight {
+		compatible = "gpio-backlight";
+		gpios = <&gpio 44 GPIO_ACTIVE_HIGH>;
+	}
+
+	...
+
+	display@0{
+		compatible = "sitronix,st7735r-jd-t18003-t01";
+		reg = <0>;
+		spi-max-frequency = <32000000>;
+		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
+		rotation = <270>;
+		backlight = &backlight;
+	};
-- 
2.7.4

Re: [PATCH v2 1/2] dt-bindings: Add binding for Sitronix ST7735R display panels

[Linus Walleij]

On Sun, Dec 10, 2017 at 11:10 PM, David Lechner <david@lechnology.com> wrote:

> This adds a new device tree binding for Sitronix ST7735R display panels,
> such as the Adafruit 1.8" TFT.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> Acked-by: Rob Herring <robh@kernel.org>

(...)
> Limor brought up an interesting point in an off-list discussion. The
> same controller can be wired to the same LCD in different ways. This is
> evident in the fbftf drivers in staging. There is an "adafruit18" and an
> "adafruit18_green" in fbftf_devices.c where apparently, two otherwise
> identical displays were wired slightly differently at the factory so that
> on one, the on-board GRAM word 0 does not correspond to pixel 0,0 on the
> LCD. It requires a special offset to the GRAM starting address in order to
> have the image displayed correctly.
>
> Additionally, fbtft supports a SainSmart 1.8" TFT [3] that uses the same
> controller, but it appears that these have different gamma curves (perhaps
> they use different LCDs?). The available pins are exactly the same as the
> Adafruit display though.

This discussion came up in relation to the Ilitek ILI9322 bindings, here
are some links:
https://marc.info/?l=dri-devel&m=150300267811979&w=2
https://marc.info/?l=devicetree&m=150438702909506&w=2
https://marc.info/?l=dri-devel&m=150628542603682&w=2

The last mail from Rob clearly indicates that he doesn't like this kind
of open-ended configuration data stashed up in the DTS.

> There was a binding acked recently for the LCD on a D-Link DIR-685 Wireless
> N Storage Router [8]. This uses the compound compatible string of "dlink,
> dir-685-panel", "ilitek,ili9322". If we want to try to keep things
> consistent, perhaps I should be adopting this pattern as well?

I think so.

> And perhaps
> it would be better to use the better known vendor name instead of the
> obscure vendors from the datasheets that I have found? For example,
> "adafruit,618" "sitronix,st7735r" instead of "jianda,jd-t18003-t01"?

I don't know this, just that it should be:

"vendor,specific-system-config", "vendor,ip-part";

Yours,
Linus Walleij

Re: [PATCH v2 1/2] dt-bindings: Add binding for Sitronix ST7735R display panels

[Noralf Trønnes]

Den 10.12.2017 23.10, skrev David Lechner:
> This adds a new device tree binding for Sitronix ST7735R display panels,
> such as the Adafruit 1.8" TFT.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>
> v2: changes:
> * None, but...
>
> I'm wondering about my choice of compatible here. I chose the name
> "sitronix,st7735r-jd-t18003-t01" to mean a Sitronix ST7735R controller
> connected to a JD-T18003-T01 LCD screen.
>
> What I am actually using, though, is an Adafruit 1.8" TFT breakout [1]. It
> has some additional electronics which just pass through signals to the
> controller. The bare display is also available from Adafruit [2].
>
> Limor brought up an interesting point in an off-list discussion. The
> same controller can be wired to the same LCD in different ways. This is
> evident in the fbftf drivers in staging. There is an "adafruit18" and an
> "adafruit18_green" in fbftf_devices.c where apparently, two otherwise
> identical displays were wired slightly differently at the factory so that
> on one, the on-board GRAM word 0 does not correspond to pixel 0,0 on the
> LCD. It requires a special offset to the GRAM starting address in order to
> have the image displayed correctly.
>
> Additionally, fbtft supports a SainSmart 1.8" TFT [3] that uses the same
> controller, but it appears that these have different gamma curves (perhaps
> they use different LCDs?). The available pins are exactly the same as the
> Adafruit display though.
>
> As I am writing this, I am looking at the JD-1800 datasheet [4] again for
> the Adafruit display and I see that in addition to specifying the size of
> the display it says "IC: ST7735B". So, I am starting to think that "jianda,
> jd-t18003-t01" would be a better compatible string since it tells you
> everything you need to know about this display. Just using the controller
> name by itself ("sitronix,st7735r") is not enough because it does not tell
> you things like the number of pixels or the physical size of the LCD.
>
> What I am not sure about, though, is how to represent the Adafruit display
> that requires the offset in the device tree. Would it be a different
> compatible string or should we add an extra property to indicate this
> quirk?
>
> On a related note, I recently submitted device tree bindings for a similar
> SPI display breakout board [5][6]. After more digging though, I found a
> datasheet for that display [7], so I'm thinking a compatible string of
> "vot,v220hf01a-t" would be better by the same reasoning above.
>
> And, I know this is getting long, but one more thing...
>
> There was a binding acked recently for the LCD on a D-Link DIR-685 Wireless
> N Storage Router [8]. This uses the compound compatible string of "dlink,
> dir-685-panel", "ilitek,ili9322". If we want to try to keep things
> consistent, perhaps I should be adopting this pattern as well? And perhaps
> it would be better to use the better known vendor name instead of the
> obscure vendors from the datasheets that I have found? For example,
> "adafruit,618" "sitronix,st7735r" instead of "jianda,jd-t18003-t01"?

I vote for this:
"jianda,jd-t18003-t01"

The display controller is part of the panel so you can't have this
panel with another controller. If the Adafruit #358 panel with
breakout board did something that made it incompatible with a bare
jianda panel, then it would make sense to call it "adafruit,something".

I don't know the full story about the green tab version of the display
with the offset problem, but it's a long time ago and I don't think
it's worth it to support it. It would require a custom dirtyfb callback.

There's also a black tab version of this Adafruit display with the
colors reversed BGR/RGB, but that also is some time back now.

I guess they all had different panels with their own names which would
result in different compatible strings. This means that the end user
would have to know excatly which version of the Adafruit display they're
using to know which compatible string to use. If the SPI MISO signal
had been routed out from the panel, it would have been possible to make
a driver just with a "adafruit,tft18" comaptible and let the driver ask
the controller which version it is. Assuming that info is available in
the controller. Not all panel vendors bother to do that.

The "mi,mi0283qt" panel for instance is used on the Adafruit PiTFT 28
and the Watterott rpi-display, making it possible to use the same
compatible for both displays and also for anyone buying that panel from
some other source.

One last thing, if the panel is made specifically for Adafruit and not
available from any other source, then it makes sense to call it
something "adafruit".

Noralf.

> [1]: https://www.adafruit.com/product/358
> [2]: https://www.adafruit.com/product/618
> [3]: https://www.sainsmart.com/products/1-8-tft-spi-lcd-screen-with-microsd-socket
> [4]: http://www.adafruit.com/datasheets/JD-T1800.pdf
> [5]: https://patchwork.freedesktop.org/patch/187038/
> [6]: https://patchwork.freedesktop.org/patch/189233/
> [7]: http://www.hotmcu.com/22-176x220-tft-lcd-with-spi-interface-p-316.html
> [8]: https://patchwork.freedesktop.org/patch/191462/
>
>   .../bindings/display/sitronix,st7735r.txt          | 35 ++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/display/sitronix,st7735r.txt
>
> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.txt b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
> new file mode 100644
> index 0000000..bbb8ba6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.txt
> @@ -0,0 +1,35 @@
> +Sitronix ST7735R display panels
> +
> +This binding is for display panels using a Sitronix ST7735R controller in SPI
> +mode.
> +
> +Required properties:
> +- compatible:	"sitronix,st7735r-jd-t18003-t01"
> +- dc-gpios:	Display data/command selection (D/CX)
> +- reset-gpios:	Reset signal (RSTX)
> +
> +The node for this driver must be a child node of a SPI controller, hence
> +all mandatory properties described in ../spi/spi-bus.txt must be specified.
> +
> +Optional properties:
> +- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> +- backlight:	phandle of the backlight device attached to the panel
> +
> +Example:
> +
> +	backlight: backlight {
> +		compatible = "gpio-backlight";
> +		gpios = <&gpio 44 GPIO_ACTIVE_HIGH>;
> +	}
> +
> +	...
> +
> +	display@0{
> +		compatible = "sitronix,st7735r-jd-t18003-t01";
> +		reg = <0>;
> +		spi-max-frequency = <32000000>;
> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> +		rotation = <270>;
> +		backlight = &backlight;
> +	};

[PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

[David Lechner]

This adds a new driver for Sitronix ST7735R display panels.

This has been tested using an Adafruit 1.8" TFT.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* Change delay from 10ms to 20ms to avoid checkpatch warning
* Use mipi_dbi_pipe_enable()/mipi_dbi_pipe_disable() to reduce duplicated code
* Rebase on drm-misc-next (tinydrm_lastclose => drm_fb_helper_lastclose)
* Use mipi_dbi_debugfs_init
* Add mipi->read_commands = NULL; since this display is write-only


 MAINTAINERS                       |   6 ++
 drivers/gpu/drm/tinydrm/Kconfig   |  10 ++
 drivers/gpu/drm/tinydrm/Makefile  |   1 +
 drivers/gpu/drm/tinydrm/st7735r.c | 219 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 drivers/gpu/drm/tinydrm/st7735r.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0e49099..2ce17f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4551,6 +4551,12 @@ S:	Maintained
 F:	drivers/gpu/drm/tinydrm/st7586.c
 F:	Documentation/devicetree/bindings/display/st7586.txt
 
+DRM DRIVER FOR SITRONIX ST7735R PANELS
+M:	David Lechner <david@lechnology.com>
+S:	Maintained
+F:	drivers/gpu/drm/tinydrm/st7735r.c
+F:	Documentation/devicetree/bindings/display/st7735r.txt
+
 DRM DRIVER FOR TDFX VIDEO CARDS
 S:	Orphan / Obsolete
 F:	drivers/gpu/drm/tdfx/
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index 90c5bd5..b0e567d 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -52,3 +52,13 @@ config TINYDRM_ST7586
 	  * LEGO MINDSTORMS EV3
 
 	  If M is selected the module will be called st7586.
+
+config TINYDRM_ST7735R
+	tristate "DRM support for Sitronix ST7735R display panels"
+	depends on DRM_TINYDRM && SPI
+	select TINYDRM_MIPI_DBI
+	help
+	  DRM driver Sitronix ST7735R with one of the following LCDs:
+	  * JD-T18003-T01 1.8" 128x160 TFT
+
+	  If M is selected the module will be called st7735r.
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 8aeee53..49a1119 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
 obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
 obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
+obj-$(CONFIG_TINYDRM_ST7735R)		+= st7735r.o
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
new file mode 100644
index 0000000..0f5c295
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -0,0 +1,219 @@
+/*
+ * DRM driver for Sitronix ST7735R panels
+ *
+ * Copyright 2017 David Lechner <david@lechnology.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/delay.h>
+#include <linux/dma-buf.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/spi/spi.h>
+#include <video/mipi_display.h>
+
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/tinydrm/mipi-dbi.h>
+#include <drm/tinydrm/tinydrm-helpers.h>
+
+#define ST7735R_FRMCTR1		0xb1
+#define ST7735R_FRMCTR2		0xb2
+#define ST7735R_FRMCTR3		0xb3
+#define ST7735R_INVCTR		0xb4
+#define ST7735R_PWCTR1		0xc0
+#define ST7735R_PWCTR2		0xc1
+#define ST7735R_PWCTR3		0xc2
+#define ST7735R_PWCTR4		0xc3
+#define ST7735R_PWCTR5		0xc4
+#define ST7735R_VMCTR1		0xc5
+#define ST7735R_GAMCTRP1	0xe0
+#define ST7735R_GAMCTRN1	0xe1
+
+#define ST7735R_MY	BIT(7)
+#define ST7735R_MX	BIT(6)
+#define ST7735R_MV	BIT(5)
+
+static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
+				struct drm_crtc_state *crtc_state)
+{
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	struct device *dev = tdev->drm->dev;
+	int ret;
+	u8 addr_mode;
+
+	DRM_DEBUG_KMS("\n");
+
+	mipi_dbi_hw_reset(mipi);
+
+	ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+		return;
+	}
+
+	msleep(150);
+
+	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(500);
+
+	mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
+	mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
+	mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 0x2c,
+			 0x2d);
+	mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
+	mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
+	mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
+	mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
+	mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
+	mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
+	mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
+	mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
+	switch (mipi->rotation) {
+	default:
+		addr_mode = ST7735R_MX | ST7735R_MY;
+		break;
+	case 90:
+		addr_mode = ST7735R_MX | ST7735R_MV;
+		break;
+	case 180:
+		addr_mode = 0;
+		break;
+	case 270:
+		addr_mode = ST7735R_MY | ST7735R_MV;
+		break;
+	}
+	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
+			 MIPI_DCS_PIXEL_FMT_16BIT);
+	mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 0x2f,
+			 0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
+			 0x02, 0x10);
+	mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 0x33,
+			 0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
+			 0x03, 0x10);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+	msleep(100);
+
+	mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
+
+	msleep(20);
+
+	mipi_dbi_pipe_enable(pipe, crtc_state);
+}
+
+static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
+	.enable		= st7735r_pipe_enable,
+	.disable	= mipi_dbi_pipe_disable,
+	.update		= tinydrm_display_pipe_update,
+	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode st7735r_mode = {
+	TINYDRM_MODE(128, 160, 28, 35),
+};
+
+DEFINE_DRM_GEM_CMA_FOPS(st7735r_fops);
+
+static struct drm_driver st7735r_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
+				  DRIVER_ATOMIC,
+	.fops			= &st7735r_fops,
+	TINYDRM_GEM_DRIVER_OPS,
+	.lastclose		= drm_fb_helper_lastclose,
+	.debugfs_init		= mipi_dbi_debugfs_init,
+	.name			= "st7735r",
+	.desc			= "Sitronix ST7735R",
+	.date			= "20171128",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static const struct of_device_id st7735r_of_match[] = {
+	{ .compatible = "sitronix,st7735r-jd-t18003-t01" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st7735r_of_match);
+
+static const struct spi_device_id st7735r_id[] = {
+	{ "st7735r-jd-t18003-t01", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, st7735r_id);
+
+static int st7735r_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct mipi_dbi *mipi;
+	struct gpio_desc *dc;
+	u32 rotation = 0;
+	int ret;
+
+	mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
+	if (!mipi)
+		return -ENOMEM;
+
+	mipi->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(mipi->reset)) {
+		DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
+		return PTR_ERR(mipi->reset);
+	}
+
+	dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dc)) {
+		DRM_DEV_ERROR(dev, "Failed to get gpio 'dc'\n");
+		return PTR_ERR(dc);
+	}
+
+	mipi->backlight = tinydrm_of_find_backlight(dev);
+	if (IS_ERR(mipi->backlight))
+		return PTR_ERR(mipi->backlight);
+
+	device_property_read_u32(dev, "rotation", &rotation);
+
+	ret = mipi_dbi_spi_init(spi, mipi, dc);
+	if (ret)
+		return ret;
+
+	/* Cannot read from Adafruit 1.8" display via SPI */
+	mipi->read_commands = NULL;
+
+	ret = mipi_dbi_init(&spi->dev, mipi, &st7735r_pipe_funcs,
+			    &st7735r_driver, &st7735r_mode, rotation);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, mipi);
+
+	return devm_tinydrm_register(&mipi->tinydrm);
+}
+
+static void st7735r_shutdown(struct spi_device *spi)
+{
+	struct mipi_dbi *mipi = spi_get_drvdata(spi);
+
+	tinydrm_shutdown(&mipi->tinydrm);
+}
+
+static struct spi_driver st7735r_spi_driver = {
+	.driver = {
+		.name = "st7735r",
+		.owner = THIS_MODULE,
+		.of_match_table = st7735r_of_match,
+	},
+	.id_table = st7735r_id,
+	.probe = st7735r_probe,
+	.shutdown = st7735r_shutdown,
+};
+module_spi_driver(st7735r_spi_driver);
+
+MODULE_DESCRIPTION("Sitronix ST7735R DRM driver");
+MODULE_AUTHOR("David Lechner <david@lechnology.com>");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

[Philippe Ombredanne]

David,

On Sun, Dec 10, 2017 at 11:10 PM, David Lechner <david@lechnology.com> wrote:
> This adds a new driver for Sitronix ST7735R display panels.
>
> This has been tested using an Adafruit 1.8" TFT.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -0,0 +1,219 @@
> +/*
> + * DRM driver for Sitronix ST7735R panels
> + *
> + * Copyright 2017 David Lechner <david@lechnology.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */

Have you considered using the new SPDX ids? Check the doc patches from
Thomas for details.

This could come out this way if you are using the C++ comment style
all the way, a style that you should at least use for the license id
as requested and commented by Linus:

> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2017 David Lechner <david@lechnology.com>
> +// DRM driver for Sitronix ST7735R panels

-- 
Cordially
Philippe Ombredanne

Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

[Noralf Trønnes]

Den 10.12.2017 23.10, skrev David Lechner:
> This adds a new driver for Sitronix ST7735R display panels.
>
> This has been tested using an Adafruit 1.8" TFT.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---

> +static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
> +				struct drm_crtc_state *crtc_state)
> +{
> +	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
> +	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> +	struct device *dev = tdev->drm->dev;
> +	int ret;
> +	u8 addr_mode;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	mipi_dbi_hw_reset(mipi);
> +
> +	ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
> +		return;
> +	}
> +
> +	msleep(150);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	msleep(500);
> +
> +	mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
> +	mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
> +	mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 0x2c,
> +			 0x2d);
> +	mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
> +	mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
> +	mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
> +	mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
> +	mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
> +	mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
> +	mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
> +	switch (mipi->rotation) {
> +	default:
> +		addr_mode = ST7735R_MX | ST7735R_MY;
> +		break;
> +	case 90:
> +		addr_mode = ST7735R_MX | ST7735R_MV;
> +		break;
> +	case 180:
> +		addr_mode = 0;
> +		break;
> +	case 270:
> +		addr_mode = ST7735R_MY | ST7735R_MV;
> +		break;
> +	}
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
> +			 MIPI_DCS_PIXEL_FMT_16BIT);
> +	mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 0x2f,
> +			 0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
> +			 0x02, 0x10);
> +	mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 0x33,
> +			 0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
> +			 0x03, 0x10);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> +
> +	msleep(100);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
> +
> +	msleep(20);
> +
> +	mipi_dbi_pipe_enable(pipe, crtc_state);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
> +	.enable		= st7735r_pipe_enable,
> +	.disable	= mipi_dbi_pipe_disable,
> +	.update		= tinydrm_display_pipe_update,
> +	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_display_mode st7735r_mode = {
> +	TINYDRM_MODE(128, 160, 28, 35),
> +};

This naming talk has made me realise that these should be named after
the panel since they're not generic controller functions.
Maybe the mode can be generic, not sure if the physical size can/will
differ, the resolution is very likely to be the same for all.

What's your view on my descision to split the MIPI DBI compatible
controllers into per controller drivers instead of having one driver
that supports them all?
I've been afraid that it will be a very big file with macro definitions
per controller and enable functions per panel.

This driver has 15 macros and ~80 panel specific lines.
With one panel supported, half the driver is boilerplate.
The mi0283qt panel (MIPI DBI compatible) is in the same ballpark.

Adding helpers for panel reset/exit_sleep, for MIPI_DCS_SET_ADDRESS_MODE
and for display_on/enter_normal/flush could cut it down some more if we
made one driver to rule them all. Maybe the enable function per panel
could be cut in half that way.

I'm starting to wonder if one driver isn't such a bad solution after all...

Noralf.

Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

[David Lechner]

On 12/11/2017 03:18 PM, Noralf Trønnes wrote:
> 
> Den 10.12.2017 23.10, skrev David Lechner:
>> This adds a new driver for Sitronix ST7735R display panels.
>>
>> This has been tested using an Adafruit 1.8" TFT.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
> 
>> +static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
>> +                struct drm_crtc_state *crtc_state)
>> +{
>> +    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
>> +    struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
>> +    struct device *dev = tdev->drm->dev;
>> +    int ret;
>> +    u8 addr_mode;
>> +
>> +    DRM_DEBUG_KMS("\n");
>> +
>> +    mipi_dbi_hw_reset(mipi);
>> +
>> +    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
>> +    if (ret) {
>> +        DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
>> +        return;
>> +    }
>> +
>> +    msleep(150);
>> +
>> +    mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
>> +    msleep(500);
>> +
>> +    mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
>> +    mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
>> +    mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 
>> 0x2c,
>> +             0x2d);
>> +    mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
>> +    mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
>> +    mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
>> +    mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
>> +    mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
>> +    mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
>> +    mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
>> +    mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
>> +    switch (mipi->rotation) {
>> +    default:
>> +        addr_mode = ST7735R_MX | ST7735R_MY;
>> +        break;
>> +    case 90:
>> +        addr_mode = ST7735R_MX | ST7735R_MV;
>> +        break;
>> +    case 180:
>> +        addr_mode = 0;
>> +        break;
>> +    case 270:
>> +        addr_mode = ST7735R_MY | ST7735R_MV;
>> +        break;
>> +    }
>> +    mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
>> +    mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
>> +             MIPI_DCS_PIXEL_FMT_16BIT);
>> +    mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 
>> 0x2f,
>> +             0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
>> +             0x02, 0x10);
>> +    mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 
>> 0x33,
>> +             0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
>> +             0x03, 0x10);
>> +    mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
>> +
>> +    msleep(100);
>> +
>> +    mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
>> +
>> +    msleep(20);
>> +
>> +    mipi_dbi_pipe_enable(pipe, crtc_state);
>> +}
>> +
>> +static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
>> +    .enable        = st7735r_pipe_enable,
>> +    .disable    = mipi_dbi_pipe_disable,
>> +    .update        = tinydrm_display_pipe_update,
>> +    .prepare_fb    = tinydrm_display_pipe_prepare_fb,
>> +};
>> +
>> +static const struct drm_display_mode st7735r_mode = {
>> +    TINYDRM_MODE(128, 160, 28, 35),
>> +};
> 
> This naming talk has made me realise that these should be named after
> the panel since they're not generic controller functions.
> Maybe the mode can be generic, not sure if the physical size can/will
> differ, the resolution is very likely to be the same for all.

Adafruit has a 1.44" 128x128px display [1] with the same controller, so 
the answer is no, the mode cannot be generic.

[1]: https://www.adafruit.com/product/2088
> 
> What's your view on my descision to split the MIPI DBI compatible
> controllers into per controller drivers instead of having one driver
> that supports them all?

My first impression was that I didn't like it so much. But now, I 
actually think that it is a good idea. I think it is a good compromise 
between some boilerplate code in every driver and a driver with so many 
quirks that it is very difficult to work on. It may make sense to 
combine some very similar controllers, but as a rule of thumb, I think 
one driver per controller will work nicely.

> I've been afraid that it will be a very big file with macro definitions
> per controller and enable functions per panel.
> 
> This driver has 15 macros and ~80 panel specific lines.
> With one panel supported, half the driver is boilerplate.
> The mi0283qt panel (MIPI DBI compatible) is in the same ballpark.
> 
> Adding helpers for panel reset/exit_sleep, for MIPI_DCS_SET_ADDRESS_MODE
> and for display_on/enter_normal/flush could cut it down some more if we
> made one driver to rule them all. Maybe the enable function per panel
> could be cut in half that way.
> 
> I'm starting to wonder if one driver isn't such a bad solution after all...

I think as we add more drivers, the parts that get duplicated will 
become obvious candidates for helper functions to refactor, but I don't 
think trying to cram everything all into one driver is such a good idea.