[V7,2/2] drm/panel: Add Sitronix ST7701 panel driver

Submitted by Jagan Teki on Jan. 11, 2019, 12:47 p.m.

Details

Message ID 20190111124714.19566-2-jagan@amarulasolutions.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Jagan Teki Jan. 11, 2019, 12:47 p.m.
ST7701 designed for small and medium sizes of TFT LCD display, is
capable of supporting up to 480RGBX864 in resolution. It provides
several system interfaces like MIPI/RGB/SPI.

Currently added support for Techstar TS8550B which is ST7701 based
480x854, 2-lane MIPI DSI LCD panel.

Driver now registering mipi_dsi device, but indeed it can extendable
for RGB if any requirement trigger in future.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v7:
- rebase on master
Changes for v6:
- use sleep delay value as per datasheet
- add panel_sleep_delay variable for panel specific delay
- use command sequnce display on and off instead panel
  functions
- add proper comments on the delays
- remove delays from command switch
- move mode type on struct display mode
- drop refresh rate value, let drm compute  
Changes for v5:
- found the chip from vendor, so added new panel driver
- here is v4: https://patchwork.kernel.org/patch/10680335/

 MAINTAINERS                                   |   6 +
 drivers/gpu/drm/panel/Kconfig                 |  10 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-sitronix-st7701.c | 427 ++++++++++++++++++
 4 files changed, 444 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-sitronix-st7701.c

Patch hide | download patch | download mbox

diff --git a/MAINTAINERS b/MAINTAINERS
index 3f6db876be1f..d2928a4d2847 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4831,6 +4831,12 @@  S:	Orphan / Obsolete
 F:	drivers/gpu/drm/sis/
 F:	include/uapi/drm/sis_drm.h
 
+DRM DRIVER FOR SITRONIX ST7701 PANELS
+M:	Jagan Teki <jagan@amarulasolutions.com>
+S:	Maintained
+F:	drivers/gpu/drm/panel/panel-sitronix-st7701.c
+F:	Documentation/devicetree/bindings/display/panel/sitronix,st7701.txt
+
 DRM DRIVER FOR SITRONIX ST7586 PANELS
 M:	David Lechner <david@lechnology.com>
 S:	Maintained
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 3f3537719beb..d93b2ba709bc 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -196,6 +196,16 @@  config DRM_PANEL_SHARP_LS043T1LE01
 	  Say Y here if you want to enable support for Sharp LS043T1LE01 qHD
 	  (540x960) DSI panel as found on the Qualcomm APQ8074 Dragonboard
 
+config DRM_PANEL_SITRONIX_ST7701
+	tristate "Sitronix ST7701 panel driver"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for the Sitronix
+	  ST7701 controller for 480X864 LCD panels with MIPI/RGB/SPI
+	  system interfaces.
+
 config DRM_PANEL_SITRONIX_ST7789V
 	tristate "Sitronix ST7789V panel"
 	depends on OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 4396658a7996..6a9b4cec1891 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -20,5 +20,6 @@  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
 obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
 obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
 obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
+obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
 obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
 obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7701.c b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
new file mode 100644
index 000000000000..8638a5463cb6
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
@@ -0,0 +1,427 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019, Amarula Solutions.
+ * Author: Jagan Teki <jagan@amarulasolutions.com>
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+#include <video/mipi_display.h>
+
+/* Command2 BKx selection command */
+#define DSI_CMD2BKX_SEL			0xFF
+
+/* Command2, BK0 commands */
+#define DSI_CMD2_BK0_PVGAMCTRL		0xB0 /* Positive Voltage Gamma Control */
+#define DSI_CMD2_BK0_NVGAMCTRL		0xB1 /* Negative Voltage Gamma Control */
+#define DSI_CMD2_BK0_LNESET		0xC0 /* Display Line setting */
+#define DSI_CMD2_BK0_PORCTRL		0xC1 /* Porch control */
+#define DSI_CMD2_BK0_INVSEL		0xC2 /* Inversion selection, Frame Rate Control */
+
+/* Command2, BK1 commands */
+#define DSI_CMD2_BK1_VRHS		0xB0 /* Vop amplitude setting */
+#define DSI_CMD2_BK1_VCOM		0xB1 /* VCOM amplitude setting */
+#define DSI_CMD2_BK1_VGHSS		0xB2 /* VGH Voltage setting */
+#define DSI_CMD2_BK1_TESTCMD		0xB3 /* TEST Command Setting */
+#define DSI_CMD2_BK1_VGLS		0xB5 /* VGL Voltage setting */
+#define DSI_CMD2_BK1_PWCTLR1		0xB7 /* Power Control 1 */
+#define DSI_CMD2_BK1_PWCTLR2		0xB8 /* Power Control 2 */
+#define DSI_CMD2_BK1_SPD1		0xC1 /* Source pre_drive timing set1 */
+#define DSI_CMD2_BK1_SPD2		0xC2 /* Source EQ2 Setting */
+#define DSI_CMD2_BK1_MIPISET1		0xD0 /* MIPI Setting 1 */
+
+/**
+ * Command2 with BK function selection.
+ *
+ * BIT[4, 0]: [CN2, BKXSEL]
+ * 10 = CMD2BK0, Command2 BK0
+ * 11 = CMD2BK1, Command2 BK1
+ * 00 = Command2 disable
+ */
+#define DSI_CMD2BK1_SEL			0x11
+#define DSI_CMD2BK0_SEL			0x10
+#define DSI_CMD2BKX_SEL_NONE		0x00
+
+/* Command2, BK0 bytes */
+#define DSI_LINESET_LINE		0x69
+#define DSI_LINESET_LDE_EN		BIT(7)
+#define DSI_LINESET_LINEDELTA		GENMASK(1, 0)
+#define DSI_CMD2_BK0_LNESET_B1		DSI_LINESET_LINEDELTA
+#define DSI_CMD2_BK0_LNESET_B0		(DSI_LINESET_LDE_EN | DSI_LINESET_LINE)
+#define DSI_INVSEL_DEFAULT		GENMASK(5, 4)
+#define DSI_INVSEL_NLINV		GENMASK(2, 0)
+#define DSI_INVSEL_RTNI			GENMASK(2, 1)
+#define DSI_CMD2_BK0_INVSEL_B1		DSI_INVSEL_RTNI
+#define DSI_CMD2_BK0_INVSEL_B0		(DSI_INVSEL_DEFAULT | DSI_INVSEL_NLINV)
+#define DSI_CMD2_BK0_PORCTRL_B0(m)	((m)->vtotal - (m)->vsync_end)
+#define DSI_CMD2_BK0_PORCTRL_B1(m)	((m)->vsync_start - (m)->vdisplay)
+
+/* Command2, BK1 bytes */
+#define DSI_CMD2_BK1_VRHA_SET		0x45
+#define DSI_CMD2_BK1_VCOM_SET		0x13
+#define DSI_CMD2_BK1_VGHSS_SET		GENMASK(2, 0)
+#define DSI_CMD2_BK1_TESTCMD_VAL	BIT(7)
+#define DSI_VGLS_DEFAULT		BIT(6)
+#define DSI_VGLS_SEL			GENMASK(2, 0)
+#define DSI_CMD2_BK1_VGLS_SET		(DSI_VGLS_DEFAULT | DSI_VGLS_SEL)
+#define DSI_PWCTLR1_AP			BIT(7) /* Gamma OP bias, max */
+#define DSI_PWCTLR1_APIS		BIT(2) /* Source OP input bias, min */
+#define DSI_PWCTLR1_APOS		BIT(0) /* Source OP output bias, min */
+#define DSI_CMD2_BK1_PWCTLR1_SET	(DSI_PWCTLR1_AP | DSI_PWCTLR1_APIS | \
+					DSI_PWCTLR1_APOS)
+#define DSI_PWCTLR2_AVDD		BIT(5) /* AVDD 6.6v */
+#define DSI_PWCTLR2_AVCL		0x0    /* AVCL -4.4v */
+#define DSI_CMD2_BK1_PWCTLR2_SET	(DSI_PWCTLR2_AVDD | DSI_PWCTLR2_AVCL)
+#define DSI_SPD1_T2D			BIT(3)
+#define DSI_CMD2_BK1_SPD1_SET		(GENMASK(6, 4) | DSI_SPD1_T2D)
+#define DSI_CMD2_BK1_SPD2_SET		DSI_CMD2_BK1_SPD1_SET
+#define DSI_MIPISET1_EOT_EN		BIT(3)
+#define DSI_CMD2_BK1_MIPISET1_SET	(BIT(7) | DSI_MIPISET1_EOT_EN)
+
+struct st7701_panel_desc {
+	const struct drm_display_mode *mode;
+	unsigned int lanes;
+	unsigned long flags;
+	enum mipi_dsi_pixel_format format;
+	const char *const *supply_names;
+	unsigned int num_supplies;
+	unsigned int panel_sleep_delay;
+};
+
+struct st7701 {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+	const struct st7701_panel_desc *desc;
+
+	struct backlight_device *backlight;
+	struct regulator_bulk_data *supplies;
+	unsigned int num_supplies;
+	struct gpio_desc *reset;
+	unsigned int sleep_delay;
+};
+
+static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
+{
+	return container_of(panel, struct st7701, panel);
+}
+
+static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
+				   size_t len)
+{
+	return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
+}
+
+#define ST7701_DSI(st7701, seq...)				\
+	{							\
+		const u8 d[] = { seq };				\
+		st7701_dsi_write(st7701, d, ARRAY_SIZE(d));	\
+	}
+
+static void st7701_init_sequence(struct st7701 *st7701)
+{
+	const struct drm_display_mode *mode = st7701->desc->mode;
+
+	ST7701_DSI(st7701, MIPI_DCS_SOFT_RESET, 0x00);
+
+	/* We need to wait 5ms before sending new commands */
+	msleep(5);
+
+	ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
+
+	msleep(st7701->sleep_delay);
+
+	/* Command2, BK0 */
+	ST7701_DSI(st7701, DSI_CMD2BKX_SEL,
+		   0x77, 0x01, 0x00, 0x00, DSI_CMD2BK0_SEL);
+	ST7701_DSI(st7701, DSI_CMD2_BK0_PVGAMCTRL, 0x00, 0x0E, 0x15, 0x0F,
+		   0x11, 0x08, 0x08, 0x08, 0x08, 0x23, 0x04, 0x13, 0x12,
+		   0x2B, 0x34, 0x1F);
+	ST7701_DSI(st7701, DSI_CMD2_BK0_NVGAMCTRL, 0x00, 0x0E, 0x95, 0x0F,
+		   0x13, 0x07, 0x09, 0x08, 0x08, 0x22, 0x04, 0x10, 0x0E,
+		   0x2C, 0x34, 0x1F);
+	ST7701_DSI(st7701, DSI_CMD2_BK0_LNESET,
+		   DSI_CMD2_BK0_LNESET_B0, DSI_CMD2_BK0_LNESET_B1);
+	ST7701_DSI(st7701, DSI_CMD2_BK0_PORCTRL,
+		   DSI_CMD2_BK0_PORCTRL_B0(mode),
+		   DSI_CMD2_BK0_PORCTRL_B1(mode));
+	ST7701_DSI(st7701, DSI_CMD2_BK0_INVSEL,
+		   DSI_CMD2_BK0_INVSEL_B0, DSI_CMD2_BK0_INVSEL_B1);
+
+	/* Command2, BK1 */
+	ST7701_DSI(st7701, DSI_CMD2BKX_SEL,
+			0x77, 0x01, 0x00, 0x00, DSI_CMD2BK1_SEL);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_VRHS, DSI_CMD2_BK1_VRHA_SET);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_VCOM, DSI_CMD2_BK1_VCOM_SET);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_VGHSS, DSI_CMD2_BK1_VGHSS_SET);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_TESTCMD, DSI_CMD2_BK1_TESTCMD_VAL);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_VGLS, DSI_CMD2_BK1_VGLS_SET);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_PWCTLR1, DSI_CMD2_BK1_PWCTLR1_SET);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_PWCTLR2, DSI_CMD2_BK1_PWCTLR2_SET);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_SPD1, DSI_CMD2_BK1_SPD1_SET);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_SPD2, DSI_CMD2_BK1_SPD2_SET);
+	ST7701_DSI(st7701, DSI_CMD2_BK1_MIPISET1, DSI_CMD2_BK1_MIPISET1_SET);
+
+	/**
+	 * ST7701_SPEC_V1.2 is unable to provide enough information above this
+	 * specific command sequence, so grab the same from vendor BSP driver.
+	 */
+	ST7701_DSI(st7701, 0xE0, 0x00, 0x00, 0x02);
+	ST7701_DSI(st7701, 0xE1, 0x0B, 0x00, 0x0D, 0x00, 0x0C, 0x00, 0x0E,
+		   0x00, 0x00, 0x44, 0x44);
+	ST7701_DSI(st7701, 0xE2, 0x33, 0x33, 0x44, 0x44, 0x64, 0x00, 0x66,
+		   0x00, 0x65, 0x00, 0x67, 0x00, 0x00);
+	ST7701_DSI(st7701, 0xE3, 0x00, 0x00, 0x33, 0x33);
+	ST7701_DSI(st7701, 0xE4, 0x44, 0x44);
+	ST7701_DSI(st7701, 0xE5, 0x0C, 0x78, 0x3C, 0xA0, 0x0E, 0x78, 0x3C,
+		   0xA0, 0x10, 0x78, 0x3C, 0xA0, 0x12, 0x78, 0x3C, 0xA0);
+	ST7701_DSI(st7701, 0xE6, 0x00, 0x00, 0x33, 0x33);
+	ST7701_DSI(st7701, 0xE7, 0x44, 0x44);
+	ST7701_DSI(st7701, 0xE8, 0x0D, 0x78, 0x3C, 0xA0, 0x0F, 0x78, 0x3C,
+		   0xA0, 0x11, 0x78, 0x3C, 0xA0, 0x13, 0x78, 0x3C, 0xA0);
+	ST7701_DSI(st7701, 0xEB, 0x02, 0x02, 0x39, 0x39, 0xEE, 0x44, 0x00);
+	ST7701_DSI(st7701, 0xEC, 0x00, 0x00);
+	ST7701_DSI(st7701, 0xED, 0xFF, 0xF1, 0x04, 0x56, 0x72, 0x3F, 0xFF,
+		   0xFF, 0xFF, 0xFF, 0xF3, 0x27, 0x65, 0x40, 0x1F, 0xFF);
+
+	/* disable Command2 */
+	ST7701_DSI(st7701, DSI_CMD2BKX_SEL,
+		   0x77, 0x01, 0x00, 0x00, DSI_CMD2BKX_SEL_NONE);
+}
+
+static int st7701_prepare(struct drm_panel *panel)
+{
+	struct st7701 *st7701 = panel_to_st7701(panel);
+	int ret;
+
+	gpiod_set_value(st7701->reset, 0);
+	msleep(20);
+
+	ret = regulator_bulk_enable(st7701->desc->num_supplies,
+				    st7701->supplies);
+	if (ret < 0)
+		return ret;
+	msleep(20);
+
+	gpiod_set_value(st7701->reset, 1);
+	msleep(20);
+
+	gpiod_set_value(st7701->reset, 0);
+	msleep(30);
+
+	gpiod_set_value(st7701->reset, 1);
+	msleep(150);
+
+	st7701_init_sequence(st7701);
+
+	return 0;
+}
+
+static int st7701_enable(struct drm_panel *panel)
+{
+	struct st7701 *st7701 = panel_to_st7701(panel);
+
+	ST7701_DSI(st7701, MIPI_DCS_SET_DISPLAY_ON, 0x00);
+	backlight_enable(st7701->backlight);
+
+	return 0;
+}
+
+static int st7701_disable(struct drm_panel *panel)
+{
+	struct st7701 *st7701 = panel_to_st7701(panel);
+
+	backlight_disable(st7701->backlight);
+	ST7701_DSI(st7701, MIPI_DCS_SET_DISPLAY_OFF, 0x00);
+
+	return 0;
+}
+
+static int st7701_unprepare(struct drm_panel *panel)
+{
+	struct st7701 *st7701 = panel_to_st7701(panel);
+
+	ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
+
+	msleep(st7701->sleep_delay);
+
+	gpiod_set_value(st7701->reset, 0);
+
+	gpiod_set_value(st7701->reset, 1);
+
+	gpiod_set_value(st7701->reset, 0);
+
+	regulator_bulk_disable(st7701->desc->num_supplies, st7701->supplies);
+
+	return 0;
+}
+
+static int st7701_get_modes(struct drm_panel *panel)
+{
+	struct st7701 *st7701 = panel_to_st7701(panel);
+	const struct drm_display_mode *desc_mode = st7701->desc->mode;
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(panel->drm, desc_mode);
+	if (!mode) {
+		dev_err(&st7701->dsi->dev, "failed to add mode %ux%ux@%u\n",
+			desc_mode->hdisplay, desc_mode->vdisplay,
+			desc_mode->vrefresh);
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+	drm_mode_probed_add(panel->connector, mode);
+
+	panel->connector->display_info.width_mm = desc_mode->width_mm;
+	panel->connector->display_info.height_mm = desc_mode->height_mm;
+
+	return 1;
+}
+
+static const struct drm_panel_funcs st7701_funcs = {
+	.disable	= st7701_disable,
+	.unprepare	= st7701_unprepare,
+	.prepare	= st7701_prepare,
+	.enable		= st7701_enable,
+	.get_modes	= st7701_get_modes,
+};
+
+static const struct drm_display_mode ts8550b_mode = {
+	.clock		= 27500,
+
+	.hdisplay	= 480,
+	.hsync_start	= 480 + 38,
+	.hsync_end	= 480 + 38 + 12,
+	.htotal		= 480 + 38 + 12 + 12,
+
+	.vdisplay	= 854,
+	.vsync_start	= 854 + 4,
+	.vsync_end	= 854 + 4 + 8,
+	.vtotal		= 854 + 4 + 8 + 18,
+
+	.width_mm	= 69,
+	.height_mm	= 139,
+
+	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static const char * const ts8550b_supply_names[] = {
+	"VCC",
+	"IOVCC",
+};
+
+static const struct st7701_panel_desc ts8550b_desc = {
+	.mode = &ts8550b_mode,
+	.lanes = 2,
+	.flags = MIPI_DSI_MODE_VIDEO,
+	.format = MIPI_DSI_FMT_RGB888,
+	.supply_names = ts8550b_supply_names,
+	.num_supplies = ARRAY_SIZE(ts8550b_supply_names),
+	.panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
+};
+
+static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	const struct st7701_panel_desc *desc;
+	struct device_node *np;
+	struct st7701 *st7701;
+	int ret, i;
+
+	st7701 = devm_kzalloc(&dsi->dev, sizeof(*st7701), GFP_KERNEL);
+	if (!st7701)
+		return -ENOMEM;
+
+	desc = of_device_get_match_data(&dsi->dev);
+	dsi->mode_flags = desc->flags;
+	dsi->format = desc->format;
+	dsi->lanes = desc->lanes;
+
+	st7701->supplies = devm_kcalloc(&dsi->dev, desc->num_supplies,
+					sizeof(*st7701->supplies),
+					GFP_KERNEL);
+	if (!st7701->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < desc->num_supplies; i++)
+		st7701->supplies[i].supply = desc->supply_names[i];
+
+	ret = devm_regulator_bulk_get(&dsi->dev, desc->num_supplies,
+				      st7701->supplies);
+	if (ret < 0)
+		return ret;
+
+	st7701->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(st7701->reset)) {
+		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
+		return PTR_ERR(st7701->reset);
+	}
+
+	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
+	if (np) {
+		st7701->backlight = of_find_backlight_by_node(np);
+		of_node_put(np);
+
+		if (!st7701->backlight)
+			return -EPROBE_DEFER;
+	}
+
+	drm_panel_init(&st7701->panel);
+
+	/* We need to wait 120ms after a sleep out command */
+	st7701->sleep_delay = 120 + desc->panel_sleep_delay;
+	st7701->panel.funcs = &st7701_funcs;
+	st7701->panel.dev = &dsi->dev;
+
+	ret = drm_panel_add(&st7701->panel);
+	if (ret < 0)
+		return ret;
+
+	mipi_dsi_set_drvdata(dsi, st7701);
+	st7701->dsi = dsi;
+	st7701->desc = desc;
+
+	return mipi_dsi_attach(dsi);
+}
+
+static int st7701_dsi_remove(struct mipi_dsi_device *dsi)
+{
+	struct st7701 *st7701 = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_panel_remove(&st7701->panel);
+
+	if (st7701->backlight)
+		put_device(&st7701->backlight->dev);
+
+	return 0;
+}
+
+static const struct of_device_id st7701_of_match[] = {
+	{ .compatible = "techstar,ts8550b", .data = &ts8550b_desc },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, st7701_of_match);
+
+static struct mipi_dsi_driver st7701_dsi_driver = {
+	.probe		= st7701_dsi_probe,
+	.remove		= st7701_dsi_remove,
+	.driver = {
+		.name		= "st7701",
+		.of_match_table	= st7701_of_match,
+	},
+};
+module_mipi_dsi_driver(st7701_dsi_driver);
+
+MODULE_AUTHOR("Jagan Teki <jagan@amarulasolutions.com>");
+MODULE_DESCRIPTION("Sitronix ST7701 LCD Panel Driver");
+MODULE_LICENSE("GPL");

Comments

Sam Ravnborg Jan. 11, 2019, 9:19 p.m.
Hi Jagan.

Gave this another more detailed read - triggered some additional comments.
Depite the comments it looks good, and this is all more or
less cosmetic improvements.

	Sam

> +struct st7701_panel_desc {
> +	const struct drm_display_mode *mode;
> +	unsigned int lanes;
> +	unsigned long flags;
> +	enum mipi_dsi_pixel_format format;
> +	const char *const *supply_names;
> +	unsigned int num_supplies;
> +	unsigned int panel_sleep_delay;
> +};
> +
> +struct st7701 {
> +	struct drm_panel panel;
> +	struct mipi_dsi_device *dsi;
> +	const struct st7701_panel_desc *desc;
> +
> +	struct backlight_device *backlight;
> +	struct regulator_bulk_data *supplies;
> +	unsigned int num_supplies;
I cannot see that num_supplies in this struct are used?


> +	struct gpio_desc *reset;
> +	unsigned int sleep_delay;
> +};
> +
> +static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct st7701, panel);
> +}
> +
> +static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
> +				   size_t len)
> +{
> +	return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
> +}
> +


> +static int st7701_prepare(struct drm_panel *panel)
> +{
> +	struct st7701 *st7701 = panel_to_st7701(panel);
> +	int ret;
> +
> +	gpiod_set_value(st7701->reset, 0);
> +	msleep(20);
> +
> +	ret = regulator_bulk_enable(st7701->desc->num_supplies,
> +				    st7701->supplies);
> +	if (ret < 0)
> +		return ret;
> +	msleep(20);
> +
> +	gpiod_set_value(st7701->reset, 1);
> +	msleep(20);
> +
> +	gpiod_set_value(st7701->reset, 0);
> +	msleep(30);
> +
> +	gpiod_set_value(st7701->reset, 1);
> +	msleep(150);
> +
> +	st7701_init_sequence(st7701);
> +
> +	return 0;
> +}
> +

> +static int st7701_unprepare(struct drm_panel *panel)
> +{
> +	struct st7701 *st7701 = panel_to_st7701(panel);
> +
> +	ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
> +
> +	msleep(st7701->sleep_delay);
> +
> +	gpiod_set_value(st7701->reset, 0);
> +
> +	gpiod_set_value(st7701->reset, 1);
> +
> +	gpiod_set_value(st7701->reset, 0);
No timing constrains here? In prepare there are sleeps intermixed.

> +
> +	regulator_bulk_disable(st7701->desc->num_supplies, st7701->supplies);
> +
> +	return 0;
> +}
> +
> +static int st7701_get_modes(struct drm_panel *panel)
> +{
> +	struct st7701 *st7701 = panel_to_st7701(panel);
> +	const struct drm_display_mode *desc_mode = st7701->desc->mode;
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, desc_mode);
> +	if (!mode) {
> +		dev_err(&st7701->dsi->dev, "failed to add mode %ux%ux@%u\n",
> +			desc_mode->hdisplay, desc_mode->vdisplay,
> +			desc_mode->vrefresh);
Use something like:
		DRM_DEV_ERROR(st7701->dev, "failed to add mode" DRM_MODE_FMT ",
			      DRM_MODE_ARG(desc_mode));

> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +	drm_mode_probed_add(panel->connector, mode);
> +
> +	panel->connector->display_info.width_mm = desc_mode->width_mm;
> +	panel->connector->display_info.height_mm = desc_mode->height_mm;
> +
> +	return 1;
> +}
> +

> +static const struct st7701_panel_desc ts8550b_desc = {
> +	.mode = &ts8550b_mode,
> +	.lanes = 2,
> +	.flags = MIPI_DSI_MODE_VIDEO,
> +	.format = MIPI_DSI_FMT_RGB888,
> +	.supply_names = ts8550b_supply_names,
> +	.num_supplies = ARRAY_SIZE(ts8550b_supply_names),
> +	.panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
In the only place this is used there is added 120 ms too.
Looks inconsistent - do we have the same delay twice?


> +
> +static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> +	const struct st7701_panel_desc *desc;
> +	struct device_node *np;
> +	struct st7701 *st7701;
> +	int ret, i;
> +
> +	st7701 = devm_kzalloc(&dsi->dev, sizeof(*st7701), GFP_KERNEL);
> +	if (!st7701)
> +		return -ENOMEM;
> +
> +	desc = of_device_get_match_data(&dsi->dev);
> +	dsi->mode_flags = desc->flags;
> +	dsi->format = desc->format;
> +	dsi->lanes = desc->lanes;
> +
> +	st7701->supplies = devm_kcalloc(&dsi->dev, desc->num_supplies,
> +					sizeof(*st7701->supplies),
> +					GFP_KERNEL);
> +	if (!st7701->supplies)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < desc->num_supplies; i++)
> +		st7701->supplies[i].supply = desc->supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(&dsi->dev, desc->num_supplies,
> +				      st7701->supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	st7701->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(st7701->reset)) {
> +		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
> +		return PTR_ERR(st7701->reset);
> +	}
> +
> +	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
> +	if (np) {
> +		st7701->backlight = of_find_backlight_by_node(np);
> +		of_node_put(np);
> +
> +		if (!st7701->backlight)
> +			return -EPROBE_DEFER;
> +	}
This is a simpler variant of the above:
	st7701->backlight = devm_of_find_backlight(dsi->dev);
	if (IS_ERR(st7701->backlight))
		return PTR_ERR(st7701->backlight);

Suggested by Noralf in another thread for a similar driver.


> +
> +	drm_panel_init(&st7701->panel);
> +
> +	/* We need to wait 120ms after a sleep out command */
> +	st7701->sleep_delay = 120 + desc->panel_sleep_delay;
This is the other place where we add 120 to the previous 80 - in total 200 ms.

> +	st7701->panel.funcs = &st7701_funcs;
> +	st7701->panel.dev = &dsi->dev;
> +
> +	ret = drm_panel_add(&st7701->panel);
> +	if (ret < 0)
> +		return ret;
> +
> +	mipi_dsi_set_drvdata(dsi, st7701);
> +	st7701->dsi = dsi;

> +	st7701->desc = desc;
This assignment could come earlier, so we do not have it unassigned when calling
other functions. As it is now it works so you can safely ignore this comment.

> +
> +	return mipi_dsi_attach(dsi);
> +}
> +
> +static int st7701_dsi_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct st7701 *st7701 = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&st7701->panel);
> +
> +	if (st7701->backlight)
> +		put_device(&st7701->backlight->dev);

Use:
	backlight_put(st7701->backlight);
Noralf Trønnes Jan. 11, 2019, 9:41 p.m.
Den 11.01.2019 22.19, skrev Sam Ravnborg:
> Hi Jagan.
> 
> Gave this another more detailed read - triggered some additional comments.
> Depite the comments it looks good, and this is all more or
> less cosmetic improvements.
> 
> 	Sam
> 
>> +struct st7701_panel_desc {
>> +	const struct drm_display_mode *mode;
>> +	unsigned int lanes;
>> +	unsigned long flags;
>> +	enum mipi_dsi_pixel_format format;
>> +	const char *const *supply_names;
>> +	unsigned int num_supplies;
>> +	unsigned int panel_sleep_delay;
>> +};
>> +
>> +struct st7701 {
>> +	struct drm_panel panel;
>> +	struct mipi_dsi_device *dsi;
>> +	const struct st7701_panel_desc *desc;
>> +
>> +	struct backlight_device *backlight;
>> +	struct regulator_bulk_data *supplies;
>> +	unsigned int num_supplies;
> I cannot see that num_supplies in this struct are used?
> 
> 
>> +	struct gpio_desc *reset;
>> +	unsigned int sleep_delay;
>> +};
>> +
>> +static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
>> +{
>> +	return container_of(panel, struct st7701, panel);
>> +}
>> +
>> +static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
>> +				   size_t len)
>> +{
>> +	return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
>> +}
>> +
> 
> 
>> +static int st7701_prepare(struct drm_panel *panel)
>> +{
>> +	struct st7701 *st7701 = panel_to_st7701(panel);
>> +	int ret;
>> +
>> +	gpiod_set_value(st7701->reset, 0);
>> +	msleep(20);
>> +
>> +	ret = regulator_bulk_enable(st7701->desc->num_supplies,
>> +				    st7701->supplies);
>> +	if (ret < 0)
>> +		return ret;
>> +	msleep(20);
>> +
>> +	gpiod_set_value(st7701->reset, 1);
>> +	msleep(20);
>> +
>> +	gpiod_set_value(st7701->reset, 0);
>> +	msleep(30);
>> +
>> +	gpiod_set_value(st7701->reset, 1);
>> +	msleep(150);
>> +
>> +	st7701_init_sequence(st7701);
>> +
>> +	return 0;
>> +}
>> +
> 
>> +static int st7701_unprepare(struct drm_panel *panel)
>> +{
>> +	struct st7701 *st7701 = panel_to_st7701(panel);
>> +
>> +	ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
>> +
>> +	msleep(st7701->sleep_delay);
>> +
>> +	gpiod_set_value(st7701->reset, 0);
>> +
>> +	gpiod_set_value(st7701->reset, 1);
>> +
>> +	gpiod_set_value(st7701->reset, 0);
> No timing constrains here? In prepare there are sleeps intermixed.
> 
>> +
>> +	regulator_bulk_disable(st7701->desc->num_supplies, st7701->supplies);
>> +
>> +	return 0;
>> +}
>> +
>> +static int st7701_get_modes(struct drm_panel *panel)
>> +{
>> +	struct st7701 *st7701 = panel_to_st7701(panel);
>> +	const struct drm_display_mode *desc_mode = st7701->desc->mode;
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_duplicate(panel->drm, desc_mode);
>> +	if (!mode) {
>> +		dev_err(&st7701->dsi->dev, "failed to add mode %ux%ux@%u\n",
>> +			desc_mode->hdisplay, desc_mode->vdisplay,
>> +			desc_mode->vrefresh);
> Use something like:
> 		DRM_DEV_ERROR(st7701->dev, "failed to add mode" DRM_MODE_FMT ",
> 			      DRM_MODE_ARG(desc_mode));
> 
>> +		return -ENOMEM;
>> +	}
>> +
>> +	drm_mode_set_name(mode);
>> +	drm_mode_probed_add(panel->connector, mode);
>> +
>> +	panel->connector->display_info.width_mm = desc_mode->width_mm;
>> +	panel->connector->display_info.height_mm = desc_mode->height_mm;
>> +
>> +	return 1;
>> +}
>> +
> 
>> +static const struct st7701_panel_desc ts8550b_desc = {
>> +	.mode = &ts8550b_mode,
>> +	.lanes = 2,
>> +	.flags = MIPI_DSI_MODE_VIDEO,
>> +	.format = MIPI_DSI_FMT_RGB888,
>> +	.supply_names = ts8550b_supply_names,
>> +	.num_supplies = ARRAY_SIZE(ts8550b_supply_names),
>> +	.panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
> In the only place this is used there is added 120 ms too.
> Looks inconsistent - do we have the same delay twice?
> 
> 
>> +
>> +static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	const struct st7701_panel_desc *desc;
>> +	struct device_node *np;
>> +	struct st7701 *st7701;
>> +	int ret, i;
>> +
>> +	st7701 = devm_kzalloc(&dsi->dev, sizeof(*st7701), GFP_KERNEL);
>> +	if (!st7701)
>> +		return -ENOMEM;
>> +
>> +	desc = of_device_get_match_data(&dsi->dev);
>> +	dsi->mode_flags = desc->flags;
>> +	dsi->format = desc->format;
>> +	dsi->lanes = desc->lanes;
>> +
>> +	st7701->supplies = devm_kcalloc(&dsi->dev, desc->num_supplies,
>> +					sizeof(*st7701->supplies),
>> +					GFP_KERNEL);
>> +	if (!st7701->supplies)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < desc->num_supplies; i++)
>> +		st7701->supplies[i].supply = desc->supply_names[i];
>> +
>> +	ret = devm_regulator_bulk_get(&dsi->dev, desc->num_supplies,
>> +				      st7701->supplies);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	st7701->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(st7701->reset)) {
>> +		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
>> +		return PTR_ERR(st7701->reset);
>> +	}
>> +
>> +	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
>> +	if (np) {
>> +		st7701->backlight = of_find_backlight_by_node(np);
>> +		of_node_put(np);
>> +
>> +		if (!st7701->backlight)
>> +			return -EPROBE_DEFER;
>> +	}
> This is a simpler variant of the above:
> 	st7701->backlight = devm_of_find_backlight(dsi->dev);
> 	if (IS_ERR(st7701->backlight))
> 		return PTR_ERR(st7701->backlight);
> 
> Suggested by Noralf in another thread for a similar driver.
> 
> 
>> +
>> +	drm_panel_init(&st7701->panel);
>> +
>> +	/* We need to wait 120ms after a sleep out command */
>> +	st7701->sleep_delay = 120 + desc->panel_sleep_delay;
> This is the other place where we add 120 to the previous 80 - in total 200 ms.
> 
>> +	st7701->panel.funcs = &st7701_funcs;
>> +	st7701->panel.dev = &dsi->dev;
>> +
>> +	ret = drm_panel_add(&st7701->panel);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	mipi_dsi_set_drvdata(dsi, st7701);
>> +	st7701->dsi = dsi;
> 
>> +	st7701->desc = desc;
> This assignment could come earlier, so we do not have it unassigned when calling
> other functions. As it is now it works so you can safely ignore this comment.
> 
>> +
>> +	return mipi_dsi_attach(dsi);
>> +}
>> +
>> +static int st7701_dsi_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct st7701 *st7701 = mipi_dsi_get_drvdata(dsi);
>> +
>> +	mipi_dsi_detach(dsi);
>> +	drm_panel_remove(&st7701->panel);
>> +
>> +	if (st7701->backlight)
>> +		put_device(&st7701->backlight->dev);
> 
> Use:
> 	backlight_put(st7701->backlight);

This happens automatically on driver detach if devm_of_find_backlight()
is used

Noralf.
Jagan Teki Jan. 12, 2019, 3:44 a.m.
On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Jagan.
>
> Gave this another more detailed read - triggered some additional comments.
> Depite the comments it looks good, and this is all more or
> less cosmetic improvements.

Thanks for the review.

>
>         Sam
>
> > +struct st7701_panel_desc {
> > +     const struct drm_display_mode *mode;
> > +     unsigned int lanes;
> > +     unsigned long flags;
> > +     enum mipi_dsi_pixel_format format;
> > +     const char *const *supply_names;
> > +     unsigned int num_supplies;
> > +     unsigned int panel_sleep_delay;
> > +};
> > +
> > +struct st7701 {
> > +     struct drm_panel panel;
> > +     struct mipi_dsi_device *dsi;
> > +     const struct st7701_panel_desc *desc;
> > +
> > +     struct backlight_device *backlight;
> > +     struct regulator_bulk_data *supplies;
> > +     unsigned int num_supplies;
> I cannot see that num_supplies in this struct are used?

Yes it is used in the code, please check in struct st7701_panel_desc.

>
>
> > +     struct gpio_desc *reset;
> > +     unsigned int sleep_delay;
> > +};
> > +
> > +static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
> > +{
> > +     return container_of(panel, struct st7701, panel);
> > +}
> > +
> > +static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
> > +                                size_t len)
> > +{
> > +     return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
> > +}
> > +
>
>
> > +static int st7701_prepare(struct drm_panel *panel)
> > +{
> > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > +     int ret;
> > +
> > +     gpiod_set_value(st7701->reset, 0);
> > +     msleep(20);
> > +
> > +     ret = regulator_bulk_enable(st7701->desc->num_supplies,
> > +                                 st7701->supplies);
> > +     if (ret < 0)
> > +             return ret;
> > +     msleep(20);
> > +
> > +     gpiod_set_value(st7701->reset, 1);
> > +     msleep(20);
> > +
> > +     gpiod_set_value(st7701->reset, 0);
> > +     msleep(30);
> > +
> > +     gpiod_set_value(st7701->reset, 1);
> > +     msleep(150);
> > +
> > +     st7701_init_sequence(st7701);
> > +
> > +     return 0;
> > +}
> > +
>
> > +static int st7701_unprepare(struct drm_panel *panel)
> > +{
> > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > +
> > +     ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
> > +
> > +     msleep(st7701->sleep_delay);
> > +
> > +     gpiod_set_value(st7701->reset, 0);
> > +
> > +     gpiod_set_value(st7701->reset, 1);
> > +
> > +     gpiod_set_value(st7701->reset, 0);
> No timing constrains here? In prepare there are sleeps intermixed.

Delay while doing unprare is not needed I suppose.

>
> > +
> > +     regulator_bulk_disable(st7701->desc->num_supplies, st7701->supplies);
> > +
> > +     return 0;
> > +}
> > +
> > +static int st7701_get_modes(struct drm_panel *panel)
> > +{
> > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > +     const struct drm_display_mode *desc_mode = st7701->desc->mode;
> > +     struct drm_display_mode *mode;
> > +
> > +     mode = drm_mode_duplicate(panel->drm, desc_mode);
> > +     if (!mode) {
> > +             dev_err(&st7701->dsi->dev, "failed to add mode %ux%ux@%u\n",
> > +                     desc_mode->hdisplay, desc_mode->vdisplay,
> > +                     desc_mode->vrefresh);
> Use something like:
>                 DRM_DEV_ERROR(st7701->dev, "failed to add mode" DRM_MODE_FMT ",
>                               DRM_MODE_ARG(desc_mode));
>
> > +             return -ENOMEM;
> > +     }
> > +
> > +     drm_mode_set_name(mode);
> > +     drm_mode_probed_add(panel->connector, mode);
> > +
> > +     panel->connector->display_info.width_mm = desc_mode->width_mm;
> > +     panel->connector->display_info.height_mm = desc_mode->height_mm;
> > +
> > +     return 1;
> > +}
> > +
>
> > +static const struct st7701_panel_desc ts8550b_desc = {
> > +     .mode = &ts8550b_mode,
> > +     .lanes = 2,
> > +     .flags = MIPI_DSI_MODE_VIDEO,
> > +     .format = MIPI_DSI_FMT_RGB888,
> > +     .supply_names = ts8550b_supply_names,
> > +     .num_supplies = ARRAY_SIZE(ts8550b_supply_names),
> > +     .panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
> In the only place this is used there is added 120 ms too.
> Looks inconsistent - do we have the same delay twice?

120ms is the one recommended by st7701 controller delay after a sleep
out command, please check it in datasheet [1], page 188. And this 80ms
is specific to TS8550B panel as per BSP code this doesn't have proper
documentation but the BSP code doing this extra 80ms.

>
>
> > +
> > +static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> > +{
> > +     const struct st7701_panel_desc *desc;
> > +     struct device_node *np;
> > +     struct st7701 *st7701;
> > +     int ret, i;
> > +
> > +     st7701 = devm_kzalloc(&dsi->dev, sizeof(*st7701), GFP_KERNEL);
> > +     if (!st7701)
> > +             return -ENOMEM;
> > +
> > +     desc = of_device_get_match_data(&dsi->dev);
> > +     dsi->mode_flags = desc->flags;
> > +     dsi->format = desc->format;
> > +     dsi->lanes = desc->lanes;
> > +
> > +     st7701->supplies = devm_kcalloc(&dsi->dev, desc->num_supplies,
> > +                                     sizeof(*st7701->supplies),
> > +                                     GFP_KERNEL);
> > +     if (!st7701->supplies)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < desc->num_supplies; i++)
> > +             st7701->supplies[i].supply = desc->supply_names[i];
> > +
> > +     ret = devm_regulator_bulk_get(&dsi->dev, desc->num_supplies,
> > +                                   st7701->supplies);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     st7701->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(st7701->reset)) {
> > +             dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
> > +             return PTR_ERR(st7701->reset);
> > +     }
> > +
> > +     np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
> > +     if (np) {
> > +             st7701->backlight = of_find_backlight_by_node(np);
> > +             of_node_put(np);
> > +
> > +             if (!st7701->backlight)
> > +                     return -EPROBE_DEFER;
> > +     }
> This is a simpler variant of the above:
>         st7701->backlight = devm_of_find_backlight(dsi->dev);
>         if (IS_ERR(st7701->backlight))
>                 return PTR_ERR(st7701->backlight);
>
> Suggested by Noralf in another thread for a similar driver.
>
>
> > +
> > +     drm_panel_init(&st7701->panel);
> > +
> > +     /* We need to wait 120ms after a sleep out command */
> > +     st7701->sleep_delay = 120 + desc->panel_sleep_delay;
> This is the other place where we add 120 to the previous 80 - in total 200 ms.

Please see above comment.

[1] http://www.startek-lcd.com/res/starteklcd/pdres/201705/20170512144242904.pdf
Sam Ravnborg Jan. 12, 2019, 10:14 a.m.
Hi Jagan.

> On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > Hi Jagan.
> >
> > Gave this another more detailed read - triggered some additional comments.
> > Depite the comments it looks good, and this is all more or
> > less cosmetic improvements.
> 
> Thanks for the review.
> 
> >
> >         Sam
> >
> > > +struct st7701_panel_desc {
> > > +     const struct drm_display_mode *mode;
> > > +     unsigned int lanes;
> > > +     unsigned long flags;
> > > +     enum mipi_dsi_pixel_format format;
> > > +     const char *const *supply_names;
> > > +     unsigned int num_supplies;
> > > +     unsigned int panel_sleep_delay;
> > > +};
> > > +
> > > +struct st7701 {
> > > +     struct drm_panel panel;
> > > +     struct mipi_dsi_device *dsi;
> > > +     const struct st7701_panel_desc *desc;
> > > +
> > > +     struct backlight_device *backlight;
> > > +     struct regulator_bulk_data *supplies;
> > > +     unsigned int num_supplies;
> > I cannot see that num_supplies in this struct are used?
> 
> Yes it is used in the code, please check in struct st7701_panel_desc.
I have applied the patch and deleted num_supplies - now build errors.
So num_supplies in struct st7701 is not used and should be deleted.

> > > +static int st7701_prepare(struct drm_panel *panel)
> > > +{
> > > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > > +     int ret;
> > > +
> > > +     gpiod_set_value(st7701->reset, 0);
> > > +     msleep(20);
> > > +
> > > +     ret = regulator_bulk_enable(st7701->desc->num_supplies,
> > > +                                 st7701->supplies);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +     msleep(20);
> > > +
> > > +     gpiod_set_value(st7701->reset, 1);
> > > +     msleep(20);
> > > +
> > > +     gpiod_set_value(st7701->reset, 0);
> > > +     msleep(30);
> > > +
> > > +     gpiod_set_value(st7701->reset, 1);
> > > +     msleep(150);
> > > +
> > > +     st7701_init_sequence(st7701);
> > > +
> > > +     return 0;
> > > +}
> > > +
> >
> > > +static int st7701_unprepare(struct drm_panel *panel)
> > > +{
> > > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > > +
> > > +     ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);

Should this be MIPI_DCS_ENTER_SLEEP_MODE?

Note - you have shortcuts fot the standard commands like
in this case:

	mipi_dsi_dcs_enter_sleep_mode(st7701->dsi);

Thay could be introduced in many palces, but I also like how all the
commands follows a consistent way to be issued.
So consider this only if this was new for you.


> > > +
> > > +     msleep(st7701->sleep_delay);
> > > +
> > > +     gpiod_set_value(st7701->reset, 0);
> > > +
> > > +     gpiod_set_value(st7701->reset, 1);
> > > +
> > > +     gpiod_set_value(st7701->reset, 0);
> > No timing constrains here? In prepare there are sleeps intermixed.
> 
> Delay while doing unprare is not needed I suppose.

If the purpose is alone to reset the display then a single write '0'
should do it I think
And there is a requirement that it must be low for a minimum of 10 us
which would be good to have here.

I aslo found in chapter 9. (page 163 - second line) this statement:
"VDDA and VDDI must be powered down with minimum 120msec."

This is similar to the unprepare delay to be found in simple-panel.c
So an unprepare delay seems in order here.


> > > +static const struct st7701_panel_desc ts8550b_desc = {
> > > +     .mode = &ts8550b_mode,
> > > +     .lanes = 2,
> > > +     .flags = MIPI_DSI_MODE_VIDEO,
> > > +     .format = MIPI_DSI_FMT_RGB888,
> > > +     .supply_names = ts8550b_supply_names,
> > > +     .num_supplies = ARRAY_SIZE(ts8550b_supply_names),
> > > +     .panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
> > In the only place this is used there is added 120 ms too.
> > Looks inconsistent - do we have the same delay twice?
> 
> 120ms is the one recommended by st7701 controller delay after a sleep
> out command, please check it in datasheet [1], page 188. And this 80ms
> is specific to TS8550B panel as per BSP code this doesn't have proper
> documentation but the BSP code doing this extra 80ms.

OK, thanks for the pointer to the datasheet.

> [1] http://www.startek-lcd.com/res/starteklcd/pdres/201705/20170512144242904.pdf


	Sam
Jagan Teki Jan. 12, 2019, 10:46 a.m.
On Sat, Jan 12, 2019 at 3:44 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Jagan.
>
> > On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> > >
> > > Hi Jagan.
> > >
> > > Gave this another more detailed read - triggered some additional comments.
> > > Depite the comments it looks good, and this is all more or
> > > less cosmetic improvements.
> >
> > Thanks for the review.
> >
> > >
> > >         Sam
> > >
> > > > +struct st7701_panel_desc {
> > > > +     const struct drm_display_mode *mode;
> > > > +     unsigned int lanes;
> > > > +     unsigned long flags;
> > > > +     enum mipi_dsi_pixel_format format;
> > > > +     const char *const *supply_names;
> > > > +     unsigned int num_supplies;
> > > > +     unsigned int panel_sleep_delay;
> > > > +};
> > > > +
> > > > +struct st7701 {
> > > > +     struct drm_panel panel;
> > > > +     struct mipi_dsi_device *dsi;
> > > > +     const struct st7701_panel_desc *desc;
> > > > +
> > > > +     struct backlight_device *backlight;
> > > > +     struct regulator_bulk_data *supplies;
> > > > +     unsigned int num_supplies;
> > > I cannot see that num_supplies in this struct are used?
> >
> > Yes it is used in the code, please check in struct st7701_panel_desc.
> I have applied the patch and deleted num_supplies - now build errors.
> So num_supplies in struct st7701 is not used and should be deleted.

Sorry, now I found it thanks, will drop in next version.

>
> > > > +static int st7701_prepare(struct drm_panel *panel)
> > > > +{
> > > > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > > > +     int ret;
> > > > +
> > > > +     gpiod_set_value(st7701->reset, 0);
> > > > +     msleep(20);
> > > > +
> > > > +     ret = regulator_bulk_enable(st7701->desc->num_supplies,
> > > > +                                 st7701->supplies);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > > +     msleep(20);
> > > > +
> > > > +     gpiod_set_value(st7701->reset, 1);
> > > > +     msleep(20);
> > > > +
> > > > +     gpiod_set_value(st7701->reset, 0);
> > > > +     msleep(30);
> > > > +
> > > > +     gpiod_set_value(st7701->reset, 1);
> > > > +     msleep(150);
> > > > +
> > > > +     st7701_init_sequence(st7701);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > >
> > > > +static int st7701_unprepare(struct drm_panel *panel)
> > > > +{
> > > > +     struct st7701 *st7701 = panel_to_st7701(panel);
> > > > +
> > > > +     ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
>
> Should this be MIPI_DCS_ENTER_SLEEP_MODE?
>
> Note - you have shortcuts fot the standard commands like
> in this case:
>
>         mipi_dsi_dcs_enter_sleep_mode(st7701->dsi);

I even tried this, but the difference wrt ST7701 can pass, same
command with type MIPI_DSI_DCS_SHORT_WRITE_PARAM but the default
mipi_dsi_dcs_enter_sleep_mode has length 0 so it's
MIPI_DSI_DCS_SHORT_WRITE type. ie reason I have not used.

>
> Thay could be introduced in many palces, but I also like how all the
> commands follows a consistent way to be issued.
> So consider this only if this was new for you.
>
>
> > > > +
> > > > +     msleep(st7701->sleep_delay);
> > > > +
> > > > +     gpiod_set_value(st7701->reset, 0);
> > > > +
> > > > +     gpiod_set_value(st7701->reset, 1);
> > > > +
> > > > +     gpiod_set_value(st7701->reset, 0);
> > > No timing constrains here? In prepare there are sleeps intermixed.
> >
> > Delay while doing unprare is not needed I suppose.
>
> If the purpose is alone to reset the display then a single write '0'
> should do it I think

I even tried this just set 0, since prepare is doing a sequence, it
good behavior to do the reverse during handoff. ie reason I just
initiated this sequence.

> And there is a requirement that it must be low for a minimum of 10 us
> which would be good to have here.

Sorry, I didn't get this requirement what is this for?

>
> I aslo found in chapter 9. (page 163 - second line) this statement:
> "VDDA and VDDI must be powered down with minimum 120msec."

Yes unprepare is doing the same, it exit from sleep out mode and wait
for 120msec and do the reset.

>
> This is similar to the unprepare delay to be found in simple-panel.c
> So an unprepare delay seems in order here.

Look like simple-panel.c is doing delay after reset initiated and
regulator disabled.
Sam Ravnborg Jan. 12, 2019, noon
> >
> > > > > +
> > > > > +     msleep(st7701->sleep_delay);
> > > > > +
> > > > > +     gpiod_set_value(st7701->reset, 0);
> > > > > +
> > > > > +     gpiod_set_value(st7701->reset, 1);
> > > > > +
> > > > > +     gpiod_set_value(st7701->reset, 0);
> > > > No timing constrains here? In prepare there are sleeps intermixed.
> > >
> > > Delay while doing unprare is not needed I suppose.
> >
> > If the purpose is alone to reset the display then a single write '0'
> > should do it I think
> 
> I even tried this just set 0, since prepare is doing a sequence, it
> good behavior to do the reverse during handoff. ie reason I just
> initiated this sequence.
> 
> > And there is a requirement that it must be low for a minimum of 10 us
> > which would be good to have here.
> 
> Sorry, I didn't get this requirement what is this for?
The st7701 will ignore reset pulse shorter than 10 us - see Trw in
table 9 reset timing (page 54).

But as we just assert reset (set it to 0), this timing constraint can be ignored.

> > I aslo found in chapter 9. (page 163 - second line) this statement:
> > "VDDA and VDDI must be powered down with minimum 120msec."
> 
> Yes unprepare is doing the same, it exit from sleep out mode and wait
> for 120msec and do the reset.
> 
> >
> > This is similar to the unprepare delay to be found in simple-panel.c
> > So an unprepare delay seems in order here.
> 
> Look like simple-panel.c is doing delay after reset initiated and
> regulator disabled.


	Sam
Jagan Teki Jan. 12, 2019, 1:25 p.m.
On Sat, Jan 12, 2019 at 5:31 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> > >
> > > > > > +
> > > > > > +     msleep(st7701->sleep_delay);
> > > > > > +
> > > > > > +     gpiod_set_value(st7701->reset, 0);
> > > > > > +
> > > > > > +     gpiod_set_value(st7701->reset, 1);
> > > > > > +
> > > > > > +     gpiod_set_value(st7701->reset, 0);
> > > > > No timing constrains here? In prepare there are sleeps intermixed.
> > > >
> > > > Delay while doing unprare is not needed I suppose.
> > >
> > > If the purpose is alone to reset the display then a single write '0'
> > > should do it I think
> >
> > I even tried this just set 0, since prepare is doing a sequence, it
> > good behavior to do the reverse during handoff. ie reason I just
> > initiated this sequence.
> >
> > > And there is a requirement that it must be low for a minimum of 10 us
> > > which would be good to have here.
> >
> > Sorry, I didn't get this requirement what is this for?
> The st7701 will ignore reset pulse shorter than 10 us - see Trw in
> table 9 reset timing (page 54).
>
> But as we just assert reset (set it to 0), this timing constraint can be ignored.

But we unaware of reset pulse duration right? it's the hardware that
bring the reset assert if we set the line 0. am I correct or do we
need to explicitly wait 10us after reset initiated?
there is family of chip Sitronix st7789v which don't taking care of
this sequence (I don't know why?) in
drivers/gpu/drm/panel/panel-sitronix-st7789v.c with Page 48 of
datasheet[2]

[2] https://www.newhavendisplay.com/appnotes/datasheets/LCDs/ST7789V.pdf
Sam Ravnborg Jan. 12, 2019, 3:33 p.m.
Hi Jagan.

> > But as we just assert reset (set it to 0), this timing constraint can be ignored.
> 
> But we unaware of reset pulse duration right? it's the hardware that
> bring the reset assert if we set the line 0. am I correct or do we
> need to explicitly wait 10us after reset initiated?
> there is family of chip Sitronix st7789v which don't taking care of
> this sequence (I don't know why?) in
> drivers/gpu/drm/panel/panel-sitronix-st7789v.c with Page 48 of
> datasheet[2]


The prepare sequence used is:
1) reset
   20 ms
2) regulator enable
   20 ms
3) unassert reset
   20 ms
4) assert reset
   30 ms
4) unassert reset
   150 ms
5) SOFT RESET
   5 ms
6) Exit SLEEP mode
   sleep_delay (120 ms)
7) COMMANDS

The enable sequence is
1) Display ON
2) backlight enable


The disable sequence is
1) backlight disable
2) Display OFF

The unprepare sequence is
1) Exit SLEEP mode  <= this should be Enter SLEEP mode (covered by previous mail)
   sleep_delay (120 ms)
2) assert reset
3) unassert reset
4) assert reset
5) regulator disable


The implementation in simple-panel supports a long list of panels so we
can assume this is a good reference implementation.
Unless your combo of st7701 + panel has special requirements.

Prepare sequence:
-----------------
- Just set reset to 1 - no need to trigger an edge.
  But if you somehow think the edge is required keep the current code
  but drop the delays that are not required.
  We can assume power is OK when it is enabled, no extra waiting required.
- No reason to unassert and then assert and then unassert reset.

In other words:
- Delay between 1 and 2 can be dropped.
- step 3 and step 4 can be dropped

Unprepare sequence:
-------------------
Enter SLEEP mode may take 120 ms so delay between 1) and 2) is OK.
No reason to do the reset dance, drop 2) and 3)
Make sure reset is completed, so wait before leaving the function.
See note 3. on page 54 in datasheet:

	"During the Resetting period, the display will be blanked
	 (The display is entering blanking sequence, which maximum
	 time is 120 ms, when Reset Starts in Sleep Out –mode.
	 The display remains the blank state in Sleep In –mode.) and
	  then return to Default condition for Hardware Reset."

We have already sent an Enter SLEEP command but we have no timing constrains
when panel is in SLEEP mode, So stick to the 120 ms.
Just use sleep_delay.

Thats my best understanding, your reading of the datasheet or your testing
may prove different.

	Sam
Jagan Teki Jan. 12, 2019, 4:32 p.m.
Hi Sam,

On Sat, Jan 12, 2019 at 9:03 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Jagan.
>
> > > But as we just assert reset (set it to 0), this timing constraint can be ignored.
> >
> > But we unaware of reset pulse duration right? it's the hardware that
> > bring the reset assert if we set the line 0. am I correct or do we
> > need to explicitly wait 10us after reset initiated?
> > there is family of chip Sitronix st7789v which don't taking care of
> > this sequence (I don't know why?) in
> > drivers/gpu/drm/panel/panel-sitronix-st7789v.c with Page 48 of
> > datasheet[2]
>
>
> The prepare sequence used is:
> 1) reset
>    20 ms
> 2) regulator enable
>    20 ms
> 3) unassert reset
>    20 ms
> 4) assert reset
>    30 ms
> 4) unassert reset
>    150 ms
> 5) SOFT RESET
>    5 ms
> 6) Exit SLEEP mode
>    sleep_delay (120 ms)
> 7) COMMANDS
>
> The enable sequence is
> 1) Display ON
> 2) backlight enable
>
>
> The disable sequence is
> 1) backlight disable
> 2) Display OFF
>
> The unprepare sequence is
> 1) Exit SLEEP mode  <= this should be Enter SLEEP mode (covered by previous mail)
>    sleep_delay (120 ms)
> 2) assert reset
> 3) unassert reset
> 4) assert reset
> 5) regulator disable
>
>
> The implementation in simple-panel supports a long list of panels so we
> can assume this is a good reference implementation.
> Unless your combo of st7701 + panel has special requirements.
>
> Prepare sequence:
> -----------------
> - Just set reset to 1 - no need to trigger an edge.
>   But if you somehow think the edge is required keep the current code
>   but drop the delays that are not required.
>   We can assume power is OK when it is enabled, no extra waiting required.
> - No reason to unassert and then assert and then unassert reset.
>
> In other words:
> - Delay between 1 and 2 can be dropped.
> - step 3 and step 4 can be dropped
>
> Unprepare sequence:
> -------------------
> Enter SLEEP mode may take 120 ms so delay between 1) and 2) is OK.
> No reason to do the reset dance, drop 2) and 3)
> Make sure reset is completed, so wait before leaving the function.
> See note 3. on page 54 in datasheet:
>
>         "During the Resetting period, the display will be blanked
>          (The display is entering blanking sequence, which maximum
>          time is 120 ms, when Reset Starts in Sleep Out –mode.
>          The display remains the blank state in Sleep In –mode.) and
>           then return to Default condition for Hardware Reset."
>
> We have already sent an Enter SLEEP command but we have no timing constrains
> when panel is in SLEEP mode, So stick to the 120 ms.
> Just use sleep_delay.

So we need this delay even after reset unlike SLEEP mode delay. OK,
thanks for the points will try to check all these points.

When did .unprepare and .disable are actually called? I turn-off the
backlight by echo 1 > /sys/class/backlight/backlight/bl_power and even
power of the board I assume the video transmission stop so it would
ended-up calling these, but I couldn't see prints. does it the same
time uart dead or something? do you have any inputs on sanity testing
of typical panels, if yes please share.

Jagan.
Sam Ravnborg Jan. 12, 2019, 4:53 p.m.
Hi Jagan.

> When did .unprepare and .disable are actually called? I turn-off the
> backlight by echo 1 > /sys/class/backlight/backlight/bl_power and even
> power of the board I assume the video transmission stop so it would
> ended-up calling these, but I couldn't see prints. does it the same
> time uart dead or something? do you have any inputs on sanity testing
> of typical panels, if yes please share.

Sorry, I do not rememebr the details to help you here.
Maybe module unload and then manipulating the panel in /sys/ would
do the trick.

	Sam