[PATCH 1/3] dt-bindings: vendor-prefixes: Add prefix for EBBG

[PATCH 1/3] dt-bindings: vendor-prefixes: Add prefix for EBBG

[Joel Selvaraj]

Add a prefix for EBBG. They manufacture displays which are used in some
Xiaomi phones, but I could not find much details about the company.

Signed-off-by: Joel Selvaraj <jo@jsfamily.in>
---
 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 80e80fa53f8a..effd1cb995cf 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -337,6 +337,8 @@ patternProperties:
     description: Embedded Artists AB
   "^ebang,.*":
     description: Zhejiang Ebang Communication Co., Ltd
+  "^ebbg,.*":
+    description: EBBG
   "^ebs-systart,.*":
     description: EBS-SYSTART GmbH
   "^ebv,.*":
-- 
2.35.1

[PATCH 2/3] dt-bindings: display: Add bindings for EBBG FT8719

[Joel Selvaraj]

Add bindings for the EBBG FT8719 6.18" 2246x1080 DSI video mode panel,
which can be found on some Xiaomi Poco F1 phones. The backlight is
managed through the QCOM WLED driver.

Signed-off-by: Joel Selvaraj <jo@jsfamily.in>
---
 .../bindings/display/panel/ebbg,ft8719.yaml   | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml b/Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml
new file mode 100644
index 000000000000..fac6c9692c55
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/ebbg,ft8719.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: EBBG FT8719 MIPI-DSI LCD panel
+
+maintainers:
+  - Joel Selvaraj <jo@jsfamily.in>
+
+description: |
+  The FT8719 panel from EBBG is a FHD+ LCD display panel with a resolution
+  of 1080x2246. It is a video mode DSI panel. The backlight is managed
+  through the QCOM WLED driver.
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    const: ebbg,ft8719
+
+  reg:
+    description: DSI virtual channel of the peripheral
+
+  reset-gpios:
+    description: phandle of gpio for reset line
+
+  vddio-supply:
+    description: phandle of the Power IC supply regulator
+
+  vddpos-supply:
+    description: phandle of the positive boost supply regulator
+
+  vddneg-supply:
+    description: phandle of the negative boost supply regulator
+
+  backlight: true
+  port: true
+
+required:
+  - compatible
+  - reg
+  - vddio-supply
+  - vddpos-supply
+  - vddneg-supply
+  - reset-gpios
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |+
+    #include <dt-bindings/gpio/gpio.h>
+    dsi0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      panel@0 {
+        compatible = "ebbg,ft8719";
+        reg = <0>;
+
+        vddio-supply = <&vreg_l14a_1p88>;
+        vddpos-supply = <&lab>;
+        vddneg-supply = <&ibb>;
+
+        reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
+
+        backlight = <&pmi8998_wled>;
+
+        port {
+          ebbg_ft8719_in_0: endpoint {
+            remote-endpoint = <&dsi0_out>;
+          };
+        };
+      };
+    };
-- 
2.35.1

[PATCH 3/3] drm/panel: introduce ebbg,ft8719 panel

[Joel Selvaraj]

Add DRM panel driver for EBBG FT8719 6.18" 2246x1080 DSI video mode
panel, which can be found on some Xiaomi Poco F1 phones. The panel's
backlight is managed through QCOM WLED driver.

Signed-off-by: Joel Selvaraj <jo@jsfamily.in>
---
 MAINTAINERS                               |   7 +
 drivers/gpu/drm/panel/Kconfig             |  11 +
 drivers/gpu/drm/panel/Makefile            |   1 +
 drivers/gpu/drm/panel/panel-ebbg-ft8719.c | 362 ++++++++++++++++++++++
 4 files changed, 381 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-ebbg-ft8719.c

diff --git a/MAINTAINERS b/MAINTAINERS
index cd0f68d4a34a..dfd2c53aea00 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6018,6 +6018,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
 F:	drivers/gpu/drm/bridge/chipone-icn6211.c
 
+DRM DRIVER FOR EBBG FT8719 PANEL
+M:	Joel Selvaraj <jo@jsfamily.in>
+S:	Maintained
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml
+F:	drivers/gpu/drm/panel/panel-ebbg-ft8719.c
+
 DRM DRIVER FOR FARADAY TVE200 TV ENCODER
 M:	Linus Walleij <linus.walleij@linaro.org>
 S:	Maintained
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 9989a316fe88..77176df2e2ec 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -114,6 +114,17 @@ config DRM_PANEL_EDP
 	  that it can be automatically turned off when the panel goes into a
 	  low power state.
 
+config DRM_PANEL_EBBG_FT8719
+	tristate "EBBG FT8719 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 EBBG FT8719
+	  video mode panel. Mainly found on Xiaomi Poco F1 mobile phone.
+	  The panel has a resolution of 1080x2246. It provides a MIPI DSI
+	  interface to the host.
+
 config DRM_PANEL_ELIDA_KD35T133
 	tristate "Elida KD35T133 panel driver"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index d99fbbce49d1..47cc20c1a770 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_DRM_PANEL_DSI_CM) += panel-dsi-cm.o
 obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_EDP) += panel-edp.o
+obj-$(CONFIG_DRM_PANEL_EBBG_FT8719) += panel-ebbg-ft8719.o
 obj-$(CONFIG_DRM_PANEL_ELIDA_KD35T133) += panel-elida-kd35t133.o
 obj-$(CONFIG_DRM_PANEL_FEIXIN_K101_IM2BA02) += panel-feixin-k101-im2ba02.o
 obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o
diff --git a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
new file mode 100644
index 000000000000..abd54c4b0c23
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 Joel Selvaraj <jo@jsfamily.in>
+ * Generated with linux-mdss-dsi-panel-driver-generator from vendor device tree:
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+static const char * const regulator_names[] = {
+	"vddio",
+	"vddpos",
+	"vddneg",
+};
+
+static const unsigned long regulator_enable_loads[] = {
+	62000,
+	100000,
+	100000
+};
+
+struct ebbg_ft8719 {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+
+	struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
+
+	struct gpio_desc *reset_gpio;
+	bool prepared;
+};
+
+static inline
+struct ebbg_ft8719 *to_ebbg_ft8719(struct drm_panel *panel)
+{
+	return container_of(panel, struct ebbg_ft8719, 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)
+
+#define dsi_dcs_write_seq(dsi, seq...) do {				\
+		static const u8 d[] = { seq };				\
+		int ret;						\
+		ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d));	\
+		if (ret < 0)						\
+			return ret;					\
+	} while (0)
+
+static void ebbg_ft8719_reset(struct ebbg_ft8719 *ctx)
+{
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	usleep_range(4000, 5000);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	usleep_range(1000, 2000);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	usleep_range(15000, 16000);
+}
+
+static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret;
+
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	dsi_dcs_write_seq(dsi, 0x00, 0x00);
+	dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19, 0x01);
+	dsi_dcs_write_seq(dsi, 0x00, 0x80);
+	dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19);
+	dsi_dcs_write_seq(dsi, 0x00, 0xa0);
+	dsi_generic_write_seq(dsi, 0xca, 0x0f, 0x0f, 0x0f);
+	dsi_dcs_write_seq(dsi, 0x00, 0x80);
+	dsi_generic_write_seq(dsi, 0xca,
+			      0xbe, 0xb5, 0xad, 0xa6, 0xa0, 0x9b, 0x96, 0x91,
+			      0x8d, 0x8a, 0x87, 0x83);
+	dsi_dcs_write_seq(dsi, 0x00, 0x90);
+	dsi_generic_write_seq(dsi, 0xca,
+			      0xfe, 0xff, 0x66, 0xf6, 0xff, 0x66, 0xfb, 0xff,
+			      0x32);
+	dsi_dcs_write_seq(dsi, 0x00, 0xa0);
+	dsi_generic_write_seq(dsi, 0xd6,
+			      0x7a, 0x79, 0x74, 0x8c, 0x8c, 0x92, 0x97, 0x9b,
+			      0x97, 0x8f, 0x80, 0x77);
+	dsi_dcs_write_seq(dsi, 0x00, 0xb0);
+	dsi_generic_write_seq(dsi, 0xd6,
+			      0x7e, 0x7d, 0x81, 0x7a, 0x7a, 0x7b, 0x7c, 0x81,
+			      0x84, 0x85, 0x80, 0x82);
+	dsi_dcs_write_seq(dsi, 0x00, 0xc0);
+	dsi_generic_write_seq(dsi, 0xd6,
+			      0x7d, 0x7d, 0x78, 0x8a, 0x89, 0x8f, 0x97, 0x97,
+			      0x8f, 0x8c, 0x80, 0x7a);
+	dsi_dcs_write_seq(dsi, 0x00, 0xd0);
+	dsi_generic_write_seq(dsi, 0xd6,
+			      0x7e, 0x7d, 0x81, 0x7c, 0x79, 0x7b, 0x7c, 0x80,
+			      0x84, 0x85, 0x80, 0x82);
+	dsi_dcs_write_seq(dsi, 0x00, 0xe0);
+	dsi_generic_write_seq(dsi, 0xd6,
+			      0x7b, 0x7b, 0x7b, 0x80, 0x80, 0x80, 0x80, 0x80,
+			      0x80, 0x80, 0x80, 0x80);
+	dsi_dcs_write_seq(dsi, 0x00, 0xf0);
+	dsi_generic_write_seq(dsi, 0xd6,
+			      0x7e, 0x7e, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+			      0x80, 0x80, 0x80, 0x80);
+	dsi_dcs_write_seq(dsi, 0x00, 0x00);
+	dsi_generic_write_seq(dsi, 0xd7,
+			      0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+			      0x80, 0x80, 0x80, 0x80);
+	dsi_dcs_write_seq(dsi, 0x00, 0x10);
+	dsi_generic_write_seq(dsi, 0xd7,
+			      0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+			      0x80, 0x80, 0x80, 0x80);
+	dsi_dcs_write_seq(dsi, 0x00, 0x00);
+	dsi_generic_write_seq(dsi, 0xff, 0x00, 0x00, 0x00);
+	dsi_dcs_write_seq(dsi, 0x00, 0x80);
+	dsi_generic_write_seq(dsi, 0xff, 0x00, 0x00);
+	dsi_dcs_write_seq(dsi, 0x91, 0x00);
+
+	ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set display brightness: %d\n", ret);
+		return ret;
+	}
+
+	dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x24);
+	dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
+
+	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
+		return ret;
+	}
+	msleep(90);
+
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set display on: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ebbg_ft8719_off(struct ebbg_ft8719 *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret;
+
+	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+	ret = mipi_dsi_dcs_set_display_off(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set display off: %d\n", ret);
+		return ret;
+	}
+	usleep_range(10000, 11000);
+
+	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enter sleep mode: %d\n", ret);
+		return ret;
+	}
+	msleep(90);
+
+	return 0;
+}
+
+static int ebbg_ft8719_prepare(struct drm_panel *panel)
+{
+	struct ebbg_ft8719 *ctx = to_ebbg_ft8719(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	if (ctx->prepared)
+		return 0;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (ret < 0)
+		return ret;
+
+	ebbg_ft8719_reset(ctx);
+
+	ret = ebbg_ft8719_on(ctx);
+	if (ret < 0) {
+		dev_err(dev, "Failed to initialize panel: %d\n", ret);
+		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+		return ret;
+	}
+
+	ctx->prepared = true;
+	return 0;
+}
+
+static int ebbg_ft8719_unprepare(struct drm_panel *panel)
+{
+	struct ebbg_ft8719 *ctx = to_ebbg_ft8719(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	if (!ctx->prepared)
+		return 0;
+
+	ret = ebbg_ft8719_off(ctx);
+	if (ret < 0)
+		dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (ret)
+		dev_err(panel->dev, "Failed to disable regulators: %d\n", ret);
+
+	ctx->prepared = false;
+	return 0;
+}
+
+static const struct drm_display_mode ebbg_ft8719_mode = {
+	.clock = (1080 + 28 + 4 + 16) * (2246 + 120 + 4 + 12) * 60 / 1000,
+	.hdisplay = 1080,
+	.hsync_start = 1080 + 28,
+	.hsync_end = 1080 + 28 + 4,
+	.htotal = 1080 + 28 + 4 + 16,
+	.vdisplay = 2246,
+	.vsync_start = 2246 + 120,
+	.vsync_end = 2246 + 120 + 4,
+	.vtotal = 2246 + 120 + 4 + 12,
+	.width_mm = 68,
+	.height_mm = 141,
+};
+
+static int ebbg_ft8719_get_modes(struct drm_panel *panel,
+				     struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(connector->dev, &ebbg_ft8719_mode);
+	if (!mode)
+		return -ENOMEM;
+
+	drm_mode_set_name(mode);
+
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	connector->display_info.width_mm = mode->width_mm;
+	connector->display_info.height_mm = mode->height_mm;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs ebbg_ft8719_panel_funcs = {
+	.prepare = ebbg_ft8719_prepare,
+	.unprepare = ebbg_ft8719_unprepare,
+	.get_modes = ebbg_ft8719_get_modes,
+};
+
+static int ebbg_ft8719_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct ebbg_ft8719 *ctx;
+	int i, ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++)
+		ctx->supplies[i].supply = regulator_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
+				      ctx->supplies);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
+	for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
+		ret = regulator_set_load(ctx->supplies[i].consumer,
+						regulator_enable_loads[i]);
+		if (ret)
+			return dev_err_probe(dev, ret,
+						 "Failed to set regulator load\n");
+	}
+
+	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ctx->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
+				     "Failed to get reset-gpios\n");
+
+	ctx->dsi = dsi;
+	mipi_dsi_set_drvdata(dsi, ctx);
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+			  MIPI_DSI_CLOCK_NON_CONTINUOUS;
+
+	drm_panel_init(&ctx->panel, dev, &ebbg_ft8719_panel_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+
+	ret = drm_panel_of_backlight(&ctx->panel);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get backlight\n");
+
+	drm_panel_add(&ctx->panel);
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to attach to DSI host: %d\n", ret);
+		drm_panel_remove(&ctx->panel);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ebbg_ft8719_remove(struct mipi_dsi_device *dsi)
+{
+	struct ebbg_ft8719 *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	ret = mipi_dsi_detach(dsi);
+	if (ret < 0)
+		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
+
+	drm_panel_remove(&ctx->panel);
+
+	return 0;
+}
+
+static const struct of_device_id ebbg_ft8719_of_match[] = {
+	{ .compatible = "ebbg,ft8719" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ebbg_ft8719_of_match);
+
+static struct mipi_dsi_driver ebbg_ft8719_driver = {
+	.probe = ebbg_ft8719_probe,
+	.remove = ebbg_ft8719_remove,
+	.driver = {
+		.name = "panel-ebbg-ft8719",
+		.of_match_table = ebbg_ft8719_of_match,
+	},
+};
+module_mipi_dsi_driver(ebbg_ft8719_driver);
+
+MODULE_AUTHOR("Joel Selvaraj <jo@jsfamily.in>");
+MODULE_DESCRIPTION("DRM driver for EBBG FT8719 video dsi panel");
+MODULE_LICENSE("GPL v2");
-- 
2.35.1

Re: [PATCH 1/3] dt-bindings: vendor-prefixes: Add prefix for EBBG

[Krzysztof Kozlowski]

On 06/05/2022 14:17, Joel Selvaraj wrote:
> Add a prefix for EBBG. They manufacture displays which are used in some
> Xiaomi phones, but I could not find much details about the company.
> 
> Signed-off-by: Joel Selvaraj <jo@jsfamily.in>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: display: Add bindings for EBBG FT8719

[Krzysztof Kozlowski]

On 06/05/2022 14:17, Joel Selvaraj wrote:
> Add bindings for the EBBG FT8719 6.18" 2246x1080 DSI video mode panel,
> which can be found on some Xiaomi Poco F1 phones. The backlight is
> managed through the QCOM WLED driver.
> 
> Signed-off-by: Joel Selvaraj <jo@jsfamily.in>
> ---
>  .../bindings/display/panel/ebbg,ft8719.yaml   | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml b/Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml
> new file mode 100644
> index 000000000000..fac6c9692c55
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/ebbg,ft8719.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: EBBG FT8719 MIPI-DSI LCD panel
> +
> +maintainers:
> +  - Joel Selvaraj <jo@jsfamily.in>
> +
> +description: |
> +  The FT8719 panel from EBBG is a FHD+ LCD display panel with a resolution
> +  of 1080x2246. It is a video mode DSI panel. The backlight is managed
> +  through the QCOM WLED driver.
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: ebbg,ft8719
> +
> +  reg:
> +    description: DSI virtual channel of the peripheral

maxItems

> +
> +  reset-gpios:
> +    description: phandle of gpio for reset line

Skip description - it's obvious.

> +
> +  vddio-supply:
> +    description: phandle of the Power IC supply regulator

s/phandle of//

> +
> +  vddpos-supply:
> +    description: phandle of the positive boost supply regulator
> +
> +  vddneg-supply:
> +    description: phandle of the negative boost supply regulator
> +
> +  backlight: true
> +  port: true

Both should not be needed - they come from panel-common.yaml. They might
stay in list

> +
> +required:
> +  - compatible
> +  - reg
> +  - vddio-supply
> +  - vddpos-supply
> +  - vddneg-supply
> +  - reset-gpios
> +  - port
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |+

No need for +

> +    #include <dt-bindings/gpio/gpio.h>
> +    dsi0 {

Just dsi



Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: display: Add bindings for EBBG FT8719

[Joel Selvaraj]

Hi Krzysztof Kozlowski,

Thank you for your review. Will fix them in v2.

Regards
Joel Selvaraj

Re: [PATCH 2/3] dt-bindings: display: Add bindings for EBBG FT8719

[Joel Selvaraj]

Hi Krzysztof Kozlowski,

A quick question.

On 07/05/22 21:02, Krzysztof Kozlowski wrote:
 >> +  backlight: true
 >> +  port: true
 >
 > Both should not be needed - they come from panel-common.yaml. They might
 > stay in list

I see that almost 54 panels mention "port: true" and 46 panels mention
"backlight: true". Almost all panels refer the panel-common.yaml too.

So I think specifying them as true is just for extra clarity that
these properties are usually used by this panel? But I am not very sure.

Should I leave them be? or it's still recommended to remove them?

Best Regards,
Joel Selvaraj

Re: [PATCH 2/3] dt-bindings: display: Add bindings for EBBG FT8719

[Krzysztof Kozlowski]

On 09/05/2022 07:08, Joel Selvaraj wrote:
> Hi Krzysztof Kozlowski,
> 
> A quick question.
> 
> On 07/05/22 21:02, Krzysztof Kozlowski wrote:
>  >> +  backlight: true
>  >> +  port: true
>  >
>  > Both should not be needed - they come from panel-common.yaml. They might
>  > stay in list
> 
> I see that almost 54 panels mention "port: true" and 46 panels mention
> "backlight: true". Almost all panels refer the panel-common.yaml too.

They need them only if they use "additionalProperties:false".
> 
> So I think specifying them as true is just for extra clarity that
> these properties are usually used by this panel? But I am not very sure.

If they don't use additionalProperties:false, then the explanation could be:
1. Just for clarity as you say, because they might want to require
property/node which is listed in the properties, otherwise it is a bit
confusing.
2. They were copying first example without actually checking...

> Should I leave them be? or it's still recommended to remove them?

It's not a big deal and I do not have strong opinion, but I would
propose to remove them from list of properties and still keep port in
"required"


Best regards,
Krzysztof

Re: [PATCH 3/3] drm/panel: introduce ebbg,ft8719 panel

[Linus Walleij]

On Fri, May 6, 2022 at 2:18 PM Joel Selvaraj <jo@jsfamily.in> wrote:

> Add DRM panel driver for EBBG FT8719 6.18" 2246x1080 DSI video mode
> panel, which can be found on some Xiaomi Poco F1 phones. The panel's
> backlight is managed through QCOM WLED driver.
>
> Signed-off-by: Joel Selvaraj <jo@jsfamily.in>

Cool!

> +#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)
> +
> +#define dsi_dcs_write_seq(dsi, seq...) do {                            \
> +               static const u8 d[] = { seq };                          \
> +               int ret;                                                \
> +               ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> +               if (ret < 0)                                            \
> +                       return ret;                                     \
> +       } while (0)

First I don't see what the do {} while (0) buys you, just use
a basic block {}.

Second look at mipi_dbi_command() in include/drm/drm_mipi_dbi.h
this is very similar.

So this utility macro should be in a generic file such as
include/drm/drm_mipi_dsi.h. (Can be added in a separate
patch.)

Third I think you need only one macro (see below).

> +static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
> +{
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct device *dev = &dsi->dev;
> +       int ret;
> +
> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +       dsi_dcs_write_seq(dsi, 0x00, 0x00);
> +       dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19, 0x01);

It's dubious that you always have dsi_dcs_write_seq()
followed by dsi_generic_write_seq().

That means mipi_dsi_generic_write() followed by
mipi_dsi_dcs_write_buffer(). But if you look at these
commands in drivers/gpu/drm/drm_mipi_dsi.c
you see that they do the same thing!

Doesn't it work to combine them into one call for each
pair?

> +       dsi_dcs_write_seq(dsi, 0x00, 0x80);
> +       dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19);

Lots of magic numbers. You don't have a datasheet do you?
So you could #define some of the magic?

> +       if (ctx->prepared)
> +               return 0;
(...)
> +       ctx->prepared = true;
> +       return 0;
(...)
> +       if (!ctx->prepared)
> +               return 0;
(...)
> +       ctx->prepared = false;
> +       return 0;

Drop this state variable it is a reimplementation of something
that the core will track for you.

The rest looks nice!

Yours,
Linus Walleij

Re: [PATCH 3/3] drm/panel: introduce ebbg,ft8719 panel

[Joel Selvaraj]

Hi Linus Walleij,

On 13/05/22 03:21, Linus Walleij wrote:
> On Fri, May 6, 2022 at 2:18 PM Joel Selvaraj <jo@jsfamily.in> wrote:
>> +#define dsi_dcs_write_seq(dsi, seq...) do {                            \
>> +               static const u8 d[] = { seq };                          \
>> +               int ret;                                                \
>> +               ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
>> +               if (ret < 0)                                            \
>> +                       return ret;                                     \
>> +       } while (0)
> 
> First I don't see what the do {} while (0) buys you, just use
> a basic block {}.

The do {} while (0) in macro ensures there is a semicolon when it's 
used. With normal blocking, it would have issues with if conditions?
I read about them here: https://stackoverflow.com/a/2381339

> Second look at mipi_dbi_command() in include/drm/drm_mipi_dbi.h
> this is very similar.

Does the ({..}) style blocking used in mipi_dbi_command help workaround 
the semicolon issue I mentioned above?

> So this utility macro should be in a generic file such as
> include/drm/drm_mipi_dsi.h. (Can be added in a separate
> patch.)

I agree. Could be done in another patch.

> Third I think you need only one macro (see below).
> 
>> +static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
>> +{
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct device *dev = &dsi->dev;
>> +       int ret;
>> +
>> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>> +
>> +       dsi_dcs_write_seq(dsi, 0x00, 0x00);
>> +       dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19, 0x01);
> 
> It's dubious that you always have dsi_dcs_write_seq()
> followed by dsi_generic_write_seq().
> 
> That means mipi_dsi_generic_write() followed by
> mipi_dsi_dcs_write_buffer(). But if you look at these
> commands in drivers/gpu/drm/drm_mipi_dsi.c
> you see that they do the same thing!

They almost do the same thing except for the msg.type values? Mostly the 
msg.type value is used to just check whether it's a long or short write 
in the msm dsi_host code. However, in mipi_dsi_create_packet function, 
the msg->type value is used to calculate packet->header[0] as follows:

packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);

Wouldn't the difference between the mipi_dsi_dcs_write_buffer's and 
mipi_dsi_generic_write's msg.type values cause issue here?

I tried using mipi_dsi_dcs_write_buffer for all commands and the panel 
worked fine, but I am not sure if it's correct to do so?

> Lots of magic numbers. You don't have a datasheet do you?
> So you could #define some of the magic?

Unfortunately, I don't have a datasheet and the power on sequence is 
taken from downstream android dts. It works pretty well though. So I 
don't think I can #define any of these magic.

 > Doesn't it work to combine them into one call for each
 > pair?
 >> +       dsi_dcs_write_seq(dsi, 0x00, 0x80);
 >> +       dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19);

By using a macro? We can... but I am not sure what (0x00, 0x80), (0x00, 
0xa0),etc type of commands signify without the datasheet, so I am not 
sure what to name them in the macro and make any sensible meaning out of it.

> 
>> +       if (ctx->prepared)
>> +               return 0;
> (...)
>> +       ctx->prepared = true;
>> +       return 0;
> (...)
>> +       if (!ctx->prepared)
>> +               return 0;
> (...)
>> +       ctx->prepared = false;
>> +       return 0;
> 
> Drop this state variable it is a reimplementation of something
> that the core will track for you.

ok. Will drop them in the next version.

> The rest looks nice!

Thanks for your review!

> Yours,
> Linus Walleij

Best Regards,
Joel Selvaraj

Re: [PATCH 3/3] drm/panel: introduce ebbg,ft8719 panel

[Linus Walleij]

On Mon, May 16, 2022 at 2:56 PM Joel Selvaraj <jo@jsfamily.in> wrote:
> On 13/05/22 03:21, Linus Walleij wrote:
> > On Fri, May 6, 2022 at 2:18 PM Joel Selvaraj <jo@jsfamily.in> wrote:
> >> +#define dsi_dcs_write_seq(dsi, seq...) do {                            \
> >> +               static const u8 d[] = { seq };                          \
> >> +               int ret;                                                \
> >> +               ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> >> +               if (ret < 0)                                            \
> >> +                       return ret;                                     \
> >> +       } while (0)
> >
> > First I don't see what the do {} while (0) buys you, just use
> > a basic block {}.
>
> The do {} while (0) in macro ensures there is a semicolon when it's
> used. With normal blocking, it would have issues with if conditions?
> I read about them here: https://stackoverflow.com/a/2381339

Hm that seems true, it enforces the semicolon ; at the end of the
statement which is nice. I suppose we should fix the other macro
as well.

Noralf added this ({}) form in 02dd95fe31693, so maybe he wants
to chip in.

> > Second look at mipi_dbi_command() in include/drm/drm_mipi_dbi.h
> > this is very similar.
>
> Does the ({..}) style blocking used in mipi_dbi_command help workaround
> the semicolon issue I mentioned above?

Nope. But add the rate limited error print please!

> > It's dubious that you always have dsi_dcs_write_seq()
> > followed by dsi_generic_write_seq().
> >
> > That means mipi_dsi_generic_write() followed by
> > mipi_dsi_dcs_write_buffer(). But if you look at these
> > commands in drivers/gpu/drm/drm_mipi_dsi.c
> > you see that they do the same thing!
>
> They almost do the same thing except for the msg.type values? Mostly the
> msg.type value is used to just check whether it's a long or short write
> in the msm dsi_host code. However, in mipi_dsi_create_packet function,
> the msg->type value is used to calculate packet->header[0] as follows:
>
> packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
>
> Wouldn't the difference between the mipi_dsi_dcs_write_buffer's and
> mipi_dsi_generic_write's msg.type values cause issue here?
>
> I tried using mipi_dsi_dcs_write_buffer for all commands and the panel
> worked fine, but I am not sure if it's correct to do so?

I think it's fine? The only issue would be if there is a DSI host controller
that only supports short writes, and in that case it should emulate
long writes by breaking long messages apart. (My amateur view at least.)

> > Lots of magic numbers. You don't have a datasheet do you?
> > So you could #define some of the magic?
>
> Unfortunately, I don't have a datasheet and the power on sequence is
> taken from downstream android dts. It works pretty well though. So I
> don't think I can #define any of these magic.

If you know which display controller the display is using (usually
Novatek nnnnn, Ilitek nnnn etc someting like that) there is often
a datasheet for the display controller available but the display per
se often obscures the display controller.

>  > Doesn't it work to combine them into one call for each
>  > pair?
>  >> +       dsi_dcs_write_seq(dsi, );
>  >> +       dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19);
>
> By using a macro? We can... but I am not sure what (0x00, 0x80), (0x00,
> 0xa0),etc type of commands signify without the datasheet, so I am not
> sure what to name them in the macro and make any sensible meaning out of it.

I meant just sending dsi_generic_write_seq() with everything in
it:

dsi_generic_write_seq(dsi, 0x00, 0x80, 0xff, 0x87, 0x19);

Instead of two writes. Doesn't this work?

Yours,
Linus Walleij

Re: [PATCH 3/3] drm/panel: introduce ebbg,ft8719 panel

[Joel Selvaraj]

Hi Linus Walleij

On 19/05/22 14:39, Linus Walleij wrote:
> Nope. But add the rate limited error print please!

Will do.

>>> Lots of magic numbers. You don't have a datasheet do you?
>>> So you could #define some of the magic?
>>
>> Unfortunately, I don't have a datasheet and the power on sequence is
>> taken from downstream android dts. It works pretty well though. So I
>> don't think I can #define any of these magic.
> 
> If you know which display controller the display is using (usually
> Novatek nnnnn, Ilitek nnnn etc someting like that) there is often
> a datasheet for the display controller available but the display per
> se often obscures the display controller.

Well, I recently figured that the panel works perfectly without all the 
magic commands. So, no need for #defines of those magics/documentation 
for now.

>>   > Doesn't it work to combine them into one call for each
>>   > pair?
>>   >> +       dsi_dcs_write_seq(dsi, );
>>   >> +       dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19);
>>
>> By using a macro? We can... but I am not sure what (0x00, 0x80), (0x00,
>> 0xa0),etc type of commands signify without the datasheet, so I am not
>> sure what to name them in the macro and make any sensible meaning out of it.
> 
> I meant just sending dsi_generic_write_seq() with everything in
> it:
> 
> dsi_generic_write_seq(dsi, 0x00, 0x80, 0xff, 0x87, 0x19);
> 
> Instead of two writes. Doesn't this work?

I am not sure about whether it will work. Can multiple DCS commands can 
be combined into single write? Don't know much about this.

Anyways since the panel works without all these magic commands, these 
will be removed in the next version.

> Yours,
> Linus Walleij

Thanks and Regards
Joel Selvaraj