[DPU PATCH 1/2] drm/panel: Add Truly Dual DSI video mode panel

[DPU PATCH 1/2] drm/panel: Add Truly Dual DSI video mode panel

[Abhinav Kumar]

From: Archit Taneja <architt@codeaurora.org>

Add support for truly dual DSI video mode panel
panel used in MSM reference platforms.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 .../bindings/display/truly,dual_wqxga.txt          |  47 ++
 drivers/gpu/drm/panel/Kconfig                      |   7 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 drivers/gpu/drm/panel/panel-truly-dual-dsi.c       | 530 +++++++++++++++++++++
 4 files changed, 585 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
 create mode 100644 drivers/gpu/drm/panel/panel-truly-dual-dsi.c

diff --git a/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt b/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
new file mode 100644
index 0000000..a1b24c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
@@ -0,0 +1,47 @@
+Truly model NT35597 1440x2560 DSI Panel
+
+Required properties:
+- compatible: should be "truly,dual_wqxga"
+- vdda-supply: phandle of the regulator that provides the supply voltage
+  Power IC supply
+- lab-supply: phandle of the regulator that provides the supply voltage
+  for LCD bias
+- ibb-supply: phandle of the regulator that provides the supply voltage
+  for LCD bias
+- reset-gpios: phandle of gpio for reset line
+  This should be 8mA, gpio can be configured using mux, pinctrl, pinctrl-names
+- mode-gpios: phandle of the gpio for choosing the mode of the display
+  for single DSI or Dual DSI
+- display-timings: Node for the Panel timings
+- link2: phandle to the secondary node of the panel
+
+Example:
+
+	dsi@ae94000 {
+		panel@0 {
+			compatible = "truly,dual_wqxga";
+			reg = <0>;
+			link2 = <&link2>;
+			vdda-supply = <&pm8998_l14>;
+
+			pinctrl-names = "default", "suspend";
+			pinctrl-0 = <&sde_dsi_active>;
+			pinctrl-1 = <&sde_dsi_suspend>;
+
+			reset-gpios = <&tlmm 6 0>;
+			mode-gpios = <&tlmm 52 0>;
+			display-timings {
+				timing0: timing-0 {
+					clock-frequency = <268316138>;
+					hactive = <1440>;
+					vactive = <2560>;
+					hfront-porch = <200>;
+					hback-porch = <64>;
+					hsync-len = <32>;
+					vfront-porch = <8>;
+					vback-porch = <7>;
+					vsync-len = <1>;
+				};
+			};
+		};
+	};
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 988048e..a63c3f7 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -168,4 +168,11 @@ config DRM_PANEL_SITRONIX_ST7789V
 	  Say Y here if you want to enable support for the Sitronix
 	  ST7789V controller for 240x320 LCD panels
 
+config DRM_PANEL_TRULY_WQXGA
+	tristate "Truly WQXGA"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for Truly WQXGA Dual DSI
+	  Video Mode panel
 endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 3d2a88d..64891f6 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
 obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
 obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
 obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
+obj-$(CONFIG_DRM_PANEL_TRULY_WQXGA) += panel-truly-dual-dsi.o
diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
new file mode 100644
index 0000000..47891ee
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
@@ -0,0 +1,530 @@
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/pinctrl/consumer.h>
+
+#include <video/mipi_display.h>
+#include <video/of_videomode.h>
+#include <video/videomode.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_mipi_dsi.h>
+
+struct truly_wqxga {
+	struct device *dev;
+	struct drm_panel panel;
+
+	struct regulator_bulk_data supplies[4];
+
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *mode_gpio;
+
+	struct backlight_device *backlight;
+	struct videomode vm;
+
+	struct mipi_dsi_device *dsi[2];
+
+	bool prepared;
+	bool enabled;
+};
+
+static inline struct truly_wqxga *panel_to_truly_wqxga(struct drm_panel *panel)
+{
+	return container_of(panel, struct truly_wqxga, panel);
+}
+
+static int truly_wqxga_power_on(struct truly_wqxga *ctx)
+{
+	int ret;
+
+	ret = regulator_set_load(ctx->supplies[1].consumer, 62000);
+	if (ret)
+		return ret;
+
+	ret = regulator_set_load(ctx->supplies[2].consumer, 100000);
+	if (ret)
+		return ret;
+
+	ret = regulator_set_load(ctx->supplies[3].consumer, 100000);
+	if (ret)
+		return ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (ret < 0)
+		return ret;
+
+	msleep(20);
+	gpiod_set_value(ctx->reset_gpio, 1);
+	usleep_range(20000, 20100);
+	gpiod_set_value(ctx->reset_gpio, 0);
+	usleep_range(20000, 20100);
+	gpiod_set_value(ctx->reset_gpio, 1);
+	usleep_range(50000, 50100);
+
+	/* dual port */
+	gpiod_set_value(ctx->mode_gpio, 0);
+
+	return 0;
+}
+
+static int truly_wqxga_power_off(struct truly_wqxga *ctx)
+{
+	int ret;
+
+	gpiod_set_value(ctx->reset_gpio, 0);
+	ret = regulator_set_load(ctx->supplies[1].consumer, 62000);
+	if (ret)
+		return ret;
+
+	return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+}
+
+static int truly_wqxga_disable(struct drm_panel *panel)
+{
+	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
+
+	if (!ctx->enabled)
+		return 0;
+
+	if (ctx->backlight) {
+		ctx->backlight->props.power = FB_BLANK_POWERDOWN;
+		backlight_update_status(ctx->backlight);
+	}
+
+	ctx->enabled = false;
+	return 0;
+}
+
+static int truly_wqxga_unprepare(struct drm_panel *panel)
+{
+	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
+	struct mipi_dsi_device **dsis = ctx->dsi;
+	int ret = 0, i;
+
+	if (!ctx->prepared)
+		return 0;
+
+	dsis[0]->mode_flags = 0;
+	dsis[1]->mode_flags = 0;
+
+	for (i = 0; i < 2; i++)
+		if (mipi_dsi_dcs_set_display_off(dsis[i]) < 0)
+			ret = -ECOMM;
+	msleep(78);
+
+	for (i = 0; i < 2; i++)
+		if (mipi_dsi_dcs_enter_sleep_mode(dsis[i]) < 0)
+			ret = -ECOMM;
+	msleep(78);
+
+	truly_wqxga_power_off(ctx);
+
+	ctx->prepared = false;
+	return ret;
+}
+
+#define MAX_LEN	5
+struct {
+	u8 commands[MAX_LEN];
+	int size;
+} panel_cmds[] = { /* CMD2_P0 */
+		   { { 0xff, 0x20 }, 2 },
+		   { { 0xfb, 0x01 }, 2 },
+		   { { 0x00, 0x01 }, 2 },
+		   { { 0x01, 0x55 }, 2 },
+		   { { 0x02, 0x45 }, 2 },
+		   { { 0x05, 0x40 }, 2 },
+		   { { 0x06, 0x19 }, 2 },
+		   { { 0x07, 0x1e }, 2 },
+		   { { 0x0b, 0x73 }, 2 },
+		   { { 0x0c, 0x73 }, 2 },
+		   { { 0x0e, 0xb0 }, 2 },
+		   { { 0x0f, 0xae }, 2 },
+		   { { 0x11, 0xb8 }, 2 },
+		   { { 0x13, 0x00 }, 2 },
+		   { { 0x58, 0x80 }, 2 },
+		   { { 0x59, 0x01 }, 2 },
+		   { { 0x5a, 0x00 }, 2 },
+		   { { 0x5b, 0x01 }, 2 },
+		   { { 0x5c, 0x80 }, 2 },
+		   { { 0x5d, 0x81 }, 2 },
+		   { { 0x5e, 0x00 }, 2 },
+		   { { 0x5f, 0x01 }, 2 },
+		   { { 0x72, 0x11 }, 2 },
+		   { { 0x68, 0x03 }, 2 },
+		   /* CMD2_P4 */
+		   { { 0xFF, 0x24 }, 2 },
+		   { { 0xFB, 0x01 }, 2 },
+		   { { 0x00, 0x1C }, 2 },
+		   { { 0x01, 0x0B }, 2 },
+		   { { 0x02, 0x0C }, 2 },
+		   { { 0x03, 0x01 }, 2 },
+		   { { 0x04, 0x0F }, 2 },
+		   { { 0x05, 0x10 }, 2 },
+		   { { 0x06, 0x10 }, 2 },
+		   { { 0x07, 0x10 }, 2 },
+		   { { 0x08, 0x89 }, 2 },
+		   { { 0x09, 0x8A }, 2 },
+		   { { 0x0A, 0x13 }, 2 },
+		   { { 0x0B, 0x13 }, 2 },
+		   { { 0x0C, 0x15 }, 2 },
+		   { { 0x0D, 0x15 }, 2 },
+		   { { 0x0E, 0x17 }, 2 },
+		   { { 0x0F, 0x17 }, 2 },
+		   { { 0x10, 0x1C }, 2 },
+		   { { 0x11, 0x0B }, 2 },
+		   { { 0x12, 0x0C }, 2 },
+		   { { 0x13, 0x01 }, 2 },
+		   { { 0x14, 0x0F }, 2 },
+		   { { 0x15, 0x10 }, 2 },
+		   { { 0x16, 0x10 }, 2 },
+		   { { 0x17, 0x10 }, 2 },
+		   { { 0x18, 0x89 }, 2 },
+		   { { 0x19, 0x8A }, 2 },
+		   { { 0x1A, 0x13 }, 2 },
+		   { { 0x1B, 0x13 }, 2 },
+		   { { 0x1C, 0x15 }, 2 },
+		   { { 0x1D, 0x15 }, 2 },
+		   { { 0x1E, 0x17 }, 2 },
+		   { { 0x1F, 0x17 }, 2 },
+		   /* STV */
+		   { { 0x20, 0x40 }, 2 },
+		   { { 0x21, 0x01 }, 2 },
+		   { { 0x22, 0x00 }, 2 },
+		   { { 0x23, 0x40 }, 2 },
+		   { { 0x24, 0x40 }, 2 },
+		   { { 0x25, 0x6D }, 2 },
+		   { { 0x26, 0x40 }, 2 },
+		   { { 0x27, 0x40 }, 2 },
+		   /* Vend */
+		   { { 0xE0, 0x00 }, 2 },
+		   { { 0xDC, 0x21 }, 2 },
+		   { { 0xDD, 0x22 }, 2 },
+		   { { 0xDE, 0x07 }, 2 },
+		   { { 0xDF, 0x07 }, 2 },
+		   { { 0xE3, 0x6D }, 2 },
+		   { { 0xE1, 0x07 }, 2 },
+		   { { 0xE2, 0x07 }, 2 },
+		   /* UD */
+		   { { 0x29, 0xD8 }, 2 },
+		   { { 0x2A, 0x2A }, 2 },
+		   /* CLK */
+		   { { 0x4B, 0x03 }, 2 },
+		   { { 0x4C, 0x11 }, 2 },
+		   { { 0x4D, 0x10 }, 2 },
+		   { { 0x4E, 0x01 }, 2 },
+		   { { 0x4F, 0x01 }, 2 },
+		   { { 0x50, 0x10 }, 2 },
+		   { { 0x51, 0x00 }, 2 },
+		   { { 0x52, 0x80 }, 2 },
+		   { { 0x53, 0x00 }, 2 },
+		   { { 0x56, 0x00 }, 2 },
+		   { { 0x54, 0x07 }, 2 },
+		   { { 0x58, 0x07 }, 2 },
+		   { { 0x55, 0x25 }, 2 },
+		   /* Reset XDONB */
+		   { { 0x5B, 0x43 }, 2 },
+		   { { 0x5C, 0x00 }, 2 },
+		   { { 0x5F, 0x73 }, 2 },
+		   { { 0x60, 0x73 }, 2 },
+		   { { 0x63, 0x22 }, 2 },
+		   { { 0x64, 0x00 }, 2 },
+		   { { 0x67, 0x08 }, 2 },
+		   { { 0x68, 0x04 }, 2 },
+		   /* Resolution:1440x2560 */
+		   { { 0x72, 0x02 }, 2 },
+		   /* mux */
+		   { { 0x7A, 0x80 }, 2 },
+		   { { 0x7B, 0x91 }, 2 },
+		   { { 0x7C, 0xD8 }, 2 },
+		   { { 0x7D, 0x60 }, 2 },
+		   { { 0x7F, 0x15 }, 2 },
+		   { { 0x75, 0x15 }, 2 },
+		   /* ABOFF */
+		   { { 0xB3, 0xC0 }, 2 },
+		   { { 0xB4, 0x00 }, 2 },
+		   { { 0xB5, 0x00 }, 2 },
+		   /* Source EQ */
+		   { { 0x78, 0x00 }, 2 },
+		   { { 0x79, 0x00 }, 2 },
+		   { { 0x80, 0x00 }, 2 },
+		   { { 0x83, 0x00 }, 2 },
+		   /* FP BP */
+		   { { 0x93, 0x0A }, 2 },
+		   { { 0x94, 0x0A }, 2 },
+		   /* Inversion Type */
+		   { { 0x8A, 0x00 }, 2 },
+		   { { 0x9B, 0xFF }, 2 },
+		   /* IMGSWAP =1 @PortSwap=1 */
+		   { { 0x9D, 0xB0 }, 2 },
+		   { { 0x9F, 0x63 }, 2 },
+		   { { 0x98, 0x10 }, 2 },
+		   /* FRM */
+		   { { 0xEC, 0x00 }, 2 },
+		   /* CMD1 */
+		   { { 0xFF, 0x10 }, 2 },
+		    /* VBP+VSA=,VFP = 10H */
+		   { { 0x3B, 0x03, 0x0A, 0x0A, }, 4 },
+		   /* FTE on */
+		   { { 0x35, 0x00 }, 2 },
+		   /* EN_BK =1(auto black) */
+		   { { 0xE5, 0x01 }, 2 },
+		   /* CMD mode(10) VDO mode(03) */
+		   { { 0xBB, 0x03 }, 2 },
+		   /* Non Reload MTP */
+		   { { 0xFB, 0x01 }, 2 },
+		   /* SlpOut + DispOn */
+		   //05 01 00 00 78 00 02 11 00
+		   //05 01 00 00 78 00 02 29 00
+};
+
+static int truly_wqxga_prepare(struct drm_panel *panel)
+{
+	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
+	struct mipi_dsi_device **dsis = ctx->dsi;
+	struct mipi_dsi_device *d;
+	int ret, i, j;
+
+	if (ctx->prepared)
+		return 0;
+
+	ret = truly_wqxga_power_on(ctx);
+	if (ret < 0)
+		return ret;
+
+	dsis[0]->mode_flags |= MIPI_DSI_MODE_LPM;
+	dsis[1]->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	for (j = 0; j < ARRAY_SIZE(panel_cmds); j++) {
+		for (i = 0; i < 2; i++) {
+			d = dsis[i];
+			ret = mipi_dsi_dcs_write_buffer(dsis[i],
+					panel_cmds[j].commands,
+					panel_cmds[j].size);
+			if (ret < 0) {
+				/* continue anyway */
+				dev_err(ctx->dev, "failed to cmd no %d, err: %d\n",
+						j, ret);
+			}
+		}
+	}
+
+
+	for (i = 0; i < 2; i++)
+		if (mipi_dsi_dcs_exit_sleep_mode(dsis[i]) < 0) {
+			dev_err(ctx->dev, "failed to exit sleep mode\n");
+			return -ECOMM;
+		}
+	msleep(78);
+
+	for (i = 0; i < 2; i++)
+		if (mipi_dsi_dcs_set_display_on(dsis[i]) < 0) {
+			dev_err(ctx->dev, "failed to send display on\n");
+			return -ECOMM;
+		}
+	msleep(78);
+
+	ctx->prepared = true;
+
+	return 0;
+}
+
+static int truly_wqxga_enable(struct drm_panel *panel)
+{
+	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
+
+	if (ctx->enabled)
+		return 0;
+
+	if (ctx->backlight) {
+		ctx->backlight->props.power = FB_BLANK_UNBLANK;
+		backlight_update_status(ctx->backlight);
+	}
+	ctx->enabled = true;
+
+	return 0;
+}
+
+static int truly_wqxga_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_create(connector->dev);
+	if (!mode) {
+		dev_err(ctx->dev, "failed to create a new display mode\n");
+		return 0;
+	}
+
+	drm_display_mode_from_videomode(&ctx->vm, mode);
+	connector->display_info.width_mm = 74;
+	connector->display_info.height_mm = 131;
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs truly_wqxga_drm_funcs = {
+	.disable = truly_wqxga_disable,
+	.unprepare = truly_wqxga_unprepare,
+	.prepare = truly_wqxga_prepare,
+	.enable = truly_wqxga_enable,
+	.get_modes = truly_wqxga_get_modes,
+};
+
+static int truly_wqxga_panel_add(struct truly_wqxga *ctx)
+{
+	struct device *dev = ctx->dev;
+	int ret;
+
+	ctx->supplies[0].supply = "vddio";
+	ctx->supplies[1].supply = "vdda";
+	ctx->supplies[2].supply = "lab";
+	ctx->supplies[3].supply = "ibb";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
+				      ctx->supplies);
+	if (ret < 0)
+		return ret;
+
+	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ctx->reset_gpio)) {
+		dev_err(dev, "cannot get reset-gpios %ld\n",
+			PTR_ERR(ctx->reset_gpio));
+		return PTR_ERR(ctx->reset_gpio);
+	}
+
+	ctx->mode_gpio = devm_gpiod_get(dev, "mode", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->mode_gpio)) {
+		dev_err(dev, "cannot get mode gpio %ld\n",
+			PTR_ERR(ctx->mode_gpio));
+		ctx->mode_gpio = NULL;
+		return PTR_ERR(ctx->mode_gpio);
+	}
+
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret) {
+		dev_err(dev, "%s: failed to set pinctrl default state, %d\n",
+			__func__, ret);
+		return ret;
+	}
+
+	ret = of_get_videomode(dev->of_node, &ctx->vm, 0);
+	if (ret < 0)
+		return ret;
+
+	drm_panel_init(&ctx->panel);
+	ctx->panel.dev = dev;
+	ctx->panel.funcs = &truly_wqxga_drm_funcs;
+	drm_panel_add(&ctx->panel);
+
+	return 0;
+}
+
+static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
+{
+	if (ctx->panel.dev)
+		drm_panel_remove(&ctx->panel);
+
+	if (ctx->backlight)
+		put_device(&ctx->backlight->dev);
+
+	if (ctx->dsi[1])
+		put_device(&ctx->dsi[1]->dev);
+}
+
+static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct truly_wqxga *ctx;
+	struct mipi_dsi_device *secondary = NULL;
+	struct device_node *np;
+	int ret;
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS |
+			  MIPI_DSI_MODE_LPM;
+
+	/* Only DSI-LINK1 node has "link2" entry. */
+	np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
+	if (np) {
+		secondary = of_find_mipi_dsi_device_by_node(np);
+		of_node_put(np);
+
+		if (!secondary)
+			return -EPROBE_DEFER;
+
+		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+		if (!ctx) {
+			put_device(&secondary->dev);
+			return -ENOMEM;
+		}
+		mipi_dsi_set_drvdata(dsi, ctx);
+
+		ctx->dev = dev;
+		ctx->dsi[0] = dsi;
+		ctx->dsi[1] = secondary;
+
+		ret = truly_wqxga_panel_add(ctx);
+		if (ret) {
+			put_device(&secondary->dev);
+			return ret;
+		}
+	}
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		if (secondary)
+			truly_wqxga_panel_del(ctx);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
+{
+	struct truly_wqxga *ctx = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+
+	/* delete panel only for the DSI1 interface */
+	if (ctx)
+		truly_wqxga_panel_del(ctx);
+
+	return 0;
+}
+
+static const struct of_device_id truly_wqxga_of_match[] = {
+	{ .compatible = "truly,dual_wqxga", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, truly_wqxga_of_match);
+
+static struct mipi_dsi_driver truly_wqxga_driver = {
+	.driver = {
+		.name = "panel_truly_wqxga",
+		.of_match_table = truly_wqxga_of_match,
+	},
+	.probe = truly_wqxga_probe,
+	.remove = truly_wqxga_remove,
+};
+module_mipi_dsi_driver(truly_wqxga_driver);
+
+MODULE_DESCRIPTION("truly wqxga DSI Panel Driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[DPU PATCH 2/2] drm/panel: add backlight control support for truly panel

[Abhinav Kumar]

Register truly panel as a backlight led device and
provide methods to control its backlight operation.

Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/panel/panel-truly-dual-dsi.c | 96 +++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
index 47891ee..5d0ef90 100644
--- a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
+++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
@@ -14,6 +14,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/leds.h>
 
 #include <video/mipi_display.h>
 #include <video/of_videomode.h>
@@ -23,6 +24,9 @@
 #include <drm/drm_panel.h>
 #include <drm/drm_mipi_dsi.h>
 
+#define BL_NODE_NAME_SIZE 32
+#define PRIM_DISPLAY_NODE 0
+
 struct truly_wqxga {
 	struct device *dev;
 	struct drm_panel panel;
@@ -33,6 +37,8 @@ struct truly_wqxga {
 	struct gpio_desc *mode_gpio;
 
 	struct backlight_device *backlight;
+	/* WLED params */
+	struct led_trigger *wled;
 	struct videomode vm;
 
 	struct mipi_dsi_device *dsi[2];
@@ -447,6 +453,83 @@ static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
 		put_device(&ctx->dsi[1]->dev);
 }
 
+static int truly_backlight_device_update_status(struct backlight_device *bd)
+{
+	int brightness;
+	int max_brightness;
+	int rc = 0;
+
+	struct truly_wqxga *ctx = dev_get_drvdata(&bd->dev);
+
+	brightness = bd->props.brightness;
+	max_brightness = bd->props.max_brightness;
+
+	if ((bd->props.power != FB_BLANK_UNBLANK) ||
+		(bd->props.state & BL_CORE_FBBLANK) ||
+		  (bd->props.state & BL_CORE_SUSPENDED))
+		brightness = 0;
+
+	if (brightness > max_brightness)
+		brightness = max_brightness;
+
+	/* Need to check WLED driver capability upstream */
+	if (ctx && ctx->wled)
+		led_trigger_event(ctx->wled, brightness);
+
+	return rc;
+}
+
+static int truly_backlight_device_get_brightness(struct backlight_device *bd)
+{
+	return bd->props.brightness;
+}
+
+static const struct backlight_ops truly_backlight_device_ops = {
+	.update_status = truly_backlight_device_update_status,
+	.get_brightness = truly_backlight_device_get_brightness,
+};
+
+static int truly_backlight_setup(struct truly_wqxga *ctx)
+{
+	struct backlight_properties props;
+	char bl_node_name[BL_NODE_NAME_SIZE];
+
+	if (!ctx) {
+		dev_err(ctx->dev, "invalid context\n");
+		return -EINVAL;
+	}
+
+	if (!ctx->backlight) {
+		memset(&props, 0, sizeof(props));
+		props.type = BACKLIGHT_RAW;
+		props.power = FB_BLANK_UNBLANK;
+		props.max_brightness = 4096;
+
+		snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
+				 PRIM_DISPLAY_NODE);
+
+		ctx->backlight =  backlight_device_register(bl_node_name,
+				ctx->dev, ctx,
+				&truly_backlight_device_ops, &props);
+
+		if (IS_ERR_OR_NULL(ctx->backlight)) {
+			pr_err("Failed to register backlight\n");
+			ctx->backlight = NULL;
+			return -ENODEV;
+		}
+
+		/* Register with the LED driver interface */
+		led_trigger_register_simple("bkl-trigger", &ctx->wled);
+
+		if (!ctx->wled) {
+			pr_err("backlight led registration failed\n");
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
+
 static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
 {
 	struct device *dev = &dsi->dev;
@@ -466,10 +549,11 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
 		secondary = of_find_mipi_dsi_device_by_node(np);
 		of_node_put(np);
 
+		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+
 		if (!secondary)
 			return -EPROBE_DEFER;
 
-		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 		if (!ctx) {
 			put_device(&secondary->dev);
 			return -ENOMEM;
@@ -485,6 +569,12 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
 			put_device(&secondary->dev);
 			return ret;
 		}
+
+		ret = truly_backlight_setup(ctx);
+		if (ret) {
+			put_device(&secondary->dev);
+			return ret;
+		}
 	}
 
 	ret = mipi_dsi_attach(dsi);
@@ -504,8 +594,10 @@ static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
 	mipi_dsi_detach(dsi);
 
 	/* delete panel only for the DSI1 interface */
-	if (ctx)
+	if (ctx) {
 		truly_wqxga_panel_del(ctx);
+		kfree(ctx);
+	}
 
 	return 0;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH 1/2] drm/panel: Add Truly Dual DSI video mode panel

[Archit Taneja]

Hi Abhinav,

Thanks for posting this driver. Some comments below.

On Saturday 07 April 2018 12:36 PM, Abhinav Kumar wrote:
> From: Archit Taneja <architt@codeaurora.org>
> 
> Add support for truly dual DSI video mode panel
> panel used in MSM reference platforms >
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>   .../bindings/display/truly,dual_wqxga.txt          |  47 ++
>   drivers/gpu/drm/panel/Kconfig                      |   7 +
>   drivers/gpu/drm/panel/Makefile                     |   1 +
>   drivers/gpu/drm/panel/panel-truly-dual-dsi.c       | 530 +++++++++++++++++++++
>   4 files changed, 585 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
>   create mode 100644 drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> 
> diff --git a/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt b/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
> new file mode 100644
> index 0000000..a1b24c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
> @@ -0,0 +1,47 @@
> +Truly model NT35597 1440x2560 DSI Panel
> +
> +Required properties:
> +- compatible: should be "truly,dual_wqxga"

The compatible string, kernel config and the driver file should be based
on the panel model no. There can be many truly based panels that
support wqxga. Something like "truly,nt35597" would be better.

> +- vdda-supply: phandle of the regulator that provides the supply voltage
> +  Power IC supply
> +- lab-supply: phandle of the regulator that provides the supply voltage
> +  for LCD bias
> +- ibb-supply: phandle of the regulator that provides the supply voltage
> +  for LCD bias

Both seem to have the same description. Aren't lab and ibb qualcomm
specific terms? Could we use the pin names specified in the panel's
data sheet?

> +- reset-gpios: phandle of gpio for reset line
> +  This should be 8mA, gpio can be configured using mux, pinctrl, pinctrl-names
> +- mode-gpios: phandle of the gpio for choosing the mode of the display
> +  for single DSI or Dual DSI

Could we describe here how to use this gpio? I.e, whether we need to set
it to low for dual DSI, etc?

> +- display-timings: Node for the Panel timings
> +- link2: phandle to the secondary node of the panel

The link2 binding was a temporary hack we used. We should use the
of-graph bindings to represent the two DSI input ports of the panel.

> +
> +Example:
> +
> +	dsi@ae94000 {
> +		panel@0 {
> +			compatible = "truly,dual_wqxga";
> +			reg = <0>;
> +			link2 = <&link2>;
> +			vdda-supply = <&pm8998_l14>;
> +
> +			pinctrl-names = "default", "suspend";
> +			pinctrl-0 = <&sde_dsi_active>;
> +			pinctrl-1 = <&sde_dsi_suspend>;
> +
> +			reset-gpios = <&tlmm 6 0>;
> +			mode-gpios = <&tlmm 52 0>;
> +			display-timings {
> +				timing0: timing-0 {
> +					clock-frequency = <268316138>;
> +					hactive = <1440>;
> +					vactive = <2560>;
> +					hfront-porch = <200>;
> +					hback-porch = <64>;
> +					hsync-len = <32>;
> +					vfront-porch = <8>;
> +					vback-porch = <7>;
> +					vsync-len = <1>;
> +				};
> +			};
> +		};
> +	};
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 988048e..a63c3f7 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -168,4 +168,11 @@ config DRM_PANEL_SITRONIX_ST7789V
>   	  Say Y here if you want to enable support for the Sitronix
>   	  ST7789V controller for 240x320 LCD panels
>   
> +config DRM_PANEL_TRULY_WQXGA
> +	tristate "Truly WQXGA"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	help
> +	  Say Y here if you want to enable support for Truly WQXGA Dual DSI
> +	  Video Mode panel
>   endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 3d2a88d..64891f6 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
>   obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
>   obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> +obj-$(CONFIG_DRM_PANEL_TRULY_WQXGA) += panel-truly-dual-dsi.o
> diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> new file mode 100644
> index 0000000..47891ee
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> @@ -0,0 +1,530 @@
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.

We should use the SPDX license headers here.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/pinctrl/consumer.h>
> +
> +#include <video/mipi_display.h>
> +#include <video/of_videomode.h>
> +#include <video/videomode.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_mipi_dsi.h>
> +
> +struct truly_wqxga {
> +	struct device *dev;
> +	struct drm_panel panel;
> +
> +	struct regulator_bulk_data supplies[4];
> +
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *mode_gpio;
> +
> +	struct backlight_device *backlight;
> +	struct videomode vm;
> +
> +	struct mipi_dsi_device *dsi[2];
> +
> +	bool prepared;
> +	bool enabled;
> +};
> +
> +static inline struct truly_wqxga *panel_to_truly_wqxga(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct truly_wqxga, panel);
> +}
> +
> +static int truly_wqxga_power_on(struct truly_wqxga *ctx)
> +{
> +	int ret;
> +
> +	ret = regulator_set_load(ctx->supplies[1].consumer, 62000);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_set_load(ctx->supplies[2].consumer, 100000);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_set_load(ctx->supplies[3].consumer, 100000);
> +	if (ret)
> +		return ret;

We could make a const static struct array specifying the regulator
names (to be used during probe) and their corresponding power on and
power off loads. That should make things a bit cleaner.

> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(20);
> +	gpiod_set_value(ctx->reset_gpio, 1);
> +	usleep_range(20000, 20100);
> +	gpiod_set_value(ctx->reset_gpio, 0);
> +	usleep_range(20000, 20100);
> +	gpiod_set_value(ctx->reset_gpio, 1);
> +	usleep_range(50000, 50100);
> +
> +	/* dual port */
> +	gpiod_set_value(ctx->mode_gpio, 0);
> +
> +	return 0;
> +}
> +
> +static int truly_wqxga_power_off(struct truly_wqxga *ctx)
> +{
> +	int ret;
> +
> +	gpiod_set_value(ctx->reset_gpio, 0);
> +	ret = regulator_set_load(ctx->supplies[1].consumer, 62000);
> +	if (ret)
> +		return ret;
> +
> +	return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +}
> +
> +static int truly_wqxga_disable(struct drm_panel *panel)
> +{
> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
> +
> +	if (!ctx->enabled)
> +		return 0;
> +
> +	if (ctx->backlight) {
> +		ctx->backlight->props.power = FB_BLANK_POWERDOWN;
> +		backlight_update_status(ctx->backlight);
> +	}
> +
> +	ctx->enabled = false;
> +	return 0;
> +}
> +
> +static int truly_wqxga_unprepare(struct drm_panel *panel)
> +{
> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
> +	struct mipi_dsi_device **dsis = ctx->dsi;
> +	int ret = 0, i;
> +
> +	if (!ctx->prepared)
> +		return 0;
> +
> +	dsis[0]->mode_flags = 0;
> +	dsis[1]->mode_flags = 0;
> +
> +	for (i = 0; i < 2; i++)
> +		if (mipi_dsi_dcs_set_display_off(dsis[i]) < 0)
> +			ret = -ECOMM;
> +	msleep(78);
> +
> +	for (i = 0; i < 2; i++)
> +		if (mipi_dsi_dcs_enter_sleep_mode(dsis[i]) < 0)
> +			ret = -ECOMM;
> +	msleep(78);
> +
> +	truly_wqxga_power_off(ctx);
> +
> +	ctx->prepared = false;
> +	return ret;
> +}
> +
> +#define MAX_LEN	5
> +struct {
> +	u8 commands[MAX_LEN];
> +	int size;
> +} panel_cmds[] = { /* CMD2_P0 */
> +		   { { 0xff, 0x20 }, 2 },
> +		   { { 0xfb, 0x01 }, 2 },
> +		   { { 0x00, 0x01 }, 2 },
> +		   { { 0x01, 0x55 }, 2 },
> +		   { { 0x02, 0x45 }, 2 },
> +		   { { 0x05, 0x40 }, 2 },
> +		   { { 0x06, 0x19 }, 2 },
> +		   { { 0x07, 0x1e }, 2 },
> +		   { { 0x0b, 0x73 }, 2 },
> +		   { { 0x0c, 0x73 }, 2 },
> +		   { { 0x0e, 0xb0 }, 2 },
> +		   { { 0x0f, 0xae }, 2 },
> +		   { { 0x11, 0xb8 }, 2 },
> +		   { { 0x13, 0x00 }, 2 },
> +		   { { 0x58, 0x80 }, 2 },
> +		   { { 0x59, 0x01 }, 2 },
> +		   { { 0x5a, 0x00 }, 2 },
> +		   { { 0x5b, 0x01 }, 2 },
> +		   { { 0x5c, 0x80 }, 2 },
> +		   { { 0x5d, 0x81 }, 2 },
> +		   { { 0x5e, 0x00 }, 2 },
> +		   { { 0x5f, 0x01 }, 2 },
> +		   { { 0x72, 0x11 }, 2 },
> +		   { { 0x68, 0x03 }, 2 },
> +		   /* CMD2_P4 */
> +		   { { 0xFF, 0x24 }, 2 },
> +		   { { 0xFB, 0x01 }, 2 },
> +		   { { 0x00, 0x1C }, 2 },
> +		   { { 0x01, 0x0B }, 2 },
> +		   { { 0x02, 0x0C }, 2 },
> +		   { { 0x03, 0x01 }, 2 },
> +		   { { 0x04, 0x0F }, 2 },
> +		   { { 0x05, 0x10 }, 2 },
> +		   { { 0x06, 0x10 }, 2 },
> +		   { { 0x07, 0x10 }, 2 },
> +		   { { 0x08, 0x89 }, 2 },
> +		   { { 0x09, 0x8A }, 2 },
> +		   { { 0x0A, 0x13 }, 2 },
> +		   { { 0x0B, 0x13 }, 2 },
> +		   { { 0x0C, 0x15 }, 2 },
> +		   { { 0x0D, 0x15 }, 2 },
> +		   { { 0x0E, 0x17 }, 2 },
> +		   { { 0x0F, 0x17 }, 2 },
> +		   { { 0x10, 0x1C }, 2 },
> +		   { { 0x11, 0x0B }, 2 },
> +		   { { 0x12, 0x0C }, 2 },
> +		   { { 0x13, 0x01 }, 2 },
> +		   { { 0x14, 0x0F }, 2 },
> +		   { { 0x15, 0x10 }, 2 },
> +		   { { 0x16, 0x10 }, 2 },
> +		   { { 0x17, 0x10 }, 2 },
> +		   { { 0x18, 0x89 }, 2 },
> +		   { { 0x19, 0x8A }, 2 },
> +		   { { 0x1A, 0x13 }, 2 },
> +		   { { 0x1B, 0x13 }, 2 },
> +		   { { 0x1C, 0x15 }, 2 },
> +		   { { 0x1D, 0x15 }, 2 },
> +		   { { 0x1E, 0x17 }, 2 },
> +		   { { 0x1F, 0x17 }, 2 },
> +		   /* STV */
> +		   { { 0x20, 0x40 }, 2 },
> +		   { { 0x21, 0x01 }, 2 },
> +		   { { 0x22, 0x00 }, 2 },
> +		   { { 0x23, 0x40 }, 2 },
> +		   { { 0x24, 0x40 }, 2 },
> +		   { { 0x25, 0x6D }, 2 },
> +		   { { 0x26, 0x40 }, 2 },
> +		   { { 0x27, 0x40 }, 2 },
> +		   /* Vend */
> +		   { { 0xE0, 0x00 }, 2 },
> +		   { { 0xDC, 0x21 }, 2 },
> +		   { { 0xDD, 0x22 }, 2 },
> +		   { { 0xDE, 0x07 }, 2 },
> +		   { { 0xDF, 0x07 }, 2 },
> +		   { { 0xE3, 0x6D }, 2 },
> +		   { { 0xE1, 0x07 }, 2 },
> +		   { { 0xE2, 0x07 }, 2 },
> +		   /* UD */
> +		   { { 0x29, 0xD8 }, 2 },
> +		   { { 0x2A, 0x2A }, 2 },
> +		   /* CLK */
> +		   { { 0x4B, 0x03 }, 2 },
> +		   { { 0x4C, 0x11 }, 2 },
> +		   { { 0x4D, 0x10 }, 2 },
> +		   { { 0x4E, 0x01 }, 2 },
> +		   { { 0x4F, 0x01 }, 2 },
> +		   { { 0x50, 0x10 }, 2 },
> +		   { { 0x51, 0x00 }, 2 },
> +		   { { 0x52, 0x80 }, 2 },
> +		   { { 0x53, 0x00 }, 2 },
> +		   { { 0x56, 0x00 }, 2 },
> +		   { { 0x54, 0x07 }, 2 },
> +		   { { 0x58, 0x07 }, 2 },
> +		   { { 0x55, 0x25 }, 2 },
> +		   /* Reset XDONB */
> +		   { { 0x5B, 0x43 }, 2 },
> +		   { { 0x5C, 0x00 }, 2 },
> +		   { { 0x5F, 0x73 }, 2 },
> +		   { { 0x60, 0x73 }, 2 },
> +		   { { 0x63, 0x22 }, 2 },
> +		   { { 0x64, 0x00 }, 2 },
> +		   { { 0x67, 0x08 }, 2 },
> +		   { { 0x68, 0x04 }, 2 },
> +		   /* Resolution:1440x2560 */
> +		   { { 0x72, 0x02 }, 2 },
> +		   /* mux */
> +		   { { 0x7A, 0x80 }, 2 },
> +		   { { 0x7B, 0x91 }, 2 },
> +		   { { 0x7C, 0xD8 }, 2 },
> +		   { { 0x7D, 0x60 }, 2 },
> +		   { { 0x7F, 0x15 }, 2 },
> +		   { { 0x75, 0x15 }, 2 },
> +		   /* ABOFF */
> +		   { { 0xB3, 0xC0 }, 2 },
> +		   { { 0xB4, 0x00 }, 2 },
> +		   { { 0xB5, 0x00 }, 2 },
> +		   /* Source EQ */
> +		   { { 0x78, 0x00 }, 2 },
> +		   { { 0x79, 0x00 }, 2 },
> +		   { { 0x80, 0x00 }, 2 },
> +		   { { 0x83, 0x00 }, 2 },
> +		   /* FP BP */
> +		   { { 0x93, 0x0A }, 2 },
> +		   { { 0x94, 0x0A }, 2 },
> +		   /* Inversion Type */
> +		   { { 0x8A, 0x00 }, 2 },
> +		   { { 0x9B, 0xFF }, 2 },
> +		   /* IMGSWAP =1 @PortSwap=1 */
> +		   { { 0x9D, 0xB0 }, 2 },
> +		   { { 0x9F, 0x63 }, 2 },
> +		   { { 0x98, 0x10 }, 2 },
> +		   /* FRM */
> +		   { { 0xEC, 0x00 }, 2 },
> +		   /* CMD1 */
> +		   { { 0xFF, 0x10 }, 2 },
> +		    /* VBP+VSA=,VFP = 10H */
> +		   { { 0x3B, 0x03, 0x0A, 0x0A, }, 4 },
> +		   /* FTE on */
> +		   { { 0x35, 0x00 }, 2 },
> +		   /* EN_BK =1(auto black) */
> +		   { { 0xE5, 0x01 }, 2 },
> +		   /* CMD mode(10) VDO mode(03) */
> +		   { { 0xBB, 0x03 }, 2 },
> +		   /* Non Reload MTP */
> +		   { { 0xFB, 0x01 }, 2 },
> +		   /* SlpOut + DispOn */
> +		   //05 01 00 00 78 00 02 11 00
> +		   //05 01 00 00 78 00 02 29 00

We can drop the commented lines. If possible, it would be nice
to have some more description about the registers, but I can imagine
that info being hard to retrieve.

> +};
> +
> +static int truly_wqxga_prepare(struct drm_panel *panel)
> +{
> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
> +	struct mipi_dsi_device **dsis = ctx->dsi;
> +	struct mipi_dsi_device *d;
> +	int ret, i, j;
> +
> +	if (ctx->prepared)
> +		return 0;
> +
> +	ret = truly_wqxga_power_on(ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsis[0]->mode_flags |= MIPI_DSI_MODE_LPM;
> +	dsis[1]->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	for (j = 0; j < ARRAY_SIZE(panel_cmds); j++) {
> +		for (i = 0; i < 2; i++) {
> +			d = dsis[i];
> +			ret = mipi_dsi_dcs_write_buffer(dsis[i],
> +					panel_cmds[j].commands,
> +					panel_cmds[j].size);
> +			if (ret < 0) {
> +				/* continue anyway */
> +				dev_err(ctx->dev, "failed to cmd no %d, err: %d\n",
> +						j, ret);

This may have been done for debug purposes. It would be probably better
to bail out.

> +			}
> +		}
> +	}
> +
> +
> +	for (i = 0; i < 2; i++)
> +		if (mipi_dsi_dcs_exit_sleep_mode(dsis[i]) < 0) {
> +			dev_err(ctx->dev, "failed to exit sleep mode\n");
> +			return -ECOMM;
> +		}
> +	msleep(78);
> +
> +	for (i = 0; i < 2; i++)
> +		if (mipi_dsi_dcs_set_display_on(dsis[i]) < 0) {
> +			dev_err(ctx->dev, "failed to send display on\n");
> +			return -ECOMM;
> +		}
> +	msleep(78);
> +
> +	ctx->prepared = true;
> +
> +	return 0;
> +}
> +
> +static int truly_wqxga_enable(struct drm_panel *panel)
> +{
> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
> +
> +	if (ctx->enabled)
> +		return 0;
> +
> +	if (ctx->backlight) {
> +		ctx->backlight->props.power = FB_BLANK_UNBLANK;
> +		backlight_update_status(ctx->backlight);
> +	}
> +	ctx->enabled = true;
> +
> +	return 0;
> +}
> +
> +static int truly_wqxga_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_create(connector->dev);
> +	if (!mode) {
> +		dev_err(ctx->dev, "failed to create a new display mode\n");
> +		return 0;
> +	}
> +
> +	drm_display_mode_from_videomode(&ctx->vm, mode);
> +	connector->display_info.width_mm = 74;
> +	connector->display_info.height_mm = 131;
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs truly_wqxga_drm_funcs = {
> +	.disable = truly_wqxga_disable,
> +	.unprepare = truly_wqxga_unprepare,
> +	.prepare = truly_wqxga_prepare,
> +	.enable = truly_wqxga_enable,
> +	.get_modes = truly_wqxga_get_modes,
> +};
> +
> +static int truly_wqxga_panel_add(struct truly_wqxga *ctx)
> +{
> +	struct device *dev = ctx->dev;
> +	int ret;
> +
> +	ctx->supplies[0].supply = "vddio";
> +	ctx->supplies[1].supply = "vdda";
> +	ctx->supplies[2].supply = "lab";
> +	ctx->supplies[3].supply = "ibb";
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
> +				      ctx->supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ctx->reset_gpio)) {
> +		dev_err(dev, "cannot get reset-gpios %ld\n",
> +			PTR_ERR(ctx->reset_gpio));
> +		return PTR_ERR(ctx->reset_gpio);
> +	}
> +
> +	ctx->mode_gpio = devm_gpiod_get(dev, "mode", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->mode_gpio)) {
> +		dev_err(dev, "cannot get mode gpio %ld\n",
> +			PTR_ERR(ctx->mode_gpio));
> +		ctx->mode_gpio = NULL;
> +		return PTR_ERR(ctx->mode_gpio);
> +	}
> +
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to set pinctrl default state, %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	ret = of_get_videomode(dev->of_node, &ctx->vm, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	drm_panel_init(&ctx->panel);
> +	ctx->panel.dev = dev;
> +	ctx->panel.funcs = &truly_wqxga_drm_funcs;
> +	drm_panel_add(&ctx->panel);
> +
> +	return 0;
> +}
> +
> +static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
> +{
> +	if (ctx->panel.dev)
> +		drm_panel_remove(&ctx->panel);
> +
> +	if (ctx->backlight)
> +		put_device(&ctx->backlight->dev);
> +
> +	if (ctx->dsi[1])
> +		put_device(&ctx->dsi[1]->dev);
> +}
> +
> +static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct truly_wqxga *ctx;
> +	struct mipi_dsi_device *secondary = NULL;
> +	struct device_node *np;
> +	int ret;
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS |
> +			  MIPI_DSI_MODE_LPM;
> +
> +	/* Only DSI-LINK1 node has "link2" entry. */
> +	np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
> +	if (np) {
> +		secondary = of_find_mipi_dsi_device_by_node(np);
> +		of_node_put(np);
> +
> +		if (!secondary)
> +			return -EPROBE_DEFER;
> +
> +		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +		if (!ctx) {
> +			put_device(&secondary->dev);
> +			return -ENOMEM;
> +		}
> +		mipi_dsi_set_drvdata(dsi, ctx);
> +
> +		ctx->dev = dev;
> +		ctx->dsi[0] = dsi;
> +		ctx->dsi[1] = secondary;
> +
> +		ret = truly_wqxga_panel_add(ctx);
> +		if (ret) {
> +			put_device(&secondary->dev);
> +			return ret;
> +		}
> +	}

This should go when we use the of-graph bindings. The second
mipi_dsi_device needs to be created manually in the driver
using mipi_dsi_device_register_full(). You could look at the
ADV7511 bridge driver as an example. We would eventually need
to call mipi_dsi_attach() on the second device too.

Thanks,
Archit

> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		if (secondary)
> +			truly_wqxga_panel_del(ctx);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct truly_wqxga *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +
> +	/* delete panel only for the DSI1 interface */
> +	if (ctx)
> +		truly_wqxga_panel_del(ctx);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id truly_wqxga_of_match[] = {
> +	{ .compatible = "truly,dual_wqxga", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, truly_wqxga_of_match);
> +
> +static struct mipi_dsi_driver truly_wqxga_driver = {
> +	.driver = {
> +		.name = "panel_truly_wqxga",
> +		.of_match_table = truly_wqxga_of_match,
> +	},
> +	.probe = truly_wqxga_probe,
> +	.remove = truly_wqxga_remove,
> +};
> +module_mipi_dsi_driver(truly_wqxga_driver);
> +
> +MODULE_DESCRIPTION("truly wqxga DSI Panel Driver");
> +MODULE_LICENSE("GPL v2");
> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH 1/2] drm/panel: Add Truly Dual DSI video mode panel

[abhinavk]

On 2018-04-08 09:36, Archit Taneja wrote:
> Hi Abhinav,
> 
> Thanks for posting this driver. Some comments below.
> 
> On Saturday 07 April 2018 12:36 PM, Abhinav Kumar wrote:
>> From: Archit Taneja <architt@codeaurora.org>
>> 
>> Add support for truly dual DSI video mode panel
>> panel used in MSM reference platforms >
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>   .../bindings/display/truly,dual_wqxga.txt          |  47 ++
>>   drivers/gpu/drm/panel/Kconfig                      |   7 +
>>   drivers/gpu/drm/panel/Makefile                     |   1 +
>>   drivers/gpu/drm/panel/panel-truly-dual-dsi.c       | 530 
>> +++++++++++++++++++++
>>   4 files changed, 585 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
>>   create mode 100644 drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt 
>> b/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
>> new file mode 100644
>> index 0000000..a1b24c1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
>> @@ -0,0 +1,47 @@
>> +Truly model NT35597 1440x2560 DSI Panel
>> +
>> +Required properties:
>> +- compatible: should be "truly,dual_wqxga"
> 
> The compatible string, kernel config and the driver file should be 
> based
> on the panel model no. There can be many truly based panels that
> support wqxga. Something like "truly,nt35597" would be better.
> 
[Abhinav] Yes, will change it in v2.

>> +- vdda-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  Power IC supply
>> +- lab-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for LCD bias
>> +- ibb-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for LCD bias
> 
> Both seem to have the same description. Aren't lab and ibb qualcomm
> specific terms? Could we use the pin names specified in the panel's
> data sheet?
> 
[Abhinav] There isnt a number specified on the data sheet. I will use 
vdispp-supply and
vdispn-supply . Does that work?


>> +- reset-gpios: phandle of gpio for reset line
>> +  This should be 8mA, gpio can be configured using mux, pinctrl, 
>> pinctrl-names
>> +- mode-gpios: phandle of the gpio for choosing the mode of the 
>> display
>> +  for single DSI or Dual DSI
> 
> Could we describe here how to use this gpio? I.e, whether we need to 
> set
> it to low for dual DSI, etc?
> 
[Abhinav] Yes, will do this in v2.
>> +- display-timings: Node for the Panel timings
>> +- link2: phandle to the secondary node of the panel
> 
> The link2 binding was a temporary hack we used. We should use the
> of-graph bindings to represent the two DSI input ports of the panel.
> 
[Abhinav] Alright will start using of-graph in the next patchset.
>> +
>> +Example:
>> +
>> +	dsi@ae94000 {
>> +		panel@0 {
>> +			compatible = "truly,dual_wqxga";
>> +			reg = <0>;
>> +			link2 = <&link2>;
>> +			vdda-supply = <&pm8998_l14>;
>> +
>> +			pinctrl-names = "default", "suspend";
>> +			pinctrl-0 = <&sde_dsi_active>;
>> +			pinctrl-1 = <&sde_dsi_suspend>;
>> +
>> +			reset-gpios = <&tlmm 6 0>;
>> +			mode-gpios = <&tlmm 52 0>;
>> +			display-timings {
>> +				timing0: timing-0 {
>> +					clock-frequency = <268316138>;
>> +					hactive = <1440>;
>> +					vactive = <2560>;
>> +					hfront-porch = <200>;
>> +					hback-porch = <64>;
>> +					hsync-len = <32>;
>> +					vfront-porch = <8>;
>> +					vback-porch = <7>;
>> +					vsync-len = <1>;
>> +				};
>> +			};
>> +		};
>> +	};
>> diff --git a/drivers/gpu/drm/panel/Kconfig 
>> b/drivers/gpu/drm/panel/Kconfig
>> index 988048e..a63c3f7 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -168,4 +168,11 @@ config DRM_PANEL_SITRONIX_ST7789V
>>   	  Say Y here if you want to enable support for the Sitronix
>>   	  ST7789V controller for 240x320 LCD panels
>>   +config DRM_PANEL_TRULY_WQXGA
>> +	tristate "Truly WQXGA"
>> +	depends on OF
>> +	depends on DRM_MIPI_DSI
>> +	help
>> +	  Say Y here if you want to enable support for Truly WQXGA Dual DSI
>> +	  Video Mode panel
>>   endmenu
[Abhinav] These configs will be updated too
>> diff --git a/drivers/gpu/drm/panel/Makefile 
>> b/drivers/gpu/drm/panel/Makefile
>> index 3d2a88d..64891f6 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -17,3 +17,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += 
>> panel-seiko-43wvf1g.o
>>   obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += 
>> panel-sharp-lq101r1sx01.o
>>   obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += 
>> panel-sharp-ls043t1le01.o
>>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
>> +obj-$(CONFIG_DRM_PANEL_TRULY_WQXGA) += panel-truly-dual-dsi.o
>> diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c 
>> b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> new file mode 100644
>> index 0000000..47891ee
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> @@ -0,0 +1,530 @@
>> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.
> 
> We should use the SPDX license headers here.
> 
[Abhinav] Is there a reference I can use? Need to make sure of this.
>> + *
>> + * This program is free software; you can redistribute it and/or 
>> modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/pinctrl/consumer.h>
>> +
>> +#include <video/mipi_display.h>
>> +#include <video/of_videomode.h>
>> +#include <video/videomode.h>
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_panel.h>
>> +#include <drm/drm_mipi_dsi.h>
>> +
>> +struct truly_wqxga {
>> +	struct device *dev;
>> +	struct drm_panel panel;
>> +
>> +	struct regulator_bulk_data supplies[4];
>> +
>> +	struct gpio_desc *reset_gpio;
>> +	struct gpio_desc *mode_gpio;
>> +
>> +	struct backlight_device *backlight;
>> +	struct videomode vm;
>> +
>> +	struct mipi_dsi_device *dsi[2];
>> +
>> +	bool prepared;
>> +	bool enabled;
>> +};
>> +
>> +static inline struct truly_wqxga *panel_to_truly_wqxga(struct 
>> drm_panel *panel)
>> +{
>> +	return container_of(panel, struct truly_wqxga, panel);
>> +}
>> +
>> +static int truly_wqxga_power_on(struct truly_wqxga *ctx)
>> +{
>> +	int ret;
>> +
>> +	ret = regulator_set_load(ctx->supplies[1].consumer, 62000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regulator_set_load(ctx->supplies[2].consumer, 100000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regulator_set_load(ctx->supplies[3].consumer, 100000);
>> +	if (ret)
>> +		return ret;
> 
> We could make a const static struct array specifying the regulator
> names (to be used during probe) and their corresponding power on and
> power off loads. That should make things a bit cleaner.
> 
[Abhinav] Yes, will do it in the next patchset.
>> +
>> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), 
>> ctx->supplies);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	msleep(20);
>> +	gpiod_set_value(ctx->reset_gpio, 1);
>> +	usleep_range(20000, 20100);
>> +	gpiod_set_value(ctx->reset_gpio, 0);
>> +	usleep_range(20000, 20100);
>> +	gpiod_set_value(ctx->reset_gpio, 1);
>> +	usleep_range(50000, 50100);
>> +
>> +	/* dual port */
>> +	gpiod_set_value(ctx->mode_gpio, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_power_off(struct truly_wqxga *ctx)
>> +{
>> +	int ret;
>> +
>> +	gpiod_set_value(ctx->reset_gpio, 0);
>> +	ret = regulator_set_load(ctx->supplies[1].consumer, 62000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), 
>> ctx->supplies);
>> +}
>> +
>> +static int truly_wqxga_disable(struct drm_panel *panel)
>> +{
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +
>> +	if (!ctx->enabled)
>> +		return 0;
>> +
>> +	if (ctx->backlight) {
>> +		ctx->backlight->props.power = FB_BLANK_POWERDOWN;
>> +		backlight_update_status(ctx->backlight);
>> +	}
>> +
>> +	ctx->enabled = false;
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_unprepare(struct drm_panel *panel)
>> +{
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +	struct mipi_dsi_device **dsis = ctx->dsi;
>> +	int ret = 0, i;
>> +
>> +	if (!ctx->prepared)
>> +		return 0;
>> +
>> +	dsis[0]->mode_flags = 0;
>> +	dsis[1]->mode_flags = 0;
>> +
>> +	for (i = 0; i < 2; i++)
>> +		if (mipi_dsi_dcs_set_display_off(dsis[i]) < 0)
>> +			ret = -ECOMM;
>> +	msleep(78);
>> +
>> +	for (i = 0; i < 2; i++)
>> +		if (mipi_dsi_dcs_enter_sleep_mode(dsis[i]) < 0)
>> +			ret = -ECOMM;
>> +	msleep(78);
>> +
>> +	truly_wqxga_power_off(ctx);
>> +
>> +	ctx->prepared = false;
>> +	return ret;
>> +}
>> +
>> +#define MAX_LEN	5
>> +struct {
>> +	u8 commands[MAX_LEN];
>> +	int size;
>> +} panel_cmds[] = { /* CMD2_P0 */
>> +		   { { 0xff, 0x20 }, 2 },
>> +		   { { 0xfb, 0x01 }, 2 },
>> +		   { { 0x00, 0x01 }, 2 },
>> +		   { { 0x01, 0x55 }, 2 },
>> +		   { { 0x02, 0x45 }, 2 },
>> +		   { { 0x05, 0x40 }, 2 },
>> +		   { { 0x06, 0x19 }, 2 },
>> +		   { { 0x07, 0x1e }, 2 },
>> +		   { { 0x0b, 0x73 }, 2 },
>> +		   { { 0x0c, 0x73 }, 2 },
>> +		   { { 0x0e, 0xb0 }, 2 },
>> +		   { { 0x0f, 0xae }, 2 },
>> +		   { { 0x11, 0xb8 }, 2 },
>> +		   { { 0x13, 0x00 }, 2 },
>> +		   { { 0x58, 0x80 }, 2 },
>> +		   { { 0x59, 0x01 }, 2 },
>> +		   { { 0x5a, 0x00 }, 2 },
>> +		   { { 0x5b, 0x01 }, 2 },
>> +		   { { 0x5c, 0x80 }, 2 },
>> +		   { { 0x5d, 0x81 }, 2 },
>> +		   { { 0x5e, 0x00 }, 2 },
>> +		   { { 0x5f, 0x01 }, 2 },
>> +		   { { 0x72, 0x11 }, 2 },
>> +		   { { 0x68, 0x03 }, 2 },
>> +		   /* CMD2_P4 */
>> +		   { { 0xFF, 0x24 }, 2 },
>> +		   { { 0xFB, 0x01 }, 2 },
>> +		   { { 0x00, 0x1C }, 2 },
>> +		   { { 0x01, 0x0B }, 2 },
>> +		   { { 0x02, 0x0C }, 2 },
>> +		   { { 0x03, 0x01 }, 2 },
>> +		   { { 0x04, 0x0F }, 2 },
>> +		   { { 0x05, 0x10 }, 2 },
>> +		   { { 0x06, 0x10 }, 2 },
>> +		   { { 0x07, 0x10 }, 2 },
>> +		   { { 0x08, 0x89 }, 2 },
>> +		   { { 0x09, 0x8A }, 2 },
>> +		   { { 0x0A, 0x13 }, 2 },
>> +		   { { 0x0B, 0x13 }, 2 },
>> +		   { { 0x0C, 0x15 }, 2 },
>> +		   { { 0x0D, 0x15 }, 2 },
>> +		   { { 0x0E, 0x17 }, 2 },
>> +		   { { 0x0F, 0x17 }, 2 },
>> +		   { { 0x10, 0x1C }, 2 },
>> +		   { { 0x11, 0x0B }, 2 },
>> +		   { { 0x12, 0x0C }, 2 },
>> +		   { { 0x13, 0x01 }, 2 },
>> +		   { { 0x14, 0x0F }, 2 },
>> +		   { { 0x15, 0x10 }, 2 },
>> +		   { { 0x16, 0x10 }, 2 },
>> +		   { { 0x17, 0x10 }, 2 },
>> +		   { { 0x18, 0x89 }, 2 },
>> +		   { { 0x19, 0x8A }, 2 },
>> +		   { { 0x1A, 0x13 }, 2 },
>> +		   { { 0x1B, 0x13 }, 2 },
>> +		   { { 0x1C, 0x15 }, 2 },
>> +		   { { 0x1D, 0x15 }, 2 },
>> +		   { { 0x1E, 0x17 }, 2 },
>> +		   { { 0x1F, 0x17 }, 2 },
>> +		   /* STV */
>> +		   { { 0x20, 0x40 }, 2 },
>> +		   { { 0x21, 0x01 }, 2 },
>> +		   { { 0x22, 0x00 }, 2 },
>> +		   { { 0x23, 0x40 }, 2 },
>> +		   { { 0x24, 0x40 }, 2 },
>> +		   { { 0x25, 0x6D }, 2 },
>> +		   { { 0x26, 0x40 }, 2 },
>> +		   { { 0x27, 0x40 }, 2 },
>> +		   /* Vend */
>> +		   { { 0xE0, 0x00 }, 2 },
>> +		   { { 0xDC, 0x21 }, 2 },
>> +		   { { 0xDD, 0x22 }, 2 },
>> +		   { { 0xDE, 0x07 }, 2 },
>> +		   { { 0xDF, 0x07 }, 2 },
>> +		   { { 0xE3, 0x6D }, 2 },
>> +		   { { 0xE1, 0x07 }, 2 },
>> +		   { { 0xE2, 0x07 }, 2 },
>> +		   /* UD */
>> +		   { { 0x29, 0xD8 }, 2 },
>> +		   { { 0x2A, 0x2A }, 2 },
>> +		   /* CLK */
>> +		   { { 0x4B, 0x03 }, 2 },
>> +		   { { 0x4C, 0x11 }, 2 },
>> +		   { { 0x4D, 0x10 }, 2 },
>> +		   { { 0x4E, 0x01 }, 2 },
>> +		   { { 0x4F, 0x01 }, 2 },
>> +		   { { 0x50, 0x10 }, 2 },
>> +		   { { 0x51, 0x00 }, 2 },
>> +		   { { 0x52, 0x80 }, 2 },
>> +		   { { 0x53, 0x00 }, 2 },
>> +		   { { 0x56, 0x00 }, 2 },
>> +		   { { 0x54, 0x07 }, 2 },
>> +		   { { 0x58, 0x07 }, 2 },
>> +		   { { 0x55, 0x25 }, 2 },
>> +		   /* Reset XDONB */
>> +		   { { 0x5B, 0x43 }, 2 },
>> +		   { { 0x5C, 0x00 }, 2 },
>> +		   { { 0x5F, 0x73 }, 2 },
>> +		   { { 0x60, 0x73 }, 2 },
>> +		   { { 0x63, 0x22 }, 2 },
>> +		   { { 0x64, 0x00 }, 2 },
>> +		   { { 0x67, 0x08 }, 2 },
>> +		   { { 0x68, 0x04 }, 2 },
>> +		   /* Resolution:1440x2560 */
>> +		   { { 0x72, 0x02 }, 2 },
>> +		   /* mux */
>> +		   { { 0x7A, 0x80 }, 2 },
>> +		   { { 0x7B, 0x91 }, 2 },
>> +		   { { 0x7C, 0xD8 }, 2 },
>> +		   { { 0x7D, 0x60 }, 2 },
>> +		   { { 0x7F, 0x15 }, 2 },
>> +		   { { 0x75, 0x15 }, 2 },
>> +		   /* ABOFF */
>> +		   { { 0xB3, 0xC0 }, 2 },
>> +		   { { 0xB4, 0x00 }, 2 },
>> +		   { { 0xB5, 0x00 }, 2 },
>> +		   /* Source EQ */
>> +		   { { 0x78, 0x00 }, 2 },
>> +		   { { 0x79, 0x00 }, 2 },
>> +		   { { 0x80, 0x00 }, 2 },
>> +		   { { 0x83, 0x00 }, 2 },
>> +		   /* FP BP */
>> +		   { { 0x93, 0x0A }, 2 },
>> +		   { { 0x94, 0x0A }, 2 },
>> +		   /* Inversion Type */
>> +		   { { 0x8A, 0x00 }, 2 },
>> +		   { { 0x9B, 0xFF }, 2 },
>> +		   /* IMGSWAP =1 @PortSwap=1 */
>> +		   { { 0x9D, 0xB0 }, 2 },
>> +		   { { 0x9F, 0x63 }, 2 },
>> +		   { { 0x98, 0x10 }, 2 },
>> +		   /* FRM */
>> +		   { { 0xEC, 0x00 }, 2 },
>> +		   /* CMD1 */
>> +		   { { 0xFF, 0x10 }, 2 },
>> +		    /* VBP+VSA=,VFP = 10H */
>> +		   { { 0x3B, 0x03, 0x0A, 0x0A, }, 4 },
>> +		   /* FTE on */
>> +		   { { 0x35, 0x00 }, 2 },
>> +		   /* EN_BK =1(auto black) */
>> +		   { { 0xE5, 0x01 }, 2 },
>> +		   /* CMD mode(10) VDO mode(03) */
>> +		   { { 0xBB, 0x03 }, 2 },
>> +		   /* Non Reload MTP */
>> +		   { { 0xFB, 0x01 }, 2 },
>> +		   /* SlpOut + DispOn */
>> +		   //05 01 00 00 78 00 02 11 00
>> +		   //05 01 00 00 78 00 02 29 00
> 
> We can drop the commented lines. If possible, it would be nice
> to have some more description about the registers, but I can imagine
> that info being hard to retrieve.
[Abhinav] Yes, will remove the commented lines. Yes will be hard to give
more info on the regs.
> 
>> +};
>> +
>> +static int truly_wqxga_prepare(struct drm_panel *panel)
>> +{
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +	struct mipi_dsi_device **dsis = ctx->dsi;
>> +	struct mipi_dsi_device *d;
>> +	int ret, i, j;
>> +
>> +	if (ctx->prepared)
>> +		return 0;
>> +
>> +	ret = truly_wqxga_power_on(ctx);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	dsis[0]->mode_flags |= MIPI_DSI_MODE_LPM;
>> +	dsis[1]->mode_flags |= MIPI_DSI_MODE_LPM;
>> +
>> +	for (j = 0; j < ARRAY_SIZE(panel_cmds); j++) {
>> +		for (i = 0; i < 2; i++) {
>> +			d = dsis[i];
>> +			ret = mipi_dsi_dcs_write_buffer(dsis[i],
>> +					panel_cmds[j].commands,
>> +					panel_cmds[j].size);
>> +			if (ret < 0) {
>> +				/* continue anyway */
>> +				dev_err(ctx->dev, "failed to cmd no %d, err: %d\n",
>> +						j, ret);
> 
> This may have been done for debug purposes. It would be probably better
> to bail out.
[Abhinav] Ok, will bail out here.
> 
>> +			}
>> +		}
>> +	}
>> +
>> +
>> +	for (i = 0; i < 2; i++)
>> +		if (mipi_dsi_dcs_exit_sleep_mode(dsis[i]) < 0) {
>> +			dev_err(ctx->dev, "failed to exit sleep mode\n");
>> +			return -ECOMM;
>> +		}
>> +	msleep(78);
>> +
>> +	for (i = 0; i < 2; i++)
>> +		if (mipi_dsi_dcs_set_display_on(dsis[i]) < 0) {
>> +			dev_err(ctx->dev, "failed to send display on\n");
>> +			return -ECOMM;
>> +		}
>> +	msleep(78);
>> +
>> +	ctx->prepared = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_enable(struct drm_panel *panel)
>> +{
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +
>> +	if (ctx->enabled)
>> +		return 0;
>> +
>> +	if (ctx->backlight) {
>> +		ctx->backlight->props.power = FB_BLANK_UNBLANK;
>> +		backlight_update_status(ctx->backlight);
>> +	}
>> +	ctx->enabled = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_get_modes(struct drm_panel *panel)
>> +{
>> +	struct drm_connector *connector = panel->connector;
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_create(connector->dev);
>> +	if (!mode) {
>> +		dev_err(ctx->dev, "failed to create a new display mode\n");
>> +		return 0;
>> +	}
>> +
>> +	drm_display_mode_from_videomode(&ctx->vm, mode);
>> +	connector->display_info.width_mm = 74;
>> +	connector->display_info.height_mm = 131;
>> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>> +	drm_mode_probed_add(connector, mode);
>> +
>> +	return 1;
>> +}
>> +
>> +static const struct drm_panel_funcs truly_wqxga_drm_funcs = {
>> +	.disable = truly_wqxga_disable,
>> +	.unprepare = truly_wqxga_unprepare,
>> +	.prepare = truly_wqxga_prepare,
>> +	.enable = truly_wqxga_enable,
>> +	.get_modes = truly_wqxga_get_modes,
>> +};
>> +
>> +static int truly_wqxga_panel_add(struct truly_wqxga *ctx)
>> +{
>> +	struct device *dev = ctx->dev;
>> +	int ret;
>> +
>> +	ctx->supplies[0].supply = "vddio";
>> +	ctx->supplies[1].supply = "vdda";
>> +	ctx->supplies[2].supply = "lab";
>> +	ctx->supplies[3].supply = "ibb";
>> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
>> +				      ctx->supplies);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(ctx->reset_gpio)) {
>> +		dev_err(dev, "cannot get reset-gpios %ld\n",
>> +			PTR_ERR(ctx->reset_gpio));
>> +		return PTR_ERR(ctx->reset_gpio);
>> +	}
>> +
>> +	ctx->mode_gpio = devm_gpiod_get(dev, "mode", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ctx->mode_gpio)) {
>> +		dev_err(dev, "cannot get mode gpio %ld\n",
>> +			PTR_ERR(ctx->mode_gpio));
>> +		ctx->mode_gpio = NULL;
>> +		return PTR_ERR(ctx->mode_gpio);
>> +	}
>> +
>> +	ret = pinctrl_pm_select_default_state(dev);
>> +	if (ret) {
>> +		dev_err(dev, "%s: failed to set pinctrl default state, %d\n",
>> +			__func__, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = of_get_videomode(dev->of_node, &ctx->vm, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	drm_panel_init(&ctx->panel);
>> +	ctx->panel.dev = dev;
>> +	ctx->panel.funcs = &truly_wqxga_drm_funcs;
>> +	drm_panel_add(&ctx->panel);
>> +
>> +	return 0;
>> +}
>> +
>> +static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
>> +{
>> +	if (ctx->panel.dev)
>> +		drm_panel_remove(&ctx->panel);
>> +
>> +	if (ctx->backlight)
>> +		put_device(&ctx->backlight->dev);
>> +
>> +	if (ctx->dsi[1])
>> +		put_device(&ctx->dsi[1]->dev);
>> +}
>> +
>> +static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct device *dev = &dsi->dev;
>> +	struct truly_wqxga *ctx;
>> +	struct mipi_dsi_device *secondary = NULL;
>> +	struct device_node *np;
>> +	int ret;
>> +
>> +	dsi->lanes = 4;
>> +	dsi->format = MIPI_DSI_FMT_RGB888;
>> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | 
>> MIPI_DSI_CLOCK_NON_CONTINUOUS |
>> +			  MIPI_DSI_MODE_LPM;
>> +
>> +	/* Only DSI-LINK1 node has "link2" entry. */
>> +	np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
>> +	if (np) {
>> +		secondary = of_find_mipi_dsi_device_by_node(np);
>> +		of_node_put(np);
>> +
>> +		if (!secondary)
>> +			return -EPROBE_DEFER;
>> +
>> +		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> +		if (!ctx) {
>> +			put_device(&secondary->dev);
>> +			return -ENOMEM;
>> +		}
>> +		mipi_dsi_set_drvdata(dsi, ctx);
>> +
>> +		ctx->dev = dev;
>> +		ctx->dsi[0] = dsi;
>> +		ctx->dsi[1] = secondary;
>> +
>> +		ret = truly_wqxga_panel_add(ctx);
>> +		if (ret) {
>> +			put_device(&secondary->dev);
>> +			return ret;
>> +		}
>> +	}
> 
> This should go when we use the of-graph bindings. The second
> mipi_dsi_device needs to be created manually in the driver
> using mipi_dsi_device_register_full(). You could look at the
> ADV7511 bridge driver as an example. We would eventually need
> to call mipi_dsi_attach() on the second device too.
> 
> Thanks,
> Archit
> 
[Abhinav] I dont think we should follow bridge's example.
In our DT, the panel nodes are listed as child nodes.
The mipi_dsi_host_register API adds all the children of using
of_mipi_dsi_device_add. This calls a mipi_dsi_device_register_full().
twice because the panel is listed as a child on both the DSIs.

In bridge, the bridge driver is not listed as a child
of the DSI node, and hence an explicit call to 
mipi_dsi_device_register_full()
is required.

Hence, yes I will use of-graph bindings and get rid of "link2" but the 
part
to register the panel only for the master controller and allocate ctx 
only then
seems required to me.

Also, an explicit call to mipi_dsi_device_register_full() doesnt seem 
necessary.

Can you please comment whether our understanding is aligned with yours 
here?

>> +
>> +	ret = mipi_dsi_attach(dsi);
>> +	if (ret < 0) {
>> +		if (secondary)
>> +			truly_wqxga_panel_del(ctx);
>> +		return ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct truly_wqxga *ctx = mipi_dsi_get_drvdata(dsi);
>> +
>> +	mipi_dsi_detach(dsi);
>> +
>> +	/* delete panel only for the DSI1 interface */
>> +	if (ctx)
>> +		truly_wqxga_panel_del(ctx);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id truly_wqxga_of_match[] = {
>> +	{ .compatible = "truly,dual_wqxga", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, truly_wqxga_of_match);
>> +
>> +static struct mipi_dsi_driver truly_wqxga_driver = {
>> +	.driver = {
>> +		.name = "panel_truly_wqxga",
>> +		.of_match_table = truly_wqxga_of_match,
>> +	},
>> +	.probe = truly_wqxga_probe,
>> +	.remove = truly_wqxga_remove,
>> +};
>> +module_mipi_dsi_driver(truly_wqxga_driver);
>> +
>> +MODULE_DESCRIPTION("truly wqxga DSI Panel Driver");
>> +MODULE_LICENSE("GPL v2");
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [DPU PATCH 2/2] drm/panel: add backlight control support for truly panel

[Sean Paul]

On Sat, Apr 07, 2018 at 12:06:53AM -0700, Abhinav Kumar wrote:
> Register truly panel as a backlight led device and
> provide methods to control its backlight operation.
> 
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/panel/panel-truly-dual-dsi.c | 96 +++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> index 47891ee..5d0ef90 100644
> --- a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> +++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> @@ -14,6 +14,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/leds.h>

Includes should be alphabetical.

>  
>  #include <video/mipi_display.h>
>  #include <video/of_videomode.h>
> @@ -23,6 +24,9 @@
>  #include <drm/drm_panel.h>
>  #include <drm/drm_mipi_dsi.h>
>  
> +#define BL_NODE_NAME_SIZE 32
> +#define PRIM_DISPLAY_NODE 0
> +
>  struct truly_wqxga {
>  	struct device *dev;
>  	struct drm_panel panel;
> @@ -33,6 +37,8 @@ struct truly_wqxga {
>  	struct gpio_desc *mode_gpio;
>  
>  	struct backlight_device *backlight;
> +	/* WLED params */
> +	struct led_trigger *wled;
>  	struct videomode vm;
>  
>  	struct mipi_dsi_device *dsi[2];
> @@ -447,6 +453,83 @@ static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
>  		put_device(&ctx->dsi[1]->dev);
>  }
>  
> +static int truly_backlight_device_update_status(struct backlight_device *bd)
> +{
> +	int brightness;
> +	int max_brightness;
> +	int rc = 0;
> +
extra line

> +	struct truly_wqxga *ctx = dev_get_drvdata(&bd->dev);
> +
> +	brightness = bd->props.brightness;
> +	max_brightness = bd->props.max_brightness;
> +
> +	if ((bd->props.power != FB_BLANK_UNBLANK) ||
> +		(bd->props.state & BL_CORE_FBBLANK) ||
> +		  (bd->props.state & BL_CORE_SUSPENDED))
> +		brightness = 0;
> +
> +	if (brightness > max_brightness)
> +		brightness = max_brightness;
> +
> +	/* Need to check WLED driver capability upstream */
> +	if (ctx && ctx->wled)

ctx can't be NULL, so no need to check for that. And if ctx->wled is null, it
doesn't seem like this function will do anything. So how about just not
registering the backlight if wled == NULL (if that's possible).

> +		led_trigger_event(ctx->wled, brightness);
> +
> +	return rc;
> +}
> +
> +static int truly_backlight_device_get_brightness(struct backlight_device *bd)
> +{
> +	return bd->props.brightness;
> +}
> +
> +static const struct backlight_ops truly_backlight_device_ops = {
> +	.update_status = truly_backlight_device_update_status,
> +	.get_brightness = truly_backlight_device_get_brightness,
> +};
> +
> +static int truly_backlight_setup(struct truly_wqxga *ctx)
> +{
> +	struct backlight_properties props;
> +	char bl_node_name[BL_NODE_NAME_SIZE];
> +
> +	if (!ctx) {
> +		dev_err(ctx->dev, "invalid context\n");
> +		return -EINVAL;
> +	}

This can't happen.

> +
> +	if (!ctx->backlight) {
> +		memset(&props, 0, sizeof(props));
> +		props.type = BACKLIGHT_RAW;
> +		props.power = FB_BLANK_UNBLANK;
> +		props.max_brightness = 4096;
> +
> +		snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
> +				 PRIM_DISPLAY_NODE);

Given that PRIM_DISPLAY_NODE is always 0, this seems like overkill for a pretty
generic name "panel0-backlight". So let's just call it "truly_backlight" in the
register call.

> +
> +		ctx->backlight =  backlight_device_register(bl_node_name,
> +				ctx->dev, ctx,
> +				&truly_backlight_device_ops, &props);
> +
> +		if (IS_ERR_OR_NULL(ctx->backlight)) {
> +			pr_err("Failed to register backlight\n");
> +			ctx->backlight = NULL;
> +			return -ENODEV;
> +		}
> +
> +		/* Register with the LED driver interface */
> +		led_trigger_register_simple("bkl-trigger", &ctx->wled);
> +
> +		if (!ctx->wled) {
> +			pr_err("backlight led registration failed\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>  {
>  	struct device *dev = &dsi->dev;
> @@ -466,10 +549,11 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>  		secondary = of_find_mipi_dsi_device_by_node(np);
>  		of_node_put(np);
>  
> +		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +

Why move this?

>  		if (!secondary)
>  			return -EPROBE_DEFER;
>  
> -		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>  		if (!ctx) {
>  			put_device(&secondary->dev);
>  			return -ENOMEM;
> @@ -485,6 +569,12 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>  			put_device(&secondary->dev);
>  			return ret;
>  		}
> +
> +		ret = truly_backlight_setup(ctx);
> +		if (ret) {
> +			put_device(&secondary->dev);
> +			return ret;
> +		}
>  	}
>  
>  	ret = mipi_dsi_attach(dsi);
> @@ -504,8 +594,10 @@ static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
>  	mipi_dsi_detach(dsi);
>  
>  	/* delete panel only for the DSI1 interface */
> -	if (ctx)
> +	if (ctx) {
>  		truly_wqxga_panel_del(ctx);
> +		kfree(ctx);
> +	}
>  
>  	return 0;
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH 2/2] drm/panel: add backlight control support for truly panel

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

Hi Sean

Thanks for the comments.

Some replies inline.

On 2018-04-13 13:46, Sean Paul wrote:
> On Sat, Apr 07, 2018 at 12:06:53AM -0700, Abhinav Kumar wrote:
>> Register truly panel as a backlight led device and
>> provide methods to control its backlight operation.
>> 
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>  drivers/gpu/drm/panel/panel-truly-dual-dsi.c | 96 
>> +++++++++++++++++++++++++++-
>>  1 file changed, 94 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c 
>> b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> index 47891ee..5d0ef90 100644
>> --- a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> +++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/pinctrl/consumer.h>
>> +#include <linux/leds.h>
> 
> Includes should be alphabetical.
[Abhinav] Ok, will take care of this in v2.
> 
>> 
>>  #include <video/mipi_display.h>
>>  #include <video/of_videomode.h>
>> @@ -23,6 +24,9 @@
>>  #include <drm/drm_panel.h>
>>  #include <drm/drm_mipi_dsi.h>
>> 
>> +#define BL_NODE_NAME_SIZE 32
>> +#define PRIM_DISPLAY_NODE 0
>> +
>>  struct truly_wqxga {
>>  	struct device *dev;
>>  	struct drm_panel panel;
>> @@ -33,6 +37,8 @@ struct truly_wqxga {
>>  	struct gpio_desc *mode_gpio;
>> 
>>  	struct backlight_device *backlight;
>> +	/* WLED params */
>> +	struct led_trigger *wled;
>>  	struct videomode vm;
>> 
>>  	struct mipi_dsi_device *dsi[2];
>> @@ -447,6 +453,83 @@ static void truly_wqxga_panel_del(struct 
>> truly_wqxga *ctx)
>>  		put_device(&ctx->dsi[1]->dev);
>>  }
>> 
>> +static int truly_backlight_device_update_status(struct 
>> backlight_device *bd)
>> +{
>> +	int brightness;
>> +	int max_brightness;
>> +	int rc = 0;
>> +
> extra line
> 
[Abhinav] Ok, will remove it in v2.

>> +	struct truly_wqxga *ctx = dev_get_drvdata(&bd->dev);
>> +
>> +	brightness = bd->props.brightness;
>> +	max_brightness = bd->props.max_brightness;
>> +
>> +	if ((bd->props.power != FB_BLANK_UNBLANK) ||
>> +		(bd->props.state & BL_CORE_FBBLANK) ||
>> +		  (bd->props.state & BL_CORE_SUSPENDED))
>> +		brightness = 0;
>> +
>> +	if (brightness > max_brightness)
>> +		brightness = max_brightness;
>> +
>> +	/* Need to check WLED driver capability upstream */
>> +	if (ctx && ctx->wled)
> 
> ctx can't be NULL, so no need to check for that. And if ctx->wled is 
> null, it
> doesn't seem like this function will do anything. So how about just not
> registering the backlight if wled == NULL (if that's possible).
[Abhinav] Yes, will remove the NULL check.
We are registering the backlight in the backlight_setup(), this was more 
of
an additional check, to make sure ctx->wled is present before we 
trigger.
Not sure if we should register again here.
> 
>> +		led_trigger_event(ctx->wled, brightness);
>> +
>> +	return rc;
>> +}
>> +
>> +static int truly_backlight_device_get_brightness(struct 
>> backlight_device *bd)
>> +{
>> +	return bd->props.brightness;
>> +}
>> +
>> +static const struct backlight_ops truly_backlight_device_ops = {
>> +	.update_status = truly_backlight_device_update_status,
>> +	.get_brightness = truly_backlight_device_get_brightness,
>> +};
>> +
>> +static int truly_backlight_setup(struct truly_wqxga *ctx)
>> +{
>> +	struct backlight_properties props;
>> +	char bl_node_name[BL_NODE_NAME_SIZE];
>> +
>> +	if (!ctx) {
>> +		dev_err(ctx->dev, "invalid context\n");
>> +		return -EINVAL;
>> +	}
> 
> This can't happen.
[Abhinav] Will remove it.
> 
>> +
>> +	if (!ctx->backlight) {
>> +		memset(&props, 0, sizeof(props));
>> +		props.type = BACKLIGHT_RAW;
>> +		props.power = FB_BLANK_UNBLANK;
>> +		props.max_brightness = 4096;
>> +
>> +		snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
>> +				 PRIM_DISPLAY_NODE);
> 
> Given that PRIM_DISPLAY_NODE is always 0, this seems like overkill for 
> a pretty
> generic name "panel0-backlight". So let's just call it 
> "truly_backlight" in the
> register call.
> 
[Abhinav] The reason for keeping it "panel0-backlight" is because 
userspace is using
this node name to write the backlight. Changing the name will need more 
changes in our
userspace.

>> +
>> +		ctx->backlight =  backlight_device_register(bl_node_name,
>> +				ctx->dev, ctx,
>> +				&truly_backlight_device_ops, &props);
>> +
>> +		if (IS_ERR_OR_NULL(ctx->backlight)) {
>> +			pr_err("Failed to register backlight\n");
>> +			ctx->backlight = NULL;
>> +			return -ENODEV;
>> +		}
>> +
>> +		/* Register with the LED driver interface */
>> +		led_trigger_register_simple("bkl-trigger", &ctx->wled);
>> +
>> +		if (!ctx->wled) {
>> +			pr_err("backlight led registration failed\n");
>> +			return -ENODEV;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>>  {
>>  	struct device *dev = &dsi->dev;
>> @@ -466,10 +549,11 @@ static int truly_wqxga_probe(struct 
>> mipi_dsi_device *dsi)
>>  		secondary = of_find_mipi_dsi_device_by_node(np);
>>  		of_node_put(np);
>> 
>> +		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> +
> 
> Why move this?
[Abhinav] Thanks for catching this, yes not required.
Will move it back.
> 
>>  		if (!secondary)
>>  			return -EPROBE_DEFER;
>> 
>> -		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>  		if (!ctx) {
>>  			put_device(&secondary->dev);
>>  			return -ENOMEM;
>> @@ -485,6 +569,12 @@ static int truly_wqxga_probe(struct 
>> mipi_dsi_device *dsi)
>>  			put_device(&secondary->dev);
>>  			return ret;
>>  		}
>> +
>> +		ret = truly_backlight_setup(ctx);
>> +		if (ret) {
>> +			put_device(&secondary->dev);
>> +			return ret;
>> +		}
>>  	}
>> 
>>  	ret = mipi_dsi_attach(dsi);
>> @@ -504,8 +594,10 @@ static int truly_wqxga_remove(struct 
>> mipi_dsi_device *dsi)
>>  	mipi_dsi_detach(dsi);
>> 
>>  	/* delete panel only for the DSI1 interface */
>> -	if (ctx)
>> +	if (ctx) {
>>  		truly_wqxga_panel_del(ctx);
>> +		kfree(ctx);
>> +	}
>> 
>>  	return 0;
>>  }
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH 2/2] drm/panel: add backlight control support for truly panel

[Sean Paul]

On Fri, Apr 13, 2018 at 01:59:29PM -0700, abhinavk@codeaurora.org wrote:
> Hi Sean
> 
> Thanks for the comments.
> 
> Some replies inline.
> 
> On 2018-04-13 13:46, Sean Paul wrote:
> > On Sat, Apr 07, 2018 at 12:06:53AM -0700, Abhinav Kumar wrote:
> > > Register truly panel as a backlight led device and
> > > provide methods to control its backlight operation.
> > > 
> > > Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> > > ---

<snip />

> > > +	if (!ctx->backlight) {
> > > +		memset(&props, 0, sizeof(props));
> > > +		props.type = BACKLIGHT_RAW;
> > > +		props.power = FB_BLANK_UNBLANK;
> > > +		props.max_brightness = 4096;
> > > +
> > > +		snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
> > > +				 PRIM_DISPLAY_NODE);
> > 
> > Given that PRIM_DISPLAY_NODE is always 0, this seems like overkill for a
> > pretty
> > generic name "panel0-backlight". So let's just call it "truly_backlight"
> > in the
> > register call.
> > 
> [Abhinav] The reason for keeping it "panel0-backlight" is because userspace
> is using
> this node name to write the backlight. Changing the name will need more
> changes in our
> userspace.
> 

Unless the userspace is opensource (I'm guessing it's not?), that's not
something we need to code around. It sounds like this is mostly a moot point
given Bjorn's comments on the v2.

Sean

<snip />

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno