[PATCH 0/5] drm/tinydrm: Cleanup

[PATCH 0/5] drm/tinydrm: Cleanup

[Noralf Trønnes]

Patches 1-4 fixes a few things that came up during the tinydrm review
but wasn't addressed at the time.

Noralf.

Noralf Trønnes (5):
  drm/tinydrm/mi0283qt: Use common include order
  drm/tinydrm/mi0283qt: Remove ili9341.h
  drm/tinydrm/mi0283qt: Clarify error message
  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/mi0283qt.c          | 92 ++++++++++++++---------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c          | 14 +++--
 include/drm/tinydrm/ili9341.h               | 54 -----------------
 4 files changed, 65 insertions(+), 129 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 1/5] 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>
---
 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 2/5] 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>
---
 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 3/5] drm/tinydrm/mi0283qt: Clarify error message

[Noralf Trønnes]

Make it clear that the printed value is the error and not the command
value itself.

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

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index c69a4d958f24..5994140f1e1e 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -58,7 +58,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
 
 	ret = regulator_enable(mipi->regulator);
 	if (ret) {
-		DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
+		DRM_DEV_ERROR(dev, "Failed to enable regulator: %d\n", ret);
 		return ret;
 	}
 
@@ -69,7 +69,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
 	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);
+		DRM_DEV_ERROR(dev, "Error sending command: %d\n", ret);
 		regulator_disable(mipi->regulator);
 		return ret;
 	}
-- 
2.14.2

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

[PATCH 4/5] 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.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/mi0283qt.c | 51 ++++++++------------------------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++-----
 2 files changed, 19 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 5994140f1e1e..8318a60e584c 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -47,9 +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 = &mipi->tinydrm;
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
 	struct device *dev = tdev->drm->dev;
 	u8 addr_mode;
 	int ret;
@@ -59,19 +61,18 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
 	ret = regulator_enable(mipi->regulator);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to enable regulator: %d\n", ret);
-		return ret;
+		return;
 	}
 
-	/* Avoid flicker by skipping setup if the bootloader has done it */
 	if (mipi_dbi_display_is_on(mipi))
-		return 0;
+		goto out_enable;
 
 	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;
+		return;
 	}
 
 	msleep(20);
@@ -137,19 +138,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_pipe_enable(pipe, crtc_state);
 }
 
 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,
@@ -230,17 +224,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);
@@ -256,25 +239,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 aa6b6ce56891..5b5ed68d8994 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -275,8 +275,9 @@ static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = {
  * @pipe: Display pipe
  * @crtc_state: CRTC state
  *
- * This function enables backlight. Drivers can use this as their
- * &drm_simple_display_pipe_funcs->enable callback.
+ * This function flushes the whole framebuffer and enables the backlight.
+ * Drivers can use this in 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)
@@ -285,8 +286,6 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *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);
@@ -316,8 +315,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)
@@ -333,6 +332,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);
 
-- 
2.14.2

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

[PATCH 5/5] 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..dadd26d53216 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;
+	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);
+	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 1/5] drm/tinydrm/mi0283qt: Use common include order

[David Lechner]

On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
> Include linux headers before drm headers as it's commonly done.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   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)
> 

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 2/5] drm/tinydrm/mi0283qt: Remove ili9341.h

[David Lechner]

On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
> 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>

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

Re: [PATCH 3/5] drm/tinydrm/mi0283qt: Clarify error message

[David Lechner]

On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
> Make it clear that the printed value is the error and not the command
> value itself.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/tinydrm/mi0283qt.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index c69a4d958f24..5994140f1e1e 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -58,7 +58,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
>   
>   	ret = regulator_enable(mipi->regulator);
>   	if (ret) {
> -		DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
> +		DRM_DEV_ERROR(dev, "Failed to enable regulator: %d\n", ret);

It is more common to use parenthesis around the error value, so that would be
more clear to me.

  +		DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret);

>   		return ret;
>   	}
>   
> @@ -69,7 +69,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
>   	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);
> +		DRM_DEV_ERROR(dev, "Error sending command: %d\n", ret);

ditto

>   		regulator_disable(mipi->regulator);
>   		return ret;
>   	}
> 

With that change:

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 4/5] drm/tinydrm/mi0283qt: Let the display pipe handle power

[David Lechner]

On 01/05/2018 10:55 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.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/tinydrm/mi0283qt.c | 51 ++++++++------------------------------
>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++-----
>   2 files changed, 19 insertions(+), 46 deletions(-)

<snip>

> @@ -316,8 +315,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)
> @@ -333,6 +332,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);
>   

If a display physically has a backlight, but it is not controllable (i.e.
mipi->backlight == NULL) and you disable the regulator, would that not cause
the display to be all white instead of blanked?

Also, even if this is OK, it seems like you should call regulator_enable()
in mipi_dbi_pipe_enable() to keep things balanced in the helper functions.

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

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

[David Lechner]

On 01/05/2018 10:56 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>
> ---
>   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..dadd26d53216 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;
> +	tconn->mode = *mode;

I see there is a special drm_mode_copy() function, so I am wondering if this
is safe.

>   	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);
> +	mode_copy = *mode;

Same here.

> +	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);
> 

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

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

[Noralf Trønnes]

Den 05.01.2018 19.59, skrev David Lechner:
> On 01/05/2018 10:55 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.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/tinydrm/mi0283qt.c | 51 
>> ++++++++------------------------------
>>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++-----
>>   2 files changed, 19 insertions(+), 46 deletions(-)
>
> <snip>
>
>> @@ -316,8 +315,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)
>> @@ -333,6 +332,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);
>
> If a display physically has a backlight, but it is not controllable (i.e.
> mipi->backlight == NULL) and you disable the regulator, would that not 
> cause
> the display to be all white instead of blanked?
>

Yes, if the regulator doesn't control the backlight. But on these simple
displays I assume that a regulator would control the whole display
including the backlight. If we get a display with a separate backlight
regulator, I think it's better to treat that as the exception rather than
the other way around.

> Also, even if this is OK, it seems like you should call 
> regulator_enable()
> in mipi_dbi_pipe_enable() to keep things balanced in the helper 
> functions.
>

Yes, I wasn't entirely happy with this. The regulator has to be enabled
before the controller is initialized, so it can't happen in this _enable 
helper.
I thought about having mipi_dbi_pipe_enable_begin/end functions, but I'm
not sure I like that either. I prefer something more descriptive.

Maybe we should drop the pipe enable helper, since it's not very likely
that anyone can use it directly as an .enable function.

What do you think about this approach:

/**
  * 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)
{
     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 (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);
         regulator_disable(mipi->regulator);
         return ret;
     }

     return 0;
}
EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);

/**
  * 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->tdev.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);


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);
     struct device *dev = tdev->drm->dev;

     ret = mipi_dbi_poweron_conditional_reset(mipi);
     if (ret < 0)
         return;
     if (ret > 0)
         goto out_enable;

     /* initialize controller */

out_enable:
     mipi_dbi_enable_flush(mipi);
}

Noralf.

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

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

[Noralf Trønnes]

Den 05.01.2018 20.14, skrev David Lechner:
> On 01/05/2018 10:56 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>
>> ---
>>   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..dadd26d53216 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;
>> +    tconn->mode = *mode;
>
> I see there is a special drm_mode_copy() function, so I am wondering 
> if this
> is safe.
>

It is safe, the underlying drm_mode_object isn't initialized/registered.
So to me an assignment like this makes it clear that it is so, but Laurent
also asked about this, so maybe I'm the odd one here.
I'll switch to drm_mode_copy().

Noralf.

>>       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);
>> +    mode_copy = *mode;
>
> Same here.
>
>> +    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);
>>
>

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

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

[David Lechner]

On 01/06/2018 06:49 AM, Noralf Trønnes wrote:
> 
> Den 05.01.2018 20.14, skrev David Lechner:
>> On 01/05/2018 10:56 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>
>>> ---
>>>   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..dadd26d53216 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;
>>> +    tconn->mode = *mode;
>>
>> I see there is a special drm_mode_copy() function, so I am wondering if this
>> is safe.
>>
> 
> It is safe, the underlying drm_mode_object isn't initialized/registered.
> So to me an assignment like this makes it clear that it is so, but Laurent
> also asked about this, so maybe I'm the odd one here.
> I'll switch to drm_mode_copy().
> 

I think a comment to this effect would be enough.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[David Lechner]

On 01/06/2018 06:45 AM, Noralf Trønnes wrote:
> 
> Den 05.01.2018 19.59, skrev David Lechner:
>> On 01/05/2018 10:55 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.
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>   drivers/gpu/drm/tinydrm/mi0283qt.c | 51 ++++++++------------------------------
>>>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++-----
>>>   2 files changed, 19 insertions(+), 46 deletions(-)
>>
>> <snip>
>>
>>> @@ -316,8 +315,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)
>>> @@ -333,6 +332,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);
>>
>> If a display physically has a backlight, but it is not controllable (i.e.
>> mipi->backlight == NULL) and you disable the regulator, would that not cause
>> the display to be all white instead of blanked?
>>
> 
> Yes, if the regulator doesn't control the backlight. But on these simple
> displays I assume that a regulator would control the whole display
> including the backlight. If we get a display with a separate backlight
> regulator, I think it's better to treat that as the exception rather than
> the other way around.

Makes sense. All of the displays I have that have a controllable backlight don't
have a regulator for the display itself, so they won't be affected anyway.

> 
>> Also, even if this is OK, it seems like you should call regulator_enable()
>> in mipi_dbi_pipe_enable() to keep things balanced in the helper functions.
>>
> 
> Yes, I wasn't entirely happy with this. The regulator has to be enabled
> before the controller is initialized, so it can't happen in this _enable helper.
> I thought about having mipi_dbi_pipe_enable_begin/end functions, but I'm
> not sure I like that either. I prefer something more descriptive.
> 
> Maybe we should drop the pipe enable helper, since it's not very likely
> that anyone can use it directly as an .enable function.
> 
> What do you think about this approach:

It looks OK to me. Although I am wondering about the conditional reset. If the
display is already on, how do we know it is configured correctly?

> 
> /**
>   * 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)
> {
>      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 (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);
>          regulator_disable(mipi->regulator);
>          return ret;
>      }
> 
>      return 0;
> }
> EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
> 
> /**
>   * 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->tdev.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);
> 
> 
> 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);
>      struct device *dev = tdev->drm->dev;
> 
>      ret = mipi_dbi_poweron_conditional_reset(mipi);
>      if (ret < 0)
>          return;
>      if (ret > 0)
>          goto out_enable;
> 
>      /* initialize controller */
> 
> out_enable:
>      mipi_dbi_enable_flush(mipi);
> }
> 
> Noralf.
> 

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

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

[Noralf Trønnes]

Den 06.01.2018 19.10, skrev David Lechner:
> On 01/06/2018 06:45 AM, Noralf Trønnes wrote:
>>
>> Den 05.01.2018 19.59, skrev David Lechner:
>>> On 01/05/2018 10:55 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.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>   drivers/gpu/drm/tinydrm/mi0283qt.c | 51 
>>>> ++++++++------------------------------
>>>>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++-----
>>>>   2 files changed, 19 insertions(+), 46 deletions(-)
>>>
>>> <snip>
>>>
>>>> @@ -316,8 +315,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)
>>>> @@ -333,6 +332,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);
>>>
>>> If a display physically has a backlight, but it is not controllable 
>>> (i.e.
>>> mipi->backlight == NULL) and you disable the regulator, would that 
>>> not cause
>>> the display to be all white instead of blanked?
>>>
>>
>> Yes, if the regulator doesn't control the backlight. But on these simple
>> displays I assume that a regulator would control the whole display
>> including the backlight. If we get a display with a separate backlight
>> regulator, I think it's better to treat that as the exception rather 
>> than
>> the other way around.
>
> Makes sense. All of the displays I have that have a controllable 
> backlight don't
> have a regulator for the display itself, so they won't be affected 
> anyway.
>
>>
>>> Also, even if this is OK, it seems like you should call 
>>> regulator_enable()
>>> in mipi_dbi_pipe_enable() to keep things balanced in the helper 
>>> functions.
>>>
>>
>> Yes, I wasn't entirely happy with this. The regulator has to be enabled
>> before the controller is initialized, so it can't happen in this 
>> _enable helper.
>> I thought about having mipi_dbi_pipe_enable_begin/end functions, but I'm
>> not sure I like that either. I prefer something more descriptive.
>>
>> Maybe we should drop the pipe enable helper, since it's not very likely
>> that anyone can use it directly as an .enable function.
>>
>> What do you think about this approach:
>
> It looks OK to me. Although I am wondering about the conditional 
> reset. If the
> display is already on, how do we know it is configured correctly?
>

The MIPI DCS spec says that the reset value of get_power_mode 0Ah is 08h.
That's 0b00001000 which differs from what we call ON: 0bX00111XX.

If mipi->read_commands isn't NULL and the MISO pin isn't connected we will
read all 1's or all 0's (unless it's floating), which isn't a problem
since it will return false in that case.

The only way to get ON if the driver didn't do it, is if the bootloader
did it. In that case we trust the bootloader.

This speeds up subsequent .enable calls, since we can avoid the msleeps.


     MIPI_DCS_GET_POWER_MODE        = 0x0A,

#define DCS_POWER_MODE_DISPLAY            BIT(2)
#define DCS_POWER_MODE_DISPLAY_NORMAL_MODE    BIT(3)
#define DCS_POWER_MODE_SLEEP_MODE        BIT(4)
#define DCS_POWER_MODE_PARTIAL_MODE        BIT(5)
#define DCS_POWER_MODE_IDLE_MODE        BIT(6)
#define DCS_POWER_MODE_RESERVED_MASK        (BIT(0) | BIT(1) | BIT(7))

bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
{
     u8 val;

     if (mipi_dbi_command_read(mipi, MIPI_DCS_GET_POWER_MODE, &val))
         return false;

     val &= ~DCS_POWER_MODE_RESERVED_MASK;

     if (val != (DCS_POWER_MODE_DISPLAY |
         DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE))
         return false;

     DRM_DEBUG_DRIVER("Display is ON\n");

     return true;
}

Noralf.

>>
>> /**
>>   * 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)
>> {
>>      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 (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);
>>          regulator_disable(mipi->regulator);
>>          return ret;
>>      }
>>
>>      return 0;
>> }
>> EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
>>
>> /**
>>   * 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->tdev.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);
>>
>>
>> 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);
>>      struct device *dev = tdev->drm->dev;
>>
>>      ret = mipi_dbi_poweron_conditional_reset(mipi);
>>      if (ret < 0)
>>          return;
>>      if (ret > 0)
>>          goto out_enable;
>>
>>      /* initialize controller */
>>
>> out_enable:
>>      mipi_dbi_enable_flush(mipi);
>> }
>>
>> Noralf.
>>
>

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