[PATCH v2 0/4] Support for LEGO MINDSTORMS EV3 LCD display

[PATCH v2 0/4] Support for LEGO MINDSTORMS EV3 LCD display

[David Lechner]

The goal of this series is to get the built-in LCD of the LEGO MINDSTORMS EV3
working.

v2 changes:
* Wrote a new driver for ST7586 instead of combining it with existing drivers
* Don't touch MIPI DBI code (other than the patch suggested by Noralf)
* New defconfig patch


David Lechner (4):
  drm/tinydrm: remove call to mipi_dbi_init() from mipi_dbi_spi_init()
  drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD
  ARM: dts: da850-lego-ev3: Add node for LCD display
  ARM: davinci_all_defconfig: enable tinydrm and ST7586

 .../devicetree/bindings/display/st7586.txt         |  26 +
 arch/arm/boot/dts/da850-lego-ev3.dts               |  24 +
 arch/arm/configs/davinci_all_defconfig             |   2 +
 drivers/gpu/drm/tinydrm/Kconfig                    |  10 +
 drivers/gpu/drm/tinydrm/Makefile                   |   1 +
 drivers/gpu/drm/tinydrm/mi0283qt.c                 |   8 +-
 drivers/gpu/drm/tinydrm/mipi-dbi.c                 |  17 +-
 drivers/gpu/drm/tinydrm/st7586.c                   | 579 +++++++++++++++++++++
 include/drm/tinydrm/mipi-dbi.h                     |   6 +-
 include/drm/tinydrm/st7586.h                       |  34 ++
 10 files changed, 688 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/st7586.txt
 create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
 create mode 100644 include/drm/tinydrm/st7586.h

-- 
2.7.4

[PATCH v2 1/4] drm/tinydrm: remove call to mipi_dbi_init() from mipi_dbi_spi_init()

[David Lechner]

This removes the call to mipi_dbi_init() from mipi_dbi_spi_init() so that
drivers can have a driver-specific implementation if needed.

Also fixed order of @dc parameter in the doc comment.

Suggested-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/gpu/drm/tinydrm/mi0283qt.c |  8 ++++++--
 drivers/gpu/drm/tinydrm/mipi-dbi.c | 17 +++++------------
 include/drm/tinydrm/mipi-dbi.h     |  6 +-----
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 482ff1c3..7e5bb7d 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -195,8 +195,12 @@ static int mi0283qt_probe(struct spi_device *spi)
 
 	device_property_read_u32(dev, "rotation", &rotation);
 
-	ret = mipi_dbi_spi_init(spi, mipi, dc, &mi0283qt_pipe_funcs,
-				&mi0283qt_driver, &mi0283qt_mode, rotation);
+	ret = mipi_dbi_spi_init(spi, mipi, dc);
+	if (ret)
+		return ret;
+
+	ret = mipi_dbi_init(&spi->dev, mipi, &mi0283qt_pipe_funcs,
+			    &mi0283qt_driver, &mi0283qt_mode, rotation);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index e10fa4b..cba9784 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -777,15 +777,12 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
 /**
  * mipi_dbi_spi_init - Initialize MIPI DBI SPI interfaced controller
  * @spi: SPI device
- * @dc: D/C gpio (optional)
  * @mipi: &mipi_dbi structure to initialize
- * @pipe_funcs: Display pipe functions
- * @driver: DRM driver
- * @mode: Display mode
- * @rotation: Initial rotation in degrees Counter Clock Wise
+ * @dc: D/C gpio (optional)
  *
  * This function sets &mipi_dbi->command, enables &mipi->read_commands for the
- * usual read commands and initializes @mipi using mipi_dbi_init().
+ * usual read commands. It should be followed by a call to mipi_dbi_init() or
+ * a driver-specific init.
  *
  * If @dc is set, a Type C Option 3 interface is assumed, if not
  * Type C Option 1.
@@ -800,11 +797,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
  * Zero on success, negative error code on failure.
  */
 int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
-		      struct gpio_desc *dc,
-		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
-		      struct drm_driver *driver,
-		      const struct drm_display_mode *mode,
-		      unsigned int rotation)
+		      struct gpio_desc *dc)
 {
 	size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0);
 	struct device *dev = &spi->dev;
@@ -850,7 +843,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
 			return -ENOMEM;
 	}
 
-	return mipi_dbi_init(dev, mipi, pipe_funcs, driver, mode, rotation);
+	return 0;
 }
 EXPORT_SYMBOL(mipi_dbi_spi_init);
 
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index d137b16..83346dd 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -62,11 +62,7 @@ mipi_dbi_from_tinydrm(struct tinydrm_device *tdev)
 }
 
 int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
-		      struct gpio_desc *dc,
-		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
-		      struct drm_driver *driver,
-		      const struct drm_display_mode *mode,
-		      unsigned int rotation);
+		      struct gpio_desc *dc);
 int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
 		  const struct drm_simple_display_pipe_funcs *pipe_funcs,
 		  struct drm_driver *driver,
-- 
2.7.4

[PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[David Lechner]

LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
module for the ST7586 controller with parameters for the EV3 LCD display.

Signed-off-by: David Lechner <david@lechnology.com>
---
 .../devicetree/bindings/display/st7586.txt         |  26 +
 drivers/gpu/drm/tinydrm/Kconfig                    |  10 +
 drivers/gpu/drm/tinydrm/Makefile                   |   1 +
 drivers/gpu/drm/tinydrm/st7586.c                   | 579 +++++++++++++++++++++
 include/drm/tinydrm/st7586.h                       |  34 ++
 5 files changed, 650 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/st7586.txt
 create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
 create mode 100644 include/drm/tinydrm/st7586.h

diff --git a/Documentation/devicetree/bindings/display/st7586.txt b/Documentation/devicetree/bindings/display/st7586.txt
new file mode 100644
index 0000000..acf784aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/st7586.txt
@@ -0,0 +1,26 @@
+Sitronix ST7586 display panel
+
+Required properties:
+- compatible:	"lego,ev3-lcd".
+
+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:
+- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
+		the panel interface mode (IM[3:0] pins):
+		- present: IM=x110 4-wire 8-bit data serial interface
+		- absent:  IM=x101 3-wire 9-bit data serial interface
+- reset-gpios:	Reset pin
+- power-supply:	A regulator node for the supply voltage.
+- backlight:	phandle of the backlight device attached to the panel
+- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
+
+Example:
+	display@0{
+		compatible = "lego,ev3-lcd";
+		reg = <0>;
+		spi-max-frequency = <10000000>;
+		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
+	};
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index f17c3ca..2e790e7 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -32,3 +32,13 @@ config TINYDRM_REPAPER
 	  2.71" TFT EPD Panel (E2271CS021)
 
 	  If M is selected the module will be called repaper.
+
+config TINYDRM_ST7586
+	tristate "DRM support for Sitronix ST7586 display panels"
+	depends on DRM_TINYDRM && SPI
+	select TINYDRM_MIPI_DBI
+	help
+	  DRM driver for the following Sitronix ST7586 panels:
+	  * LEGO MINDSTORMS EV3
+
+	  If M is selected the module will be called st7586.
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 95bb4d4..0c184bd 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
 # Displays
 obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
+obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
new file mode 100644
index 0000000..6093021
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -0,0 +1,579 @@
+/*
+ * DRM driver for Sitronix ST7586 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/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <video/mipi_display.h>
+
+#include <drm/tinydrm/mipi-dbi.h>
+#include <drm/tinydrm/st7586.h>
+#include <drm/tinydrm/tinydrm-helpers.h>
+
+static inline u8 st7586_rgb565_to_gray2(u16 rgb)
+{
+	u8 r = (rgb & 0xf800) >> 11;
+	u8 g = (rgb & 0x07e0) >> 5;
+	u8 b = rgb & 0x001f;
+	/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+	/* g is already * 2 because it is 6-bit */
+	u8 gray5 = (3 * r + 3 * g + b) / 10;
+
+	return gray5 >> 3;
+}
+
+static void st7586_from_rgb565(u8 *dst, void *vaddr,
+			       struct drm_framebuffer *fb,
+			       struct drm_clip_rect *clip)
+{
+	size_t len;
+	unsigned int x, y;
+	u16 *src, *buf;
+	u8 val;
+
+	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
+	clip->x1 = clip->x1 / 3 * 3;
+	clip->x2 = (clip->x2 + 2) / 3 * 3;
+	len = (clip->x2 - clip->x1) * sizeof(u16);
+
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	for (y = clip->y1; y < clip->y2; y++) {
+		src = vaddr + (y * fb->pitches[0]);
+		src += clip->x1;
+		memcpy(buf, src, len);
+		src = buf;
+		for (x = clip->x1; x < clip->x2; x += 3) {
+			val = st7586_rgb565_to_gray2(*src++) << 6;
+			if (val & 0xC0)
+				val |= 0x20;
+			val |= st7586_rgb565_to_gray2(*src++) << 3;
+			if (val & 0x18)
+				val |= 0x04;
+			val |= st7586_rgb565_to_gray2(*src++);
+			*dst++ = ~val;
+		}
+	}
+
+	/* now adjust the clip so it applies to dst */
+	clip->x1 /= 3;
+	clip->x2 /= 3;
+
+	kfree(buf);
+}
+
+static inline u8 st7586_xrgb8888_to_gray2(u32 rgb)
+{
+	u8 r = (rgb & 0x00ff0000) >> 16;
+	u8 g = (rgb & 0x0000ff00) >> 8;
+	u8 b = rgb & 0x000000ff;
+	/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+	u8 gray8 = (3 * r + 6 * g + b) / 10;
+
+	return gray8 >> 6;
+}
+
+static void st7586_from_xrgb8888(u8 *dst, void *vaddr,
+				 struct drm_framebuffer *fb,
+				 struct drm_clip_rect *clip)
+{
+	size_t len;
+	unsigned int x, y;
+	u32 *src, *buf;
+	u8 val;
+
+	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
+	clip->x1 = clip->x1 / 3 * 3;
+	clip->x2 = (clip->x2 + 2) / 3 * 3;
+	len = (clip->x2 - clip->x1) * sizeof(u32);
+
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	for (y = clip->y1; y < clip->y2; y++) {
+		src = vaddr + (y * fb->pitches[0]);
+		src += clip->x1;
+		memcpy(buf, src, len);
+		src = buf;
+		for (x = clip->x1; x < clip->x2; x += 3) {
+			val = st7586_xrgb8888_to_gray2(*src++) << 6;
+			if (val & 0xC0)
+				val |= 0x20;
+			val |= st7586_xrgb8888_to_gray2(*src++) << 3;
+			if (val & 0x18)
+				val |= 0x04;
+			val |= st7586_xrgb8888_to_gray2(*src++);
+			*dst++ = ~val;
+		}
+	}
+
+	/* now adjust the clip so it applies to dst */
+	clip->x1 /= 3;
+	clip->x2 /= 3;
+
+	kfree(buf);
+}
+
+static int st7586_mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
+				    struct drm_clip_rect *clip)
+{
+	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
+	struct drm_format_name_buf format_name;
+	void *src = cma_obj->vaddr;
+	int ret = 0;
+
+	if (import_attach) {
+		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
+					       DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
+	}
+
+	switch (fb->format->format) {
+	case DRM_FORMAT_RGB565:
+		st7586_from_rgb565(dst, src, fb, clip);
+		break;
+	case DRM_FORMAT_XRGB8888:
+		st7586_from_xrgb8888(dst, src, fb, clip);
+		break;
+	default:
+		dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
+			     drm_get_format_name(fb->format->format,
+						 &format_name));
+		ret = -EINVAL;
+		break;
+	}
+
+	if (import_attach)
+		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
+					     DMA_FROM_DEVICE);
+	return ret;
+}
+
+static int st7586_mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
+				    struct drm_file *file_priv,
+				    unsigned int flags, unsigned int color,
+				    struct drm_clip_rect *clips,
+				    unsigned int num_clips)
+{
+	struct tinydrm_device *tdev = fb->dev->dev_private;
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	struct drm_clip_rect clip;
+	int ret = 0;
+
+	mutex_lock(&tdev->dirty_lock);
+
+	if (!mipi->enabled)
+		goto out_unlock;
+
+	/* fbdev can flush even when we're not interested */
+	if (tdev->pipe.plane.fb != fb)
+		goto out_unlock;
+
+	tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width,
+			    fb->height);
+
+	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
+		  clip.x1, clip.x2, clip.y1, clip.y2);
+
+	ret = st7586_mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip);
+	if (ret)
+		goto out_unlock;
+
+	/* NB: st7586_mipi_dbi_buf_copy() modifies clip */
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
+			 (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF,
+			 (clip.x2 >> 8) & 0xFF, (clip.x2 - 1) & 0xFF);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
+			 (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
+			 (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
+
+	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
+				   (u8 *)mipi->tx_buf,
+				   (clip.x2 - clip.x1) * (clip.y2 - clip.y1));
+
+out_unlock:
+	mutex_unlock(&tdev->dirty_lock);
+
+	if (ret)
+		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
+			     ret);
+
+	return ret;
+}
+
+static const struct drm_framebuffer_funcs st7586_mipi_dbi_fb_funcs = {
+	.destroy	= drm_fb_cma_destroy,
+	.create_handle	= drm_fb_cma_create_handle,
+	.dirty		= st7586_mipi_dbi_fb_dirty,
+};
+
+void st7586_mipi_dbi_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 drm_framebuffer *fb = pipe->plane.fb;
+
+	DRM_DEBUG_KMS("\n");
+
+	mipi->enabled = true;
+	if (fb)
+		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+
+	tinydrm_enable_backlight(mipi->backlight);
+}
+
+static void st7586_mipi_dbi_blank(struct mipi_dbi *mipi)
+{
+	struct drm_device *drm = mipi->tinydrm.drm;
+	u16 height = drm->mode_config.min_height;
+	u16 width = (drm->mode_config.min_width + 2) / 3;
+	size_t len = width * height;
+
+	memset(mipi->tx_buf, 0, len);
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS, 0, 0,
+			 (width >> 8) & 0xFF, (width - 1) & 0xFF);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS, 0, 0,
+			 (height >> 8) & 0xFF, (height - 1) & 0xFF);
+	mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
+			     (u8 *)mipi->tx_buf, len);
+}
+
+static void st7586_mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+
+	DRM_DEBUG_KMS("\n");
+
+	mipi->enabled = false;
+
+	if (mipi->backlight)
+		tinydrm_disable_backlight(mipi->backlight);
+	else
+		st7586_mipi_dbi_blank(mipi);
+}
+
+static const u32 st7586_mipi_dbi_formats[] = {
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+};
+
+static int st7586_mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
+		const struct drm_simple_display_pipe_funcs *pipe_funcs,
+		struct drm_driver *driver, const struct drm_display_mode *mode,
+		unsigned int rotation)
+{
+	size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
+	struct tinydrm_device *tdev = &mipi->tinydrm;
+	int ret;
+
+	if (!mipi->command)
+		return -EINVAL;
+
+	mutex_init(&mipi->cmdlock);
+
+	mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL);
+	if (!mipi->tx_buf)
+		return -ENOMEM;
+
+	ret = devm_tinydrm_init(dev, tdev, &st7586_mipi_dbi_fb_funcs, driver);
+	if (ret)
+		return ret;
+
+	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
+					DRM_MODE_CONNECTOR_VIRTUAL,
+					st7586_mipi_dbi_formats,
+					ARRAY_SIZE(st7586_mipi_dbi_formats),
+					mode, rotation);
+	if (ret)
+		return ret;
+
+	tdev->drm->mode_config.preferred_depth = 16;
+	mipi->rotation = rotation;
+
+	drm_mode_config_reset(tdev->drm);
+
+	DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",
+		      tdev->drm->mode_config.preferred_depth, rotation);
+
+	return 0;
+}
+
+static int st7586_init(struct mipi_dbi *mipi)
+{
+	struct tinydrm_device *tdev = &mipi->tinydrm;
+	struct device *dev = tdev->drm->dev;
+	u8 addr_mode;
+	int ret;
+
+	DRM_DEBUG_KMS("\n");
+
+	ret = regulator_enable(mipi->regulator);
+	if (ret) {
+		dev_err(dev, "Failed to enable regulator %d\n", ret);
+		return ret;
+	}
+
+	/* Avoid flicker by skipping setup if the bootloader has done it */
+	if (mipi_dbi_display_is_on(mipi))
+		return 0;
+
+	mipi_dbi_hw_reset(mipi);
+	ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
+	if (ret) {
+		dev_err(dev, "Error sending command %d\n", ret);
+		regulator_disable(mipi->regulator);
+		return ret;
+	}
+
+	mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
+
+	msleep(10);
+
+	mipi_dbi_command(mipi, ST7586_OTP_READ);
+
+	msleep(20);
+
+	mipi_dbi_command(mipi, ST7586_OTP_CTRL_OUT);
+	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
+
+	msleep(50);
+
+	mipi_dbi_command(mipi, ST7586_SET_VOP_OFFSET, 0x00);
+	mipi_dbi_command(mipi, ST7586_SET_VOP, 0xe3, 0x00);
+	mipi_dbi_command(mipi, ST7586_SET_BIAS_SYSTEM, 0x02);
+	mipi_dbi_command(mipi, ST7586_SET_BOOST_LEVEL, 0x04);
+	mipi_dbi_command(mipi, ST7586_ENABLE_ANALOG, 0x1d);
+	mipi_dbi_command(mipi, ST7586_SET_NLINE_INV, 0x00);
+	mipi_dbi_command(mipi, ST7586_DISP_MODE_GRAY);
+	mipi_dbi_command(mipi, ST7586_ENABLE_DDRAM, 0x02);
+
+	switch (mipi->rotation) {
+	default:
+		addr_mode = 0x00;
+		break;
+	case 90:
+		addr_mode = ST7586_DISP_CTRL_MY;
+		break;
+	case 180:
+		addr_mode = ST7586_DISP_CTRL_MX | ST7586_DISP_CTRL_MY;
+		break;
+	case 270:
+		addr_mode = ST7586_DISP_CTRL_MX;
+		break;
+	}
+	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+
+	mipi_dbi_command(mipi, ST7586_SET_DISP_DUTY, 0x7f);
+	mipi_dbi_command(mipi, ST7586_SET_PART_DISP, 0xa0);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PARTIAL_AREA, 0x00, 0x00, 0x00, 0x77);
+	mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
+
+	msleep(100);
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+	return 0;
+}
+
+static void st7586_fini(void *data)
+{
+	struct mipi_dbi *mipi = data;
+
+	DRM_DEBUG_KMS("\n");
+	regulator_disable(mipi->regulator);
+}
+
+static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
+	.enable = st7586_mipi_dbi_pipe_enable,
+	.disable = st7586_mipi_dbi_pipe_disable,
+	.update = tinydrm_display_pipe_update,
+	.prepare_fb = tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode st7586_mode = {
+	TINYDRM_MODE(178, 128, 37, 27),
+};
+
+DEFINE_DRM_GEM_CMA_FOPS(st7586_fops);
+
+static struct drm_driver st7586_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
+				  DRIVER_ATOMIC,
+	.fops			= &st7586_fops,
+	TINYDRM_GEM_DRIVER_OPS,
+	.lastclose		= tinydrm_lastclose,
+	.debugfs_init		= mipi_dbi_debugfs_init,
+	.name			= "st7586",
+	.desc			= "Sitronix ST7586",
+	.date			= "20170801",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static const struct of_device_id st7586_of_match[] = {
+	{ .compatible = "lego,ev3-lcd" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st7586_of_match);
+
+static const struct spi_device_id st7586_id[] = {
+	{ "ev3-lcd", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, st7586_id);
+
+static int st7586_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct tinydrm_device *tdev;
+	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_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(mipi->reset)) {
+		dev_err(dev, "Failed to get gpio 'reset'\n");
+		return PTR_ERR(mipi->reset);
+	}
+
+	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dc)) {
+		dev_err(dev, "Failed to get gpio 'dc'\n");
+		return PTR_ERR(dc);
+	}
+
+	mipi->regulator = devm_regulator_get(dev, "power");
+	if (IS_ERR(mipi->regulator))
+		return PTR_ERR(mipi->regulator);
+
+	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;
+
+	/*
+	 * we are using 8-bit data, so we are not actually swapping anything,
+	 * but setting mipi->swap_bytes makes mipi_dbi_typec3_command() do the
+	 * right thing and not use 16-bit transfers (which results in swapped
+	 * bytes on little-endian systems and causes out of order data to be
+	 * sent to the display).
+	 */
+	mipi->swap_bytes = true;
+
+	ret = st7586_mipi_dbi_init(&spi->dev, mipi, &st7586_pipe_funcs,
+				   &st7586_driver, &st7586_mode, rotation);
+	if (ret)
+		return ret;
+
+	ret = st7586_init(mipi);
+	if (ret)
+		return ret;
+
+	/* use devres to fini after drm unregister (drv->remove is before) */
+	ret = devm_add_action(dev, st7586_fini, mipi);
+	if (ret) {
+		st7586_fini(mipi);
+		return ret;
+	}
+
+	tdev = &mipi->tinydrm;
+
+	ret = devm_tinydrm_register(tdev);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, mipi);
+
+	DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",
+			 tdev->drm->driver->name, dev_name(dev),
+			 spi->max_speed_hz / 1000000,
+			 tdev->drm->primary->index);
+
+	return 0;
+}
+
+static void st7586_shutdown(struct spi_device *spi)
+{
+	struct mipi_dbi *mipi = spi_get_drvdata(spi);
+
+	tinydrm_shutdown(&mipi->tinydrm);
+}
+
+static int __maybe_unused st7586_pm_suspend(struct device *dev)
+{
+	struct mipi_dbi *mipi = dev_get_drvdata(dev);
+	int ret;
+
+	ret = tinydrm_suspend(&mipi->tinydrm);
+	if (ret)
+		return ret;
+
+	st7586_fini(mipi);
+
+	return 0;
+}
+
+static int __maybe_unused st7586_pm_resume(struct device *dev)
+{
+	struct mipi_dbi *mipi = dev_get_drvdata(dev);
+	int ret;
+
+	ret = st7586_init(mipi);
+	if (ret)
+		return ret;
+
+	return tinydrm_resume(&mipi->tinydrm);
+}
+
+static const struct dev_pm_ops st7586_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(st7586_pm_suspend, st7586_pm_resume)
+};
+
+static struct spi_driver st7586_spi_driver = {
+	.driver = {
+		.name = "st7586",
+		.owner = THIS_MODULE,
+		.of_match_table = st7586_of_match,
+		.pm = &st7586_pm_ops,
+	},
+	.id_table = st7586_id,
+	.probe = st7586_probe,
+	.shutdown = st7586_shutdown,
+};
+module_spi_driver(st7586_spi_driver);
+
+MODULE_DESCRIPTION("Sitronix ST7586 DRM driver");
+MODULE_AUTHOR("David Lechner <david@lechnology.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/drm/tinydrm/st7586.h b/include/drm/tinydrm/st7586.h
new file mode 100644
index 0000000..18fb56b
--- /dev/null
+++ b/include/drm/tinydrm/st7586.h
@@ -0,0 +1,34 @@
+/*
+ * ST7586 LCD controller
+ *
+ * Copyright (C) 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.
+ */
+
+#ifndef __LINUX_ST7856_H
+#define __LINUX_ST7856_H
+
+#define ST7586_DISP_MODE_GRAY	0x38
+#define ST7586_DISP_MODE_MONO	0x39
+#define ST7586_ENABLE_DDRAM	0x3a
+#define ST7586_SET_DISP_DUTY	0xb0
+#define ST7586_SET_PART_DISP	0xb4
+#define ST7586_SET_NLINE_INV	0xb5
+#define ST7586_SET_VOP		0xc0
+#define ST7586_SET_BIAS_SYSTEM	0xc3
+#define ST7586_SET_BOOST_LEVEL	0xc4
+#define ST7586_SET_VOP_OFFSET	0xc7
+#define ST7586_ENABLE_ANALOG	0xd0
+#define ST7586_AUTO_READ_CTRL	0xd7
+#define ST7586_OTP_RW_CTRL	0xe0
+#define ST7586_OTP_CTRL_OUT	0xe1
+#define ST7586_OTP_READ		0xe3
+
+#define ST7586_DISP_CTRL_MX	BIT(6)
+#define ST7586_DISP_CTRL_MY	BIT(7)
+
+#endif /* __LINUX_ST7856_H */
-- 
2.7.4

[PATCH v2 3/4] ARM: dts: da850-lego-ev3: Add node for LCD display

[David Lechner]

This adds a new node for the LEGO MINDSTORMS EV3 LCD display.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/boot/dts/da850-lego-ev3.dts | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/da850-lego-ev3.dts b/arch/arm/boot/dts/da850-lego-ev3.dts
index 45983c0..249b317 100644
--- a/arch/arm/boot/dts/da850-lego-ev3.dts
+++ b/arch/arm/boot/dts/da850-lego-ev3.dts
@@ -249,6 +249,15 @@
 			0x4c 0x00000080 0x000000f0
 		>;
 	};
+
+	ev3_lcd_pins: pinmux_lcd {
+		pinctrl-single,bits = <
+			/* SIMO, GP2[11], GP2[12], CLK */
+			0x14 0x00188100 0x00ffff00
+			/* GP5[0] */
+			0x30 0x80000000 0xf0000000
+		>;
+	};
 };
 
 &pinconf {
@@ -357,6 +366,21 @@
 	};
 };
 
+&spi1 {
+	status = "okay";
+	pinctrl-0 = <&ev3_lcd_pins>;
+	pinctrl-names = "default";
+	cs-gpios = <&gpio 44 GPIO_ACTIVE_LOW>;
+
+	display@0{
+		compatible = "lego,ev3-lcd";
+		reg = <0>;
+		spi-max-frequency = <10000000>;
+		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
+	};
+};
+
 &ehrpwm0 {
 	status = "okay";
 };
-- 
2.7.4

[PATCH v2 4/4] ARM: davinci_all_defconfig: enable tinydrm and ST7586

[David Lechner]

This enables the tinydrm and ST7586 panel modules used by the display
on LEGO MINDSTORMS EV3.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/configs/davinci_all_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
index 06e2e2a..27d9720 100644
--- a/arch/arm/configs/davinci_all_defconfig
+++ b/arch/arm/configs/davinci_all_defconfig
@@ -143,6 +143,8 @@ CONFIG_VIDEO_ADV7343=m
 CONFIG_DRM=m
 CONFIG_DRM_TILCDC=m
 CONFIG_DRM_DUMB_VGA_DAC=m
+CONFIG_DRM_TINYDRM=m
+CONFIG_TINYDRM_ST7586=m
 CONFIG_FB=y
 CONFIG_FIRMWARE_EDID=y
 CONFIG_FB_DA8XX=y
-- 
2.7.4

Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[Noralf Trønnes]

Den 02.08.2017 00.11, skrev David Lechner:
> LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
> module for the ST7586 controller with parameters for the EV3 LCD display.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>   .../devicetree/bindings/display/st7586.txt         |  26 +
>   drivers/gpu/drm/tinydrm/Kconfig                    |  10 +
>   drivers/gpu/drm/tinydrm/Makefile                   |   1 +
>   drivers/gpu/drm/tinydrm/st7586.c                   | 579 +++++++++++++++++++++
>   include/drm/tinydrm/st7586.h                       |  34 ++

Please add a MAINTAINERS entry.
That way you'll be notified on changes.

>   5 files changed, 650 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/display/st7586.txt
>   create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
>   create mode 100644 include/drm/tinydrm/st7586.h
>
> diff --git a/Documentation/devicetree/bindings/display/st7586.txt b/Documentation/devicetree/bindings/display/st7586.txt
> new file mode 100644
> index 0000000..acf784aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/st7586.txt
> @@ -0,0 +1,26 @@
> +Sitronix ST7586 display panel
> +
> +Required properties:
> +- compatible:	"lego,ev3-lcd".
> +
> +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:
> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
> +		the panel interface mode (IM[3:0] pins):
> +		- present: IM=x110 4-wire 8-bit data serial interface
> +		- absent:  IM=x101 3-wire 9-bit data serial interface

This doesn't match the ST7586 datasheet I have, the pins are called
IF[3:1] and the values are different.

> +- reset-gpios:	Reset pin
> +- power-supply:	A regulator node for the supply voltage.
> +- backlight:	phandle of the backlight device attached to the panel
> +- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> +
> +Example:
> +	display@0{
> +		compatible = "lego,ev3-lcd";
> +		reg = <0>;
> +		spi-max-frequency = <10000000>;
> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> +	};
> diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
> index f17c3ca..2e790e7 100644
> --- a/drivers/gpu/drm/tinydrm/Kconfig
> +++ b/drivers/gpu/drm/tinydrm/Kconfig
> @@ -32,3 +32,13 @@ config TINYDRM_REPAPER
>   	  2.71" TFT EPD Panel (E2271CS021)
>   
>   	  If M is selected the module will be called repaper.
> +
> +config TINYDRM_ST7586
> +	tristate "DRM support for Sitronix ST7586 display panels"
> +	depends on DRM_TINYDRM && SPI
> +	select TINYDRM_MIPI_DBI
> +	help
> +	  DRM driver for the following Sitronix ST7586 panels:
> +	  * LEGO MINDSTORMS EV3
> +
> +	  If M is selected the module will be called st7586.
> diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
> index 95bb4d4..0c184bd 100644
> --- a/drivers/gpu/drm/tinydrm/Makefile
> +++ b/drivers/gpu/drm/tinydrm/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
>   # Displays
>   obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
>   obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
> +obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> new file mode 100644
> index 0000000..6093021
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -0,0 +1,579 @@
> +/*
> + * DRM driver for Sitronix ST7586 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/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <video/mipi_display.h>
> +
> +#include <drm/tinydrm/mipi-dbi.h>
> +#include <drm/tinydrm/st7586.h>
> +#include <drm/tinydrm/tinydrm-helpers.h>
> +
> +static inline u8 st7586_rgb565_to_gray2(u16 rgb)
> +{
> +	u8 r = (rgb & 0xf800) >> 11;
> +	u8 g = (rgb & 0x07e0) >> 5;
> +	u8 b = rgb & 0x001f;
> +	/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> +	/* g is already * 2 because it is 6-bit */
> +	u8 gray5 = (3 * r + 3 * g + b) / 10;
> +
> +	return gray5 >> 3;
> +}
> +
> +static void st7586_from_rgb565(u8 *dst, void *vaddr,
> +			       struct drm_framebuffer *fb,
> +			       struct drm_clip_rect *clip)
> +{
> +	size_t len;
> +	unsigned int x, y;
> +	u16 *src, *buf;
> +	u8 val;
> +
> +	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> +	clip->x1 = clip->x1 / 3 * 3;
> +	clip->x2 = (clip->x2 + 2) / 3 * 3;
> +	len = (clip->x2 - clip->x1) * sizeof(u16);
> +
> +	buf = kmalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	for (y = clip->y1; y < clip->y2; y++) {
> +		src = vaddr + (y * fb->pitches[0]);
> +		src += clip->x1;
> +		memcpy(buf, src, len);
> +		src = buf;
> +		for (x = clip->x1; x < clip->x2; x += 3) {
> +			val = st7586_rgb565_to_gray2(*src++) << 6;
> +			if (val & 0xC0)
> +				val |= 0x20;
> +			val |= st7586_rgb565_to_gray2(*src++) << 3;
> +			if (val & 0x18)
> +				val |= 0x04;
> +			val |= st7586_rgb565_to_gray2(*src++);
> +			*dst++ = ~val;
> +		}
> +	}
> +
> +	/* now adjust the clip so it applies to dst */
> +	clip->x1 /= 3;
> +	clip->x2 /= 3;
> +
> +	kfree(buf);
> +}
> +
> +static inline u8 st7586_xrgb8888_to_gray2(u32 rgb)
> +{
> +	u8 r = (rgb & 0x00ff0000) >> 16;
> +	u8 g = (rgb & 0x0000ff00) >> 8;
> +	u8 b = rgb & 0x000000ff;
> +	/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> +	u8 gray8 = (3 * r + 6 * g + b) / 10;
> +
> +	return gray8 >> 6;
> +}
> +
> +static void st7586_from_xrgb8888(u8 *dst, void *vaddr,
> +				 struct drm_framebuffer *fb,
> +				 struct drm_clip_rect *clip)
> +{
> +	size_t len;
> +	unsigned int x, y;
> +	u32 *src, *buf;
> +	u8 val;
> +
> +	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> +	clip->x1 = clip->x1 / 3 * 3;
> +	clip->x2 = (clip->x2 + 2) / 3 * 3;
> +	len = (clip->x2 - clip->x1) * sizeof(u32);
> +
> +	buf = kmalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	for (y = clip->y1; y < clip->y2; y++) {
> +		src = vaddr + (y * fb->pitches[0]);
> +		src += clip->x1;
> +		memcpy(buf, src, len);
> +		src = buf;
> +		for (x = clip->x1; x < clip->x2; x += 3) {
> +			val = st7586_xrgb8888_to_gray2(*src++) << 6;
> +			if (val & 0xC0)
> +				val |= 0x20;
> +			val |= st7586_xrgb8888_to_gray2(*src++) << 3;
> +			if (val & 0x18)
> +				val |= 0x04;
> +			val |= st7586_xrgb8888_to_gray2(*src++);
> +			*dst++ = ~val;
> +		}
> +	}
> +
> +	/* now adjust the clip so it applies to dst */
> +	clip->x1 /= 3;
> +	clip->x2 /= 3;
> +
> +	kfree(buf);
> +}

Please use tinydrm_xrgb8888_to_gray8().

> +
> +static int st7586_mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,

You can shorten your st7586_mipi_dbi_* to just st7586_*

> +				    struct drm_clip_rect *clip)
> +{
> +	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> +	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
> +	struct drm_format_name_buf format_name;
> +	void *src = cma_obj->vaddr;
> +	int ret = 0;
> +
> +	if (import_attach) {
> +		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> +					       DMA_FROM_DEVICE);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (fb->format->format) {
> +	case DRM_FORMAT_RGB565:
> +		st7586_from_rgb565(dst, src, fb, clip);
> +		break;
> +	case DRM_FORMAT_XRGB8888:
> +		st7586_from_xrgb8888(dst, src, fb, clip);
> +		break;
> +	default:
> +		dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
> +			     drm_get_format_name(fb->format->format,
> +						 &format_name));
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	if (import_attach)
> +		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> +					     DMA_FROM_DEVICE);
> +	return ret;
> +}
> +
> +static int st7586_mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
> +				    struct drm_file *file_priv,
> +				    unsigned int flags, unsigned int color,
> +				    struct drm_clip_rect *clips,
> +				    unsigned int num_clips)
> +{
> +	struct tinydrm_device *tdev = fb->dev->dev_private;
> +	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> +	struct drm_clip_rect clip;
> +	int ret = 0;
> +
> +	mutex_lock(&tdev->dirty_lock);
> +
> +	if (!mipi->enabled)
> +		goto out_unlock;
> +
> +	/* fbdev can flush even when we're not interested */
> +	if (tdev->pipe.plane.fb != fb)
> +		goto out_unlock;
> +
> +	tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width,
> +			    fb->height);
> +
> +	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
> +		  clip.x1, clip.x2, clip.y1, clip.y2);
> +
> +	ret = st7586_mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip);
> +	if (ret)
> +		goto out_unlock;
> +
> +	/* NB: st7586_mipi_dbi_buf_copy() modifies clip */
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
> +			 (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF,
> +			 (clip.x2 >> 8) & 0xFF, (clip.x2 - 1) & 0xFF);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
> +			 (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
> +			 (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
> +
> +	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
> +				   (u8 *)mipi->tx_buf,
> +				   (clip.x2 - clip.x1) * (clip.y2 - clip.y1));
> +
> +out_unlock:
> +	mutex_unlock(&tdev->dirty_lock);
> +
> +	if (ret)
> +		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
> +			     ret);
> +
> +	return ret;
> +}
> +
> +static const struct drm_framebuffer_funcs st7586_mipi_dbi_fb_funcs = {
> +	.destroy	= drm_fb_cma_destroy,
> +	.create_handle	= drm_fb_cma_create_handle,
> +	.dirty		= st7586_mipi_dbi_fb_dirty,
> +};
> +
> +void st7586_mipi_dbi_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 drm_framebuffer *fb = pipe->plane.fb;
> +
> +	DRM_DEBUG_KMS("\n");
> +
You should use this function to enable the regulator and init the
controller. Don't look at mi0283qt it should have been fixed.

> +	mipi->enabled = true;
> +	if (fb)

I don't think it's possible to enable the pipe without a framebuffer,
but I wasn't able find out. drm is complex...

> +		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
> +
> +	tinydrm_enable_backlight(mipi->backlight);
> +}
> +
> +static void st7586_mipi_dbi_blank(struct mipi_dbi *mipi)
> +{
> +	struct drm_device *drm = mipi->tinydrm.drm;
> +	u16 height = drm->mode_config.min_height;
> +	u16 width = (drm->mode_config.min_width + 2) / 3;
> +	size_t len = width * height;
> +
> +	memset(mipi->tx_buf, 0, len);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS, 0, 0,
> +			 (width >> 8) & 0xFF, (width - 1) & 0xFF);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS, 0, 0,
> +			 (height >> 8) & 0xFF, (height - 1) & 0xFF);
> +	mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
> +			     (u8 *)mipi->tx_buf, len);
> +}
> +
> +static void st7586_mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> +	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
> +	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	mipi->enabled = false;
> +
> +	if (mipi->backlight)
> +		tinydrm_disable_backlight(mipi->backlight);
> +	else
> +		st7586_mipi_dbi_blank(mipi);

And disable the regulator here.

> +}
> +
> +static const u32 st7586_mipi_dbi_formats[] = {
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,

Why 2 emulation formats?
I chose DRM_FORMAT_XRGB8888 since I expected everything to support it.
Please use only that.

> +};
> +
> +static int st7586_mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
> +		const struct drm_simple_display_pipe_funcs *pipe_funcs,
> +		struct drm_driver *driver, const struct drm_display_mode *mode,
> +		unsigned int rotation)
> +{
> +	size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
> +	struct tinydrm_device *tdev = &mipi->tinydrm;
> +	int ret;
> +
> +	if (!mipi->command)
> +		return -EINVAL;

You know this is set after calling mipi_dbi_spi_init(), so you don't 
have to check.

> +
> +	mutex_init(&mipi->cmdlock);
> +
> +	mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL);
> +	if (!mipi->tx_buf)
> +		return -ENOMEM;
> +
> +	ret = devm_tinydrm_init(dev, tdev, &st7586_mipi_dbi_fb_funcs, driver);
> +	if (ret)
> +		return ret;
> +
> +	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
> +					DRM_MODE_CONNECTOR_VIRTUAL,
> +					st7586_mipi_dbi_formats,
> +					ARRAY_SIZE(st7586_mipi_dbi_formats),
> +					mode, rotation);
> +	if (ret)
> +		return ret;
> +
> +	tdev->drm->mode_config.preferred_depth = 16;
> +	mipi->rotation = rotation;
> +
> +	drm_mode_config_reset(tdev->drm);
> +
> +	DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",
> +		      tdev->drm->mode_config.preferred_depth, rotation);
> +
> +	return 0;
> +}
> +
> +static int st7586_init(struct mipi_dbi *mipi)
> +{
> +	struct tinydrm_device *tdev = &mipi->tinydrm;
> +	struct device *dev = tdev->drm->dev;
> +	u8 addr_mode;
> +	int ret;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	ret = regulator_enable(mipi->regulator);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulator %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Avoid flicker by skipping setup if the bootloader has done it */
> +	if (mipi_dbi_display_is_on(mipi))

Looking at the datasheet it seems that it's not possible to read from
the controller via SPI?

> +		return 0;
> +
> +	mipi_dbi_hw_reset(mipi);
> +	ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
> +	if (ret) {
> +		dev_err(dev, "Error sending command %d\n", ret);
> +		regulator_disable(mipi->regulator);
> +		return ret;
> +	}
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
> +
> +	msleep(10);
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_READ);
> +
> +	msleep(20);
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_CTRL_OUT);
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> +
> +	msleep(50);
> +
> +	mipi_dbi_command(mipi, ST7586_SET_VOP_OFFSET, 0x00);
> +	mipi_dbi_command(mipi, ST7586_SET_VOP, 0xe3, 0x00);
> +	mipi_dbi_command(mipi, ST7586_SET_BIAS_SYSTEM, 0x02);
> +	mipi_dbi_command(mipi, ST7586_SET_BOOST_LEVEL, 0x04);
> +	mipi_dbi_command(mipi, ST7586_ENABLE_ANALOG, 0x1d);
> +	mipi_dbi_command(mipi, ST7586_SET_NLINE_INV, 0x00);
> +	mipi_dbi_command(mipi, ST7586_DISP_MODE_GRAY);
> +	mipi_dbi_command(mipi, ST7586_ENABLE_DDRAM, 0x02);
> +
> +	switch (mipi->rotation) {
> +	default:
> +		addr_mode = 0x00;
> +		break;
> +	case 90:
> +		addr_mode = ST7586_DISP_CTRL_MY;
> +		break;
> +	case 180:
> +		addr_mode = ST7586_DISP_CTRL_MX | ST7586_DISP_CTRL_MY;
> +		break;
> +	case 270:
> +		addr_mode = ST7586_DISP_CTRL_MX;
> +		break;
> +	}
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> +
> +	mipi_dbi_command(mipi, ST7586_SET_DISP_DUTY, 0x7f);
> +	mipi_dbi_command(mipi, ST7586_SET_PART_DISP, 0xa0);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PARTIAL_AREA, 0x00, 0x00, 0x00, 0x77);
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
> +
> +	msleep(100);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> +
> +	return 0;
> +}
> +
> +static void st7586_fini(void *data)
> +{
> +	struct mipi_dbi *mipi = data;
> +
> +	DRM_DEBUG_KMS("\n");
> +	regulator_disable(mipi->regulator);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
> +	.enable = st7586_mipi_dbi_pipe_enable,
> +	.disable = st7586_mipi_dbi_pipe_disable,
> +	.update = tinydrm_display_pipe_update,
> +	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_display_mode st7586_mode = {
> +	TINYDRM_MODE(178, 128, 37, 27),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(st7586_fops);
> +
> +static struct drm_driver st7586_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
> +				  DRIVER_ATOMIC,
> +	.fops			= &st7586_fops,
> +	TINYDRM_GEM_DRIVER_OPS,
> +	.lastclose		= tinydrm_lastclose,
> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "st7586",
> +	.desc			= "Sitronix ST7586",
> +	.date			= "20170801",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static const struct of_device_id st7586_of_match[] = {
> +	{ .compatible = "lego,ev3-lcd" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, st7586_of_match);
> +
> +static const struct spi_device_id st7586_id[] = {
> +	{ "ev3-lcd", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(spi, st7586_id);
> +
> +static int st7586_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct tinydrm_device *tdev;
> +	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_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(mipi->reset)) {
> +		dev_err(dev, "Failed to get gpio 'reset'\n");
> +		return PTR_ERR(mipi->reset);
> +	}
> +
> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc)) {
> +		dev_err(dev, "Failed to get gpio 'dc'\n");
> +		return PTR_ERR(dc);
> +	}
> +
> +	mipi->regulator = devm_regulator_get(dev, "power");
> +	if (IS_ERR(mipi->regulator))
> +		return PTR_ERR(mipi->regulator);
> +
> +	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;

Since you are using mipi_dbi_debugfs_init and the controller can't be
read, disable reading:

mipi->read_commands = NULL;

> +
> +	/*
> +	 * we are using 8-bit data, so we are not actually swapping anything,
> +	 * but setting mipi->swap_bytes makes mipi_dbi_typec3_command() do the
> +	 * right thing and not use 16-bit transfers (which results in swapped
> +	 * bytes on little-endian systems and causes out of order data to be
> +	 * sent to the display).
> +	 */
> +	mipi->swap_bytes = true;
> +
> +	ret = st7586_mipi_dbi_init(&spi->dev, mipi, &st7586_pipe_funcs,
> +				   &st7586_driver, &st7586_mode, rotation);
> +	if (ret)
> +		return ret;
> +
> +	ret = st7586_init(mipi);

This isn't necessary when you use .enable/.disable to control power/init,
and you can drop the devm_add_action.

> +	if (ret)
> +		return ret;
> +
> +	/* use devres to fini after drm unregister (drv->remove is before) */
> +	ret = devm_add_action(dev, st7586_fini, mipi);
> +	if (ret) {
> +		st7586_fini(mipi);
> +		return ret;
> +	}
> +
> +	tdev = &mipi->tinydrm;
> +
> +	ret = devm_tinydrm_register(tdev);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, mipi);
> +
> +	DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",
> +			 tdev->drm->driver->name, dev_name(dev),
> +			 spi->max_speed_hz / 1000000,
> +			 tdev->drm->primary->index);
> +
> +	return 0;
> +}
> +
> +static void st7586_shutdown(struct spi_device *spi)
> +{
> +	struct mipi_dbi *mipi = spi_get_drvdata(spi);
> +
> +	tinydrm_shutdown(&mipi->tinydrm);
> +}
> +
> +static int __maybe_unused st7586_pm_suspend(struct device *dev)
> +{
> +	struct mipi_dbi *mipi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = tinydrm_suspend(&mipi->tinydrm);
> +	if (ret)
> +		return ret;
> +
> +	st7586_fini(mipi);
> +

With .enable/.disable controlling power, tinydrm_suspend() is enough.

> +	return 0;
> +}
> +
> +static int __maybe_unused st7586_pm_resume(struct device *dev)
> +{
> +	struct mipi_dbi *mipi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = st7586_init(mipi);
> +	if (ret)
> +		return ret;
> +

Same goes here.

> +	return tinydrm_resume(&mipi->tinydrm);
> +}
> +
> +static const struct dev_pm_ops st7586_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(st7586_pm_suspend, st7586_pm_resume)
> +};
> +
> +static struct spi_driver st7586_spi_driver = {
> +	.driver = {
> +		.name = "st7586",
> +		.owner = THIS_MODULE,
> +		.of_match_table = st7586_of_match,
> +		.pm = &st7586_pm_ops,
> +	},
> +	.id_table = st7586_id,
> +	.probe = st7586_probe,
> +	.shutdown = st7586_shutdown,
> +};
> +module_spi_driver(st7586_spi_driver);
> +
> +MODULE_DESCRIPTION("Sitronix ST7586 DRM driver");
> +MODULE_AUTHOR("David Lechner <david@lechnology.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/tinydrm/st7586.h b/include/drm/tinydrm/st7586.h
> new file mode 100644
> index 0000000..18fb56b
> --- /dev/null
> +++ b/include/drm/tinydrm/st7586.h

Please put this in drivers/drm/tinydrm, or you can even put the defines
in the driver if you want, it's quite short.
ili9341.h shouldn't have ended up in include/drm/tinydrm.

Noralf.

> @@ -0,0 +1,34 @@
> +/*
> + * ST7586 LCD controller
> + *
> + * Copyright (C) 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.
> + */
> +
> +#ifndef __LINUX_ST7856_H
> +#define __LINUX_ST7856_H
> +
> +#define ST7586_DISP_MODE_GRAY	0x38
> +#define ST7586_DISP_MODE_MONO	0x39
> +#define ST7586_ENABLE_DDRAM	0x3a
> +#define ST7586_SET_DISP_DUTY	0xb0
> +#define ST7586_SET_PART_DISP	0xb4
> +#define ST7586_SET_NLINE_INV	0xb5
> +#define ST7586_SET_VOP		0xc0
> +#define ST7586_SET_BIAS_SYSTEM	0xc3
> +#define ST7586_SET_BOOST_LEVEL	0xc4
> +#define ST7586_SET_VOP_OFFSET	0xc7
> +#define ST7586_ENABLE_ANALOG	0xd0
> +#define ST7586_AUTO_READ_CTRL	0xd7
> +#define ST7586_OTP_RW_CTRL	0xe0
> +#define ST7586_OTP_CTRL_OUT	0xe1
> +#define ST7586_OTP_READ		0xe3
> +
> +#define ST7586_DISP_CTRL_MX	BIT(6)
> +#define ST7586_DISP_CTRL_MY	BIT(7)
> +
> +#endif /* __LINUX_ST7856_H */

Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[Noralf Trønnes]

Den 02.08.2017 00.11, skrev David Lechner:
> LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
> module for the ST7586 controller with parameters for the EV3 LCD display.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>   .../devicetree/bindings/display/st7586.txt         |  26 +

I forgot, the binding doc should be a separate patch.

Noralf.

>   drivers/gpu/drm/tinydrm/Kconfig                    |  10 +
>   drivers/gpu/drm/tinydrm/Makefile                   |   1 +
>   drivers/gpu/drm/tinydrm/st7586.c                   | 579 +++++++++++++++++++++
>   include/drm/tinydrm/st7586.h                       |  34 ++
>   5 files changed, 650 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/display/st7586.txt
>   create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
>   create mode 100644 include/drm/tinydrm/st7586.h
>
> diff --git a/Documentation/devicetree/bindings/display/st7586.txt b/Documentation/devicetree/bindings/display/st7586.txt
> new file mode 100644
> index 0000000..acf784aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/st7586.txt
> @@ -0,0 +1,26 @@
> +Sitronix ST7586 display panel
> +
> +Required properties:
> +- compatible:	"lego,ev3-lcd".
> +
> +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:
> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
> +		the panel interface mode (IM[3:0] pins):
> +		- present: IM=x110 4-wire 8-bit data serial interface
> +		- absent:  IM=x101 3-wire 9-bit data serial interface
> +- reset-gpios:	Reset pin
> +- power-supply:	A regulator node for the supply voltage.
> +- backlight:	phandle of the backlight device attached to the panel
> +- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> +
> +Example:
> +	display@0{
> +		compatible = "lego,ev3-lcd";
> +		reg = <0>;
> +		spi-max-frequency = <10000000>;
> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> +	};
> diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
> index f17c3ca..2e790e7 100644
> --- a/drivers/gpu/drm/tinydrm/Kconfig
> +++ b/drivers/gpu/drm/tinydrm/Kconfig
> @@ -32,3 +32,13 @@ config TINYDRM_REPAPER
>   	  2.71" TFT EPD Panel (E2271CS021)
>   
>   	  If M is selected the module will be called repaper.
> +
> +config TINYDRM_ST7586
> +	tristate "DRM support for Sitronix ST7586 display panels"
> +	depends on DRM_TINYDRM && SPI
> +	select TINYDRM_MIPI_DBI
> +	help
> +	  DRM driver for the following Sitronix ST7586 panels:
> +	  * LEGO MINDSTORMS EV3
> +
> +	  If M is selected the module will be called st7586.
> diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
> index 95bb4d4..0c184bd 100644
> --- a/drivers/gpu/drm/tinydrm/Makefile
> +++ b/drivers/gpu/drm/tinydrm/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
>   # Displays
>   obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
>   obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
> +obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> new file mode 100644
> index 0000000..6093021
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -0,0 +1,579 @@
> +/*
> + * DRM driver for Sitronix ST7586 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/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <video/mipi_display.h>
> +
> +#include <drm/tinydrm/mipi-dbi.h>
> +#include <drm/tinydrm/st7586.h>
> +#include <drm/tinydrm/tinydrm-helpers.h>
> +
> +static inline u8 st7586_rgb565_to_gray2(u16 rgb)
> +{
> +	u8 r = (rgb & 0xf800) >> 11;
> +	u8 g = (rgb & 0x07e0) >> 5;
> +	u8 b = rgb & 0x001f;
> +	/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> +	/* g is already * 2 because it is 6-bit */
> +	u8 gray5 = (3 * r + 3 * g + b) / 10;
> +
> +	return gray5 >> 3;
> +}
> +
> +static void st7586_from_rgb565(u8 *dst, void *vaddr,
> +			       struct drm_framebuffer *fb,
> +			       struct drm_clip_rect *clip)
> +{
> +	size_t len;
> +	unsigned int x, y;
> +	u16 *src, *buf;
> +	u8 val;
> +
> +	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> +	clip->x1 = clip->x1 / 3 * 3;
> +	clip->x2 = (clip->x2 + 2) / 3 * 3;
> +	len = (clip->x2 - clip->x1) * sizeof(u16);
> +
> +	buf = kmalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	for (y = clip->y1; y < clip->y2; y++) {
> +		src = vaddr + (y * fb->pitches[0]);
> +		src += clip->x1;
> +		memcpy(buf, src, len);
> +		src = buf;
> +		for (x = clip->x1; x < clip->x2; x += 3) {
> +			val = st7586_rgb565_to_gray2(*src++) << 6;
> +			if (val & 0xC0)
> +				val |= 0x20;
> +			val |= st7586_rgb565_to_gray2(*src++) << 3;
> +			if (val & 0x18)
> +				val |= 0x04;
> +			val |= st7586_rgb565_to_gray2(*src++);
> +			*dst++ = ~val;
> +		}
> +	}
> +
> +	/* now adjust the clip so it applies to dst */
> +	clip->x1 /= 3;
> +	clip->x2 /= 3;
> +
> +	kfree(buf);
> +}
> +
> +static inline u8 st7586_xrgb8888_to_gray2(u32 rgb)
> +{
> +	u8 r = (rgb & 0x00ff0000) >> 16;
> +	u8 g = (rgb & 0x0000ff00) >> 8;
> +	u8 b = rgb & 0x000000ff;
> +	/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> +	u8 gray8 = (3 * r + 6 * g + b) / 10;
> +
> +	return gray8 >> 6;
> +}
> +
> +static void st7586_from_xrgb8888(u8 *dst, void *vaddr,
> +				 struct drm_framebuffer *fb,
> +				 struct drm_clip_rect *clip)
> +{
> +	size_t len;
> +	unsigned int x, y;
> +	u32 *src, *buf;
> +	u8 val;
> +
> +	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> +	clip->x1 = clip->x1 / 3 * 3;
> +	clip->x2 = (clip->x2 + 2) / 3 * 3;
> +	len = (clip->x2 - clip->x1) * sizeof(u32);
> +
> +	buf = kmalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	for (y = clip->y1; y < clip->y2; y++) {
> +		src = vaddr + (y * fb->pitches[0]);
> +		src += clip->x1;
> +		memcpy(buf, src, len);
> +		src = buf;
> +		for (x = clip->x1; x < clip->x2; x += 3) {
> +			val = st7586_xrgb8888_to_gray2(*src++) << 6;
> +			if (val & 0xC0)
> +				val |= 0x20;
> +			val |= st7586_xrgb8888_to_gray2(*src++) << 3;
> +			if (val & 0x18)
> +				val |= 0x04;
> +			val |= st7586_xrgb8888_to_gray2(*src++);
> +			*dst++ = ~val;
> +		}
> +	}
> +
> +	/* now adjust the clip so it applies to dst */
> +	clip->x1 /= 3;
> +	clip->x2 /= 3;
> +
> +	kfree(buf);
> +}
> +
> +static int st7586_mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
> +				    struct drm_clip_rect *clip)
> +{
> +	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> +	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
> +	struct drm_format_name_buf format_name;
> +	void *src = cma_obj->vaddr;
> +	int ret = 0;
> +
> +	if (import_attach) {
> +		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> +					       DMA_FROM_DEVICE);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (fb->format->format) {
> +	case DRM_FORMAT_RGB565:
> +		st7586_from_rgb565(dst, src, fb, clip);
> +		break;
> +	case DRM_FORMAT_XRGB8888:
> +		st7586_from_xrgb8888(dst, src, fb, clip);
> +		break;
> +	default:
> +		dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
> +			     drm_get_format_name(fb->format->format,
> +						 &format_name));
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	if (import_attach)
> +		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> +					     DMA_FROM_DEVICE);
> +	return ret;
> +}
> +
> +static int st7586_mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
> +				    struct drm_file *file_priv,
> +				    unsigned int flags, unsigned int color,
> +				    struct drm_clip_rect *clips,
> +				    unsigned int num_clips)
> +{
> +	struct tinydrm_device *tdev = fb->dev->dev_private;
> +	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> +	struct drm_clip_rect clip;
> +	int ret = 0;
> +
> +	mutex_lock(&tdev->dirty_lock);
> +
> +	if (!mipi->enabled)
> +		goto out_unlock;
> +
> +	/* fbdev can flush even when we're not interested */
> +	if (tdev->pipe.plane.fb != fb)
> +		goto out_unlock;
> +
> +	tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width,
> +			    fb->height);
> +
> +	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
> +		  clip.x1, clip.x2, clip.y1, clip.y2);
> +
> +	ret = st7586_mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip);
> +	if (ret)
> +		goto out_unlock;
> +
> +	/* NB: st7586_mipi_dbi_buf_copy() modifies clip */
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
> +			 (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF,
> +			 (clip.x2 >> 8) & 0xFF, (clip.x2 - 1) & 0xFF);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
> +			 (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
> +			 (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
> +
> +	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
> +				   (u8 *)mipi->tx_buf,
> +				   (clip.x2 - clip.x1) * (clip.y2 - clip.y1));
> +
> +out_unlock:
> +	mutex_unlock(&tdev->dirty_lock);
> +
> +	if (ret)
> +		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
> +			     ret);
> +
> +	return ret;
> +}
> +
> +static const struct drm_framebuffer_funcs st7586_mipi_dbi_fb_funcs = {
> +	.destroy	= drm_fb_cma_destroy,
> +	.create_handle	= drm_fb_cma_create_handle,
> +	.dirty		= st7586_mipi_dbi_fb_dirty,
> +};
> +
> +void st7586_mipi_dbi_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 drm_framebuffer *fb = pipe->plane.fb;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	mipi->enabled = true;
> +	if (fb)
> +		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
> +
> +	tinydrm_enable_backlight(mipi->backlight);
> +}
> +
> +static void st7586_mipi_dbi_blank(struct mipi_dbi *mipi)
> +{
> +	struct drm_device *drm = mipi->tinydrm.drm;
> +	u16 height = drm->mode_config.min_height;
> +	u16 width = (drm->mode_config.min_width + 2) / 3;
> +	size_t len = width * height;
> +
> +	memset(mipi->tx_buf, 0, len);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS, 0, 0,
> +			 (width >> 8) & 0xFF, (width - 1) & 0xFF);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS, 0, 0,
> +			 (height >> 8) & 0xFF, (height - 1) & 0xFF);
> +	mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
> +			     (u8 *)mipi->tx_buf, len);
> +}
> +
> +static void st7586_mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> +	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
> +	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	mipi->enabled = false;
> +
> +	if (mipi->backlight)
> +		tinydrm_disable_backlight(mipi->backlight);
> +	else
> +		st7586_mipi_dbi_blank(mipi);
> +}
> +
> +static const u32 st7586_mipi_dbi_formats[] = {
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static int st7586_mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
> +		const struct drm_simple_display_pipe_funcs *pipe_funcs,
> +		struct drm_driver *driver, const struct drm_display_mode *mode,
> +		unsigned int rotation)
> +{
> +	size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
> +	struct tinydrm_device *tdev = &mipi->tinydrm;
> +	int ret;
> +
> +	if (!mipi->command)
> +		return -EINVAL;
> +
> +	mutex_init(&mipi->cmdlock);
> +
> +	mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL);
> +	if (!mipi->tx_buf)
> +		return -ENOMEM;
> +
> +	ret = devm_tinydrm_init(dev, tdev, &st7586_mipi_dbi_fb_funcs, driver);
> +	if (ret)
> +		return ret;
> +
> +	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
> +					DRM_MODE_CONNECTOR_VIRTUAL,
> +					st7586_mipi_dbi_formats,
> +					ARRAY_SIZE(st7586_mipi_dbi_formats),
> +					mode, rotation);
> +	if (ret)
> +		return ret;
> +
> +	tdev->drm->mode_config.preferred_depth = 16;
> +	mipi->rotation = rotation;
> +
> +	drm_mode_config_reset(tdev->drm);
> +
> +	DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",
> +		      tdev->drm->mode_config.preferred_depth, rotation);
> +
> +	return 0;
> +}
> +
> +static int st7586_init(struct mipi_dbi *mipi)
> +{
> +	struct tinydrm_device *tdev = &mipi->tinydrm;
> +	struct device *dev = tdev->drm->dev;
> +	u8 addr_mode;
> +	int ret;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	ret = regulator_enable(mipi->regulator);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulator %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Avoid flicker by skipping setup if the bootloader has done it */
> +	if (mipi_dbi_display_is_on(mipi))
> +		return 0;
> +
> +	mipi_dbi_hw_reset(mipi);
> +	ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
> +	if (ret) {
> +		dev_err(dev, "Error sending command %d\n", ret);
> +		regulator_disable(mipi->regulator);
> +		return ret;
> +	}
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
> +
> +	msleep(10);
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_READ);
> +
> +	msleep(20);
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_CTRL_OUT);
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> +
> +	msleep(50);
> +
> +	mipi_dbi_command(mipi, ST7586_SET_VOP_OFFSET, 0x00);
> +	mipi_dbi_command(mipi, ST7586_SET_VOP, 0xe3, 0x00);
> +	mipi_dbi_command(mipi, ST7586_SET_BIAS_SYSTEM, 0x02);
> +	mipi_dbi_command(mipi, ST7586_SET_BOOST_LEVEL, 0x04);
> +	mipi_dbi_command(mipi, ST7586_ENABLE_ANALOG, 0x1d);
> +	mipi_dbi_command(mipi, ST7586_SET_NLINE_INV, 0x00);
> +	mipi_dbi_command(mipi, ST7586_DISP_MODE_GRAY);
> +	mipi_dbi_command(mipi, ST7586_ENABLE_DDRAM, 0x02);
> +
> +	switch (mipi->rotation) {
> +	default:
> +		addr_mode = 0x00;
> +		break;
> +	case 90:
> +		addr_mode = ST7586_DISP_CTRL_MY;
> +		break;
> +	case 180:
> +		addr_mode = ST7586_DISP_CTRL_MX | ST7586_DISP_CTRL_MY;
> +		break;
> +	case 270:
> +		addr_mode = ST7586_DISP_CTRL_MX;
> +		break;
> +	}
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> +
> +	mipi_dbi_command(mipi, ST7586_SET_DISP_DUTY, 0x7f);
> +	mipi_dbi_command(mipi, ST7586_SET_PART_DISP, 0xa0);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PARTIAL_AREA, 0x00, 0x00, 0x00, 0x77);
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
> +
> +	msleep(100);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> +
> +	return 0;
> +}
> +
> +static void st7586_fini(void *data)
> +{
> +	struct mipi_dbi *mipi = data;
> +
> +	DRM_DEBUG_KMS("\n");
> +	regulator_disable(mipi->regulator);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
> +	.enable = st7586_mipi_dbi_pipe_enable,
> +	.disable = st7586_mipi_dbi_pipe_disable,
> +	.update = tinydrm_display_pipe_update,
> +	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_display_mode st7586_mode = {
> +	TINYDRM_MODE(178, 128, 37, 27),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(st7586_fops);
> +
> +static struct drm_driver st7586_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
> +				  DRIVER_ATOMIC,
> +	.fops			= &st7586_fops,
> +	TINYDRM_GEM_DRIVER_OPS,
> +	.lastclose		= tinydrm_lastclose,
> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "st7586",
> +	.desc			= "Sitronix ST7586",
> +	.date			= "20170801",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static const struct of_device_id st7586_of_match[] = {
> +	{ .compatible = "lego,ev3-lcd" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, st7586_of_match);
> +
> +static const struct spi_device_id st7586_id[] = {
> +	{ "ev3-lcd", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(spi, st7586_id);
> +
> +static int st7586_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct tinydrm_device *tdev;
> +	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_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(mipi->reset)) {
> +		dev_err(dev, "Failed to get gpio 'reset'\n");
> +		return PTR_ERR(mipi->reset);
> +	}
> +
> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc)) {
> +		dev_err(dev, "Failed to get gpio 'dc'\n");
> +		return PTR_ERR(dc);
> +	}
> +
> +	mipi->regulator = devm_regulator_get(dev, "power");
> +	if (IS_ERR(mipi->regulator))
> +		return PTR_ERR(mipi->regulator);
> +
> +	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;
> +
> +	/*
> +	 * we are using 8-bit data, so we are not actually swapping anything,
> +	 * but setting mipi->swap_bytes makes mipi_dbi_typec3_command() do the
> +	 * right thing and not use 16-bit transfers (which results in swapped
> +	 * bytes on little-endian systems and causes out of order data to be
> +	 * sent to the display).
> +	 */
> +	mipi->swap_bytes = true;
> +
> +	ret = st7586_mipi_dbi_init(&spi->dev, mipi, &st7586_pipe_funcs,
> +				   &st7586_driver, &st7586_mode, rotation);
> +	if (ret)
> +		return ret;
> +
> +	ret = st7586_init(mipi);
> +	if (ret)
> +		return ret;
> +
> +	/* use devres to fini after drm unregister (drv->remove is before) */
> +	ret = devm_add_action(dev, st7586_fini, mipi);
> +	if (ret) {
> +		st7586_fini(mipi);
> +		return ret;
> +	}
> +
> +	tdev = &mipi->tinydrm;
> +
> +	ret = devm_tinydrm_register(tdev);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, mipi);
> +
> +	DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",
> +			 tdev->drm->driver->name, dev_name(dev),
> +			 spi->max_speed_hz / 1000000,
> +			 tdev->drm->primary->index);
> +
> +	return 0;
> +}
> +
> +static void st7586_shutdown(struct spi_device *spi)
> +{
> +	struct mipi_dbi *mipi = spi_get_drvdata(spi);
> +
> +	tinydrm_shutdown(&mipi->tinydrm);
> +}
> +
> +static int __maybe_unused st7586_pm_suspend(struct device *dev)
> +{
> +	struct mipi_dbi *mipi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = tinydrm_suspend(&mipi->tinydrm);
> +	if (ret)
> +		return ret;
> +
> +	st7586_fini(mipi);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused st7586_pm_resume(struct device *dev)
> +{
> +	struct mipi_dbi *mipi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = st7586_init(mipi);
> +	if (ret)
> +		return ret;
> +
> +	return tinydrm_resume(&mipi->tinydrm);
> +}
> +
> +static const struct dev_pm_ops st7586_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(st7586_pm_suspend, st7586_pm_resume)
> +};
> +
> +static struct spi_driver st7586_spi_driver = {
> +	.driver = {
> +		.name = "st7586",
> +		.owner = THIS_MODULE,
> +		.of_match_table = st7586_of_match,
> +		.pm = &st7586_pm_ops,
> +	},
> +	.id_table = st7586_id,
> +	.probe = st7586_probe,
> +	.shutdown = st7586_shutdown,
> +};
> +module_spi_driver(st7586_spi_driver);
> +
> +MODULE_DESCRIPTION("Sitronix ST7586 DRM driver");
> +MODULE_AUTHOR("David Lechner <david@lechnology.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/tinydrm/st7586.h b/include/drm/tinydrm/st7586.h
> new file mode 100644
> index 0000000..18fb56b
> --- /dev/null
> +++ b/include/drm/tinydrm/st7586.h
> @@ -0,0 +1,34 @@
> +/*
> + * ST7586 LCD controller
> + *
> + * Copyright (C) 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.
> + */
> +
> +#ifndef __LINUX_ST7856_H
> +#define __LINUX_ST7856_H
> +
> +#define ST7586_DISP_MODE_GRAY	0x38
> +#define ST7586_DISP_MODE_MONO	0x39
> +#define ST7586_ENABLE_DDRAM	0x3a
> +#define ST7586_SET_DISP_DUTY	0xb0
> +#define ST7586_SET_PART_DISP	0xb4
> +#define ST7586_SET_NLINE_INV	0xb5
> +#define ST7586_SET_VOP		0xc0
> +#define ST7586_SET_BIAS_SYSTEM	0xc3
> +#define ST7586_SET_BOOST_LEVEL	0xc4
> +#define ST7586_SET_VOP_OFFSET	0xc7
> +#define ST7586_ENABLE_ANALOG	0xd0
> +#define ST7586_AUTO_READ_CTRL	0xd7
> +#define ST7586_OTP_RW_CTRL	0xe0
> +#define ST7586_OTP_CTRL_OUT	0xe1
> +#define ST7586_OTP_READ		0xe3
> +
> +#define ST7586_DISP_CTRL_MX	BIT(6)
> +#define ST7586_DISP_CTRL_MY	BIT(7)
> +
> +#endif /* __LINUX_ST7856_H */

Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[David Lechner]

On 08/02/2017 08:03 AM, Noralf Trønnes wrote:
> 
> 
> Please use tinydrm_xrgb8888_to_gray8().

I considered this, but is seems excessive to loop through the entire fb 
twice just to make a 4x6 cursor blink.

> You should use this function to enable the regulator and init the
> controller. Don't look at mi0283qt it should have been fixed.

ACK. But this is why I was not keen on writing a standalone driver. I 
know some things about kernel development, but not everything. How am I 
supposed to know what is OK to copy from other drivers and what is not? 
And if I am going to be put down as the maintainer of this driver, it 
bothers me that I don't know so much about how it all works.

> 
> Why 2 emulation formats?
> I chose DRM_FORMAT_XRGB8888 since I expected everything to support it.
> Please use only that.

Because my graphics library currently does not work with XRGB8888. ;-)

I can fix the graphics library and drop the RGB565 format in the kernel 
driver.

Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[Noralf Trønnes]

Den 02.08.2017 18.24, skrev David Lechner:
> On 08/02/2017 08:03 AM, Noralf Trønnes wrote:
>>
>>
>> Please use tinydrm_xrgb8888_to_gray8().
>
> I considered this, but is seems excessive to loop through the entire 
> fb twice just to make a 4x6 cursor blink.
>

Yes, you're right about that.

Can you change tinydrm_xrgb8888_to_gray8 to match the other
copy/conversion functions so we can pass it a clip rectangle:

int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr,
                 struct drm_framebuffer *fb,
                 struct drm_clip_rect *clip)

And move the import_attach part to the driver (repaper).
repaper doesn't do partial updates, so pass a full clip.

>> You should use this function to enable the regulator and init the
>> controller. Don't look at mi0283qt it should have been fixed.
>
> ACK. But this is why I was not keen on writing a standalone driver. I 
> know some things about kernel development, but not everything. How am 
> I supposed to know what is OK to copy from other drivers and what is not? 

You can't know, and it's part of the review process to sort that out.
That being said, I really should have fixed mi0283qt, because it will
be copied...

> And if I am going to be put down as the maintainer of this driver, it 
> bothers me that I don't know so much about how it all works.
>

You don't need to know all the details. If I make a change, you can
assume that I know what I'm doing and ack it, but I need someone to
look at my patches, I can't apply my own patches without anyone looking
at it, we all make mistakes. And if someone else sends a patch and you
don't know if it is good or not, just ping me and I'll look at it.
Either way I have to look at all tinydrm patches before applying them.
I'm not keen on being default maintainer on drivers that I can't test.
The upside of being a maintainer is that you control the future of the
driver, what changes goes in.

And I'm kind of in the same boat as you, I know tinydrm, but drm is
very complex and I don't know half of what's going on. If I'm stuck or
out of my league, I have to ask for help. Like with adding drm formats.

Noralf.

Re: [PATCH v2 1/4] drm/tinydrm: remove call to mipi_dbi_init() from mipi_dbi_spi_init()

[Noralf Trønnes]

Den 02.08.2017 00.11, skrev David Lechner:
> This removes the call to mipi_dbi_init() from mipi_dbi_spi_init() so that
> drivers can have a driver-specific implementation if needed.
>
> Also fixed order of @dc parameter in the doc comment.
>
> Suggested-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Add this when you resend the series, if I haven't applied it.

>   drivers/gpu/drm/tinydrm/mi0283qt.c |  8 ++++++--
>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 17 +++++------------
>   include/drm/tinydrm/mipi-dbi.h     |  6 +-----
>   3 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 482ff1c3..7e5bb7d 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -195,8 +195,12 @@ static int mi0283qt_probe(struct spi_device *spi)
>   
>   	device_property_read_u32(dev, "rotation", &rotation);
>   
> -	ret = mipi_dbi_spi_init(spi, mipi, dc, &mi0283qt_pipe_funcs,
> -				&mi0283qt_driver, &mi0283qt_mode, rotation);
> +	ret = mipi_dbi_spi_init(spi, mipi, dc);
> +	if (ret)
> +		return ret;
> +
> +	ret = mipi_dbi_init(&spi->dev, mipi, &mi0283qt_pipe_funcs,
> +			    &mi0283qt_driver, &mi0283qt_mode, rotation);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index e10fa4b..cba9784 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -777,15 +777,12 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>   /**
>    * mipi_dbi_spi_init - Initialize MIPI DBI SPI interfaced controller
>    * @spi: SPI device
> - * @dc: D/C gpio (optional)
>    * @mipi: &mipi_dbi structure to initialize
> - * @pipe_funcs: Display pipe functions
> - * @driver: DRM driver
> - * @mode: Display mode
> - * @rotation: Initial rotation in degrees Counter Clock Wise
> + * @dc: D/C gpio (optional)
>    *
>    * This function sets &mipi_dbi->command, enables &mipi->read_commands for the
> - * usual read commands and initializes @mipi using mipi_dbi_init().
> + * usual read commands. It should be followed by a call to mipi_dbi_init() or
> + * a driver-specific init.
>    *
>    * If @dc is set, a Type C Option 3 interface is assumed, if not
>    * Type C Option 1.
> @@ -800,11 +797,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>    * Zero on success, negative error code on failure.
>    */
>   int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
> -		      struct gpio_desc *dc,
> -		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
> -		      struct drm_driver *driver,
> -		      const struct drm_display_mode *mode,
> -		      unsigned int rotation)
> +		      struct gpio_desc *dc)
>   {
>   	size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0);
>   	struct device *dev = &spi->dev;
> @@ -850,7 +843,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
>   			return -ENOMEM;
>   	}
>   
> -	return mipi_dbi_init(dev, mipi, pipe_funcs, driver, mode, rotation);
> +	return 0;
>   }
>   EXPORT_SYMBOL(mipi_dbi_spi_init);
>   
> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
> index d137b16..83346dd 100644
> --- a/include/drm/tinydrm/mipi-dbi.h
> +++ b/include/drm/tinydrm/mipi-dbi.h
> @@ -62,11 +62,7 @@ mipi_dbi_from_tinydrm(struct tinydrm_device *tdev)
>   }
>   
>   int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
> -		      struct gpio_desc *dc,
> -		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
> -		      struct drm_driver *driver,
> -		      const struct drm_display_mode *mode,
> -		      unsigned int rotation);
> +		      struct gpio_desc *dc);
>   int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
>   		  const struct drm_simple_display_pipe_funcs *pipe_funcs,
>   		  struct drm_driver *driver,

Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[Rob Herring]

On Tue, Aug 01, 2017 at 05:11:48PM -0500, David Lechner wrote:
> LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
> module for the ST7586 controller with parameters for the EV3 LCD display.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  .../devicetree/bindings/display/st7586.txt         |  26 +

Please split bindings to a separate patch.

>  drivers/gpu/drm/tinydrm/Kconfig                    |  10 +
>  drivers/gpu/drm/tinydrm/Makefile                   |   1 +
>  drivers/gpu/drm/tinydrm/st7586.c                   | 579 +++++++++++++++++++++
>  include/drm/tinydrm/st7586.h                       |  34 ++
>  5 files changed, 650 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/st7586.txt
>  create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
>  create mode 100644 include/drm/tinydrm/st7586.h
> 
> diff --git a/Documentation/devicetree/bindings/display/st7586.txt b/Documentation/devicetree/bindings/display/st7586.txt
> new file mode 100644
> index 0000000..acf784aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/st7586.txt
> @@ -0,0 +1,26 @@
> +Sitronix ST7586 display panel
> +
> +Required properties:
> +- compatible:	"lego,ev3-lcd".
> +
> +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:
> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
> +		the panel interface mode (IM[3:0] pins):
> +		- present: IM=x110 4-wire 8-bit data serial interface
> +		- absent:  IM=x101 3-wire 9-bit data serial interface

This is a Lego specific pin which in turn drives IM (or IF) signals? 
This should have a vendor prefix with either lego or Sitronix depending 
on the answer.

Presence/absence doesn't sense. Doesn't the state of the GPIO line 
matter? You should just say if not present, the interface defaults to 
3-wire mode.

> +- reset-gpios:	Reset pin
> +- power-supply:	A regulator node for the supply voltage.
> +- backlight:	phandle of the backlight device attached to the panel
> +- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> +
> +Example:
> +	display@0{
> +		compatible = "lego,ev3-lcd";
> +		reg = <0>;
> +		spi-max-frequency = <10000000>;
> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> +	};