[PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[Guido Günther]

v4 fixes up the DT binding example and uses a wider cc list since I
failed to extend that when touching more files.

The panel is a 5.5" 720x1440 TFT LCD MIPI DSI panel with built in
touchscreen and backlight as found in the Librem 5 devkit.

These patches are against linux next as of 2019-03-22. v3 got acked by Sam
Ravnborg:

  https://lists.freedesktop.org/archives/dri-devel/2019-March/209326.html

Changes from v4
* As per review comments from Fabio Estevam
  * Add missing unit name in dt binding docs

Changes from v3
* Forward to next-20190322
* Add MAINTAINERS entry

Changes from v2
* As per review comments from Sam Ravnborg
  * Lowercase sentinel
  * Drop '_panel' postfix
  * DRM_DEV_ logging instead of plain DRM_
* Add Reviewed-by:
* Add "panel-rocktech-" to the driver name following
  the pattern from other drm panel drivers.

Changes from v1
* As per review comments from Sam Ravnborg
  * Make SPDX-License-Identifier match MODULE_LICENSE
  * Sort include files alphabetically
  * Drop drmP.h and use individual includes
  * Drop superfuous 'x' in mode printout on error path
  * Allpixelson_set: Add proper space around '*'
  * Drop superfluous put_device(&ctx->backlight->dev);
  * Add /* Sentinel */ in jh057n_of_match
  * Drop jh057n->enabled
  * Drop drm_display_info_set_bus_formats
* Kconfig: Depend on BACKLIGHT_CLASS_DEVICE which somehow got lost
* Move jh057n_enable close to jh057n_disable

Guido Günther (3):
  dt-bindings: Add vendor prefix for ROCKTECH DISPLAYS LIMITED
  dt-bindings: Add Rocktech jh057n00900 panel bindings
  drm/panel: Add Rocktech jh057n00900 panel driver

 .../display/panel/rocktech,jh057n00900.txt    |  18 +
 .../devicetree/bindings/vendor-prefixes.txt   |   1 +
 MAINTAINERS                                   |   6 +
 drivers/gpu/drm/panel/Kconfig                 |  13 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../drm/panel/panel-rocktech-jh057n00900.c    | 386 ++++++++++++++++++
 6 files changed, 425 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.txt
 create mode 100644 drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c

-- 
2.20.1

[PATCH v5 1/3] dt-bindings: Add vendor prefix for ROCKTECH DISPLAYS LIMITED

[Guido Günther]

Add ROCKTECH DISPLAYS LIMITED (https://rocktech.com.hk) LCD panel
supplier.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index fae55f50b2d3..326a4b278186 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -346,6 +346,7 @@ ricoh	Ricoh Co. Ltd.
 rikomagic	Rikomagic Tech Corp. Ltd
 riscv	RISC-V Foundation
 rockchip	Fuzhou Rockchip Electronics Co., Ltd
+rocktech	ROCKTECH DISPLAYS LIMITED
 rohm	ROHM Semiconductor Co., Ltd
 ronbo   Ronbo Electronics
 roofull	Shenzhen Roofull Technology Co, Ltd
-- 
2.20.1

[PATCH v5 2/3] dt-bindings: Add Rocktech jh057n00900 panel bindings

[Guido Günther]

The Rocktec jh057n00900 is a 5.5" MIPI DSI video mode panel with a
720x1440 resolution and a built in backlight.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 .../display/panel/rocktech,jh057n00900.txt     | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.txt

diff --git a/Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.txt b/Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.txt
new file mode 100644
index 000000000000..1b5763200cf6
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.txt
@@ -0,0 +1,18 @@
+Rocktech jh057n00900 5.5" 720x1440 TFT LCD panel
+
+Required properties:
+- compatible: should be "rocktech,jh057n00900"
+- reg: DSI virtual channel of the peripheral
+- reset-gpios: panel reset gpio
+- backlight: phandle of the backlight device attached to the panel
+
+Example:
+
+	&mipi_dsi {
+		panel@0 {
+			compatible = "rocktech,jh057n00900";
+			reg = <0>;
+			backlight = <&backlight>;
+			reset-gpios = <&gpio3 13 GPIO_ACTIVE_LOW>;
+		};
+	};
-- 
2.20.1

[PATCH v5 3/3] drm/panel: Add Rocktech jh057n00900 panel driver

[Guido Günther]

Support Rocktech jh057n00900 5.5" 720x1440 TFT LCD panel. It is a MIPI
DSI video mode panel.

The panel seems to use a Sitronix ST7703 look alike (most of the
commands look similar to the ST7703's data sheet but use a different
number of parameters). The initial version of the DSI init sequence
(including sleeps) were provided by the vendor. Sleeps were reduced
considerably though to speed up initialization.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 MAINTAINERS                                   |   6 +
 drivers/gpu/drm/panel/Kconfig                 |  13 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../drm/panel/panel-rocktech-jh057n00900.c    | 386 ++++++++++++++++++
 4 files changed, 406 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8f1f505920df..57968eabe844 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4999,6 +4999,12 @@ S:	Orphan / Obsolete
 F:	drivers/gpu/drm/r128/
 F:	include/uapi/drm/r128_drm.h
 
+DRM DRIVER FOR ROCKTECH JH057N00900 PANELS
+M:	Guido Günther <agx@sigxcpu.org>
+S:	Maintained
+F:	drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
+F:	Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.txt
+
 DRM DRIVER FOR SAVAGE VIDEO CARDS
 S:	Orphan / Obsolete
 F:	drivers/gpu/drm/savage/
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index f53f817356db..645ceb5776a9 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -149,6 +149,19 @@ config DRM_PANEL_RAYDIUM_RM68200
 	  Say Y here if you want to enable support for Raydium RM68200
 	  720x1280 DSI video mode panel.
 
+config DRM_PANEL_ROCKTECH_JH057N00900
+	tristate "Rocktech JH057N00900 MIPI touchscreen panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for Rocktech JH057N00900
+	  MIPI DSI panel as e.g. used in the Librem 5 devkit. It has a
+	  resolution of 720x1440 pixels, a built in backlight and touch
+	  controller.
+	  Touch input support is provided by the goodix driver and needs to be
+	  selected separately.
+
 config DRM_PANEL_RONBO_RB070D30
 	tristate "Ronbo Electronics RB070D30 panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 7834947a53b0..0e739bbc44b8 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
 obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
 obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
+obj-$(CONFIG_DRM_PANEL_ROCKTECH_JH057N00900) += panel-rocktech-jh057n00900.o
 obj-$(CONFIG_DRM_PANEL_RONBO_RB070D30) += panel-ronbo-rb070d30.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
new file mode 100644
index 000000000000..158a6d548068
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
@@ -0,0 +1,386 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Rockteck jh057n00900 5.5" MIPI-DSI panel driver
+ *
+ * Copyright (C) Purism SPC 2019
+ */
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+#include <linux/backlight.h>
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/media-bus-format.h>
+#include <linux/module.h>
+#include <video/display_timing.h>
+#include <video/mipi_display.h>
+
+#define DRV_NAME "panel-rocktech-jh057n00900"
+
+/* Manufacturer specific Commands send via DSI */
+#define ST7703_CMD_ALL_PIXEL_OFF 0x22
+#define ST7703_CMD_ALL_PIXEL_ON	 0x23
+#define ST7703_CMD_SETDISP	 0xB2
+#define ST7703_CMD_SETRGBIF	 0xB3
+#define ST7703_CMD_SETCYC	 0xB4
+#define ST7703_CMD_SETBGP	 0xB5
+#define ST7703_CMD_SETVCOM	 0xB6
+#define ST7703_CMD_SETOTP	 0xB7
+#define ST7703_CMD_SETPOWER_EXT	 0xB8
+#define ST7703_CMD_SETEXTC	 0xB9
+#define ST7703_CMD_SETMIPI	 0xBA
+#define ST7703_CMD_SETVDC	 0xBC
+#define ST7703_CMD_SETSCR	 0xC0
+#define ST7703_CMD_SETPOWER	 0xC1
+#define ST7703_CMD_SETPANEL	 0xCC
+#define ST7703_CMD_SETGAMMA	 0xE0
+#define ST7703_CMD_SETEQ	 0xE3
+#define ST7703_CMD_SETGIP1	 0xE9
+#define ST7703_CMD_SETGIP2	 0xEA
+
+struct jh057n {
+	struct device *dev;
+	struct drm_panel panel;
+	struct gpio_desc *reset_gpio;
+	struct backlight_device *backlight;
+	bool prepared;
+
+	struct dentry *debugfs;
+};
+
+static inline struct jh057n *panel_to_jh057n(struct drm_panel *panel)
+{
+	return container_of(panel, struct jh057n, panel);
+}
+
+#define dsi_generic_write_seq(dsi, seq...) do {				\
+		static const u8 d[] = { seq };				\
+		int ret;						\
+		ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));	\
+		if (ret < 0)						\
+			return ret;					\
+	} while (0)
+
+static int jh057n_init_sequence(struct jh057n *ctx)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	struct device *dev = ctx->dev;
+	int ret;
+
+	/*
+	 * Init sequence was supplied by the panel vendor. Most of the commands
+	 * resemble the ST7703 but the number of parameters often don't match
+	 * so it's likely a clone.
+	 */
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETEXTC,
+			      0xF1, 0x12, 0x83);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETRGBIF,
+			      0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00,
+			      0x00, 0x00);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETSCR,
+			      0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
+			      0x00);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETVDC, 0x4E);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0B);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETCYC, 0x80);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETDISP, 0xF0, 0x12, 0x30);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETEQ,
+			      0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
+			      0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETBGP, 0x08, 0x08);
+	msleep(20);
+
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETVCOM, 0x3F, 0x3F);
+	dsi_generic_write_seq(dsi, 0xBF, 0x02, 0x11, 0x00);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP1,
+			      0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5, 0x12,
+			      0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,
+			      0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00,
+			      0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88,
+			      0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64,
+			      0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
+			      0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP2,
+			      0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88,
+			      0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13,
+			      0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
+			      0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x0A,
+			      0xA5, 0x00, 0x00, 0x00, 0x00);
+	dsi_generic_write_seq(dsi, ST7703_CMD_SETGAMMA,
+			      0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41, 0x37,
+			      0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10, 0x11,
+			      0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
+			      0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
+			      0x11, 0x18);
+	msleep(20);
+
+	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "Failed to exit sleep mode");
+		return ret;
+	}
+	/* Panel is operational 120 msec after reset */
+	msleep(60);
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret)
+		return ret;
+
+	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done");
+	return 0;
+}
+
+static int jh057n_enable(struct drm_panel *panel)
+{
+	struct jh057n *ctx = panel_to_jh057n(panel);
+
+	return backlight_enable(ctx->backlight);
+}
+
+static int jh057n_disable(struct drm_panel *panel)
+{
+	struct jh057n *ctx = panel_to_jh057n(panel);
+
+	return backlight_disable(ctx->backlight);
+}
+
+static int jh057n_unprepare(struct drm_panel *panel)
+{
+	struct jh057n *ctx = panel_to_jh057n(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+	if (!ctx->prepared)
+		return 0;
+
+	mipi_dsi_dcs_set_display_off(dsi);
+	ctx->prepared = false;
+
+	return 0;
+}
+
+static int jh057n_prepare(struct drm_panel *panel)
+{
+	struct jh057n *ctx = panel_to_jh057n(panel);
+	int ret;
+
+	if (ctx->prepared)
+		return 0;
+
+	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel.");
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	usleep_range(20, 40);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	msleep(20);
+
+	ret = jh057n_init_sequence(ctx);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d", ret);
+		return ret;
+	}
+
+	ctx->prepared = true;
+
+	return 0;
+}
+
+static const struct drm_display_mode default_mode = {
+	.hdisplay    = 720,
+	.hsync_start = 720 + 90,
+	.hsync_end   = 720 + 90 + 20,
+	.htotal	     = 720 + 90 + 20 + 20,
+	.vdisplay    = 1440,
+	.vsync_start = 1440 + 20,
+	.vsync_end   = 1440 + 20 + 4,
+	.vtotal	     = 1440 + 20 + 4 + 12,
+	.vrefresh    = 60,
+	.clock	     = 75276,
+	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	.width_mm    = 65,
+	.height_mm   = 130,
+};
+
+static int jh057n_get_modes(struct drm_panel *panel)
+{
+	struct jh057n *ctx = panel_to_jh057n(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(panel->drm, &default_mode);
+	if (!mode) {
+		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u",
+			      default_mode.hdisplay, default_mode.vdisplay,
+			      default_mode.vrefresh);
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	panel->connector->display_info.width_mm = mode->width_mm;
+	panel->connector->display_info.height_mm = mode->height_mm;
+	drm_mode_probed_add(panel->connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs jh057n_drm_funcs = {
+	.disable   = jh057n_disable,
+	.unprepare = jh057n_unprepare,
+	.prepare   = jh057n_prepare,
+	.enable	   = jh057n_enable,
+	.get_modes = jh057n_get_modes,
+};
+
+static int allpixelson_set(void *data, u64 val)
+{
+	struct jh057n *ctx = data;
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on");
+	dsi_generic_write_seq(dsi, ST7703_CMD_ALL_PIXEL_ON);
+	msleep(val * 1000);
+	/* Reset the panel to get video back */
+	drm_panel_disable(&ctx->panel);
+	drm_panel_unprepare(&ctx->panel);
+	drm_panel_prepare(&ctx->panel);
+	drm_panel_enable(&ctx->panel);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(allpixelson_fops, NULL,
+			allpixelson_set, "%llu\n");
+
+static int jh057n_debugfs_init(struct jh057n *ctx)
+{
+	struct dentry *f;
+
+	ctx->debugfs = debugfs_create_dir(DRV_NAME, NULL);
+	if (!ctx->debugfs)
+		return -ENOMEM;
+
+	f = debugfs_create_file("allpixelson", 0600,
+				ctx->debugfs, ctx, &allpixelson_fops);
+	if (!f)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void jh057n_debugfs_remove(struct jh057n *ctx)
+{
+	debugfs_remove_recursive(ctx->debugfs);
+	ctx->debugfs = NULL;
+}
+
+static int jh057n_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct jh057n *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->reset_gpio)) {
+		DRM_DEV_ERROR(dev, "cannot get reset gpio");
+		return PTR_ERR(ctx->reset_gpio);
+	}
+
+	mipi_dsi_set_drvdata(dsi, ctx);
+
+	ctx->dev = dev;
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
+		MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
+
+	ctx->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(ctx->backlight))
+		return PTR_ERR(ctx->backlight);
+
+	drm_panel_init(&ctx->panel);
+	ctx->panel.dev = dev;
+	ctx->panel.funcs = &jh057n_drm_funcs;
+
+	drm_panel_add(&ctx->panel);
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?");
+		drm_panel_remove(&ctx->panel);
+		return ret;
+	}
+
+	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready",
+		     default_mode.hdisplay, default_mode.vdisplay,
+		     default_mode.vrefresh,
+		     mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
+
+	jh057n_debugfs_init(ctx);
+	return 0;
+}
+
+static void jh057n_shutdown(struct mipi_dsi_device *dsi)
+{
+	struct jh057n *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	ret = jh057n_unprepare(&ctx->panel);
+	if (ret < 0)
+		DRM_DEV_ERROR(&dsi->dev, "Failed to unprepare panel: %d\n",
+			      ret);
+
+	ret = jh057n_disable(&ctx->panel);
+	if (ret < 0)
+		DRM_DEV_ERROR(&dsi->dev, "Failed to disable panel: %d\n",
+			      ret);
+}
+
+static int jh057n_remove(struct mipi_dsi_device *dsi)
+{
+	struct jh057n *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	jh057n_shutdown(dsi);
+
+	ret = mipi_dsi_detach(dsi);
+	if (ret < 0)
+		DRM_DEV_ERROR(&dsi->dev, "Failed to detach from DSI host: %d\n",
+			      ret);
+
+	drm_panel_remove(&ctx->panel);
+
+	jh057n_debugfs_remove(ctx);
+
+	return 0;
+}
+
+static const struct of_device_id jh057n_of_match[] = {
+	{ .compatible = "rocktech,jh057n00900" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, jh057n_of_match);
+
+static struct mipi_dsi_driver jh057n_driver = {
+	.probe	= jh057n_probe,
+	.remove = jh057n_remove,
+	.shutdown = jh057n_shutdown,
+	.driver = {
+		.name = DRV_NAME,
+		.of_match_table = jh057n_of_match,
+	},
+};
+module_mipi_dsi_driver(jh057n_driver);
+
+MODULE_AUTHOR("Guido Günther <agx@sigxcpu.org>");
+MODULE_DESCRIPTION("DRM driver for Rocktech JH057N00900 MIPI DSI panel");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

Re: [PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2741 bytes --]

On Mon, Apr 01, 2019 at 12:35:32PM +0200, Guido Günther wrote:
> v4 fixes up the DT binding example and uses a wider cc list since I
> failed to extend that when touching more files.
> 
> The panel is a 5.5" 720x1440 TFT LCD MIPI DSI panel with built in
> touchscreen and backlight as found in the Librem 5 devkit.
> 
> These patches are against linux next as of 2019-03-22. v3 got acked by Sam
> Ravnborg:
> 
>   https://lists.freedesktop.org/archives/dri-devel/2019-March/209326.html
> 
> Changes from v4
> * As per review comments from Fabio Estevam
>   * Add missing unit name in dt binding docs
> 
> Changes from v3
> * Forward to next-20190322
> * Add MAINTAINERS entry
> 
> Changes from v2
> * As per review comments from Sam Ravnborg
>   * Lowercase sentinel
>   * Drop '_panel' postfix
>   * DRM_DEV_ logging instead of plain DRM_
> * Add Reviewed-by:
> * Add "panel-rocktech-" to the driver name following
>   the pattern from other drm panel drivers.
> 
> Changes from v1
> * As per review comments from Sam Ravnborg
>   * Make SPDX-License-Identifier match MODULE_LICENSE
>   * Sort include files alphabetically
>   * Drop drmP.h and use individual includes
>   * Drop superfuous 'x' in mode printout on error path
>   * Allpixelson_set: Add proper space around '*'
>   * Drop superfluous put_device(&ctx->backlight->dev);
>   * Add /* Sentinel */ in jh057n_of_match
>   * Drop jh057n->enabled
>   * Drop drm_display_info_set_bus_formats
> * Kconfig: Depend on BACKLIGHT_CLASS_DEVICE which somehow got lost
> * Move jh057n_enable close to jh057n_disable
> 
> Guido Günther (3):
>   dt-bindings: Add vendor prefix for ROCKTECH DISPLAYS LIMITED
>   dt-bindings: Add Rocktech jh057n00900 panel bindings
>   drm/panel: Add Rocktech jh057n00900 panel driver
> 
>  .../display/panel/rocktech,jh057n00900.txt    |  18 +
>  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
>  MAINTAINERS                                   |   6 +
>  drivers/gpu/drm/panel/Kconfig                 |  13 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../drm/panel/panel-rocktech-jh057n00900.c    | 386 ++++++++++++++++++
>  6 files changed, 425 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c

Applied, thanks.

checkpatch does complain about the dsi_generic_write_seq() macro
definition, because it uses flow control statements, but there are
already similar macros in other drivers, so I let this slide. We may
want to eventually come up with something better and then replace these
macros for the other drivers as well.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[Joe Perches]

On Wed, 2019-04-03 at 18:17 +0200, Thierry Reding wrote:
> On Mon, Apr 01, 2019 at 12:35:32PM +0200, Guido Günther wrote:
> > v4 fixes up the DT binding example and uses a wider cc list since I
> > failed to extend that when touching more files.
[]
> Applied, thanks.
> 
> checkpatch does complain about the dsi_generic_write_seq() macro
> definition, because it uses flow control statements, but there are
> already similar macros in other drivers, so I let this slide. We may
> want to eventually come up with something better and then replace these
> macros for the other drivers as well.

Dunno about the other drivers, but the mechanism isn't
particularly nice as it separates the init identifier
from the data being written.

It might be better to use something like a struct for
each command and a for loop for each block of commands.

Also the 0xBF value used in one of the init sequence
writes does not have an identifier #define in the
'Manufacturer specific Commands send via DSI' block
which is odd.

Re: [PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[Joe Perches]

On Wed, 2019-04-03 at 10:11 -0700, Joe Perches wrote:
> On Wed, 2019-04-03 at 18:17 +0200, Thierry Reding wrote:
> > On Mon, Apr 01, 2019 at 12:35:32PM +0200, Guido Günther wrote:
> > > v4 fixes up the DT binding example and uses a wider cc list since I
> > > failed to extend that when touching more files.
> []
> > Applied, thanks.
> > 
> > checkpatch does complain about the dsi_generic_write_seq() macro
> > definition, because it uses flow control statements, but there are
> > already similar macros in other drivers, so I let this slide. We may
> > want to eventually come up with something better and then replace these
> > macros for the other drivers as well.
> 
> Dunno about the other drivers, but the mechanism isn't
> particularly nice as it separates the init identifier
> from the data being written.
> 
> It might be better to use something like a struct for
> each command and a for loop for each block of commands.
> 
> Also the 0xBF value used in one of the init sequence
> writes does not have an identifier #define in the
> 'Manufacturer specific Commands send via DSI' block
> which is odd.

Perhaps something like this below
(though adding a bunch of lines to avoid a macro goto isn't great)
It does seem to read a bit better to me though.
---
 drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 210 +++++++++++++--------
 1 file changed, 136 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
index 158a6d548068..7862863db5f7 100644
--- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
+++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
@@ -20,27 +20,6 @@
 
 #define DRV_NAME "panel-rocktech-jh057n00900"
 
-/* Manufacturer specific Commands send via DSI */
-#define ST7703_CMD_ALL_PIXEL_OFF 0x22
-#define ST7703_CMD_ALL_PIXEL_ON	 0x23
-#define ST7703_CMD_SETDISP	 0xB2
-#define ST7703_CMD_SETRGBIF	 0xB3
-#define ST7703_CMD_SETCYC	 0xB4
-#define ST7703_CMD_SETBGP	 0xB5
-#define ST7703_CMD_SETVCOM	 0xB6
-#define ST7703_CMD_SETOTP	 0xB7
-#define ST7703_CMD_SETPOWER_EXT	 0xB8
-#define ST7703_CMD_SETEXTC	 0xB9
-#define ST7703_CMD_SETMIPI	 0xBA
-#define ST7703_CMD_SETVDC	 0xBC
-#define ST7703_CMD_SETSCR	 0xC0
-#define ST7703_CMD_SETPOWER	 0xC1
-#define ST7703_CMD_SETPANEL	 0xCC
-#define ST7703_CMD_SETGAMMA	 0xE0
-#define ST7703_CMD_SETEQ	 0xE3
-#define ST7703_CMD_SETGIP1	 0xE9
-#define ST7703_CMD_SETGIP2	 0xEA
-
 struct jh057n {
 	struct device *dev;
 	struct drm_panel panel;
@@ -51,75 +30,153 @@ struct jh057n {
 	struct dentry *debugfs;
 };
 
+struct st7703_cmd {
+	const size_t	size;
+	const u8	data[];
+};
+
+#define st7703_cmd_data(cmd, ...)					\
+	.size = 1 + (sizeof((u8[]){__VA_ARGS__})/sizeof(u8)),		\
+	.data = {cmd, __VA_ARGS__}
+
+/* Manufacturer specific Commands send via DSI */
+static const struct st7703_cmd ST7703_CMD_ALL_PIXEL_OFF = {
+	st7703_cmd_data(0x22)
+};
+static const struct st7703_cmd ST7703_CMD_ALL_PIXEL_ON = {
+	st7703_cmd_data(0x23)
+};
+static const struct st7703_cmd ST7703_CMD_SETDISP = {
+	st7703_cmd_data(0xB2,
+			0xF0, 0x12, 0x30)
+};
+static const struct st7703_cmd ST7703_CMD_SETRGBIF = {
+	st7703_cmd_data(0xB3,
+			0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00,
+			0x00, 0x00)
+};
+static const struct st7703_cmd ST7703_CMD_SETCYC = {
+	st7703_cmd_data(0xB4,
+			0x80)
+};
+static const struct st7703_cmd ST7703_CMD_SETBGP = {
+	st7703_cmd_data(0xB5,
+			0x08, 0x08)
+};
+static const struct st7703_cmd ST7703_CMD_SETVCOM = {
+	st7703_cmd_data(0xB6,
+			0x3F, 0x3F)
+};
+static const struct st7703_cmd ST7703_CMD_SETOTP = {
+	st7703_cmd_data(0xB7)
+};
+static const struct st7703_cmd ST7703_CMD_SETPOWER_EXT = {
+	st7703_cmd_data(0xB8)
+};
+static const struct st7703_cmd ST7703_CMD_SETEXTC = {
+	st7703_cmd_data(0xB9,
+			0xF1, 0x12, 0x83)
+};
+static const struct st7703_cmd ST7703_CMD_SETMIPI = {
+	st7703_cmd_data(0xBA)
+};
+static const struct st7703_cmd ST7703_CMD_SETVDC = {
+	st7703_cmd_data(0xBC,
+			0x4E)
+};
+static const struct st7703_cmd ST7703_CMD_SETSCR = {
+	st7703_cmd_data(0xC0,
+			0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
+			0x00)
+};
+static const struct st7703_cmd ST7703_CMD_SETPOWER = {
+	st7703_cmd_data(0xC1)
+};
+static const struct st7703_cmd ST7703_CMD_SETPANEL = {
+	st7703_cmd_data(0xCC,
+			0x0B)
+};
+static const struct st7703_cmd ST7703_CMD_SETGAMMA = {
+	st7703_cmd_data(0xE0,
+			0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41, 0x37,
+			0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10, 0x11,
+			0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
+			0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
+			0x11, 0x18)
+};
+static const struct st7703_cmd ST7703_CMD_SETEQ = {
+	st7703_cmd_data(0xE3,
+			0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
+			0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10)
+};
+static const struct st7703_cmd ST7703_CMD_SETGIP1 = {
+	st7703_cmd_data(0xE9,
+			0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5, 0x12,
+			0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,
+			0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00,
+			0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88,
+			0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64,
+			0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
+			0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
+};
+
+static const struct st7703_cmd ST7703_CMD_SETGIP2 = {
+	st7703_cmd_data(0xEA,
+			0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88,
+			0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13,
+			0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
+			0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x0A,
+			0xA5, 0x00, 0x00, 0x00, 0x00)
+};
+
 static inline struct jh057n *panel_to_jh057n(struct drm_panel *panel)
 {
 	return container_of(panel, struct jh057n, panel);
 }
 
-#define dsi_generic_write_seq(dsi, seq...) do {				\
-		static const u8 d[] = { seq };				\
-		int ret;						\
-		ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));	\
-		if (ret < 0)						\
-			return ret;					\
-	} while (0)
-
 static int jh057n_init_sequence(struct jh057n *ctx)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
 	struct device *dev = ctx->dev;
+	int i;
 	int ret;
+	static const struct {
+		const struct st7703_cmd *cmd;
+		int msleep;
+	} st7703_init[] = {
+		{&ST7703_CMD_SETEXTC, 0},
+		{&ST7703_CMD_SETRGBIF, 0},
+		{&ST7703_CMD_SETSCR, 0},
+		{&ST7703_CMD_SETVDC, 0},
+		{&ST7703_CMD_SETPANEL, 0},
+		{&ST7703_CMD_SETCYC, 0},
+		{&ST7703_CMD_SETDISP, 0},
+		{&ST7703_CMD_SETEQ, 0},
+		{&ST7703_CMD_SETBGP, 20},
+		{&ST7703_CMD_SETVCOM, 0},
+		{&ST7703_CMD_SETGIP1, 0},
+		{&ST7703_CMD_SETGIP2, 0},
+		{&ST7703_CMD_SETGAMMA, 20},
+};
 
 	/*
 	 * Init sequence was supplied by the panel vendor. Most of the commands
 	 * resemble the ST7703 but the number of parameters often don't match
 	 * so it's likely a clone.
 	 */
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETEXTC,
-			      0xF1, 0x12, 0x83);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETRGBIF,
-			      0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00,
-			      0x00, 0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETSCR,
-			      0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
-			      0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETVDC, 0x4E);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0B);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETCYC, 0x80);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETDISP, 0xF0, 0x12, 0x30);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETEQ,
-			      0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
-			      0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETBGP, 0x08, 0x08);
-	msleep(20);
 
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETVCOM, 0x3F, 0x3F);
-	dsi_generic_write_seq(dsi, 0xBF, 0x02, 0x11, 0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP1,
-			      0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5, 0x12,
-			      0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,
-			      0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00,
-			      0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88,
-			      0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64,
-			      0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
-			      0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP2,
-			      0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-			      0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88,
-			      0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13,
-			      0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
-			      0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00,
-			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x0A,
-			      0xA5, 0x00, 0x00, 0x00, 0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETGAMMA,
-			      0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41, 0x37,
-			      0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10, 0x11,
-			      0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
-			      0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
-			      0x11, 0x18);
-	msleep(20);
+	for (i = 0; i < ARRAY_SIZE(st7703_init); i++) {
+		ret = mipi_dsi_generic_write(dsi, st7703_init[i].cmd->data,
+					     st7703_init[i].cmd->size);
+		if (ret < 0)
+			return ret;
+		if (st7703_init[i].msleep)
+			msleep(st7703_init[i].msleep);
+	}
 
 	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
 	if (ret < 0) {
@@ -240,9 +297,14 @@ static int allpixelson_set(void *data, u64 val)
 {
 	struct jh057n *ctx = data;
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	int ret;
 
 	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on");
-	dsi_generic_write_seq(dsi, ST7703_CMD_ALL_PIXEL_ON);
+	ret = mipi_dsi_generic_write(dsi, ST7703_CMD_ALL_PIXEL_ON.data,
+				     ST7703_CMD_ALL_PIXEL_ON.size);
+	if (ret < 0)							\
+		return ret;						\
+
 	msleep(val * 1000);
 	/* Reset the panel to get video back */
 	drm_panel_disable(&ctx->panel);

Re: [PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[Sam Ravnborg]

Hi Joe.

Thanks for your patch.

> ---
>  drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 210 +++++++++++++--------
>  1 file changed, 136 insertions(+), 74 deletions(-)

Hmmm, add more lines than it deletes.

> 
> diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> index 158a6d548068..7862863db5f7 100644
> --- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> +++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> @@ -20,27 +20,6 @@
>  
>  #define DRV_NAME "panel-rocktech-jh057n00900"
>  
> -/* Manufacturer specific Commands send via DSI */
> -#define ST7703_CMD_ALL_PIXEL_OFF 0x22
> -#define ST7703_CMD_ALL_PIXEL_ON	 0x23
> -#define ST7703_CMD_SETDISP	 0xB2
> -#define ST7703_CMD_SETRGBIF	 0xB3
> -#define ST7703_CMD_SETCYC	 0xB4
> -#define ST7703_CMD_SETBGP	 0xB5
> -#define ST7703_CMD_SETVCOM	 0xB6
> -#define ST7703_CMD_SETOTP	 0xB7
> -#define ST7703_CMD_SETPOWER_EXT	 0xB8
> -#define ST7703_CMD_SETEXTC	 0xB9
> -#define ST7703_CMD_SETMIPI	 0xBA
> -#define ST7703_CMD_SETVDC	 0xBC
> -#define ST7703_CMD_SETSCR	 0xC0
> -#define ST7703_CMD_SETPOWER	 0xC1
> -#define ST7703_CMD_SETPANEL	 0xCC
> -#define ST7703_CMD_SETGAMMA	 0xE0
> -#define ST7703_CMD_SETEQ	 0xE3
> -#define ST7703_CMD_SETGIP1	 0xE9
> -#define ST7703_CMD_SETGIP2	 0xEA
> -
>  struct jh057n {
>  	struct device *dev;
>  	struct drm_panel panel;
> @@ -51,75 +30,153 @@ struct jh057n {
>  	struct dentry *debugfs;
>  };
>  
> +struct st7703_cmd {
> +	const size_t	size;
> +	const u8	data[];
> +};
> +
> +#define st7703_cmd_data(cmd, ...)					\
> +	.size = 1 + (sizeof((u8[]){__VA_ARGS__})/sizeof(u8)),		\
> +	.data = {cmd, __VA_ARGS__}
> +
> +/* Manufacturer specific Commands send via DSI */
> +static const struct st7703_cmd ST7703_CMD_ALL_PIXEL_OFF = {
> +	st7703_cmd_data(0x22)
> +};
> +static const struct st7703_cmd ST7703_CMD_ALL_PIXEL_ON = {
> +	st7703_cmd_data(0x23)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETDISP = {
> +	st7703_cmd_data(0xB2,
> +			0xF0, 0x12, 0x30)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETRGBIF = {
> +	st7703_cmd_data(0xB3,
> +			0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00,
> +			0x00, 0x00)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETCYC = {
> +	st7703_cmd_data(0xB4,
> +			0x80)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETBGP = {
> +	st7703_cmd_data(0xB5,
> +			0x08, 0x08)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETVCOM = {
> +	st7703_cmd_data(0xB6,
> +			0x3F, 0x3F)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETOTP = {
> +	st7703_cmd_data(0xB7)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETPOWER_EXT = {
> +	st7703_cmd_data(0xB8)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETEXTC = {
> +	st7703_cmd_data(0xB9,
> +			0xF1, 0x12, 0x83)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETMIPI = {
> +	st7703_cmd_data(0xBA)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETVDC = {
> +	st7703_cmd_data(0xBC,
> +			0x4E)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETSCR = {
> +	st7703_cmd_data(0xC0,
> +			0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
> +			0x00)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETPOWER = {
> +	st7703_cmd_data(0xC1)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETPANEL = {
> +	st7703_cmd_data(0xCC,
> +			0x0B)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETGAMMA = {
> +	st7703_cmd_data(0xE0,
> +			0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41, 0x37,
> +			0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10, 0x11,
> +			0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
> +			0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
> +			0x11, 0x18)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETEQ = {
> +	st7703_cmd_data(0xE3,
> +			0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
> +			0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETGIP1 = {
> +	st7703_cmd_data(0xE9,
> +			0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5, 0x12,
> +			0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,
> +			0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00,
> +			0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88,
> +			0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64,
> +			0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
> +			0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
> +};
> +
> +static const struct st7703_cmd ST7703_CMD_SETGIP2 = {
> +	st7703_cmd_data(0xEA,
> +			0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88,
> +			0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13,
> +			0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
> +			0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x0A,
> +			0xA5, 0x00, 0x00, 0x00, 0x00)
> +};
> +
>  static inline struct jh057n *panel_to_jh057n(struct drm_panel *panel)
>  {
>  	return container_of(panel, struct jh057n, panel);
>  }
>  
> -#define dsi_generic_write_seq(dsi, seq...) do {				\
> -		static const u8 d[] = { seq };				\
> -		int ret;						\
> -		ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));	\
> -		if (ret < 0)						\
> -			return ret;					\
> -	} while (0)
The above macro was the one triggering this patch.
And frankly it looks nice and simple.

The old code is IMO more readable.
- We have all the commands listed in the order they
  are used and in a rahter compatch format.
- It is obvious when we need delays.
- We have traditional #defines for the constants we know

This macro:
> +#define st7703_cmd_data(cmd, ...)                                    \
> +     .size = 1 + (sizeof((u8[]){__VA_ARGS__})/sizeof(u8)),           \
> +     .data = {cmd, __VA_ARGS__}
is again IMO not easier to follow than the above.

This is all to some extent bikeshedding, but I suggest
to keep the current code.
It is simple and it is tested.

Thanks for trying to come up with a better solution.

	Sam

Re: [PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[Joe Perches]

On Wed, 2019-04-03 at 23:07 +0200, Sam Ravnborg wrote:
> Hi Joe.
> 
> Thanks for your patch.
> 
> > ---
> >  drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 210 +++++++++++++--------
> >  1 file changed, 136 insertions(+), 74 deletions(-)
> 
> Hmmm, add more lines than it deletes.

Yeah, I said that too.

> > -#define dsi_generic_write_seq(dsi, seq...) do {				\
> > -		static const u8 d[] = { seq };				\
> > -		int ret;						\
> > -		ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));	\
> > -		if (ret < 0)						\
> > -			return ret;					\
> > -	} while (0)
> The above macro was the one triggering this patch.
> And frankly it looks nice and simple.
> 
> The old code is IMO more readable.
> - We have all the commands listed in the order they
>   are used and in a rahter compatch format.

This too.

> - It is obvious when we need delays.

Here too, also it allows an easy way to update
if there is another delay needed between 2 uses.

> - We have traditional #defines for the constants we know

And the values for the data for those constants are
separated from the constants themselves as well as
at least 1 missing constant.

> This is all to some extent bikeshedding,

Yup.  Still, I think what I suggested is more readable ;)

> but I suggest
> to keep the current code.
> It is simple and it is tested.

btw: The object code for either is the same size on x86-64

> Thanks for trying to come up with a better solution.

n/c.

cheers, Joe

Re: [PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[Guido Günther]

Hi,
On Wed, Apr 03, 2019 at 10:11:00AM -0700, Joe Perches wrote:
> On Wed, 2019-04-03 at 18:17 +0200, Thierry Reding wrote:
> > On Mon, Apr 01, 2019 at 12:35:32PM +0200, Guido Günther wrote:
> > > v4 fixes up the DT binding example and uses a wider cc list since I
> > > failed to extend that when touching more files.
> []
> > Applied, thanks.
> > 
> > checkpatch does complain about the dsi_generic_write_seq() macro
> > definition, because it uses flow control statements, but there are
> > already similar macros in other drivers, so I let this slide. We may
> > want to eventually come up with something better and then replace these
> > macros for the other drivers as well.
> 
> Dunno about the other drivers, but the mechanism isn't
> particularly nice as it separates the init identifier
> from the data being written.
> 
> It might be better to use something like a struct for
> each command and a for loop for each block of commands.
> 
> Also the 0xBF value used in one of the init sequence
> writes does not have an identifier #define in the
> 'Manufacturer specific Commands send via DSI' block
> which is odd.

As written in the commit message the IC in that panel looks similar to
the ST7703 but seems to be one of it's clones and the 0xBF isn't
documented here at all so I've queued this:

diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
index 158a6d548068..e58888f91007 100644
--- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
+++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
@@ -33,6 +33,7 @@
 #define ST7703_CMD_SETEXTC	 0xB9
 #define ST7703_CMD_SETMIPI	 0xBA
 #define ST7703_CMD_SETVDC	 0xBC
+#define ST7703_CMD_UNKNOWN0	 0xBF
 #define ST7703_CMD_SETSCR	 0xC0
 #define ST7703_CMD_SETPOWER	 0xC1
 #define ST7703_CMD_SETPANEL	 0xCC
@@ -94,7 +95,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
 	msleep(20);
 
 	dsi_generic_write_seq(dsi, ST7703_CMD_SETVCOM, 0x3F, 0x3F);
-	dsi_generic_write_seq(dsi, 0xBF, 0x02, 0x11, 0x00);
+	dsi_generic_write_seq(dsi, ST7703_CMD_UNKNOWN0, 0x02, 0x11, 0x00);
 	dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP1,
 			      0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5, 0x12,
 			      0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,

Cheers,
 -- Guido

[PATCH] drm/panel: Rocktech jh057n00900: Add terminating newlines to logging

[Joe Perches]

These were missing '\n' terminations, add them.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
index 158a6d548068..d88ea8da2ec2 100644
--- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
+++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
@@ -123,7 +123,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
 
 	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
 	if (ret < 0) {
-		DRM_DEV_ERROR(dev, "Failed to exit sleep mode");
+		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
 		return ret;
 	}
 	/* Panel is operational 120 msec after reset */
@@ -132,7 +132,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
 	if (ret)
 		return ret;
 
-	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done");
+	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done\n");
 	return 0;
 }
 
@@ -172,7 +172,7 @@ static int jh057n_prepare(struct drm_panel *panel)
 	if (ctx->prepared)
 		return 0;
 
-	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel.");
+	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
 	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
 	usleep_range(20, 40);
 	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
@@ -180,7 +180,8 @@ static int jh057n_prepare(struct drm_panel *panel)
 
 	ret = jh057n_init_sequence(ctx);
 	if (ret < 0) {
-		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d", ret);
+		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
+			      ret);
 		return ret;
 	}
 
@@ -212,7 +213,7 @@ static int jh057n_get_modes(struct drm_panel *panel)
 
 	mode = drm_mode_duplicate(panel->drm, &default_mode);
 	if (!mode) {
-		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u",
+		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u\n",
 			      default_mode.hdisplay, default_mode.vdisplay,
 			      default_mode.vrefresh);
 		return -ENOMEM;
@@ -241,7 +242,7 @@ static int allpixelson_set(void *data, u64 val)
 	struct jh057n *ctx = data;
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
 
-	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on");
+	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on\n");
 	dsi_generic_write_seq(dsi, ST7703_CMD_ALL_PIXEL_ON);
 	msleep(val * 1000);
 	/* Reset the panel to get video back */
@@ -290,7 +291,7 @@ static int jh057n_probe(struct mipi_dsi_device *dsi)
 
 	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(ctx->reset_gpio)) {
-		DRM_DEV_ERROR(dev, "cannot get reset gpio");
+		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
 		return PTR_ERR(ctx->reset_gpio);
 	}
 
@@ -315,12 +316,12 @@ static int jh057n_probe(struct mipi_dsi_device *dsi)
 
 	ret = mipi_dsi_attach(dsi);
 	if (ret < 0) {
-		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?");
+		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?\n");
 		drm_panel_remove(&ctx->panel);
 		return ret;
 	}
 
-	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready",
+	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
 		     default_mode.hdisplay, default_mode.vdisplay,
 		     default_mode.vrefresh,
 		     mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);

Re: [PATCH] drm/panel: Rocktech jh057n00900: Add terminating newlines to logging

[Guido Günther]

Hi Joe,
On Thu, Apr 04, 2019 at 08:06:09AM -0700, Joe Perches wrote:
> These were missing '\n' terminations, add them.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> index 158a6d548068..d88ea8da2ec2 100644
> --- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> +++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> @@ -123,7 +123,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
>  
>  	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>  	if (ret < 0) {
> -		DRM_DEV_ERROR(dev, "Failed to exit sleep mode");
> +		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
>  		return ret;
>  	}
>  	/* Panel is operational 120 msec after reset */
> @@ -132,7 +132,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
>  	if (ret)
>  		return ret;
>  
> -	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done");
> +	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done\n");
>  	return 0;
>  }
>  
> @@ -172,7 +172,7 @@ static int jh057n_prepare(struct drm_panel *panel)
>  	if (ctx->prepared)
>  		return 0;
>  
> -	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel.");
> +	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
>  	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>  	usleep_range(20, 40);
>  	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> @@ -180,7 +180,8 @@ static int jh057n_prepare(struct drm_panel *panel)
>  
>  	ret = jh057n_init_sequence(ctx);
>  	if (ret < 0) {
> -		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d", ret);
> +		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
> +			      ret);
>  		return ret;
>  	}
>  
> @@ -212,7 +213,7 @@ static int jh057n_get_modes(struct drm_panel *panel)
>  
>  	mode = drm_mode_duplicate(panel->drm, &default_mode);
>  	if (!mode) {
> -		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u",
> +		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u\n",
>  			      default_mode.hdisplay, default_mode.vdisplay,
>  			      default_mode.vrefresh);
>  		return -ENOMEM;
> @@ -241,7 +242,7 @@ static int allpixelson_set(void *data, u64 val)
>  	struct jh057n *ctx = data;
>  	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>  
> -	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on");
> +	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on\n");
>  	dsi_generic_write_seq(dsi, ST7703_CMD_ALL_PIXEL_ON);
>  	msleep(val * 1000);
>  	/* Reset the panel to get video back */
> @@ -290,7 +291,7 @@ static int jh057n_probe(struct mipi_dsi_device *dsi)
>  
>  	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>  	if (IS_ERR(ctx->reset_gpio)) {
> -		DRM_DEV_ERROR(dev, "cannot get reset gpio");
> +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
>  		return PTR_ERR(ctx->reset_gpio);
>  	}
>  
> @@ -315,12 +316,12 @@ static int jh057n_probe(struct mipi_dsi_device *dsi)
>  
>  	ret = mipi_dsi_attach(dsi);
>  	if (ret < 0) {
> -		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?");
> +		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?\n");
>  		drm_panel_remove(&ctx->panel);
>  		return ret;
>  	}
>  
> -	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready",
> +	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
>  		     default_mode.hdisplay, default_mode.vdisplay,
>  		     default_mode.vrefresh,
>  		     mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
> 

Reviewed-By: Guido Günther <agx@sigxcpu.org>

Thanks,
 -- Guido

P.S.: The cc: list is huge, is there a reason for going way past what
get_maintainers puts out for this patch?

Re: [PATCH] drm/panel: Rocktech jh057n00900: Add terminating newlines to logging

[Joe Perches]

On Thu, 2019-04-04 at 17:48 +0200, Guido Günther wrote:
> Hi Joe,
[]
> P.S.: The cc: list is huge, is there a reason for going way past what
> get_maintainers puts out for this patch?

No idea, I just used reply-all to your patch.

Re: [PATCH] drm/panel: Rocktech jh057n00900: Add terminating newlines to logging

[Sam Ravnborg]

Hi Joe.

On Thu, Apr 04, 2019 at 08:06:09AM -0700, Joe Perches wrote:
> These were missing '\n' terminations, add them.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> index 158a6d548068..d88ea8da2ec2 100644
> --- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> +++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> @@ -123,7 +123,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
>  
>  	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>  	if (ret < 0) {
> -		DRM_DEV_ERROR(dev, "Failed to exit sleep mode");
> +		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");

I was under the impression that newlines was optional these days.
Should we always use them with logging?

I did not find any obvious clues in linux/printk.h

	Sam

Re: [PATCH] drm/panel: Rocktech jh057n00900: Add terminating newlines to logging

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

On Thu, Apr 04, 2019 at 08:06:09AM -0700, Joe Perches wrote:
> These were missing '\n' terminations, add them.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/panel: Rocktech jh057n00900: Add terminating newlines to logging

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4125 bytes --]

On Thu, Apr 04, 2019 at 05:48:50PM +0200, Guido Günther wrote:
> Hi Joe,
> On Thu, Apr 04, 2019 at 08:06:09AM -0700, Joe Perches wrote:
> > These were missing '\n' terminations, add them.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > index 158a6d548068..d88ea8da2ec2 100644
> > --- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > +++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > @@ -123,7 +123,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
> >  
> >  	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> >  	if (ret < 0) {
> > -		DRM_DEV_ERROR(dev, "Failed to exit sleep mode");
> > +		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
> >  		return ret;
> >  	}
> >  	/* Panel is operational 120 msec after reset */
> > @@ -132,7 +132,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
> >  	if (ret)
> >  		return ret;
> >  
> > -	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done");
> > +	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done\n");
> >  	return 0;
> >  }
> >  
> > @@ -172,7 +172,7 @@ static int jh057n_prepare(struct drm_panel *panel)
> >  	if (ctx->prepared)
> >  		return 0;
> >  
> > -	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel.");
> > +	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
> >  	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> >  	usleep_range(20, 40);
> >  	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> > @@ -180,7 +180,8 @@ static int jh057n_prepare(struct drm_panel *panel)
> >  
> >  	ret = jh057n_init_sequence(ctx);
> >  	if (ret < 0) {
> > -		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d", ret);
> > +		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
> > +			      ret);
> >  		return ret;
> >  	}
> >  
> > @@ -212,7 +213,7 @@ static int jh057n_get_modes(struct drm_panel *panel)
> >  
> >  	mode = drm_mode_duplicate(panel->drm, &default_mode);
> >  	if (!mode) {
> > -		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u",
> > +		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u\n",
> >  			      default_mode.hdisplay, default_mode.vdisplay,
> >  			      default_mode.vrefresh);
> >  		return -ENOMEM;
> > @@ -241,7 +242,7 @@ static int allpixelson_set(void *data, u64 val)
> >  	struct jh057n *ctx = data;
> >  	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> >  
> > -	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on");
> > +	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on\n");
> >  	dsi_generic_write_seq(dsi, ST7703_CMD_ALL_PIXEL_ON);
> >  	msleep(val * 1000);
> >  	/* Reset the panel to get video back */
> > @@ -290,7 +291,7 @@ static int jh057n_probe(struct mipi_dsi_device *dsi)
> >  
> >  	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> >  	if (IS_ERR(ctx->reset_gpio)) {
> > -		DRM_DEV_ERROR(dev, "cannot get reset gpio");
> > +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
> >  		return PTR_ERR(ctx->reset_gpio);
> >  	}
> >  
> > @@ -315,12 +316,12 @@ static int jh057n_probe(struct mipi_dsi_device *dsi)
> >  
> >  	ret = mipi_dsi_attach(dsi);
> >  	if (ret < 0) {
> > -		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?");
> > +		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?\n");
> >  		drm_panel_remove(&ctx->panel);
> >  		return ret;
> >  	}
> >  
> > -	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready",
> > +	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
> >  		     default_mode.hdisplay, default_mode.vdisplay,
> >  		     default_mode.vrefresh,
> >  		     mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
> > 
> 
> Reviewed-By: Guido Günther <agx@sigxcpu.org>

Nit: the correct tag is "Reviewed-by:", note the lowercase "by". I only
noticed because checkpatch complained, and fixed it up.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/panel: Rocktech jh057n00900: Add terminating newlines to logging

[Joe Perches]

On Thu, 2019-04-04 at 19:00 +0200, Sam Ravnborg wrote:
> Hi Joe.
> 
> On Thu, Apr 04, 2019 at 08:06:09AM -0700, Joe Perches wrote:
> > These were missing '\n' terminations, add them.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > index 158a6d548068..d88ea8da2ec2 100644
> > --- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > +++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > @@ -123,7 +123,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
> >  
> >  	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> >  	if (ret < 0) {
> > -		DRM_DEV_ERROR(dev, "Failed to exit sleep mode");
> > +		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
> 
> I was under the impression that newlines was optional these days.
> Should we always use them with logging?

Yes.

The general problem is KERN_CONT/pr_cont uses where interleaving
is still possible from multiple threads.

> I did not find any obvious clues in linux/printk.h

I'll see about adding something one day to the Documentation.
Likely in coding-style.rst.

Re: [PATCH] drm/panel: Rocktech jh057n00900: Add terminating newlines to logging

[Sam Ravnborg]

Hi Joe.

> > >  
> > >  	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > >  	if (ret < 0) {
> > > -		DRM_DEV_ERROR(dev, "Failed to exit sleep mode");
> > > +		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
> > 
> > I was under the impression that newlines was optional these days.
> > Should we always use them with logging?
> 
> Yes.
> 
> The general problem is KERN_CONT/pr_cont uses where interleaving
> is still possible from multiple threads.
> 
> > I did not find any obvious clues in linux/printk.h
> 
> I'll see about adding something one day to the Documentation.
> Likely in coding-style.rst.

Thanks for the clarification.
A seperate doc on logging would be awesome.

	Sam