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
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)
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 */
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);
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
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);
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);
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);
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).
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);
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.
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);
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
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);
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
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
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
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.
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
David Lechner -
Noralf Trønnes