[PATCH 1/3] dt-bindings: Add vendor prefix for Xinpeng Technology

[PATCH 1/3] dt-bindings: Add vendor prefix for Xinpeng Technology

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Shenzhen Xinpeng Technology Co., Ltd produces for example display panels.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 6046f4555852..85e7c26a05c7 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1056,6 +1056,8 @@ patternProperties:
     description: Extreme Engineering Solutions (X-ES)
   "^xillybus,.*":
     description: Xillybus Ltd.
+  "^xinpeng,.*":
+    description: Shenzhen Xinpeng Technology Co., Ltd
   "^xlnx,.*":
     description: Xilinx
   "^xunlong,.*":
-- 
2.24.0

[PATCH 2/3] dt-bindings: display: panel: Add binding document for Xinpeng XPP055C272

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The XPP055C272 is a 5.5" 720x1280 DSI display.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 .../display/panel/xinpeng,xpp055c272.yaml     | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml b/Documentation/devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml
new file mode 100644
index 000000000000..4515d44d189b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/sony,acx424akp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xinpeng XPP055C272 720x1280 DSI panel
+
+maintainers:
+  - Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    const: xinpeng,xpp055c272
+  reg: true
+  backlight: true
+  port: true
+  reset-gpios: true
+  iovcc-supply:
+     description: regulator that supplies the iovcc voltage
+  vci-supply:
+     description: regulator that supplies the vci voltage
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    dsi@ff450000 {
+        panel@0 {
+            compatible = "xinpeng,xpp055c272";
+            reg = <0>;
+            backlight = <&backlight>;
+            iovcc-supply = <&vcc_1v8>;
+            vci-supply = <&vcc3v3_lcd>;
+        };
+    };
+
+...
-- 
2.24.0

Re: [PATCH 2/3] dt-bindings: display: panel: Add binding document for Xinpeng XPP055C272

[Sam Ravnborg]

Hi Heiko.


On Mon, Dec 09, 2019 at 03:42:07PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> The XPP055C272 is a 5.5" 720x1280 DSI display.
Can we get the size info included in the title in the binding?
Then all relavant info is in the binding, and no git digging is needed.

> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  .../display/panel/xinpeng,xpp055c272.yaml     | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml b/Documentation/devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml
> new file mode 100644
> index 000000000000..4515d44d189b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/sony,acx424akp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xinpeng XPP055C272 720x1280 DSI panel
> +
> +maintainers:
> +  - Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: xinpeng,xpp055c272
> +  reg: true
> +  backlight: true
> +  port: true
> +  reset-gpios: true
> +  iovcc-supply:
> +     description: regulator that supplies the iovcc voltage
> +  vci-supply:
> +     description: regulator that supplies the vci voltage
> +
> +required:
> +  - compatible
> +  - reg
One would assume the display also required power to operate.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    dsi@ff450000 {
> +        panel@0 {
> +            compatible = "xinpeng,xpp055c272";
> +            reg = <0>;
> +            backlight = <&backlight>;
> +            iovcc-supply = <&vcc_1v8>;
> +            vci-supply = <&vcc3v3_lcd>;
> +        };
> +    };
> +
> +...

With the few things mentioned above fixed:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

[PATCH 3/3] drm/panel: add panel driver for Xinpeng XPP055C272 panels

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Base on the somewhat similar Rocktech driver but adapted for
panel-specific init of the XPP055C272.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/gpu/drm/panel/Kconfig                 |  10 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-xinpeng-xpp055c272.c  | 433 ++++++++++++++++++
 3 files changed, 444 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index f152bc4eeb53..fb1ded47677e 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -355,4 +355,14 @@ config DRM_PANEL_TRULY_NT35597_WQXGA
 	help
 	  Say Y here if you want to enable support for Truly NT35597 WQXGA Dual DSI
 	  Video Mode panel
+
+config DRM_PANEL_XINPENG_XPP055C272
+	tristate "Xinpeng XPP055C272 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 Xinpeng
+	  XPP055C272 controller for 720x1280 LCD panels with MIPI/RGB/SPI
+	  system interfaces.
 endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index b6cd39fe0f20..71d7722146a7 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_PANEL_TPO_TD028TTEC1) += panel-tpo-td028ttec1.o
 obj-$(CONFIG_DRM_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o
 obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o
 obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
+obj-$(CONFIG_DRM_PANEL_XINPENG_XPP055C272) += panel-xinpeng-xpp055c272.o
diff --git a/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c b/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c
new file mode 100644
index 000000000000..9c5fbf1879c3
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xinpeng xpp055c272 5.5" MIPI-DSI panel driver
+ * Copyright (C) 2019 Theobroma Systems Design und Consulting GmbH
+ *
+ * based on
+ *
+ * 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 <linux/regulator/consumer.h>
+#include <video/display_timing.h>
+#include <video/mipi_display.h>
+
+#define DRV_NAME "panel-xinpeng-xpp055c272"
+
+/* Manufacturer specific Commands send via DSI */
+#define XPP055C272_CMD_ALL_PIXEL_OFF	0x22
+#define XPP055C272_CMD_ALL_PIXEL_ON		0x23
+#define XPP055C272_CMD_SETDISP		0xb2
+#define XPP055C272_CMD_SETRGBIF		0xb3
+#define XPP055C272_CMD_SETCYC		0xb4
+#define XPP055C272_CMD_SETBGP		0xb5
+#define XPP055C272_CMD_SETVCOM		0xb6
+#define XPP055C272_CMD_SETOTP		0xb7
+#define XPP055C272_CMD_SETPOWER_EXT		0xb8
+#define XPP055C272_CMD_SETEXTC		0xb9
+#define XPP055C272_CMD_SETMIPI		0xbA
+#define XPP055C272_CMD_SETVDC		0xbc
+#define XPP055C272_CMD_SETPCR		0xbf
+#define XPP055C272_CMD_SETSCR		0xc0
+#define XPP055C272_CMD_SETPOWER		0xc1
+#define XPP055C272_CMD_SETECO		0xc6
+#define XPP055C272_CMD_SETPANEL		0xcc
+#define XPP055C272_CMD_SETGAMMA		0xe0
+#define XPP055C272_CMD_SETEQ		0xe3
+#define XPP055C272_CMD_SETGIP1		0xe9
+#define XPP055C272_CMD_SETGIP2		0xea
+
+struct xpp055c272 {
+	struct device *dev;
+	struct drm_panel panel;
+	struct gpio_desc *reset_gpio;
+	struct backlight_device *backlight;
+	struct regulator *vci;
+	struct regulator *iovcc;
+	bool prepared;
+	bool enabled;
+};
+
+static inline struct xpp055c272 *panel_to_xpp055c272(struct drm_panel *panel)
+{
+	return container_of(panel, struct xpp055c272, panel);
+}
+
+#define dsi_generic_write_seq(dsi, cmd, seq...) do {			\
+		static const u8 d[] = { seq };				\
+		int ret;						\
+		ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d));	\
+		if (ret < 0)						\
+			return ret;					\
+	} while (0)
+
+static int xpp055c272_init_sequence(struct xpp055c272 *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 without much
+	 * documentation.
+	 */
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEXTC, 0xf1, 0x12, 0x83);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETMIPI,
+			      0x33, 0x81, 0x05, 0xf9, 0x0e, 0x0e, 0x00, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
+			      0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4f, 0x01,
+			      0x00, 0x00, 0x37);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETPOWER_EXT, 0x25);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETPCR, 0x02, 0x11, 0x00);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETRGBIF,
+			      0x0c, 0x10, 0x0a, 0x50, 0x03, 0xff, 0x00, 0x00,
+			      0x00, 0x00);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETSCR,
+			      0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
+			      0x00);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETVDC, 0x46);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETPANEL, 0x0b);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETCYC, 0x80);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETDISP, 0xc8, 0x12, 0x30);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEQ,
+			      0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
+			      0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETPOWER,
+			      0x53, 0x00, 0x1e, 0x1e, 0x77, 0xe1, 0xcc, 0xdd,
+			      0x67, 0x77, 0x33, 0x33);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETECO, 0x00, 0x00, 0xff,
+			      0xff, 0x01, 0xff);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETBGP, 0x09, 0x09);
+	msleep(20);
+
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETVCOM, 0x87, 0x95);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETGIP1,
+			      0xc2, 0x10, 0x05, 0x05, 0x10, 0x05, 0xa0, 0x12,
+			      0x31, 0x23, 0x3f, 0x81, 0x0a, 0xa0, 0x37, 0x18,
+			      0x00, 0x80, 0x01, 0x00, 0x00, 0x00, 0x00, 0x80,
+			      0x01, 0x00, 0x00, 0x00, 0x48, 0xf8, 0x86, 0x42,
+			      0x08, 0x88, 0x88, 0x80, 0x88, 0x88, 0x88, 0x58,
+			      0xf8, 0x87, 0x53, 0x18, 0x88, 0x88, 0x81, 0x88,
+			      0x88, 0x88, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETGIP2,
+			      0x00, 0x1a, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x1f, 0x88, 0x81, 0x35,
+			      0x78, 0x88, 0x88, 0x85, 0x88, 0x88, 0x88, 0x0f,
+			      0x88, 0x80, 0x24, 0x68, 0x88, 0x88, 0x84, 0x88,
+			      0x88, 0x88, 0x23, 0x10, 0x00, 0x00, 0x1c, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x05,
+			      0xa0, 0x00, 0x00, 0x00, 0x00);
+	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETGAMMA,
+			      0x00, 0x06, 0x08, 0x2a, 0x31, 0x3f, 0x38, 0x36,
+			      0x07, 0x0c, 0x0d, 0x11, 0x13, 0x12, 0x13, 0x11,
+			      0x18, 0x00, 0x06, 0x08, 0x2a, 0x31, 0x3f, 0x38,
+			      0x36, 0x07, 0x0c, 0x0d, 0x11, 0x13, 0x12, 0x13,
+			      0x11, 0x18);
+
+	msleep(60);
+
+	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "Failed to exit sleep mode: %d\n", ret);
+		return ret;
+	}
+
+	/* T9: 120ms */
+	msleep(120);
+
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret)
+		return ret;
+
+	msleep(50);
+
+	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done\n");
+	return 0;
+}
+
+static int xpp055c272_unprepare(struct drm_panel *panel)
+{
+	struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	int ret;
+
+	if (!ctx->prepared)
+		return 0;
+
+	ret = mipi_dsi_dcs_set_display_off(dsi);
+	if (ret < 0)
+		DRM_DEV_ERROR(ctx->dev, "failed to set display off: %d\n",
+			      ret);
+
+	mipi_dsi_dcs_enter_sleep_mode(dsi);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev, "failed to enter sleep mode: %d\n",
+			      ret);
+		return ret;
+	}
+
+	regulator_disable(ctx->iovcc);
+	regulator_disable(ctx->vci);
+
+	ctx->prepared = false;
+
+	return 0;
+}
+
+static int xpp055c272_prepare(struct drm_panel *panel)
+{
+	struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
+	int ret;
+
+	if (ctx->prepared)
+		return 0;
+
+	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
+	ret = regulator_enable(ctx->vci);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev,
+			      "Failed to enable vci supply: %d\n", ret);
+		return ret;
+	}
+	ret = regulator_enable(ctx->iovcc);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev,
+			      "Failed to enable iovcc supply: %d\n", ret);
+		goto disable_vci;
+	}
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	/* T6: 10us */
+	usleep_range(10, 20);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+
+	/* T8: 20ms */
+	msleep(20);
+
+	ret = xpp055c272_init_sequence(ctx);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
+			      ret);
+		goto disable_iovcc;
+	}
+
+	ctx->prepared = true;
+
+	return 0;
+
+disable_iovcc:
+	regulator_disable(ctx->iovcc);
+disable_vci:
+	regulator_disable(ctx->vci);
+	return ret;
+}
+
+static int xpp055c272_enable(struct drm_panel *panel)
+{
+	struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
+	int ret;
+
+	if (ctx->enabled)
+		return 0;
+
+	ret = backlight_enable(ctx->backlight);
+	if (ret) {
+		DRM_DEV_ERROR(ctx->dev,
+			      "Failed to enable backlight %d\n", ret);
+		return ret;
+	}
+
+	ctx->enabled = true;
+
+	return 0;
+}
+
+static int xpp055c272_disable(struct drm_panel *panel)
+{
+	struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
+
+	if (!ctx->enabled)
+		return 0;
+
+	backlight_disable(ctx->backlight);
+
+	ctx->enabled = false;
+
+	return 0;
+}
+
+static const struct drm_display_mode default_mode = {
+	.hdisplay    = 720,
+	.hsync_start = 720 + 40,
+	.hsync_end   = 720 + 40 + 10,
+	.htotal	     = 720 + 40 + 10 + 40,
+	.vdisplay    = 1280,
+	.vsync_start = 1280 + 22,
+	.vsync_end   = 1280 + 22 + 4,
+	.vtotal	     = 1280 + 22 + 4 + 11,
+	.vrefresh    = 60,
+	.clock	     = 64000,
+	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	.width_mm    = 68,
+	.height_mm   = 121,
+};
+
+static int xpp055c272_get_modes(struct drm_panel *panel)
+{
+	struct xpp055c272 *ctx = panel_to_xpp055c272(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\n",
+			      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 xpp055c272_funcs = {
+	.disable   = xpp055c272_disable,
+	.unprepare = xpp055c272_unprepare,
+	.prepare   = xpp055c272_prepare,
+	.enable	   = xpp055c272_enable,
+	.get_modes = xpp055c272_get_modes,
+};
+
+static int xpp055c272_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct xpp055c272 *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->reset_gpio)) {
+		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
+		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_LPM | MIPI_DSI_MODE_EOT_PACKET;
+
+	ctx->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(ctx->backlight))
+		return PTR_ERR(ctx->backlight);
+
+	ctx->vci = devm_regulator_get(dev, "vci");
+	if (IS_ERR(ctx->vci)) {
+		ret = PTR_ERR(ctx->vci);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev,
+				      "Failed to request vci regulator: %d\n",
+				      ret);
+		return ret;
+	}
+	ctx->iovcc = devm_regulator_get(dev, "iovcc");
+	if (IS_ERR(ctx->iovcc)) {
+		ret = PTR_ERR(ctx->iovcc);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev,
+				      "Failed to request iovcc regulator: %d\n",
+				      ret);
+		return ret;
+	}
+
+	drm_panel_init(&ctx->panel, &dsi->dev, &xpp055c272_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+	drm_panel_add(&ctx->panel);
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed: %d\n", ret);
+		drm_panel_remove(&ctx->panel);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void xpp055c272_shutdown(struct mipi_dsi_device *dsi)
+{
+	struct xpp055c272 *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	ret = drm_panel_unprepare(&ctx->panel);
+	if (ret < 0)
+		DRM_DEV_ERROR(&dsi->dev, "Failed to unprepare panel: %d\n",
+			      ret);
+
+	ret = drm_panel_disable(&ctx->panel);
+	if (ret < 0)
+		DRM_DEV_ERROR(&dsi->dev, "Failed to disable panel: %d\n",
+			      ret);
+}
+
+static int xpp055c272_remove(struct mipi_dsi_device *dsi)
+{
+	struct xpp055c272 *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	xpp055c272_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);
+
+	return 0;
+}
+
+static const struct of_device_id xpp055c272_of_match[] = {
+	{ .compatible = "xinpeng,xpp055c272" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, xpp055c272_of_match);
+
+static struct mipi_dsi_driver xpp055c272_driver = {
+	.probe	= xpp055c272_probe,
+	.remove = xpp055c272_remove,
+	.shutdown = xpp055c272_shutdown,
+	.driver = {
+		.name = DRV_NAME,
+		.of_match_table = xpp055c272_of_match,
+	},
+};
+module_mipi_dsi_driver(xpp055c272_driver);
+
+MODULE_AUTHOR("Heiko Stuebner <heiko.stuebner@theobroma-systems.com>");
+MODULE_DESCRIPTION("DRM driver for Xinpeng xpp055c272 MIPI DSI panel");
+MODULE_LICENSE("GPL v2");
-- 
2.24.0

Re: [PATCH 3/3] drm/panel: add panel driver for Xinpeng XPP055C272 panels

[Sam Ravnborg]

Hi Heiko.

Thanks for another nice panel driver patch.
There are some changes in drm-misc-next so the patch
applies but it no longer builds.
Please fix.

drm_panel now includes support for backlight - see other drivers.
Please look into the possibility to use the drm_panel supported
backlight for this driver.

On top of this a few comments below.

> +
> +#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 <linux/regulator/consumer.h>
> +#include <video/display_timing.h>
> +#include <video/mipi_display.h>

Please split the include files in blocks like this.

#include <linux/*>

#include <video/*>

#include <drm/*>

Keep the empty lines between the blocks.
And each block must be sorted alphabetically.


> +
> +#define DRV_NAME "panel-xinpeng-xpp055c272"
It is only used once - just spell it out where you use it.

> +
> +/* Manufacturer specific Commands send via DSI */
> +#define XPP055C272_CMD_ALL_PIXEL_OFF	0x22
> +#define XPP055C272_CMD_ALL_PIXEL_ON		0x23
> +#define XPP055C272_CMD_SETDISP		0xb2
> +#define XPP055C272_CMD_SETRGBIF		0xb3
> +#define XPP055C272_CMD_SETCYC		0xb4
> +#define XPP055C272_CMD_SETBGP		0xb5
> +#define XPP055C272_CMD_SETVCOM		0xb6
> +#define XPP055C272_CMD_SETOTP		0xb7
> +#define XPP055C272_CMD_SETPOWER_EXT		0xb8
> +#define XPP055C272_CMD_SETEXTC		0xb9
> +#define XPP055C272_CMD_SETMIPI		0xbA
> +#define XPP055C272_CMD_SETVDC		0xbc
> +#define XPP055C272_CMD_SETPCR		0xbf
> +#define XPP055C272_CMD_SETSCR		0xc0
> +#define XPP055C272_CMD_SETPOWER		0xc1
> +#define XPP055C272_CMD_SETECO		0xc6
> +#define XPP055C272_CMD_SETPANEL		0xcc
> +#define XPP055C272_CMD_SETGAMMA		0xe0
> +#define XPP055C272_CMD_SETEQ		0xe3
> +#define XPP055C272_CMD_SETGIP1		0xe9
> +#define XPP055C272_CMD_SETGIP2		0xea
> +
> +struct xpp055c272 {
> +	struct device *dev;
> +	struct drm_panel panel;
> +	struct gpio_desc *reset_gpio;
> +	struct backlight_device *backlight;
> +	struct regulator *vci;
> +	struct regulator *iovcc;
> +	bool prepared;
> +	bool enabled;
> +};
> +
> +static inline struct xpp055c272 *panel_to_xpp055c272(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct xpp055c272, panel);
> +}
> +
> +#define dsi_generic_write_seq(dsi, cmd, seq...) do {			\
> +		static const u8 d[] = { seq };				\
> +		int ret;						\
> +		ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d));	\
> +		if (ret < 0)						\
> +			return ret;					\
> +	} while (0)
This macro return an error code if a write fails.

> +
> +static int xpp055c272_init_sequence(struct xpp055c272 *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 without much
> +	 * documentation.
> +	 */
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEXTC, 0xf1, 0x12, 0x83);
But all uses of the macro here ignore the error.

Consider the following solution - to keep the code simple.

#define dsi_generic_write_seq(dsi, cmd, seq...) do {                    \
		static const u8 d[] = { seq };                          \
		if (ret != 0)                                            \
			return ret;					\
		ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d));   \
     } while (0)

Notice that the macro uses an alredy defined "ret" variable.
ret must be init to 0 - and must be checked after the last use of the
macro.

With the above mipi_dsi_dcs_write() is skipped if ret indicate an error.
And you can check ret after performing all the write calls.

You loose the exact point where ret failed but then the code does not
blindly continue ignoring the error.

> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETMIPI,
> +			      0x33, 0x81, 0x05, 0xf9, 0x0e, 0x0e, 0x00, 0x00,
> +			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
> +			      0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4f, 0x01,
> +			      0x00, 0x00, 0x37);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETPOWER_EXT, 0x25);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETPCR, 0x02, 0x11, 0x00);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETRGBIF,
> +			      0x0c, 0x10, 0x0a, 0x50, 0x03, 0xff, 0x00, 0x00,
> +			      0x00, 0x00);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETSCR,
> +			      0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
> +			      0x00);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETVDC, 0x46);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETPANEL, 0x0b);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETCYC, 0x80);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETDISP, 0xc8, 0x12, 0x30);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEQ,
> +			      0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
> +			      0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETPOWER,
> +			      0x53, 0x00, 0x1e, 0x1e, 0x77, 0xe1, 0xcc, 0xdd,
> +			      0x67, 0x77, 0x33, 0x33);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETECO, 0x00, 0x00, 0xff,
> +			      0xff, 0x01, 0xff);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETBGP, 0x09, 0x09);
> +	msleep(20);
> +
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETVCOM, 0x87, 0x95);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETGIP1,
> +			      0xc2, 0x10, 0x05, 0x05, 0x10, 0x05, 0xa0, 0x12,
> +			      0x31, 0x23, 0x3f, 0x81, 0x0a, 0xa0, 0x37, 0x18,
> +			      0x00, 0x80, 0x01, 0x00, 0x00, 0x00, 0x00, 0x80,
> +			      0x01, 0x00, 0x00, 0x00, 0x48, 0xf8, 0x86, 0x42,
> +			      0x08, 0x88, 0x88, 0x80, 0x88, 0x88, 0x88, 0x58,
> +			      0xf8, 0x87, 0x53, 0x18, 0x88, 0x88, 0x81, 0x88,
> +			      0x88, 0x88, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
> +			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETGIP2,
> +			      0x00, 0x1a, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00,
> +			      0x00, 0x00, 0x00, 0x00, 0x1f, 0x88, 0x81, 0x35,
> +			      0x78, 0x88, 0x88, 0x85, 0x88, 0x88, 0x88, 0x0f,
> +			      0x88, 0x80, 0x24, 0x68, 0x88, 0x88, 0x84, 0x88,
> +			      0x88, 0x88, 0x23, 0x10, 0x00, 0x00, 0x1c, 0x00,
> +			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x05,
> +			      0xa0, 0x00, 0x00, 0x00, 0x00);
> +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETGAMMA,
> +			      0x00, 0x06, 0x08, 0x2a, 0x31, 0x3f, 0x38, 0x36,
> +			      0x07, 0x0c, 0x0d, 0x11, 0x13, 0x12, 0x13, 0x11,
> +			      0x18, 0x00, 0x06, 0x08, 0x2a, 0x31, 0x3f, 0x38,
> +			      0x36, 0x07, 0x0c, 0x0d, 0x11, 0x13, 0x12, 0x13,
> +			      0x11, 0x18);
> +
> +	msleep(60);
> +


> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to exit sleep mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* T9: 120ms */
> +	msleep(120);
> +
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret)
> +		return ret;
These two mipi calls should maybe be in xpp055c272_prepare()
In this way you keep the code symmtric.

xpp055c272_unprepare() turns off the display and enter sleep mode.
xpp055c272_prepare() does the opposite.

Bikeshedding - but I looked for the symmetric variant.

> +
> +	msleep(50);
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done\n");
> +	return 0;
> +}
> +
> +static int xpp055c272_unprepare(struct drm_panel *panel)
> +{
> +	struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	if (!ctx->prepared)
> +		return 0;
> +
> +	ret = mipi_dsi_dcs_set_display_off(dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(ctx->dev, "failed to set display off: %d\n",
> +			      ret);
> +
> +	mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev, "failed to enter sleep mode: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	regulator_disable(ctx->iovcc);
> +	regulator_disable(ctx->vci);
> +
> +	ctx->prepared = false;
> +
> +	return 0;
> +}
> +
> +static int xpp055c272_prepare(struct drm_panel *panel)
> +{
> +	struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
> +	int ret;
> +
> +	if (ctx->prepared)
> +		return 0;
> +
> +	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
> +	ret = regulator_enable(ctx->vci);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +			      "Failed to enable vci supply: %d\n", ret);
> +		return ret;
> +	}
> +	ret = regulator_enable(ctx->iovcc);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +			      "Failed to enable iovcc supply: %d\n", ret);
> +		goto disable_vci;
> +	}
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +	/* T6: 10us */
> +	usleep_range(10, 20);
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +
> +	/* T8: 20ms */
> +	msleep(20);
> +
> +	ret = xpp055c272_init_sequence(ctx);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
> +			      ret);
> +		goto disable_iovcc;
> +	}
> +
> +	ctx->prepared = true;
> +
> +	return 0;
> +
> +disable_iovcc:
> +	regulator_disable(ctx->iovcc);
> +disable_vci:
> +	regulator_disable(ctx->vci);
> +	return ret;
> +}
> +
> +static int xpp055c272_enable(struct drm_panel *panel)
> +{
> +	struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
> +	int ret;
> +
> +	if (ctx->enabled)
> +		return 0;
> +
> +	ret = backlight_enable(ctx->backlight);
> +	if (ret) {
> +		DRM_DEV_ERROR(ctx->dev,
> +			      "Failed to enable backlight %d\n", ret);
> +		return ret;
> +	}
> +
> +	ctx->enabled = true;
> +
> +	return 0;
> +}
> +
> +static int xpp055c272_disable(struct drm_panel *panel)
> +{
> +	struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
> +
> +	if (!ctx->enabled)
> +		return 0;
> +
> +	backlight_disable(ctx->backlight);
> +
> +	ctx->enabled = false;
> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode default_mode = {
> +	.hdisplay    = 720,
> +	.hsync_start = 720 + 40,
> +	.hsync_end   = 720 + 40 + 10,
> +	.htotal	     = 720 + 40 + 10 + 40,
> +	.vdisplay    = 1280,
> +	.vsync_start = 1280 + 22,
> +	.vsync_end   = 1280 + 22 + 4,
> +	.vtotal	     = 1280 + 22 + 4 + 11,
> +	.vrefresh    = 60,
> +	.clock	     = 64000,
> +	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +	.width_mm    = 68,
> +	.height_mm   = 121,
> +};
> +
> +static int xpp055c272_get_modes(struct drm_panel *panel)
> +{
> +	struct xpp055c272 *ctx = panel_to_xpp055c272(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\n",
> +			      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 xpp055c272_funcs = {
> +	.disable   = xpp055c272_disable,
> +	.unprepare = xpp055c272_unprepare,
> +	.prepare   = xpp055c272_prepare,
> +	.enable	   = xpp055c272_enable,
> +	.get_modes = xpp055c272_get_modes,
> +};
> +
> +static int xpp055c272_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct xpp055c272 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset_gpio)) {
> +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
> +		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_LPM | MIPI_DSI_MODE_EOT_PACKET;
> +
> +	ctx->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(ctx->backlight))
> +		return PTR_ERR(ctx->backlight);
> +
> +	ctx->vci = devm_regulator_get(dev, "vci");
> +	if (IS_ERR(ctx->vci)) {
> +		ret = PTR_ERR(ctx->vci);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev,
> +				      "Failed to request vci regulator: %d\n",
> +				      ret);
> +		return ret;
> +	}
> +	ctx->iovcc = devm_regulator_get(dev, "iovcc");
> +	if (IS_ERR(ctx->iovcc)) {
> +		ret = PTR_ERR(ctx->iovcc);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev,
> +				      "Failed to request iovcc regulator: %d\n",
> +				      ret);
> +		return ret;
> +	}
> +
> +	drm_panel_init(&ctx->panel, &dsi->dev, &xpp055c272_funcs,
> +		       DRM_MODE_CONNECTOR_DSI);
> +	drm_panel_add(&ctx->panel);
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed: %d\n", ret);
> +		drm_panel_remove(&ctx->panel);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void xpp055c272_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct xpp055c272 *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = drm_panel_unprepare(&ctx->panel);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(&dsi->dev, "Failed to unprepare panel: %d\n",
> +			      ret);
> +
> +	ret = drm_panel_disable(&ctx->panel);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(&dsi->dev, "Failed to disable panel: %d\n",
> +			      ret);
> +}
> +
> +static int xpp055c272_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct xpp055c272 *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	xpp055c272_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);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xpp055c272_of_match[] = {
> +	{ .compatible = "xinpeng,xpp055c272" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, xpp055c272_of_match);
> +
> +static struct mipi_dsi_driver xpp055c272_driver = {
> +	.probe	= xpp055c272_probe,
> +	.remove = xpp055c272_remove,
> +	.shutdown = xpp055c272_shutdown,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = xpp055c272_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(xpp055c272_driver);
> +
> +MODULE_AUTHOR("Heiko Stuebner <heiko.stuebner@theobroma-systems.com>");
> +MODULE_DESCRIPTION("DRM driver for Xinpeng xpp055c272 MIPI DSI panel");
> +MODULE_LICENSE("GPL v2");

Looks forward to see v2 of the driver.

	Sam

Re: [PATCH 3/3] drm/panel: add panel driver for Xinpeng XPP055C272 panels

[Heiko Stübner]

Hi Sam,

thanks for the thorough review :-)

Am Samstag, 14. Dezember 2019, 09:17:30 CET schrieb Sam Ravnborg:
> > +#define dsi_generic_write_seq(dsi, cmd, seq...) do {			\
> > +		static const u8 d[] = { seq };				\
> > +		int ret;						\
> > +		ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d));	\
> > +		if (ret < 0)						\
> > +			return ret;					\
> > +	} while (0)
> This macro return an error code if a write fails.
> 
> > +
> > +static int xpp055c272_init_sequence(struct xpp055c272 *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 without much
> > +	 * documentation.
> > +	 */
> > +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEXTC, 0xf1, 0x12, 0x83);
> But all uses of the macro here ignore the error.

hmm, am I way off track here?

	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEXTC, 0xf1, 0x12, 0x83);
	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETMIPI,
			      0x33, 0x81, 0x05, 0xf9, 0x0e, 0x0e, 0x00, 0x00,
			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
			      0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4f, 0x01,
			      0x00, 0x00, 0x37);
	...

should just expand to

do {
		static const u8 d[] = { 0xf1, 0x12, 0x83 };
		int ret;
		ret = mipi_dsi_dcs_write(dsi, XPP055C272_CMD_SETEXTC, d, ARRAY_SIZE(d));
		if (ret < 0)
			return ret;
} while (0)
do {
		static const u8 d[] = { 0x33, 0x81, 0x05, 0xf9, 0x0e, 0x0e, 0x00, 0x00,
			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
			      0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4f, 0x01,
			      0x00, 0x00, 0x37 };
		int ret;
		ret = mipi_dsi_dcs_write(dsi, XPP055C272_CMD_SETMIPI, d, ARRAY_SIZE(d));
		if (ret < 0)
			return ret;
} while (0)
...

so every write instance will actually return an error if it happens and not
continue on with the next init item.

Or I'm not thinking correctly at 0:07 ;-)


Heiko

Re: [PATCH 3/3] drm/panel: add panel driver for Xinpeng XPP055C272 panels

[Sam Ravnborg]

Hi Heiko.

> Am Samstag, 14. Dezember 2019, 09:17:30 CET schrieb Sam Ravnborg:
> > > +#define dsi_generic_write_seq(dsi, cmd, seq...) do {			\
> > > +		static const u8 d[] = { seq };				\
> > > +		int ret;						\
> > > +		ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d));	\
> > > +		if (ret < 0)						\
> > > +			return ret;					\
> > > +	} while (0)
> > This macro return an error code if a write fails.
> > 
> > > +
> > > +static int xpp055c272_init_sequence(struct xpp055c272 *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 without much
> > > +	 * documentation.
> > > +	 */
> > > +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEXTC, 0xf1, 0x12, 0x83);
> > But all uses of the macro here ignore the error.
> 
> hmm, am I way off track here?
> 
> 	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEXTC, 0xf1, 0x12, 0x83);
> 	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETMIPI,
> 			      0x33, 0x81, 0x05, 0xf9, 0x0e, 0x0e, 0x00, 0x00,
> 			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
> 			      0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4f, 0x01,
> 			      0x00, 0x00, 0x37);
> 	...
> 
> should just expand to
> 
> do {
> 		static const u8 d[] = { 0xf1, 0x12, 0x83 };
> 		int ret;
> 		ret = mipi_dsi_dcs_write(dsi, XPP055C272_CMD_SETEXTC, d, ARRAY_SIZE(d));
> 		if (ret < 0)
> 			return ret;
> } while (0)
> do {
> 		static const u8 d[] = { 0x33, 0x81, 0x05, 0xf9, 0x0e, 0x0e, 0x00, 0x00,
> 			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
> 			      0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4f, 0x01,
> 			      0x00, 0x00, 0x37 };
> 		int ret;
> 		ret = mipi_dsi_dcs_write(dsi, XPP055C272_CMD_SETMIPI, d, ARRAY_SIZE(d));
> 		if (ret < 0)
> 			return ret;
> } while (0)
> ...
> 
> so every write instance will actually return an error if it happens and not
> continue on with the next init item.
The idea was that if a write returned an error then do not even attempt
more writes. So if a write fails we do not loose the original error
code, assuming subsequent write would also fail.

I have looked through all the panel drivers now, and the majority does
not check if the write goes wrong.
So following the pattern on the other panels you can also decide to just
ignore the return value of mipi_dsi_dcs_write() rahter than trying to
invent the check I tried to explain.

	Sam

Re: [PATCH 3/3] drm/panel: add panel driver for Xinpeng XPP055C272 panels

[Heiko Stuebner]

Hi Sam,

Am Sonntag, 15. Dezember 2019, 09:29:16 CET schrieb Sam Ravnborg:
> Hi Heiko.
> 
> > Am Samstag, 14. Dezember 2019, 09:17:30 CET schrieb Sam Ravnborg:
> > > > +#define dsi_generic_write_seq(dsi, cmd, seq...) do {			\
> > > > +		static const u8 d[] = { seq };				\
> > > > +		int ret;						\
> > > > +		ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d));	\
> > > > +		if (ret < 0)						\
> > > > +			return ret;					\
> > > > +	} while (0)
> > > This macro return an error code if a write fails.
> > > 
> > > > +
> > > > +static int xpp055c272_init_sequence(struct xpp055c272 *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 without much
> > > > +	 * documentation.
> > > > +	 */
> > > > +	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEXTC, 0xf1, 0x12, 0x83);
> > > But all uses of the macro here ignore the error.
> > 
> > hmm, am I way off track here?
> > 
> > 	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETEXTC, 0xf1, 0x12, 0x83);
> > 	dsi_generic_write_seq(dsi, XPP055C272_CMD_SETMIPI,
> > 			      0x33, 0x81, 0x05, 0xf9, 0x0e, 0x0e, 0x00, 0x00,
> > 			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
> > 			      0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4f, 0x01,
> > 			      0x00, 0x00, 0x37);
> > 	...
> > 
> > should just expand to
> > 
> > do {
> > 		static const u8 d[] = { 0xf1, 0x12, 0x83 };
> > 		int ret;
> > 		ret = mipi_dsi_dcs_write(dsi, XPP055C272_CMD_SETEXTC, d, ARRAY_SIZE(d));
> > 		if (ret < 0)
> > 			return ret;
> > } while (0)
> > do {
> > 		static const u8 d[] = { 0x33, 0x81, 0x05, 0xf9, 0x0e, 0x0e, 0x00, 0x00,
> > 			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
> > 			      0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4f, 0x01,
> > 			      0x00, 0x00, 0x37 };
> > 		int ret;
> > 		ret = mipi_dsi_dcs_write(dsi, XPP055C272_CMD_SETMIPI, d, ARRAY_SIZE(d));
> > 		if (ret < 0)
> > 			return ret;
> > } while (0)
> > ...
> > 
> > so every write instance will actually return an error if it happens and not
> > continue on with the next init item.
> The idea was that if a write returned an error then do not even attempt
> more writes. So if a write fails we do not loose the original error
> code, assuming subsequent write would also fail.

Shouldn't the code above do exactly that? ... Because it's like

	ret = dcs_write(...)
	if (ret <0)
		return ret;

So if any of the dcs_writes goes wrong it should just return the
error code from that write from the function and not try any more
writes. (or I'm blind and do not see something ;-) )


Heiko

> I have looked through all the panel drivers now, and the majority does
> not check if the write goes wrong.
> So following the pattern on the other panels you can also decide to just
> ignore the return value of mipi_dsi_dcs_write() rahter than trying to
> invent the check I tried to explain.
> 
> 	Sam
> 

Re: [PATCH 3/3] drm/panel: add panel driver for Xinpeng XPP055C272 panels

[Sam Ravnborg]

Hi Heiko.

> > The idea was that if a write returned an error then do not even attempt
> > more writes. So if a write fails we do not loose the original error
> > code, assuming subsequent write would also fail.
> 
> Shouldn't the code above do exactly that? ... Because it's like
> 
> 	ret = dcs_write(...)
> 	if (ret <0)
> 		return ret;
> 
> So if any of the dcs_writes goes wrong it should just return the
> error code from that write from the function and not try any more
> writes. (or I'm blind and do not see something ;-) )

You are right, the code does it already.

	Sam

Re: [PATCH 1/3] dt-bindings: Add vendor prefix for Xinpeng Technology

[Sam Ravnborg]

Hi Heiko.


On Mon, Dec 09, 2019 at 03:42:06PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> Shenzhen Xinpeng Technology Co., Ltd produces for example display panels.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 6046f4555852..85e7c26a05c7 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1056,6 +1056,8 @@ patternProperties:
>      description: Extreme Engineering Solutions (X-ES)
>    "^xillybus,.*":
>      description: Xillybus Ltd.
> +  "^xinpeng,.*":
> +    description: Shenzhen Xinpeng Technology Co., Ltd
>    "^xlnx,.*":
>      description: Xilinx
>    "^xunlong,.*":

Looks good.
Acked-by: Sam Ravnborg <sam@ravnborg.org>