[PATCH v2 0/6] drm/tinydrm: Cleanup

[PATCH v2 0/6] drm/tinydrm: Cleanup

[Noralf Trønnes]

Fix a few things that came up during the tinydrm review but wasn't
addressed at the time.

Noralf.

Changes since version 1:
- Add mipi_dbi_enable_flush() instead of the pipe enable helper
  mipi_dbi_pipe_enable() that doesn't really fit anymore.
- Add common poweron-reset functionality.
- Use drm_mode_copy() instead of direct assignment

Noralf Trønnes (6):
  drm/tinydrm/mi0283qt: Use common include order
  drm/tinydrm/mi0283qt: Remove ili9341.h
  drm/tinydrm/mipi-dbi: Add mipi_dbi_enable_flush()
  drm/tinydrm/mipi-dbi: Add poweron-reset functions
  drm/tinydrm/mi0283qt: Let the display pipe handle power
  drm/tinydrm: Embed the mode in tinydrm_connector

 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c |  34 ++++-----
 drivers/gpu/drm/tinydrm/ili9225.c           |   6 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c          | 104 ++++++++++++----------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c          |  97 +++++++++++++++++++++-----
 drivers/gpu/drm/tinydrm/st7586.c            |  15 ++--
 drivers/gpu/drm/tinydrm/st7735r.c           |  10 +--
 include/drm/tinydrm/ili9341.h               |  54 ---------------
 include/drm/tinydrm/mipi-dbi.h              |   5 +-
 8 files changed, 147 insertions(+), 178 deletions(-)
 delete mode 100644 include/drm/tinydrm/ili9341.h

-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/6] drm/tinydrm/mi0283qt: Use common include order

[Noralf Trønnes]

Include linux headers before drm headers as it's commonly done.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: David Lechner <david@lechnology.com>
---
 drivers/gpu/drm/tinydrm/mi0283qt.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 674d407640be..45f02b6052b1 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -9,17 +9,18 @@
  * (at your option) any later version.
  */
 
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_modeset_helper.h>
-#include <drm/tinydrm/ili9341.h>
-#include <drm/tinydrm/mipi-dbi.h>
-#include <drm/tinydrm/tinydrm-helpers.h>
 #include <linux/delay.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 <drm/drm_fb_helper.h>
+#include <drm/drm_modeset_helper.h>
+#include <drm/tinydrm/ili9341.h>
+#include <drm/tinydrm/mipi-dbi.h>
+#include <drm/tinydrm/tinydrm-helpers.h>
 #include <video/mipi_display.h>
 
 static int mi0283qt_init(struct mipi_dbi *mipi)
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/6] drm/tinydrm/mi0283qt: Remove ili9341.h

[Noralf Trønnes]

No need for a public header file for the command macros.
Just include the necessary ones in the driver.

Also use the MIPI_DCS_PIXEL_FMT_16BIT macro.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: David Lechner <david@lechnology.com>
---
 drivers/gpu/drm/tinydrm/mi0283qt.c | 28 ++++++++++++++++++--
 include/drm/tinydrm/ili9341.h      | 54 --------------------------------------
 2 files changed, 26 insertions(+), 56 deletions(-)
 delete mode 100644 include/drm/tinydrm/ili9341.h

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 45f02b6052b1..c69a4d958f24 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -18,11 +18,35 @@
 
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_modeset_helper.h>
-#include <drm/tinydrm/ili9341.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 #include <video/mipi_display.h>
 
+#define ILI9341_FRMCTR1		0xb1
+#define ILI9341_DISCTRL		0xb6
+#define ILI9341_ETMOD		0xb7
+
+#define ILI9341_PWCTRL1		0xc0
+#define ILI9341_PWCTRL2		0xc1
+#define ILI9341_VMCTRL1		0xc5
+#define ILI9341_VMCTRL2		0xc7
+#define ILI9341_PWCTRLA		0xcb
+#define ILI9341_PWCTRLB		0xcf
+
+#define ILI9341_PGAMCTRL	0xe0
+#define ILI9341_NGAMCTRL	0xe1
+#define ILI9341_DTCTRLA		0xe8
+#define ILI9341_DTCTRLB		0xea
+#define ILI9341_PWRSEQ		0xed
+
+#define ILI9341_EN3GAM		0xf2
+#define ILI9341_PUMPCTRL	0xf7
+
+#define ILI9341_MADCTL_BGR	BIT(3)
+#define ILI9341_MADCTL_MV	BIT(5)
+#define ILI9341_MADCTL_MX	BIT(6)
+#define ILI9341_MADCTL_MY	BIT(7)
+
 static int mi0283qt_init(struct mipi_dbi *mipi)
 {
 	struct tinydrm_device *tdev = &mipi->tinydrm;
@@ -69,7 +93,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
 	mipi_dbi_command(mipi, ILI9341_VMCTRL2, 0xbe);
 
 	/* Memory Access Control */
-	mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT, 0x55);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT);
 
 	switch (mipi->rotation) {
 	default:
diff --git a/include/drm/tinydrm/ili9341.h b/include/drm/tinydrm/ili9341.h
deleted file mode 100644
index 807a09f43cad..000000000000
--- a/include/drm/tinydrm/ili9341.h
+++ /dev/null
@@ -1,54 +0,0 @@
-/*
- * ILI9341 LCD controller
- *
- * Copyright 2016 Noralf Trønnes
- *
- * 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_ILI9341_H
-#define __LINUX_ILI9341_H
-
-#define ILI9341_FRMCTR1    0xb1
-#define ILI9341_FRMCTR2    0xb2
-#define ILI9341_FRMCTR3    0xb3
-#define ILI9341_INVTR      0xb4
-#define ILI9341_PRCTR      0xb5
-#define ILI9341_DISCTRL    0xb6
-#define ILI9341_ETMOD      0xb7
-
-#define ILI9341_PWCTRL1    0xc0
-#define ILI9341_PWCTRL2    0xc1
-#define ILI9341_VMCTRL1    0xc5
-#define ILI9341_VMCTRL2    0xc7
-#define ILI9341_PWCTRLA    0xcb
-#define ILI9341_PWCTRLB    0xcf
-
-#define ILI9341_RDID1      0xda
-#define ILI9341_RDID2      0xdb
-#define ILI9341_RDID3      0xdc
-#define ILI9341_RDID4      0xd3
-
-#define ILI9341_PGAMCTRL   0xe0
-#define ILI9341_NGAMCTRL   0xe1
-#define ILI9341_DGAMCTRL1  0xe2
-#define ILI9341_DGAMCTRL2  0xe3
-#define ILI9341_DTCTRLA    0xe8
-#define ILI9341_DTCTRLB    0xea
-#define ILI9341_PWRSEQ     0xed
-
-#define ILI9341_EN3GAM     0xf2
-#define ILI9341_IFCTRL     0xf6
-#define ILI9341_PUMPCTRL   0xf7
-
-#define ILI9341_MADCTL_MH  BIT(2)
-#define ILI9341_MADCTL_BGR BIT(3)
-#define ILI9341_MADCTL_ML  BIT(4)
-#define ILI9341_MADCTL_MV  BIT(5)
-#define ILI9341_MADCTL_MX  BIT(6)
-#define ILI9341_MADCTL_MY  BIT(7)
-
-#endif /* __LINUX_ILI9341_H */
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/6] drm/tinydrm/mipi-dbi: Add mipi_dbi_enable_flush()

[Noralf Trønnes]

Add and use a function for enabling, flushing and turning on backlight.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/ili9225.c  |  6 +-----
 drivers/gpu/drm/tinydrm/mipi-dbi.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/tinydrm/st7586.c   |  6 +-----
 drivers/gpu/drm/tinydrm/st7735r.c  |  2 +-
 include/drm/tinydrm/mipi-dbi.h     |  1 +
 5 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index c0cf49849302..a0759502b81a 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -180,7 +180,6 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 {
 	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
-	struct drm_framebuffer *fb = pipe->plane.fb;
 	struct device *dev = tdev->drm->dev;
 	int ret;
 	u8 am_id;
@@ -269,10 +268,7 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
 
-	mipi->enabled = true;
-
-	if (fb)
-		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+	mipi_dbi_enable_flush(mipi);
 }
 
 static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index aa6b6ce56891..ecc5c81a93ac 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -270,6 +270,26 @@ static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = {
 	.dirty		= mipi_dbi_fb_dirty,
 };
 
+/**
+ * mipi_dbi_enable_flush - MIPI DBI enable helper
+ * @mipi: MIPI DBI structure
+ *
+ * This function sets &mipi_dbi->enabled, flushes the whole framebuffer and
+ * enables the backlight. Drivers can use this in their
+ * &drm_simple_display_pipe_funcs->enable callback.
+ */
+void mipi_dbi_enable_flush(struct mipi_dbi *mipi)
+{
+    struct drm_framebuffer *fb = mipi->tinydrm.pipe.plane.fb;
+
+    mipi->enabled = true;
+    if (fb)
+        fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+
+    tinydrm_enable_backlight(mipi->backlight);
+}
+EXPORT_SYMBOL(mipi_dbi_enable_flush);
+
 /**
  * mipi_dbi_pipe_enable - MIPI DBI pipe enable helper
  * @pipe: Display pipe
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index 5aebfceb740e..9fd4423c8e70 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -179,7 +179,6 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 {
 	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
-	struct drm_framebuffer *fb = pipe->plane.fb;
 	struct device *dev = tdev->drm->dev;
 	int ret;
 	u8 addr_mode;
@@ -241,10 +240,7 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
 
-	mipi->enabled = true;
-
-	if (fb)
-		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+	mipi_dbi_enable_flush(mipi);
 }
 
 static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
index 98ff447f40b4..1f38e15da676 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -102,7 +102,7 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	msleep(20);
 
-	mipi_dbi_pipe_enable(pipe, crtc_state);
+	mipi_dbi_enable_flush(mipi);
 }
 
 static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index 5d0e82b36eaf..6441d9a9161a 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -67,6 +67,7 @@ int 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);
+void mipi_dbi_enable_flush(struct mipi_dbi *mipi);
 void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
 			  struct drm_crtc_state *crtc_state);
 void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/6] drm/tinydrm/mipi-dbi: Add poweron-reset functions

[Noralf Trønnes]

Split out common poweron-reset functionality.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++----------
 drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tinydrm/st7586.c   |  9 ++----
 drivers/gpu/drm/tinydrm/st7735r.c  |  8 ++---
 include/drm/tinydrm/mipi-dbi.h     |  2 ++
 5 files changed, 73 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index c69a4d958f24..2a78bcd35045 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -49,31 +49,17 @@
 
 static int mi0283qt_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) {
-		DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
+	ret = mipi_dbi_poweron_conditional_reset(mipi);
+	if (ret < 0)
 		return ret;
-	}
-
-	/* Avoid flicker by skipping setup if the bootloader has done it */
-	if (mipi_dbi_display_is_on(mipi))
+	if (ret > 0)
 		return 0;
 
-	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);
-		regulator_disable(mipi->regulator);
-		return ret;
-	}
-
 	msleep(20);
 
 	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index ecc5c81a93ac..eea6803ff223 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -463,6 +463,7 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
 
 	val &= ~DCS_POWER_MODE_RESERVED_MASK;
 
+	/* The poweron/reset value is 08h DCS_POWER_MODE_DISPLAY_NORMAL_MODE */
 	if (val != (DCS_POWER_MODE_DISPLAY |
 	    DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE))
 		return false;
@@ -473,6 +474,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
 }
 EXPORT_SYMBOL(mipi_dbi_display_is_on);
 
+static int mipi_dbi_por_conditional(struct mipi_dbi *mipi, bool cond)
+{
+	struct device *dev = mipi->tinydrm.drm->dev;
+	int ret;
+
+	if (mipi->regulator) {
+		ret = regulator_enable(mipi->regulator);
+		if (ret) {
+			DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret);
+			return ret;
+		}
+	}
+
+	if (cond && mipi_dbi_display_is_on(mipi))
+		return 1;
+
+	mipi_dbi_hw_reset(mipi);
+	ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret);
+		if (mipi->regulator)
+			regulator_disable(mipi->regulator);
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * mipi_dbi_poweron_reset - MIPI DBI poweron and reset
+ * @mipi: MIPI DBI structure
+ *
+ * This function enables the regulator if used and does a hardware and software
+ * reset.
+ *
+ * Returns:
+ * Zero on success, or a negative error code.
+ */
+int mipi_dbi_poweron_reset(struct mipi_dbi *mipi)
+{
+	return mipi_dbi_por_conditional(mipi, false);
+}
+EXPORT_SYMBOL(mipi_dbi_poweron_reset);
+
+/**
+ * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and conditional reset
+ * @mipi: MIPI DBI structure
+ *
+ * This function enables the regulator if used and if the display is off, it
+ * does a hardware and software reset. If mipi_dbi_display_is_on() determines
+ * that the display is on, no reset is performed.
+ *
+ * Returns:
+ * Zero if the controller was reset, 1 if the display was already on, or a
+ * negative error code.
+ */
+int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi)
+{
+	return mipi_dbi_por_conditional(mipi, true);
+}
+EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
+
 #if IS_ENABLED(CONFIG_SPI)
 
 /**
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index 9fd4423c8e70..a6396ef9cc4a 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -179,19 +179,16 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 {
 	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, ST7586_AUTO_READ_CTRL, 0x9f);
-	if (ret) {
-		DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+	ret = mipi_dbi_poweron_reset(mipi);
+	if (ret)
 		return;
-	}
 
+	mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
 	mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
 
 	msleep(10);
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
index 1f38e15da676..650257ad0193 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -46,13 +46,9 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	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);
+	ret = mipi_dbi_poweron_reset(mipi);
+	if (ret)
 		return;
-	}
 
 	msleep(150);
 
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index 6441d9a9161a..795a4a2205bb 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -73,6 +73,8 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
 void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
 void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
 bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
+int mipi_dbi_poweron_reset(struct mipi_dbi *mipi);
+int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi);
 u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
 
 int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/6] drm/tinydrm/mi0283qt: Let the display pipe handle power

[Noralf Trønnes]

It's better to leave power handling and controller init to the
modesetting machinery using the simple pipe .enable and .disable
callbacks. Remove unused mipi_dbi_pipe_enable().

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/mi0283qt.c | 47 ++++++++------------------------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c | 32 ++++----------------------
 include/drm/tinydrm/mipi-dbi.h     |  2 --
 3 files changed, 15 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 2a78bcd35045..35b8b7c421b0 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -47,8 +47,11 @@
 #define ILI9341_MADCTL_MX	BIT(6)
 #define ILI9341_MADCTL_MY	BIT(7)
 
-static int mi0283qt_init(struct mipi_dbi *mipi)
+static void mi0283qt_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);
 	u8 addr_mode;
 	int ret;
 
@@ -56,9 +59,9 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
 
 	ret = mipi_dbi_poweron_conditional_reset(mipi);
 	if (ret < 0)
-		return ret;
+		return;
 	if (ret > 0)
-		return 0;
+		goto out_enable;
 
 	msleep(20);
 
@@ -123,19 +126,12 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
 	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
 	msleep(100);
 
-	return 0;
-}
-
-static void mi0283qt_fini(void *data)
-{
-	struct mipi_dbi *mipi = data;
-
-	DRM_DEBUG_KMS("\n");
-	regulator_disable(mipi->regulator);
+out_enable:
+	mipi_dbi_enable_flush(mipi);
 }
 
 static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
-	.enable = mipi_dbi_pipe_enable,
+	.enable = mi0283qt_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = tinydrm_display_pipe_update,
 	.prepare_fb = tinydrm_display_pipe_prepare_fb,
@@ -216,17 +212,6 @@ static int mi0283qt_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = mi0283qt_init(mipi);
-	if (ret)
-		return ret;
-
-	/* use devres to fini after drm unregister (drv->remove is before) */
-	ret = devm_add_action(dev, mi0283qt_fini, mipi);
-	if (ret) {
-		mi0283qt_fini(mipi);
-		return ret;
-	}
-
 	spi_set_drvdata(spi, mipi);
 
 	return devm_tinydrm_register(&mipi->tinydrm);
@@ -242,25 +227,13 @@ static void mi0283qt_shutdown(struct spi_device *spi)
 static int __maybe_unused mi0283qt_pm_suspend(struct device *dev)
 {
 	struct mipi_dbi *mipi = dev_get_drvdata(dev);
-	int ret;
 
-	ret = drm_mode_config_helper_suspend(mipi->tinydrm.drm);
-	if (ret)
-		return ret;
-
-	mi0283qt_fini(mipi);
-
-	return 0;
+	return drm_mode_config_helper_suspend(mipi->tinydrm.drm);
 }
 
 static int __maybe_unused mi0283qt_pm_resume(struct device *dev)
 {
 	struct mipi_dbi *mipi = dev_get_drvdata(dev);
-	int ret;
-
-	ret = mi0283qt_init(mipi);
-	if (ret)
-		return ret;
 
 	drm_mode_config_helper_resume(mipi->tinydrm.drm);
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index eea6803ff223..5cafbe16bb43 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -290,31 +290,6 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi)
 }
 EXPORT_SYMBOL(mipi_dbi_enable_flush);
 
-/**
- * mipi_dbi_pipe_enable - MIPI DBI pipe enable helper
- * @pipe: Display pipe
- * @crtc_state: CRTC state
- *
- * This function enables backlight. Drivers can use this as their
- * &drm_simple_display_pipe_funcs->enable callback.
- */
-void 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);
-}
-EXPORT_SYMBOL(mipi_dbi_pipe_enable);
-
 static void mipi_dbi_blank(struct mipi_dbi *mipi)
 {
 	struct drm_device *drm = mipi->tinydrm.drm;
@@ -336,8 +311,8 @@ static void mipi_dbi_blank(struct mipi_dbi *mipi)
  * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper
  * @pipe: Display pipe
  *
- * This function disables backlight if present or if not the
- * display memory is blanked. Drivers can use this as their
+ * This function disables backlight if present, if not the display memory is
+ * blanked. The regulator is disabled if in use. Drivers can use this as their
  * &drm_simple_display_pipe_funcs->disable callback.
  */
 void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
@@ -353,6 +328,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
 		tinydrm_disable_backlight(mipi->backlight);
 	else
 		mipi_dbi_blank(mipi);
+
+	if (mipi->regulator)
+		regulator_disable(mipi->regulator);
 }
 EXPORT_SYMBOL(mipi_dbi_pipe_disable);
 
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index 795a4a2205bb..44e824af2ef6 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -68,8 +68,6 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
 		  struct drm_driver *driver,
 		  const struct drm_display_mode *mode, unsigned int rotation);
 void mipi_dbi_enable_flush(struct mipi_dbi *mipi);
-void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
-			  struct drm_crtc_state *crtc_state);
 void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
 void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
 bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/6] drm/tinydrm: Embed the mode in tinydrm_connector

[Noralf Trønnes]

Embed the mode in tinydrm_connector instead of doing an devm_ allocation.
Remove unnecessary use of ret variable at the end of
tinydrm_display_pipe_init().

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 +++++++++++------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
index f41fc506ff87..11ae950b0fc9 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
@@ -15,7 +15,7 @@
 
 struct tinydrm_connector {
 	struct drm_connector base;
-	const struct drm_display_mode *mode;
+	struct drm_display_mode mode;
 };
 
 static inline struct tinydrm_connector *
@@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct drm_connector *connector)
 	struct tinydrm_connector *tconn = to_tinydrm_connector(connector);
 	struct drm_display_mode *mode;
 
-	mode = drm_mode_duplicate(connector->dev, tconn->mode);
+	mode = drm_mode_duplicate(connector->dev, &tconn->mode);
 	if (!mode) {
 		DRM_ERROR("Failed to duplicate mode\n");
 		return 0;
@@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm,
 	if (!tconn)
 		return ERR_PTR(-ENOMEM);
 
-	tconn->mode = mode;
+	drm_mode_copy(&tconn->mode, mode);
 	connector = &tconn->base;
 
 	drm_connector_helper_add(connector, &tinydrm_connector_hfuncs);
@@ -199,35 +199,27 @@ tinydrm_display_pipe_init(struct tinydrm_device *tdev,
 			  unsigned int rotation)
 {
 	struct drm_device *drm = tdev->drm;
-	struct drm_display_mode *mode_copy;
+	struct drm_display_mode mode_copy;
 	struct drm_connector *connector;
 	int ret;
 
-	mode_copy = devm_kmalloc(drm->dev, sizeof(*mode_copy), GFP_KERNEL);
-	if (!mode_copy)
-		return -ENOMEM;
-
-	*mode_copy = *mode;
-	ret = tinydrm_rotate_mode(mode_copy, rotation);
+	drm_mode_copy(&mode_copy, mode);
+	ret = tinydrm_rotate_mode(&mode_copy, rotation);
 	if (ret) {
 		DRM_ERROR("Illegal rotation value %u\n", rotation);
 		return -EINVAL;
 	}
 
-	drm->mode_config.min_width = mode_copy->hdisplay;
-	drm->mode_config.max_width = mode_copy->hdisplay;
-	drm->mode_config.min_height = mode_copy->vdisplay;
-	drm->mode_config.max_height = mode_copy->vdisplay;
+	drm->mode_config.min_width = mode_copy.hdisplay;
+	drm->mode_config.max_width = mode_copy.hdisplay;
+	drm->mode_config.min_height = mode_copy.vdisplay;
+	drm->mode_config.max_height = mode_copy.vdisplay;
 
-	connector = tinydrm_connector_create(drm, mode_copy, connector_type);
+	connector = tinydrm_connector_create(drm, &mode_copy, connector_type);
 	if (IS_ERR(connector))
 		return PTR_ERR(connector);
 
-	ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats,
-					   format_count, NULL, connector);
-	if (ret)
-		return ret;
-
-	return 0;
+	return drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats,
+					    format_count, NULL, connector);
 }
 EXPORT_SYMBOL(tinydrm_display_pipe_init);
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/tinydrm/mipi-dbi: Add poweron-reset functions

[Noralf Trønnes]

Den 07.01.2018 18.44, skrev Noralf Trønnes:
> Split out common poweron-reset functionality.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++----------
>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/tinydrm/st7586.c   |  9 ++----
>   drivers/gpu/drm/tinydrm/st7735r.c  |  8 ++---
>   include/drm/tinydrm/mipi-dbi.h     |  2 ++
>   5 files changed, 73 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index c69a4d958f24..2a78bcd35045 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -49,31 +49,17 @@
>   
>   static int mi0283qt_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) {
> -		DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
> +	ret = mipi_dbi_poweron_conditional_reset(mipi);
> +	if (ret < 0)
>   		return ret;
> -	}
> -
> -	/* Avoid flicker by skipping setup if the bootloader has done it */
> -	if (mipi_dbi_display_is_on(mipi))
> +	if (ret > 0)
>   		return 0;
>   
> -	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);
> -		regulator_disable(mipi->regulator);
> -		return ret;
> -	}
> -
>   	msleep(20);
>   
>   	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index ecc5c81a93ac..eea6803ff223 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -463,6 +463,7 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
>   
>   	val &= ~DCS_POWER_MODE_RESERVED_MASK;
>   
> +	/* The poweron/reset value is 08h DCS_POWER_MODE_DISPLAY_NORMAL_MODE */
>   	if (val != (DCS_POWER_MODE_DISPLAY |
>   	    DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE))
>   		return false;
> @@ -473,6 +474,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
>   }
>   EXPORT_SYMBOL(mipi_dbi_display_is_on);
>   
> +static int mipi_dbi_por_conditional(struct mipi_dbi *mipi, bool cond)
> +{
> +	struct device *dev = mipi->tinydrm.drm->dev;
> +	int ret;
> +
> +	if (mipi->regulator) {
> +		ret = regulator_enable(mipi->regulator);
> +		if (ret) {
> +			DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (cond && mipi_dbi_display_is_on(mipi))
> +		return 1;
> +
> +	mipi_dbi_hw_reset(mipi);
> +	ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret);
> +		if (mipi->regulator)
> +			regulator_disable(mipi->regulator);
> +		return ret;
> +	}

I forgot about the case where we don't have hw reset:

     /*
      * If we did a hw reset, we know the controller is in Sleep mode and
      * per MIPI DSC spec should wait 5ms after soft reset. If we didn't,
      * we assume worst case and wait 120ms.
      */
     if (mipi->reset)
         usleep_range(5000, 20000);
     else
         msleep(120);

Noralf.

> +
> +	return 0;
> +}
> +
> +/**
> + * mipi_dbi_poweron_reset - MIPI DBI poweron and reset
> + * @mipi: MIPI DBI structure
> + *
> + * This function enables the regulator if used and does a hardware and software
> + * reset.
> + *
> + * Returns:
> + * Zero on success, or a negative error code.
> + */
> +int mipi_dbi_poweron_reset(struct mipi_dbi *mipi)
> +{
> +	return mipi_dbi_por_conditional(mipi, false);
> +}
> +EXPORT_SYMBOL(mipi_dbi_poweron_reset);
> +
> +/**
> + * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and conditional reset
> + * @mipi: MIPI DBI structure
> + *
> + * This function enables the regulator if used and if the display is off, it
> + * does a hardware and software reset. If mipi_dbi_display_is_on() determines
> + * that the display is on, no reset is performed.
> + *
> + * Returns:
> + * Zero if the controller was reset, 1 if the display was already on, or a
> + * negative error code.
> + */
> +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi)
> +{
> +	return mipi_dbi_por_conditional(mipi, true);
> +}
> +EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
> +
>   #if IS_ENABLED(CONFIG_SPI)
>   
>   /**
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> index 9fd4423c8e70..a6396ef9cc4a 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -179,19 +179,16 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>   {
>   	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, ST7586_AUTO_READ_CTRL, 0x9f);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
> +	ret = mipi_dbi_poweron_reset(mipi);
> +	if (ret)
>   		return;
> -	}
>   
> +	mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
>   	mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
>   
>   	msleep(10);
> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
> index 1f38e15da676..650257ad0193 100644
> --- a/drivers/gpu/drm/tinydrm/st7735r.c
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -46,13 +46,9 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
>   
>   	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);
> +	ret = mipi_dbi_poweron_reset(mipi);
> +	if (ret)
>   		return;
> -	}
>   
>   	msleep(150);
>   
> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
> index 6441d9a9161a..795a4a2205bb 100644
> --- a/include/drm/tinydrm/mipi-dbi.h
> +++ b/include/drm/tinydrm/mipi-dbi.h
> @@ -73,6 +73,8 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
>   void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
>   void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
>   bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
> +int mipi_dbi_poweron_reset(struct mipi_dbi *mipi);
> +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi);
>   u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
>   
>   int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/tinydrm/mipi-dbi: Add mipi_dbi_enable_flush()

[David Lechner]

On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
> Add and use a function for enabling, flushing and turning on backlight.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/tinydrm/mipi-dbi: Add poweron-reset functions

[David Lechner]

On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
> Split out common poweron-reset functionality.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++----------
>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/tinydrm/st7586.c   |  9 ++----
>   drivers/gpu/drm/tinydrm/st7735r.c  |  8 ++---
>   include/drm/tinydrm/mipi-dbi.h     |  2 ++
>   5 files changed, 73 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index c69a4d958f24..2a78bcd35045 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -49,31 +49,17 @@
>   
>   static int mi0283qt_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) {
> -		DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
> +	ret = mipi_dbi_poweron_conditional_reset(mipi);
> +	if (ret < 0)
>   		return ret;
> -	}
> -
> -	/* Avoid flicker by skipping setup if the bootloader has done it */
> -	if (mipi_dbi_display_is_on(mipi))
> +	if (ret > 0)
>   		return 0;

If I am reading the patch right, it looks like there are two

	if (ret > 0)
    		return 0;

in a row with nothing in between when this is applied.

>   
> -	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);
> -		regulator_disable(mipi->regulator);
> -		return ret;
> -	}
> -
>   	msleep(20);
>   
>   	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index ecc5c81a93ac..eea6803ff223 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -463,6 +463,7 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
>   
>   	val &= ~DCS_POWER_MODE_RESERVED_MASK;
>   
> +	/* The poweron/reset value is 08h DCS_POWER_MODE_DISPLAY_NORMAL_MODE */
>   	if (val != (DCS_POWER_MODE_DISPLAY |
>   	    DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE))
>   		return false;
> @@ -473,6 +474,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
>   }
>   EXPORT_SYMBOL(mipi_dbi_display_is_on);
>   
> +static int mipi_dbi_por_conditional(struct mipi_dbi *mipi, bool cond)

I know it is long, but it would be nice to spell out poweron_reset here
instead of "por".

> +{
> +	struct device *dev = mipi->tinydrm.drm->dev;
> +	int ret;
> +
> +	if (mipi->regulator) {
> +		ret = regulator_enable(mipi->regulator);
> +		if (ret) {
> +			DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (cond && mipi_dbi_display_is_on(mipi))
> +		return 1;
> +
> +	mipi_dbi_hw_reset(mipi);
> +	ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);

It seems unnecessary to do a soft reset after a hard reset, but I suppose some displays
don't have a hard reset and the extra soft reset shouldn't hurt anything.

> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret);
> +		if (mipi->regulator)
> +			regulator_disable(mipi->regulator);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * mipi_dbi_poweron_reset - MIPI DBI poweron and reset
> + * @mipi: MIPI DBI structure
> + *
> + * This function enables the regulator if used and does a hardware and software
> + * reset.
> + *
> + * Returns:
> + * Zero on success, or a negative error code.
> + */
> +int mipi_dbi_poweron_reset(struct mipi_dbi *mipi)
> +{
> +	return mipi_dbi_por_conditional(mipi, false);
> +}
> +EXPORT_SYMBOL(mipi_dbi_poweron_reset);
> +
> +/**
> + * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and conditional reset
> + * @mipi: MIPI DBI structure
> + *
> + * This function enables the regulator if used and if the display is off, it
> + * does a hardware and software reset. If mipi_dbi_display_is_on() determines
> + * that the display is on, no reset is performed.
> + *
> + * Returns:
> + * Zero if the controller was reset, 1 if the display was already on, or a
> + * negative error code.
> + */
> +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi)
> +{
> +	return mipi_dbi_por_conditional(mipi, true);
> +}
> +EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
> +
>   #if IS_ENABLED(CONFIG_SPI)
>   
>   /**
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> index 9fd4423c8e70..a6396ef9cc4a 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -179,19 +179,16 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>   {
>   	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, ST7586_AUTO_READ_CTRL, 0x9f);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
> +	ret = mipi_dbi_poweron_reset(mipi);
> +	if (ret)
>   		return;
> -	}
>   
> +	mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
>   	mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
>   
>   	msleep(10);
> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
> index 1f38e15da676..650257ad0193 100644
> --- a/drivers/gpu/drm/tinydrm/st7735r.c
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -46,13 +46,9 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
>   
>   	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);
> +	ret = mipi_dbi_poweron_reset(mipi);
> +	if (ret)
>   		return;
> -	}
>   
>   	msleep(150);
>   
> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
> index 6441d9a9161a..795a4a2205bb 100644
> --- a/include/drm/tinydrm/mipi-dbi.h
> +++ b/include/drm/tinydrm/mipi-dbi.h
> @@ -73,6 +73,8 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
>   void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
>   void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
>   bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
> +int mipi_dbi_poweron_reset(struct mipi_dbi *mipi);
> +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi);
>   u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
>   
>   int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/tinydrm/mipi-dbi: Add poweron-reset functions

[David Lechner]

On 01/08/2018 07:38 PM, David Lechner wrote:
> On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
>> Split out common poweron-reset functionality.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++----------
>>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tinydrm/st7586.c   |  9 ++----
>>   drivers/gpu/drm/tinydrm/st7735r.c  |  8 ++---
>>   include/drm/tinydrm/mipi-dbi.h     |  2 ++
>>   5 files changed, 73 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index c69a4d958f24..2a78bcd35045 100644
>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> @@ -49,31 +49,17 @@
>>   static int mi0283qt_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) {
>> -        DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
>> +    ret = mipi_dbi_poweron_conditional_reset(mipi);
>> +    if (ret < 0)
>>           return ret;
>> -    }
>> -
>> -    /* Avoid flicker by skipping setup if the bootloader has done it */
>> -    if (mipi_dbi_display_is_on(mipi))
>> +    if (ret > 0)
>>           return 0;
> 
> If I am reading the patch right, it looks like there are two
> 
>      if (ret > 0)
>             return 0;
> 
> in a row with nothing in between when this is applied.
> 

I see now that I missed < vs. >. Probably better to say (ret == 1) instead of (ret > 0).
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/tinydrm/mi0283qt: Let the display pipe handle power

[David Lechner]

On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
> It's better to leave power handling and controller init to the
> modesetting machinery using the simple pipe .enable and .disable
> callbacks. Remove unused mipi_dbi_pipe_enable().
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/tinydrm: Embed the mode in tinydrm_connector

[David Lechner]

On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
> Embed the mode in tinydrm_connector instead of doing an devm_ allocation.
> Remove unnecessary use of ret variable at the end of
> tinydrm_display_pipe_init().
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

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


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/tinydrm: Embed the mode in tinydrm_connector

[Daniel Vetter]

On Mon, Jan 08, 2018 at 07:46:27PM -0600, David Lechner wrote:
> On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
> > Embed the mode in tinydrm_connector instead of doing an devm_ allocation.
> > Remove unnecessary use of ret variable at the end of
> > tinydrm_display_pipe_init().
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> 
> Reviewed-by: David Lechner <david@lechnology.com>

Since you two work together quit a bit, should we add David as drm-misc
committer?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/tinydrm: Embed the mode in tinydrm_connector

[Noralf Trønnes]

Den 09.01.2018 11.08, skrev Daniel Vetter:
> On Mon, Jan 08, 2018 at 07:46:27PM -0600, David Lechner wrote:
>> On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
>>> Embed the mode in tinydrm_connector instead of doing an devm_ allocation.
>>> Remove unnecessary use of ret variable at the end of
>>> tinydrm_display_pipe_init().
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>> Reviewed-by: David Lechner <david@lechnology.com>
> Since you two work together quit a bit, should we add David as drm-misc
> committer?

I would love that. What do you say David?
This would mean that you will commit your own work after getting ack/rb.

Noralf.

> -Daniel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/tinydrm/mipi-dbi: Add poweron-reset functions

[Noralf Trønnes]

Den 09.01.2018 02.38, skrev David Lechner:
> On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
>> Split out common poweron-reset functionality.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++----------
>>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 
>> ++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tinydrm/st7586.c   |  9 ++----
>>   drivers/gpu/drm/tinydrm/st7735r.c  |  8 ++---
>>   include/drm/tinydrm/mipi-dbi.h     |  2 ++
>>   5 files changed, 73 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c 
>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index c69a4d958f24..2a78bcd35045 100644
>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> @@ -49,31 +49,17 @@
>>     static int mi0283qt_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) {
>> -        DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
>> +    ret = mipi_dbi_poweron_conditional_reset(mipi);
>> +    if (ret < 0)
>>           return ret;
>> -    }
>> -
>> -    /* Avoid flicker by skipping setup if the bootloader has done it */
>> -    if (mipi_dbi_display_is_on(mipi))
>> +    if (ret > 0)
>>           return 0;
>
> If I am reading the patch right, it looks like there are two
>
>     if (ret > 0)
>            return 0;
>
> in a row with nothing in between when this is applied.
>
>>   -    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);
>> -        regulator_disable(mipi->regulator);
>> -        return ret;
>> -    }
>> -
>>       msleep(20);
>>         mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c 
>> b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> index ecc5c81a93ac..eea6803ff223 100644
>> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> @@ -463,6 +463,7 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
>>         val &= ~DCS_POWER_MODE_RESERVED_MASK;
>>   +    /* The poweron/reset value is 08h 
>> DCS_POWER_MODE_DISPLAY_NORMAL_MODE */
>>       if (val != (DCS_POWER_MODE_DISPLAY |
>>           DCS_POWER_MODE_DISPLAY_NORMAL_MODE | 
>> DCS_POWER_MODE_SLEEP_MODE))
>>           return false;
>> @@ -473,6 +474,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
>>   }
>>   EXPORT_SYMBOL(mipi_dbi_display_is_on);
>>   +static int mipi_dbi_por_conditional(struct mipi_dbi *mipi, bool cond)
>
> I know it is long, but it would be nice to spell out poweron_reset here
> instead of "por".
>
>> +{
>> +    struct device *dev = mipi->tinydrm.drm->dev;
>> +    int ret;
>> +
>> +    if (mipi->regulator) {
>> +        ret = regulator_enable(mipi->regulator);
>> +        if (ret) {
>> +            DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", 
>> ret);
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    if (cond && mipi_dbi_display_is_on(mipi))
>> +        return 1;
>> +
>> +    mipi_dbi_hw_reset(mipi);
>> +    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
>
> It seems unnecessary to do a soft reset after a hard reset, but I 
> suppose some displays
> don't have a hard reset and the extra soft reset shouldn't hurt anything.
>

Both ILI9341 and ST7735R (not ST7586S) lists soft reset as part of the
Power Flow Chart, but it's not explicit about it being required or not:

Power on sequence
HW reset
SW reset
State is now Sleep in

I looked in the MIPI DBI spec and there's nothing about requiring both
hw _and_ soft reset. But I have seen hard and soft reset in many panel
init's, so I think we keep this as the default. It has a 5ms penalty.
I could shave that off in mipi_dbi_hw_reset(). It keeps reset low for
20ms, but the spec says it just has to be longer than 9us with noise
spikes less than 20ns wide:

-    msleep(20);
+    usleep_range(20, 1000);

Noralf.

>> +    if (ret) {
>> +        DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret);
>> +        if (mipi->regulator)
>> +            regulator_disable(mipi->regulator);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * mipi_dbi_poweron_reset - MIPI DBI poweron and reset
>> + * @mipi: MIPI DBI structure
>> + *
>> + * This function enables the regulator if used and does a hardware 
>> and software
>> + * reset.
>> + *
>> + * Returns:
>> + * Zero on success, or a negative error code.
>> + */
>> +int mipi_dbi_poweron_reset(struct mipi_dbi *mipi)
>> +{
>> +    return mipi_dbi_por_conditional(mipi, false);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_poweron_reset);
>> +
>> +/**
>> + * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and 
>> conditional reset
>> + * @mipi: MIPI DBI structure
>> + *
>> + * This function enables the regulator if used and if the display is 
>> off, it
>> + * does a hardware and software reset. If mipi_dbi_display_is_on() 
>> determines
>> + * that the display is on, no reset is performed.
>> + *
>> + * Returns:
>> + * Zero if the controller was reset, 1 if the display was already 
>> on, or a
>> + * negative error code.
>> + */
>> +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi)
>> +{
>> +    return mipi_dbi_por_conditional(mipi, true);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
>> +
>>   #if IS_ENABLED(CONFIG_SPI)
>>     /**
>> diff --git a/drivers/gpu/drm/tinydrm/st7586.c 
>> b/drivers/gpu/drm/tinydrm/st7586.c
>> index 9fd4423c8e70..a6396ef9cc4a 100644
>> --- a/drivers/gpu/drm/tinydrm/st7586.c
>> +++ b/drivers/gpu/drm/tinydrm/st7586.c
>> @@ -179,19 +179,16 @@ static void st7586_pipe_enable(struct 
>> drm_simple_display_pipe *pipe,
>>   {
>>       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, ST7586_AUTO_READ_CTRL, 0x9f);
>> -    if (ret) {
>> -        DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
>> +    ret = mipi_dbi_poweron_reset(mipi);
>> +    if (ret)
>>           return;
>> -    }
>>   +    mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
>>       mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
>>         msleep(10);
>> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c 
>> b/drivers/gpu/drm/tinydrm/st7735r.c
>> index 1f38e15da676..650257ad0193 100644
>> --- a/drivers/gpu/drm/tinydrm/st7735r.c
>> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
>> @@ -46,13 +46,9 @@ static void jd_t18003_t01_pipe_enable(struct 
>> drm_simple_display_pipe *pipe,
>>         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);
>> +    ret = mipi_dbi_poweron_reset(mipi);
>> +    if (ret)
>>           return;
>> -    }
>>         msleep(150);
>>   diff --git a/include/drm/tinydrm/mipi-dbi.h 
>> b/include/drm/tinydrm/mipi-dbi.h
>> index 6441d9a9161a..795a4a2205bb 100644
>> --- a/include/drm/tinydrm/mipi-dbi.h
>> +++ b/include/drm/tinydrm/mipi-dbi.h
>> @@ -73,6 +73,8 @@ void mipi_dbi_pipe_enable(struct 
>> drm_simple_display_pipe *pipe,
>>   void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
>>   void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
>>   bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
>> +int mipi_dbi_poweron_reset(struct mipi_dbi *mipi);
>> +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi);
>>   u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
>>     int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/tinydrm/mipi-dbi: Add poweron-reset functions

[David Lechner]

On 01/09/2018 09:01 AM, Noralf Trønnes wrote:
> 
> Den 09.01.2018 02.38, skrev David Lechner:
>> On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
>>> Split out common poweron-reset functionality.
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>   drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++----------
>>>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/tinydrm/st7586.c   |  9 ++----
>>>   drivers/gpu/drm/tinydrm/st7735r.c  |  8 ++---
>>>   include/drm/tinydrm/mipi-dbi.h     |  2 ++
>>>   5 files changed, 73 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> index c69a4d958f24..2a78bcd35045 100644
>>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> @@ -49,31 +49,17 @@
>>>     static int mi0283qt_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) {
>>> -        DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
>>> +    ret = mipi_dbi_poweron_conditional_reset(mipi);
>>> +    if (ret < 0)
>>>           return ret;
>>> -    }
>>> -
>>> -    /* Avoid flicker by skipping setup if the bootloader has done it */
>>> -    if (mipi_dbi_display_is_on(mipi))
>>> +    if (ret > 0)
>>>           return 0;
>>
>> If I am reading the patch right, it looks like there are two
>>
>>     if (ret > 0)
>>            return 0;
>>
>> in a row with nothing in between when this is applied.
>>
>>>   -    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);
>>> -        regulator_disable(mipi->regulator);
>>> -        return ret;
>>> -    }
>>> -
>>>       msleep(20);
>>>         mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
>>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>> index ecc5c81a93ac..eea6803ff223 100644
>>> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>> @@ -463,6 +463,7 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
>>>         val &= ~DCS_POWER_MODE_RESERVED_MASK;
>>>   +    /* The poweron/reset value is 08h DCS_POWER_MODE_DISPLAY_NORMAL_MODE */
>>>       if (val != (DCS_POWER_MODE_DISPLAY |
>>>           DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE))
>>>           return false;
>>> @@ -473,6 +474,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
>>>   }
>>>   EXPORT_SYMBOL(mipi_dbi_display_is_on);
>>>   +static int mipi_dbi_por_conditional(struct mipi_dbi *mipi, bool cond)
>>
>> I know it is long, but it would be nice to spell out poweron_reset here
>> instead of "por".
>>
>>> +{
>>> +    struct device *dev = mipi->tinydrm.drm->dev;
>>> +    int ret;
>>> +
>>> +    if (mipi->regulator) {
>>> +        ret = regulator_enable(mipi->regulator);
>>> +        if (ret) {
>>> +            DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret);
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    if (cond && mipi_dbi_display_is_on(mipi))
>>> +        return 1;
>>> +
>>> +    mipi_dbi_hw_reset(mipi);
>>> +    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
>>
>> It seems unnecessary to do a soft reset after a hard reset, but I suppose some displays
>> don't have a hard reset and the extra soft reset shouldn't hurt anything.
>>
> 
> Both ILI9341 and ST7735R (not ST7586S) lists soft reset as part of the
> Power Flow Chart, but it's not explicit about it being required or not:
> 
> Power on sequence
> HW reset
> SW reset
> State is now Sleep in
> 
> I looked in the MIPI DBI spec and there's nothing about requiring both
> hw _and_ soft reset. But I have seen hard and soft reset in many panel
> init's, so I think we keep this as the default. It has a 5ms penalty.
> I could shave that off in mipi_dbi_hw_reset(). It keeps reset low for
> 20ms, but the spec says it just has to be longer than 9us with noise
> spikes less than 20ns wide:
> 
> -    msleep(20);
> +    usleep_range(20, 1000);
> 

Sounds good to me.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/tinydrm: Embed the mode in tinydrm_connector

[David Lechner]

On 01/09/2018 08:05 AM, Noralf Trønnes wrote:
> 
> Den 09.01.2018 11.08, skrev Daniel Vetter:
>> On Mon, Jan 08, 2018 at 07:46:27PM -0600, David Lechner wrote:
>>> On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
>>>> Embed the mode in tinydrm_connector instead of doing an devm_ allocation.
>>>> Remove unnecessary use of ret variable at the end of
>>>> tinydrm_display_pipe_init().
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>> Reviewed-by: David Lechner <david@lechnology.com>
>> Since you two work together quit a bit, should we add David as drm-misc
>> committer?
> 
> I would love that. What do you say David?
> This would mean that you will commit your own work after getting ack/rb.

Well... I suppose that would be alright.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/6] drm/tinydrm: Embed the mode in tinydrm_connector

[Daniel Vetter]

On Tue, Jan 09, 2018 at 09:09:32PM -0600, David Lechner wrote:
> On 01/09/2018 08:05 AM, Noralf Trønnes wrote:
> > 
> > Den 09.01.2018 11.08, skrev Daniel Vetter:
> > > On Mon, Jan 08, 2018 at 07:46:27PM -0600, David Lechner wrote:
> > > > On 01/07/2018 11:44 AM, Noralf Trønnes wrote:
> > > > > Embed the mode in tinydrm_connector instead of doing an devm_ allocation.
> > > > > Remove unnecessary use of ret variable at the end of
> > > > > tinydrm_display_pipe_init().
> > > > > 
> > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > > ---
> > > > Reviewed-by: David Lechner <david@lechnology.com>
> > > Since you two work together quit a bit, should we add David as drm-misc
> > > committer?
> > 
> > I would love that. What do you say David?
> > This would mean that you will commit your own work after getting ack/rb.
> 
> Well... I suppose that would be alright.

Please request a freedesktop account for the drm-misc group per

https://www.freedesktop.org/wiki/AccountRequests/

We use a few scripts to handle technicalities and prevent oopsies:

https://01.org/linuxgraphics/gfx-docs/maintainer-tools/dim.html#quickstart

For questions just ping Noralf or any of the other drm-misc
maintainers/committers - #dri-devel on freenode is a good place.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel