[DPU PATCH v2 1/2] drm/panel: Add Truly NT35597 panel

[DPU PATCH v2 1/2] drm/panel: Add Truly NT35597 panel

[Abhinav Kumar]

From: Archit Taneja <architt@codeaurora.org>

Add support for Truly NT35597 panel used
in MSM reference platforms.

Changes in v2:
- Renamed panel to truly,nt35597
- Added SPDX license
- Used endpoints instead of custom node
- Renamed and cleaned up power supplies

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

diff --git a/Documentation/devicetree/bindings/display/truly,nt35597.txt b/Documentation/devicetree/bindings/display/truly,nt35597.txt
new file mode 100644
index 0000000..22b6f19
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/truly,nt35597.txt
@@ -0,0 +1,47 @@
+Truly model NT35597 1440x2560 DSI Panel
+
+Required properties:
+- compatible: should be "truly,nt35597"
+- vdda-supply: phandle of the regulator that provides the supply voltage
+  Power IC supply
+- vdispp-supply: phandle of the regulator that provides the supply voltage
+  for positive LCD bias
+- vdispn-supply: phandle of the regulator that provides the supply voltage
+  for negative 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
+  This should be low for dual DSI and high for single DSI mode
+- display-timings: Node for the Panel timings
+
+Example:
+
+	dsi@ae94000 {
+		panel@0 {
+			compatible = "truly,nt35597";
+			reg = <0>;
+			vdda-supply = <&pm8998_l14>;
+			vdispp-supply = <&lab_regulator>;
+			vdispn-supply = <&ibb_regulator>;
+			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..9f0c490 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_NT35597_WQXGA
+	tristate "Truly WQXGA"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for Truly NT35597 WQXGA Dual DSI
+	  Video Mode panel
 endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 3d2a88d..b5b4b60 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_NT35597_WQXGA) += panel-truly-nt35597.o
diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
new file mode 100644
index 0000000..5bd5fdc
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
@@ -0,0 +1,597 @@
+// SPDX-License-Identifier: GPL-2.0
+/* 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/of_graph.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>
+
+static const char * const regulator_names[] = {
+	"vdda",
+	"vdispp",
+	"vdispn"
+};
+
+static unsigned long regulator_enable_loads[] = {
+	62000,
+	100000,
+	100000};
+
+static unsigned long regulator_disable_loads[] = {
+	80,
+	100,
+	100};
+
+struct truly_wqxga {
+	struct device *dev;
+	struct drm_panel panel;
+
+	struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
+
+	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, i;
+
+	for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
+		ret = regulator_set_load(ctx->supplies[i].consumer,
+				regulator_enable_loads[i]);
+		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, i;
+
+	gpiod_set_value(ctx->reset_gpio, 0);
+
+	for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
+		ret = regulator_set_load(ctx->supplies[i].consumer,
+				regulator_disable_loads[i]);
+		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 },
+};
+
+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) {
+				dev_err(ctx->dev, "failed to cmd no %d, err: %d\n",
+						j, ret);
+				return 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, i;
+
+	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 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);
+}
+
+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 *dsi1;
+	struct mipi_dsi_host *dsi1_host;
+	int ret = 0;
+	const struct mipi_dsi_device_info info = {.type = "trulynt35597",
+		.channel = 0,
+		.node = NULL,
+	};
+
+	/* This device represents itself as one with
+	 * two input ports which are fed by the output
+	 * ports of the two DSI controllers . The DSI0
+	 * is the master controller and has most of the
+	 * panel related info in its child node.
+	 */
+
+	/* configure master DSI device */
+	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;
+
+	/* get the dsi1 output port node */
+	dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
+	if (!dsi1) {
+		dev_err(dev, "failed to get remote node\n");
+		return -ENODEV;
+	}
+
+	dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
+	if (!dsi1_host) {
+		dev_err(dev, "failed to find dsi host\n");
+		ret = -EPROBE_DEFER;
+		goto err_host;
+	}
+
+	/* register the second DSI device */
+	secondary = mipi_dsi_device_register_full(dsi1_host,
+		&info);
+
+	if (IS_ERR(secondary)) {
+		dev_err(dev, "failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		goto err_dsi_device;
+	}
+
+	/* configure secondary DSI device */
+	secondary->lanes = 4;
+	secondary->format = MIPI_DSI_FMT_RGB888;
+	secondary->mode_flags = MIPI_DSI_MODE_VIDEO |
+		MIPI_DSI_CLOCK_NON_CONTINUOUS |
+		MIPI_DSI_MODE_LPM;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+
+	if (!ctx) {
+		ret = -ENOMEM;
+		goto err_dsi_ctx;
+	}
+
+	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) {
+		dev_err(dev, "failed to add panel\n");
+		goto err_panel_add;
+	}
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		dev_err(dev, "master dsi attach failed\n");
+		goto err_dsi_attach;
+	}
+
+	ret = mipi_dsi_attach(secondary);
+	if (ret < 0) {
+		dev_err(dev, "mipi_dsi_attach on secondary failed\n");
+		goto err_dsi_attach_sec;
+	}
+
+	of_node_put(dsi1);
+
+	return 0;
+
+err_dsi_attach_sec:
+	mipi_dsi_detach(ctx->dsi[0]);
+err_dsi_attach:
+	truly_wqxga_panel_del(ctx);
+err_panel_add:
+	mipi_dsi_device_unregister(secondary);
+err_dsi_ctx:
+err_dsi_device:
+err_host:
+	of_node_put(dsi1);
+	return ret;
+}
+
+static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
+{
+	struct truly_wqxga *ctx = mipi_dsi_get_drvdata(dsi);
+
+	if (ctx) {
+		if (ctx->dsi[0])
+			mipi_dsi_detach(ctx->dsi[0]);
+		if (ctx->dsi[1]) {
+			mipi_dsi_detach(ctx->dsi[1]);
+			mipi_dsi_device_unregister(ctx->dsi[1]);
+		}
+		truly_wqxga_panel_del(ctx);
+		kfree(ctx);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id truly_wqxga_of_match[] = {
+	{ .compatible = "truly,nt35597", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, truly_wqxga_of_match);
+
+static struct mipi_dsi_driver truly_wqxga_driver = {
+	.driver = {
+		.name = "panel_truly_nt35597",
+		.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 nt35597 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 v2 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.

Changes in v2:
- Removed redundant NULL checks
- Arranged headers alphabetically
- Formatting fixes

Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/panel/panel-truly-nt35597.c | 91 +++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
index 5bd5fdc..ec9f973 100644
--- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
+++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/gpio/consumer.h>
+#include <linux/leds.h>
 #include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pinctrl/consumer.h>
@@ -25,6 +26,9 @@
 #include <drm/drm_panel.h>
 #include <drm/drm_mipi_dsi.h>
 
+#define BL_NODE_NAME_SIZE 32
+#define PRIM_DISPLAY_NODE 0
+
 static const char * const regulator_names[] = {
 	"vdda",
 	"vdispp",
@@ -51,6 +55,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];
@@ -454,6 +460,82 @@ static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
 		drm_panel_remove(&ctx->panel);
 }
 
+static void truly_wqxga_bkl_unregister(struct truly_wqxga *ctx)
+{
+	if (ctx->backlight)
+		put_device(&ctx->backlight->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;
+
+	if (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->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;
@@ -530,6 +612,12 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
 		goto err_panel_add;
 	}
 
+	ret = truly_backlight_setup(ctx);
+	if (ret) {
+		dev_err(dev, "backlight setup failed\n");
+		goto err_backlight;
+	}
+
 	ret = mipi_dsi_attach(dsi);
 	if (ret < 0) {
 		dev_err(dev, "master dsi attach failed\n");
@@ -549,6 +637,8 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
 err_dsi_attach_sec:
 	mipi_dsi_detach(ctx->dsi[0]);
 err_dsi_attach:
+	truly_wqxga_bkl_unregister(ctx);
+err_backlight:
 	truly_wqxga_panel_del(ctx);
 err_panel_add:
 	mipi_dsi_device_unregister(secondary);
@@ -570,6 +660,7 @@ static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
 			mipi_dsi_detach(ctx->dsi[1]);
 			mipi_dsi_device_unregister(ctx->dsi[1]);
 		}
+		truly_wqxga_bkl_unregister(ctx);
 		truly_wqxga_panel_del(ctx);
 		kfree(ctx);
 	}
-- 
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 v2 2/2] drm/panel: add backlight control support for truly panel

[Bjorn Andersson]

On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:

> Register truly panel as a backlight led device and
> provide methods to control its backlight operation.
> 
> Changes in v2:
> - Removed redundant NULL checks
> - Arranged headers alphabetically
> - Formatting fixes

The change log goes below the "---" line.

> 
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
[..]
> +static int truly_backlight_setup(struct truly_wqxga *ctx)
> +{
> +	struct backlight_properties props;
> +	char bl_node_name[BL_NODE_NAME_SIZE];
> +
> +	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;
> +		}

It seems like you're registering a backlight driver for the sake of
invoking the LED backlight trigger to control the WLED.

The WLED is a backlight driver, so all you should have to do is add the
following line to your probe:

	ctx->backlight = devm_of_find_backlight(dev);

and then add "backlight = <&wled>" to your dt node.

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

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

[Sean Paul]

On Mon, Apr 16, 2018 at 12:41 PM Bjorn Andersson
<bjorn.andersson@linaro.org>
wrote:

> On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:

> > Register truly panel as a backlight led device and
> > provide methods to control its backlight operation.
> >
> > Changes in v2:
> > - Removed redundant NULL checks
> > - Arranged headers alphabetically
> > - Formatting fixes

> The change log goes below the "---" line.

FYI: In drm, people generally prefer to put them above --- so the history
is preserved in the logs. Some choose to annotate with the name of the
person suggesting the change for added information/credit.

Sean



> >
> > Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> > ---
> [..]
> > +static int truly_backlight_setup(struct truly_wqxga *ctx)
> > +{
> > +     struct backlight_properties props;
> > +     char bl_node_name[BL_NODE_NAME_SIZE];
> > +
> > +     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;
> > +             }

> It seems like you're registering a backlight driver for the sake of
> invoking the LED backlight trigger to control the WLED.

> The WLED is a backlight driver, so all you should have to do is add the
> following line to your probe:

>          ctx->backlight = devm_of_find_backlight(dev);

> and then add "backlight = <&wled>" to your dt node.

> Regards,
> Bjorn
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

Hi Bjorn

Thanks for the review.

Reply inline.

On 2018-04-16 09:41, Bjorn Andersson wrote:
> On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:
> 
>> Register truly panel as a backlight led device and
>> provide methods to control its backlight operation.
>> 
>> Changes in v2:
>> - Removed redundant NULL checks
>> - Arranged headers alphabetically
>> - Formatting fixes
> 
> The change log goes below the "---" line.
> 
[Abhinav] As sean mentioned, its quite common to list it to view it in 
the log.
This practice has been followed by quite a few in DRM
Another reference here https://patchwork.freedesktop.org/patch/211361/

>> 
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
> [..]
>> +static int truly_backlight_setup(struct truly_wqxga *ctx)
>> +{
>> +	struct backlight_properties props;
>> +	char bl_node_name[BL_NODE_NAME_SIZE];
>> +
>> +	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;
>> +		}
> 
> It seems like you're registering a backlight driver for the sake of
> invoking the LED backlight trigger to control the WLED.
> 
> The WLED is a backlight driver, so all you should have to do is add the
> following line to your probe:
> 
> 	ctx->backlight = devm_of_find_backlight(dev);
> 
> and then add "backlight = <&wled>" to your dt node.
> 
> Regards,
> Bjorn
[Abhinav] Thats not the only purpose of backlight_device_register().
We want to hook up our panel with the parent backlight driver in
drivers/video/backlight/backlight.c and also route all the 
update_backlight_status()
calls through the sysfs of the newly registered node.

The of_find_backlight() method doesnt seem to allow us to register our 
own
sysfs method.

BTW, this isnt something which we are doing uniquely.
There are other panels which seem to be doing this :

drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c

Can you please comment on whether we can have our own sysfs without
the device_register()?

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

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

[Bjorn Andersson]

On Mon 16 Apr 15:45 PDT 2018, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 
> Thanks for the review.
> 
> Reply inline.
> 
> On 2018-04-16 09:41, Bjorn Andersson wrote:
> > On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:
> > 
> > > Register truly panel as a backlight led device and
> > > provide methods to control its backlight operation.
> > > 
> > > Changes in v2:
> > > - Removed redundant NULL checks
> > > - Arranged headers alphabetically
> > > - Formatting fixes
> > 
> > The change log goes below the "---" line.
> > 
> [Abhinav] As sean mentioned, its quite common to list it to view it in the
> log.
> This practice has been followed by quite a few in DRM
> Another reference here https://patchwork.freedesktop.org/patch/211361/
> 

If that's the practice in DRM land, then that's what you should do.

> > > 
> > > Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> > > ---
> > [..]
> > > +static int truly_backlight_setup(struct truly_wqxga *ctx)
> > > +{
> > > +	struct backlight_properties props;
> > > +	char bl_node_name[BL_NODE_NAME_SIZE];
> > > +
> > > +	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;
> > > +		}
> > 
> > It seems like you're registering a backlight driver for the sake of
> > invoking the LED backlight trigger to control the WLED.
> > 
> > The WLED is a backlight driver, so all you should have to do is add the
> > following line to your probe:
> > 
> > 	ctx->backlight = devm_of_find_backlight(dev);
> > 
> > and then add "backlight = <&wled>" to your dt node.
> > 
> > Regards,
> > Bjorn
> [Abhinav] Thats not the only purpose of backlight_device_register().
> We want to hook up our panel with the parent backlight driver in
> drivers/video/backlight/backlight.c and also route all the
> update_backlight_status()
> calls through the sysfs of the newly registered node.
> 
> The of_find_backlight() method doesnt seem to allow us to register our own
> sysfs method.
> 

Are you saying that you want to be able to create an alias for the wled
entry already in /sys/class/backlight named panel0-backlight?

> BTW, this isnt something which we are doing uniquely.
> There are other panels which seem to be doing this :
> 
> drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
> 

What seems to be going on in the s6e3ha2 driver is that the hardware has
some sort of builtin backlight control, so the driver is creating a
backlight driver for the purpose of exposing those controls.

> Can you please comment on whether we can have our own sysfs without
> the device_register()?
> 

If the panel isn't actually a piece of backlight hardware then it should
not register a backlight device. Why do you need your own sysfs?

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

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

[abhinavk]

Hi Bjorn

Thanks for the comments.

Reply inline.

On 2018-04-16 23:13, Bjorn Andersson wrote:
> On Mon 16 Apr 15:45 PDT 2018, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> Thanks for the review.
>> 
>> Reply inline.
>> 
>> On 2018-04-16 09:41, Bjorn Andersson wrote:
>> > On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:
>> >
>> > > Register truly panel as a backlight led device and
>> > > provide methods to control its backlight operation.
>> > >
>> > > Changes in v2:
>> > > - Removed redundant NULL checks
>> > > - Arranged headers alphabetically
>> > > - Formatting fixes
>> >
>> > The change log goes below the "---" line.
>> >
>> [Abhinav] As sean mentioned, its quite common to list it to view it in 
>> the
>> log.
>> This practice has been followed by quite a few in DRM
>> Another reference here https://patchwork.freedesktop.org/patch/211361/
>> 
> 
> If that's the practice in DRM land, then that's what you should do.
> 
>> > >
>> > > Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> > > ---
>> > [..]
>> > > +static int truly_backlight_setup(struct truly_wqxga *ctx)
>> > > +{
>> > > +	struct backlight_properties props;
>> > > +	char bl_node_name[BL_NODE_NAME_SIZE];
>> > > +
>> > > +	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;
>> > > +		}
>> >
>> > It seems like you're registering a backlight driver for the sake of
>> > invoking the LED backlight trigger to control the WLED.
>> >
>> > The WLED is a backlight driver, so all you should have to do is add the
>> > following line to your probe:
>> >
>> > 	ctx->backlight = devm_of_find_backlight(dev);
>> >
>> > and then add "backlight = <&wled>" to your dt node.
>> >
>> > Regards,
>> > Bjorn
>> [Abhinav] Thats not the only purpose of backlight_device_register().
>> We want to hook up our panel with the parent backlight driver in
>> drivers/video/backlight/backlight.c and also route all the
>> update_backlight_status()
>> calls through the sysfs of the newly registered node.
>> 
>> The of_find_backlight() method doesnt seem to allow us to register our 
>> own
>> sysfs method.
>> 
> 
> Are you saying that you want to be able to create an alias for the wled
> entry already in /sys/class/backlight named panel0-backlight?
> 
>> BTW, this isnt something which we are doing uniquely.
>> There are other panels which seem to be doing this :
>> 
>> drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
>> 
> 
> What seems to be going on in the s6e3ha2 driver is that the hardware 
> has
> some sort of builtin backlight control, so the driver is creating a
> backlight driver for the purpose of exposing those controls.
> 
>> Can you please comment on whether we can have our own sysfs without
>> the device_register()?
>> 
> 
> If the panel isn't actually a piece of backlight hardware then it 
> should
> not register a backlight device. Why do you need your own sysfs?
> 
> Regards,
> Bjorn
[Abhinav] This particular panel isnt a piece of backlight hardware.
But, we want to have our own sysfs to give flexibility to our userspace
to write and read stuff for its proprietary uses.
Thats how our downstream has been working so far and hence to maintain
the compatibility would like to have it.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sujeev Dias]

Testing my responsde


On 04/17/2018 11:21 AM, abhinavk@codeaurora.org wrote:
> Hi Bjorn
>
> Thanks for the comments.
>
> Reply inline.
>
> On 2018-04-16 23:13, Bjorn Andersson wrote:
>> On Mon 16 Apr 15:45 PDT 2018, abhinavk@codeaurora.org wrote:
>>
>>> Hi Bjorn
>>>
>>> Thanks for the review.
>>>
>>> Reply inline.
>>>
>>> On 2018-04-16 09:41, Bjorn Andersson wrote:
>>> > On Sat 14 Apr 00:25 PDT 2018, Abhinav Kumar wrote:
>>> >
>>> > > Register truly panel as a backlight led device and
>>> > > provide methods to control its backlight operation.
>>> > >
>>> > > Changes in v2:
>>> > > - Removed redundant NULL checks
>>> > > - Arranged headers alphabetically
>>> > > - Formatting fixes
>>> >
>>> > The change log goes below the "---" line.
>>> >
>>> [Abhinav] As sean mentioned, its quite common to list it to view it 
>>> in the
>>> log.
>>> This practice has been followed by quite a few in DRM
>>> Another reference here https://patchwork.freedesktop.org/patch/211361/
>>>
>>
>> If that's the practice in DRM land, then that's what you should do.
>>
>>> > >
>>> > > Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>>> > > ---
>>> > [..]
>>> > > +static int truly_backlight_setup(struct truly_wqxga *ctx)
>>> > > +{
>>> > > +    struct backlight_properties props;
>>> > > +    char bl_node_name[BL_NODE_NAME_SIZE];
>>> > > +
>>> > > +    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;
>>> > > +        }
>>> >
>>> > It seems like you're registering a backlight driver for the sake of
>>> > invoking the LED backlight trigger to control the WLED.
>>> >
>>> > The WLED is a backlight driver, so all you should have to do is 
>>> add the
>>> > following line to your probe:
>>> >
>>> >     ctx->backlight = devm_of_find_backlight(dev);
>>> >
>>> > and then add "backlight = <&wled>" to your dt node.
>>> >
>>> > Regards,
>>> > Bjorn
>>> [Abhinav] Thats not the only purpose of backlight_device_register().
>>> We want to hook up our panel with the parent backlight driver in
>>> drivers/video/backlight/backlight.c and also route all the
>>> update_backlight_status()
>>> calls through the sysfs of the newly registered node.
>>>
>>> The of_find_backlight() method doesnt seem to allow us to register 
>>> our own
>>> sysfs method.
>>>
>>
>> Are you saying that you want to be able to create an alias for the wled
>> entry already in /sys/class/backlight named panel0-backlight?
>>
>>> BTW, this isnt something which we are doing uniquely.
>>> There are other panels which seem to be doing this :
>>>
>>> drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
>>>
>>
>> What seems to be going on in the s6e3ha2 driver is that the hardware has
>> some sort of builtin backlight control, so the driver is creating a
>> backlight driver for the purpose of exposing those controls.
>>
>>> Can you please comment on whether we can have our own sysfs without
>>> the device_register()?
>>>
>>
>> If the panel isn't actually a piece of backlight hardware then it should
>> not register a backlight device. Why do you need your own sysfs?
>>
>> Regards,
>> Bjorn
> [Abhinav] This particular panel isnt a piece of backlight hardware.
> But, we want to have our own sysfs to give flexibility to our userspace
> to write and read stuff for its proprietary uses.
> Thats how our downstream has been working so far and hence to maintain
> the compatibility would like to have it.
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> -- 
> 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

-- 
Qualcomm Innovation Center, Inc. is a member of 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 v2 2/2] drm/panel: add backlight control support for truly panel

[Bjorn Andersson]

On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
> On 2018-04-16 23:13, Bjorn Andersson wrote:
[..]
> > If the panel isn't actually a piece of backlight hardware then it should
> > not register a backlight device. Why do you need your own sysfs?
> > 
> > Regards,
> > Bjorn
> [Abhinav] This particular panel isnt a piece of backlight hardware.
> But, we want to have our own sysfs to give flexibility to our userspace
> to write and read stuff for its proprietary uses.

Please be more specific in your replies, no one will accept code that
"does stuff" and expecting a reviewer to spend time guessing or pulling
the information out of you is a sure way to get your patches ignored.

Exactly what kind of stuff are you talking about here and exactly which
problem are you solving.

> Thats how our downstream has been working so far and hence to maintain
> the compatibility would like to have it.

Make your proprietary code work with the upstream kernel and you
shouldn't ever have to modify it.

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

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

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

Hi Bjorn

Apologies if the prev reply wasnt clear.

Hope this one is.

reply inline.

On 2018-04-17 14:29, Bjorn Andersson wrote:
> On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
>> On 2018-04-16 23:13, Bjorn Andersson wrote:
> [..]
>> > If the panel isn't actually a piece of backlight hardware then it should
>> > not register a backlight device. Why do you need your own sysfs?
>> >
>> > Regards,
>> > Bjorn
>> [Abhinav] This particular panel isnt a piece of backlight hardware.
>> But, we want to have our own sysfs to give flexibility to our 
>> userspace
>> to write and read stuff for its proprietary uses.
> 
> Please be more specific in your replies, no one will accept code that
> "does stuff" and expecting a reviewer to spend time guessing or pulling
> the information out of you is a sure way to get your patches ignored.
> 
> Exactly what kind of stuff are you talking about here and exactly which
> problem are you solving.
> 
>> Thats how our downstream has been working so far and hence to maintain
>> the compatibility would like to have it.
> 
> Make your proprietary code work with the upstream kernel and you
> shouldn't ever have to modify it.
> 
> Regards,
> Bjorn

[Abhinav] We have a few userspace clients today for the backlight sysfs 
node
which read/write directly to 
"/sys/class/backlight/panel0-backlight/brightness"
When I said "stuff" I was only referring to the brightness value.
This separate sysfs node allows us to validate those userspace features 
of ours
which directly edit the backlight value on our reference platform.
Since our reference platform uses this panel,hence having our own sysfs 
alias helps.
Otherwise, whenever we try to use this panel with upstream code we will 
have to end up
changing all those places in our userspace/framework to change the sysfs 
path.
Hence we wanted to preserve that sysfs node name.
The wled device is not created under /sys/class/backlight but under
/sys/class/leds/wled.
So we will have to change the name of this node across all our 
userspace.

If this isnt a convincing reason enough to have its own sysfs node path, 
I will use
your approach.

Let me know your comments or if this is still not clear.

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

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

[abhinavk]

Adding another point.

On 2018-04-17 17:04, abhinavk@codeaurora.org wrote:
> Hi Bjorn
> 
> Apologies if the prev reply wasnt clear.
> 
> Hope this one is.
> 
> reply inline.
> 
> On 2018-04-17 14:29, Bjorn Andersson wrote:
>> On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
>>> On 2018-04-16 23:13, Bjorn Andersson wrote:
>> [..]
>>> > If the panel isn't actually a piece of backlight hardware then it should
>>> > not register a backlight device. Why do you need your own sysfs?
>>> >
>>> > Regards,
>>> > Bjorn
>>> [Abhinav] This particular panel isnt a piece of backlight hardware.
>>> But, we want to have our own sysfs to give flexibility to our 
>>> userspace
>>> to write and read stuff for its proprietary uses.
>> 
>> Please be more specific in your replies, no one will accept code that
>> "does stuff" and expecting a reviewer to spend time guessing or 
>> pulling
>> the information out of you is a sure way to get your patches ignored.
>> 
>> Exactly what kind of stuff are you talking about here and exactly 
>> which
>> problem are you solving.
>> 
>>> Thats how our downstream has been working so far and hence to 
>>> maintain
>>> the compatibility would like to have it.
>> 
>> Make your proprietary code work with the upstream kernel and you
>> shouldn't ever have to modify it.
>> 
>> Regards,
>> Bjorn
> 
> [Abhinav] We have a few userspace clients today for the backlight sysfs 
> node
> which read/write directly to 
> "/sys/class/backlight/panel0-backlight/brightness"
> When I said "stuff" I was only referring to the brightness value.
> This separate sysfs node allows us to validate those userspace features 
> of ours
> which directly edit the backlight value on our reference platform.
> Since our reference platform uses this panel,hence having our own
> sysfs alias helps.
> Otherwise, whenever we try to use this panel with upstream code we
> will have to end up
> changing all those places in our userspace/framework to change the 
> sysfs path.
> Hence we wanted to preserve that sysfs node name.
> The wled device is not created under /sys/class/backlight but under
> /sys/class/leds/wled.
> So we will have to change the name of this node across all our 
> userspace.
> 
> If this isnt a convincing reason enough to have its own sysfs node
> path, I will use
> your approach.
> 
> Let me know your comments or if this is still not clear.
> 
[Abhinav] We also might not be using the brightness value "as-it-is".

We will potentially scale it up/down based on some requirements.

If we do not have our own sysfs alias for this, how do we account for
providing this interface for our chipset specific backlight dependent
feature.

Can you please comment on this?

>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Thompson]

On Tue, Apr 17, 2018 at 05:42:04PM -0700, abhinavk@codeaurora.org wrote:
> Adding another point.
> 
> On 2018-04-17 17:04, abhinavk@codeaurora.org wrote:
> > Hi Bjorn
> > 
> > Apologies if the prev reply wasnt clear.
> > 
> > Hope this one is.
> > 
> > reply inline.
> > 
> > On 2018-04-17 14:29, Bjorn Andersson wrote:
> > > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
> > > > On 2018-04-16 23:13, Bjorn Andersson wrote:
> > > [..]
> > > > > If the panel isn't actually a piece of backlight hardware then it should
> > > > > not register a backlight device. Why do you need your own sysfs?
> > > > >
> > > > > Regards,
> > > > > Bjorn
> > > > [Abhinav] This particular panel isnt a piece of backlight hardware.
> > > > But, we want to have our own sysfs to give flexibility to our
> > > > userspace
> > > > to write and read stuff for its proprietary uses.
> > > 
> > > Please be more specific in your replies, no one will accept code that
> > > "does stuff" and expecting a reviewer to spend time guessing or
> > > pulling
> > > the information out of you is a sure way to get your patches ignored.
> > > 
> > > Exactly what kind of stuff are you talking about here and exactly
> > > which
> > > problem are you solving.
> > > 
> > > > Thats how our downstream has been working so far and hence to
> > > > maintain
> > > > the compatibility would like to have it.
> > > 
> > > Make your proprietary code work with the upstream kernel and you
> > > shouldn't ever have to modify it.
> > > 
> > > Regards,
> > > Bjorn
> > 
> > [Abhinav] We have a few userspace clients today for the backlight sysfs
> > node
> > which read/write directly to
> > "/sys/class/backlight/panel0-backlight/brightness"
> > When I said "stuff" I was only referring to the brightness value.
> > This separate sysfs node allows us to validate those userspace features
> > of ours
> > which directly edit the backlight value on our reference platform.
> > Since our reference platform uses this panel,hence having our own
> > sysfs alias helps.
> > Otherwise, whenever we try to use this panel with upstream code we
> > will have to end up
> > changing all those places in our userspace/framework to change the sysfs
> > path.
> > Hence we wanted to preserve that sysfs node name.
> > The wled device is not created under /sys/class/backlight but under
> > /sys/class/leds/wled.
> > So we will have to change the name of this node across all our
> > userspace.
> > 
> > If this isnt a convincing reason enough to have its own sysfs node
> > path, I will use
> > your approach.
> > 
> > Let me know your comments or if this is still not clear.
> > 
> [Abhinav] We also might not be using the brightness value "as-it-is".
> 
> We will potentially scale it up/down based on some requirements.
> 
> If we do not have our own sysfs alias for this, how do we account for
> providing this interface for our chipset specific backlight dependent
> feature.
> 
> Can you please comment on this?

Not easily. It's rather unclear what this chipset specific backlight
dependent feature you have alluded to is so how can we suggest how to
control or model it in the upstream kernel?

I can make a guess that is might be to do with brightness curves... but
I'd really prefer not to have to guess.

There are some problems with the current interface because it is not
well defined with the brightness control is linear or
logarithmic/perceptual (patches welcome) but for other common embedded
backlights (pwm_bl particularly) we expect calibration of the
brightness curve to be a job for the device tree (because it is a
property of the hardware it can be described in the DT) and there are
patches pending to improve this.


Daniel.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sean Paul]

On Tue, Apr 17, 2018 at 05:04:56PM -0700, abhinavk@codeaurora.org wrote:
> Hi Bjorn
> 
> Apologies if the prev reply wasnt clear.
> 
> Hope this one is.
> 
> reply inline.
> 
> On 2018-04-17 14:29, Bjorn Andersson wrote:
> > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
> > > On 2018-04-16 23:13, Bjorn Andersson wrote:
> > [..]
> > > > If the panel isn't actually a piece of backlight hardware then it should
> > > > not register a backlight device. Why do you need your own sysfs?
> > > >
> > > > Regards,
> > > > Bjorn
> > > [Abhinav] This particular panel isnt a piece of backlight hardware.
> > > But, we want to have our own sysfs to give flexibility to our
> > > userspace
> > > to write and read stuff for its proprietary uses.
> > 
> > Please be more specific in your replies, no one will accept code that
> > "does stuff" and expecting a reviewer to spend time guessing or pulling
> > the information out of you is a sure way to get your patches ignored.
> > 
> > Exactly what kind of stuff are you talking about here and exactly which
> > problem are you solving.
> > 
> > > Thats how our downstream has been working so far and hence to maintain
> > > the compatibility would like to have it.
> > 
> > Make your proprietary code work with the upstream kernel and you
> > shouldn't ever have to modify it.
> > 
> > Regards,
> > Bjorn
> 
> [Abhinav] We have a few userspace clients today for the backlight sysfs node
> which read/write directly to
> "/sys/class/backlight/panel0-backlight/brightness"
> When I said "stuff" I was only referring to the brightness value.
> This separate sysfs node allows us to validate those userspace features of
> ours
> which directly edit the backlight value on our reference platform.
> Since our reference platform uses this panel,hence having our own sysfs
> alias helps.
> Otherwise, whenever we try to use this panel with upstream code we will have
> to end up
> changing all those places in our userspace/framework to change the sysfs
> path.
> Hence we wanted to preserve that sysfs node name.
> The wled device is not created under /sys/class/backlight but under
> /sys/class/leds/wled.
> So we will have to change the name of this node across all our userspace.

As I mentioned on the previous review, coding around closed source userspace
isn't something that we want to do. It's hard enough to accommodate open
source u/s as it is.

Sean

> 
> If this isnt a convincing reason enough to have its own sysfs node path, I
> will use
> your approach.
> 
> Let me know your comments or if this is still not clear.
> 
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
> --
> 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

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

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

[Sean Paul]

On Wed, Apr 18, 2018 at 11:52:18AM +0100, Daniel Thompson wrote:
> On Tue, Apr 17, 2018 at 05:42:04PM -0700, abhinavk@codeaurora.org wrote:
> > Adding another point.
> > 
> > On 2018-04-17 17:04, abhinavk@codeaurora.org wrote:
> > > Hi Bjorn
> > > 
> > > Apologies if the prev reply wasnt clear.
> > > 
> > > Hope this one is.
> > > 
> > > reply inline.
> > > 
> > > On 2018-04-17 14:29, Bjorn Andersson wrote:
> > > > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
> > > > > On 2018-04-16 23:13, Bjorn Andersson wrote:
> > > > [..]
> > > > > > If the panel isn't actually a piece of backlight hardware then it should
> > > > > > not register a backlight device. Why do you need your own sysfs?
> > > > > >
> > > > > > Regards,
> > > > > > Bjorn
> > > > > [Abhinav] This particular panel isnt a piece of backlight hardware.
> > > > > But, we want to have our own sysfs to give flexibility to our
> > > > > userspace
> > > > > to write and read stuff for its proprietary uses.
> > > > 
> > > > Please be more specific in your replies, no one will accept code that
> > > > "does stuff" and expecting a reviewer to spend time guessing or
> > > > pulling
> > > > the information out of you is a sure way to get your patches ignored.
> > > > 
> > > > Exactly what kind of stuff are you talking about here and exactly
> > > > which
> > > > problem are you solving.
> > > > 
> > > > > Thats how our downstream has been working so far and hence to
> > > > > maintain
> > > > > the compatibility would like to have it.
> > > > 
> > > > Make your proprietary code work with the upstream kernel and you
> > > > shouldn't ever have to modify it.
> > > > 
> > > > Regards,
> > > > Bjorn
> > > 
> > > [Abhinav] We have a few userspace clients today for the backlight sysfs
> > > node
> > > which read/write directly to
> > > "/sys/class/backlight/panel0-backlight/brightness"
> > > When I said "stuff" I was only referring to the brightness value.
> > > This separate sysfs node allows us to validate those userspace features
> > > of ours
> > > which directly edit the backlight value on our reference platform.
> > > Since our reference platform uses this panel,hence having our own
> > > sysfs alias helps.
> > > Otherwise, whenever we try to use this panel with upstream code we
> > > will have to end up
> > > changing all those places in our userspace/framework to change the sysfs
> > > path.
> > > Hence we wanted to preserve that sysfs node name.
> > > The wled device is not created under /sys/class/backlight but under
> > > /sys/class/leds/wled.
> > > So we will have to change the name of this node across all our
> > > userspace.
> > > 
> > > If this isnt a convincing reason enough to have its own sysfs node
> > > path, I will use
> > > your approach.
> > > 
> > > Let me know your comments or if this is still not clear.
> > > 
> > [Abhinav] We also might not be using the brightness value "as-it-is".
> > 
> > We will potentially scale it up/down based on some requirements.
> > 
> > If we do not have our own sysfs alias for this, how do we account for
> > providing this interface for our chipset specific backlight dependent
> > feature.
> > 
> > Can you please comment on this?
> 
> Not easily. It's rather unclear what this chipset specific backlight
> dependent feature you have alluded to is so how can we suggest how to
> control or model it in the upstream kernel?
> 

The code is here:

https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/mtp-squashed/drivers/gpu/drm/msm/dsi-staging/dsi_display.c#L76

AFAICT, there's nothing fancy in the kernel aside from scaling the brightness
level down twice. I assume the magic is in userspace. My initial reaction was
that the scaling factor should just be applied in userspace. Especially since
the scaling factor reduces the resolution of the backlight, and that's not
immediately obvious by looking at "brightness".

Sean


> I can make a guess that is might be to do with brightness curves... but
> I'd really prefer not to have to guess.
> 
> There are some problems with the current interface because it is not
> well defined with the brightness control is linear or
> logarithmic/perceptual (patches welcome) but for other common embedded
> backlights (pwm_bl particularly) we expect calibration of the
> brightness curve to be a job for the device tree (because it is a
> property of the hardware it can be described in the DT) and there are
> patches pending to improve this.
> 
> 
> Daniel.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

Thanks Daniel and Sean for your comments.

Yes, the magic and algorithm is in userspace.

After this discussion, it seems like the complexity of the userspace 
having to
figure out which device node to use and to scale the backlight 
accordingly is not
a strong enough reason to handle this in the driver as it seems like 
there are other
opensource userspace clients doing the same thing.

I will submit another patchset to use the method suggested by Bjorne.

Thanks

Abhinav
On 2018-04-18 06:42, Sean Paul wrote:
> On Wed, Apr 18, 2018 at 11:52:18AM +0100, Daniel Thompson wrote:
>> On Tue, Apr 17, 2018 at 05:42:04PM -0700, abhinavk@codeaurora.org 
>> wrote:
>> > Adding another point.
>> >
>> > On 2018-04-17 17:04, abhinavk@codeaurora.org wrote:
>> > > Hi Bjorn
>> > >
>> > > Apologies if the prev reply wasnt clear.
>> > >
>> > > Hope this one is.
>> > >
>> > > reply inline.
>> > >
>> > > On 2018-04-17 14:29, Bjorn Andersson wrote:
>> > > > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
>> > > > > On 2018-04-16 23:13, Bjorn Andersson wrote:
>> > > > [..]
>> > > > > > If the panel isn't actually a piece of backlight hardware then it should
>> > > > > > not register a backlight device. Why do you need your own sysfs?
>> > > > > >
>> > > > > > Regards,
>> > > > > > Bjorn
>> > > > > [Abhinav] This particular panel isnt a piece of backlight hardware.
>> > > > > But, we want to have our own sysfs to give flexibility to our
>> > > > > userspace
>> > > > > to write and read stuff for its proprietary uses.
>> > > >
>> > > > Please be more specific in your replies, no one will accept code that
>> > > > "does stuff" and expecting a reviewer to spend time guessing or
>> > > > pulling
>> > > > the information out of you is a sure way to get your patches ignored.
>> > > >
>> > > > Exactly what kind of stuff are you talking about here and exactly
>> > > > which
>> > > > problem are you solving.
>> > > >
>> > > > > Thats how our downstream has been working so far and hence to
>> > > > > maintain
>> > > > > the compatibility would like to have it.
>> > > >
>> > > > Make your proprietary code work with the upstream kernel and you
>> > > > shouldn't ever have to modify it.
>> > > >
>> > > > Regards,
>> > > > Bjorn
>> > >
>> > > [Abhinav] We have a few userspace clients today for the backlight sysfs
>> > > node
>> > > which read/write directly to
>> > > "/sys/class/backlight/panel0-backlight/brightness"
>> > > When I said "stuff" I was only referring to the brightness value.
>> > > This separate sysfs node allows us to validate those userspace features
>> > > of ours
>> > > which directly edit the backlight value on our reference platform.
>> > > Since our reference platform uses this panel,hence having our own
>> > > sysfs alias helps.
>> > > Otherwise, whenever we try to use this panel with upstream code we
>> > > will have to end up
>> > > changing all those places in our userspace/framework to change the sysfs
>> > > path.
>> > > Hence we wanted to preserve that sysfs node name.
>> > > The wled device is not created under /sys/class/backlight but under
>> > > /sys/class/leds/wled.
>> > > So we will have to change the name of this node across all our
>> > > userspace.
>> > >
>> > > If this isnt a convincing reason enough to have its own sysfs node
>> > > path, I will use
>> > > your approach.
>> > >
>> > > Let me know your comments or if this is still not clear.
>> > >
>> > [Abhinav] We also might not be using the brightness value "as-it-is".
>> >
>> > We will potentially scale it up/down based on some requirements.
>> >
>> > If we do not have our own sysfs alias for this, how do we account for
>> > providing this interface for our chipset specific backlight dependent
>> > feature.
>> >
>> > Can you please comment on this?
>> 
>> Not easily. It's rather unclear what this chipset specific backlight
>> dependent feature you have alluded to is so how can we suggest how to
>> control or model it in the upstream kernel?
>> 
> 
> The code is here:
> 
> https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/mtp-squashed/drivers/gpu/drm/msm/dsi-staging/dsi_display.c#L76
> 
> AFAICT, there's nothing fancy in the kernel aside from scaling the 
> brightness
> level down twice. I assume the magic is in userspace. My initial 
> reaction was
> that the scaling factor should just be applied in userspace. Especially 
> since
> the scaling factor reduces the resolution of the backlight, and that's 
> not
> immediately obvious by looking at "brightness".
> 
> Sean
> 
> 
>> I can make a guess that is might be to do with brightness curves... 
>> but
>> I'd really prefer not to have to guess.
>> 
>> There are some problems with the current interface because it is not
>> well defined with the brightness control is linear or
>> logarithmic/perceptual (patches welcome) but for other common embedded
>> backlights (pwm_bl particularly) we expect calibration of the
>> brightness curve to be a job for the device tree (because it is a
>> property of the hardware it can be described in the DT) and there are
>> patches pending to improve this.
>> 
>> 
>> Daniel.
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Bjorn Andersson]

On Tue 17 Apr 17:04 PDT 2018, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 
> Apologies if the prev reply wasnt clear.
> 
> Hope this one is.
> 

Much better, now we can discuss the actual issues :)

> reply inline.
> 
> On 2018-04-17 14:29, Bjorn Andersson wrote:
> > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
> > > On 2018-04-16 23:13, Bjorn Andersson wrote:
> > [..]
> > > > If the panel isn't actually a piece of backlight hardware then it should
> > > > not register a backlight device. Why do you need your own sysfs?
> > > >
> > > > Regards,
> > > > Bjorn
> > > [Abhinav] This particular panel isnt a piece of backlight hardware.
> > > But, we want to have our own sysfs to give flexibility to our
> > > userspace
> > > to write and read stuff for its proprietary uses.
> > 
> > Please be more specific in your replies, no one will accept code that
> > "does stuff" and expecting a reviewer to spend time guessing or pulling
> > the information out of you is a sure way to get your patches ignored.
> > 
> > Exactly what kind of stuff are you talking about here and exactly which
> > problem are you solving.
> > 
> > > Thats how our downstream has been working so far and hence to maintain
> > > the compatibility would like to have it.
> > 
> > Make your proprietary code work with the upstream kernel and you
> > shouldn't ever have to modify it.
> > 
> > Regards,
> > Bjorn
> 
> [Abhinav] We have a few userspace clients today for the backlight sysfs node
> which read/write directly to
> "/sys/class/backlight/panel0-backlight/brightness"
> When I said "stuff" I was only referring to the brightness value.
> This separate sysfs node allows us to validate those userspace
> features of ours which directly edit the backlight value on our
> reference platform.

That's nice, but you're enforcing that either all panel drivers must
implement this backlight wrapper or that your customers must modify
their user space to match their backlight implementation.

> Since our reference platform uses this panel,hence having our own
> sysfs alias helps.  Otherwise, whenever we try to use this panel with
> upstream code we will have to end up changing all those places in our
> userspace/framework to change the sysfs path.

The actual problem comes down to "how does user space know the name of
the backlight instance associated with the panel" and this is a valid
question to pursue.

But given your current design you could just scan for the one and only
backlight device available in the system; as your use of the static name
"panel0-backlight" doesn't allow multiple backlights anyway.


If your goal is simply to ship your reference with something that you
can show work, then just replace the hard coded panel0-backlight with
the name of the wled backlight device. Customers can change panels as
they wish, but in the event that they plug in a different backlight
controller they would need to modify the code.

> Hence we wanted to preserve that sysfs node name.
> The wled device is not created under /sys/class/backlight but under
> /sys/class/leds/wled.
> So we will have to change the name of this node across all our userspace.
> 

Hard coding /sys/class/backlight/panel0-backlight in your user space
instead of /sys/class/leds/wled is hardly a gain, in particular since
the cost is 94 insertions - per panel driver.

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

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

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

Hi Bjorn

Thanks very much for the detailed response.

Yes, we decided that userspace hardcoding this node name is not a strong 
enough
reason to register as another backlight device.

Had one follow up question though.

The QC WLED driver, drivers/leds/leds-qpnp-wled.c is not registering 
itself as a backlight device.

It only registers as a led device.

In that case, we cannot invoke of_find_backlight_by_node to get a handle 
on it.

One question we have is , is it required that every WLED driver register 
itself as a backlight device too?

In this case since it is not doing so, but we need to trigger it for the 
backlight.

Is there any way we can do this?

OR shall we just abandon the backlight control out of this driver 
entirely.

Thanks

Abhinav

On 2018-04-18 21:00, Bjorn Andersson wrote:
> On Tue 17 Apr 17:04 PDT 2018, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> Apologies if the prev reply wasnt clear.
>> 
>> Hope this one is.
>> 
> 
> Much better, now we can discuss the actual issues :)
> 
>> reply inline.
>> 
>> On 2018-04-17 14:29, Bjorn Andersson wrote:
>> > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
>> > > On 2018-04-16 23:13, Bjorn Andersson wrote:
>> > [..]
>> > > > If the panel isn't actually a piece of backlight hardware then it should
>> > > > not register a backlight device. Why do you need your own sysfs?
>> > > >
>> > > > Regards,
>> > > > Bjorn
>> > > [Abhinav] This particular panel isnt a piece of backlight hardware.
>> > > But, we want to have our own sysfs to give flexibility to our
>> > > userspace
>> > > to write and read stuff for its proprietary uses.
>> >
>> > Please be more specific in your replies, no one will accept code that
>> > "does stuff" and expecting a reviewer to spend time guessing or pulling
>> > the information out of you is a sure way to get your patches ignored.
>> >
>> > Exactly what kind of stuff are you talking about here and exactly which
>> > problem are you solving.
>> >
>> > > Thats how our downstream has been working so far and hence to maintain
>> > > the compatibility would like to have it.
>> >
>> > Make your proprietary code work with the upstream kernel and you
>> > shouldn't ever have to modify it.
>> >
>> > Regards,
>> > Bjorn
>> 
>> [Abhinav] We have a few userspace clients today for the backlight 
>> sysfs node
>> which read/write directly to
>> "/sys/class/backlight/panel0-backlight/brightness"
>> When I said "stuff" I was only referring to the brightness value.
>> This separate sysfs node allows us to validate those userspace
>> features of ours which directly edit the backlight value on our
>> reference platform.
> 
> That's nice, but you're enforcing that either all panel drivers must
> implement this backlight wrapper or that your customers must modify
> their user space to match their backlight implementation.
> 
>> Since our reference platform uses this panel,hence having our own
>> sysfs alias helps.  Otherwise, whenever we try to use this panel with
>> upstream code we will have to end up changing all those places in our
>> userspace/framework to change the sysfs path.
> 
> The actual problem comes down to "how does user space know the name of
> the backlight instance associated with the panel" and this is a valid
> question to pursue.
> 
> But given your current design you could just scan for the one and only
> backlight device available in the system; as your use of the static 
> name
> "panel0-backlight" doesn't allow multiple backlights anyway.
> 
> 
> If your goal is simply to ship your reference with something that you
> can show work, then just replace the hard coded panel0-backlight with
> the name of the wled backlight device. Customers can change panels as
> they wish, but in the event that they plug in a different backlight
> controller they would need to modify the code.
> 
>> Hence we wanted to preserve that sysfs node name.
>> The wled device is not created under /sys/class/backlight but under
>> /sys/class/leds/wled.
>> So we will have to change the name of this node across all our 
>> userspace.
>> 
> 
> Hard coding /sys/class/backlight/panel0-backlight in your user space
> instead of /sys/class/leds/wled is hardly a gain, in particular since
> the cost is 94 insertions - per panel driver.
> 
> Regards,
> Bjorn
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Bjorn Andersson]

On Tue 17 Apr 17:42 PDT 2018, abhinavk@codeaurora.org wrote:

> Adding another point.
> 
> On 2018-04-17 17:04, abhinavk@codeaurora.org wrote:
> > Hi Bjorn
> > 
> > Apologies if the prev reply wasnt clear.
> > 
> > Hope this one is.
> > 
> > reply inline.
> > 
> > On 2018-04-17 14:29, Bjorn Andersson wrote:
> > > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
> > > > On 2018-04-16 23:13, Bjorn Andersson wrote:
> > > [..]
> > > > > If the panel isn't actually a piece of backlight hardware then it should
> > > > > not register a backlight device. Why do you need your own sysfs?
> > > > >
> > > > > Regards,
> > > > > Bjorn
> > > > [Abhinav] This particular panel isnt a piece of backlight hardware.
> > > > But, we want to have our own sysfs to give flexibility to our
> > > > userspace
> > > > to write and read stuff for its proprietary uses.
> > > 
> > > Please be more specific in your replies, no one will accept code that
> > > "does stuff" and expecting a reviewer to spend time guessing or
> > > pulling
> > > the information out of you is a sure way to get your patches ignored.
> > > 
> > > Exactly what kind of stuff are you talking about here and exactly
> > > which
> > > problem are you solving.
> > > 
> > > > Thats how our downstream has been working so far and hence to
> > > > maintain
> > > > the compatibility would like to have it.
> > > 
> > > Make your proprietary code work with the upstream kernel and you
> > > shouldn't ever have to modify it.
> > > 
> > > Regards,
> > > Bjorn
> > 
> > [Abhinav] We have a few userspace clients today for the backlight sysfs
> > node
> > which read/write directly to
> > "/sys/class/backlight/panel0-backlight/brightness"
> > When I said "stuff" I was only referring to the brightness value.
> > This separate sysfs node allows us to validate those userspace features
> > of ours
> > which directly edit the backlight value on our reference platform.
> > Since our reference platform uses this panel,hence having our own
> > sysfs alias helps.
> > Otherwise, whenever we try to use this panel with upstream code we
> > will have to end up
> > changing all those places in our userspace/framework to change the sysfs
> > path.
> > Hence we wanted to preserve that sysfs node name.
> > The wled device is not created under /sys/class/backlight but under
> > /sys/class/leds/wled.
> > So we will have to change the name of this node across all our
> > userspace.
> > 
> > If this isnt a convincing reason enough to have its own sysfs node
> > path, I will use
> > your approach.
> > 
> > Let me know your comments or if this is still not clear.
> > 
> [Abhinav] We also might not be using the brightness value "as-it-is".
> 
> We will potentially scale it up/down based on some requirements.
> 
> If we do not have our own sysfs alias for this, how do we account for
> providing this interface for our chipset specific backlight dependent
> feature.
> 
> Can you please comment on this?
> 

What kind of requirements would this be and what do you mean with scale
it up/down?

As the current implementation is proposed any magic added on top would
work for this panel driver and wouldn't be available for any other
panel, which doesn't sounds optimal.

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

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

[Bjorn Andersson]

On Wed 18 Apr 21:23 PDT 2018, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 

Hi Abhinav,

> Thanks very much for the detailed response.
> 

You're welcome.

> Yes, we decided that userspace hardcoding this node name is not a
> strong enough reason to register as another backlight device.
> 
> Had one follow up question though.
> 
> The QC WLED driver, drivers/leds/leds-qpnp-wled.c is not registering itself
> as a backlight device.
> 
> It only registers as a led device.
> 
> In that case, we cannot invoke of_find_backlight_by_node to get a handle on
> it.
> 
> One question we have is , is it required that every WLED driver register
> itself as a backlight device too?
> 
> In this case since it is not doing so, but we need to trigger it for the
> backlight.
> 
> Is there any way we can do this?
> 

It seems I answered this in private, so let me summarize my answer for
the record.

The downstream driver for the Qualcomm WLED registers a LED, but the
WLED driver should register a backlight device. There is a driver
(drivers/video/backlight/pm8941-wled.c) that does that for the WLED
version found in PM8941, so the appropriate solution to this problem is
to extend that to support the PMIC you're looking at.

> OR shall we just abandon the backlight control out of this driver entirely.
> 

The backlight handling in the panel driver serves the purpose of
blanking and unblanking the associated backlight device, given the
status of the panel. And this is still considered valuable.

It does sounds like a reasonable idea to extend this to also cover
brightness management, but as you might guess this becomes a larger and
completely generic issue.

Regards,
Bjorn

> Thanks
> 
> Abhinav
> 
> On 2018-04-18 21:00, Bjorn Andersson wrote:
> > On Tue 17 Apr 17:04 PDT 2018, abhinavk@codeaurora.org wrote:
> > 
> > > Hi Bjorn
> > > 
> > > Apologies if the prev reply wasnt clear.
> > > 
> > > Hope this one is.
> > > 
> > 
> > Much better, now we can discuss the actual issues :)
> > 
> > > reply inline.
> > > 
> > > On 2018-04-17 14:29, Bjorn Andersson wrote:
> > > > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
> > > > > On 2018-04-16 23:13, Bjorn Andersson wrote:
> > > > [..]
> > > > > > If the panel isn't actually a piece of backlight hardware then it should
> > > > > > not register a backlight device. Why do you need your own sysfs?
> > > > > >
> > > > > > Regards,
> > > > > > Bjorn
> > > > > [Abhinav] This particular panel isnt a piece of backlight hardware.
> > > > > But, we want to have our own sysfs to give flexibility to our
> > > > > userspace
> > > > > to write and read stuff for its proprietary uses.
> > > >
> > > > Please be more specific in your replies, no one will accept code that
> > > > "does stuff" and expecting a reviewer to spend time guessing or pulling
> > > > the information out of you is a sure way to get your patches ignored.
> > > >
> > > > Exactly what kind of stuff are you talking about here and exactly which
> > > > problem are you solving.
> > > >
> > > > > Thats how our downstream has been working so far and hence to maintain
> > > > > the compatibility would like to have it.
> > > >
> > > > Make your proprietary code work with the upstream kernel and you
> > > > shouldn't ever have to modify it.
> > > >
> > > > Regards,
> > > > Bjorn
> > > 
> > > [Abhinav] We have a few userspace clients today for the backlight
> > > sysfs node
> > > which read/write directly to
> > > "/sys/class/backlight/panel0-backlight/brightness"
> > > When I said "stuff" I was only referring to the brightness value.
> > > This separate sysfs node allows us to validate those userspace
> > > features of ours which directly edit the backlight value on our
> > > reference platform.
> > 
> > That's nice, but you're enforcing that either all panel drivers must
> > implement this backlight wrapper or that your customers must modify
> > their user space to match their backlight implementation.
> > 
> > > Since our reference platform uses this panel,hence having our own
> > > sysfs alias helps.  Otherwise, whenever we try to use this panel with
> > > upstream code we will have to end up changing all those places in our
> > > userspace/framework to change the sysfs path.
> > 
> > The actual problem comes down to "how does user space know the name of
> > the backlight instance associated with the panel" and this is a valid
> > question to pursue.
> > 
> > But given your current design you could just scan for the one and only
> > backlight device available in the system; as your use of the static name
> > "panel0-backlight" doesn't allow multiple backlights anyway.
> > 
> > 
> > If your goal is simply to ship your reference with something that you
> > can show work, then just replace the hard coded panel0-backlight with
> > the name of the wled backlight device. Customers can change panels as
> > they wish, but in the event that they plug in a different backlight
> > controller they would need to modify the code.
> > 
> > > Hence we wanted to preserve that sysfs node name.
> > > The wled device is not created under /sys/class/backlight but under
> > > /sys/class/leds/wled.
> > > So we will have to change the name of this node across all our
> > > userspace.
> > > 
> > 
> > Hard coding /sys/class/backlight/panel0-backlight in your user space
> > instead of /sys/class/leds/wled is hardly a gain, in particular since
> > the cost is 94 insertions - per panel driver.
> > 
> > Regards,
> > Bjorn
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 1/2] drm/panel: Add Truly NT35597 panel

[Archit Taneja]

Hi,

On Saturday 14 April 2018 12:55 PM, Abhinav Kumar wrote:
> From: Archit Taneja <architt@codeaurora.org>

You can drop DPU from the subject. Also, you'd need to add
Theirry Reading for panel related patches, and Rob Herring
for an Ack on the DT bindings.

I think you can change the author to yourself. You've had to
make plenty of changes to get this in upstream state. You
can keep my Signed-off-by, though.

> 
> Add support for Truly NT35597 panel used
> in MSM reference platforms.

You can mention here that the panel supports both single
and dual DSI modes, and that we support only dual-DSI
mode for now.

> 
> Changes in v2:
> - Renamed panel to truly,nt35597
> - Added SPDX license
> - Used endpoints instead of custom node
> - Renamed and cleaned up power supplies
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>   .../devicetree/bindings/display/truly,nt35597.txt  |  47 ++
>   drivers/gpu/drm/panel/Kconfig                      |   7 +
>   drivers/gpu/drm/panel/Makefile                     |   1 +
>   drivers/gpu/drm/panel/panel-truly-nt35597.c        | 597 +++++++++++++++++++++
>   4 files changed, 652 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/display/truly,nt35597.txt
>   create mode 100644 drivers/gpu/drm/panel/panel-truly-nt35597.c
> 
> diff --git a/Documentation/devicetree/bindings/display/truly,nt35597.txt b/Documentation/devicetree/bindings/display/truly,nt35597.txt
> new file mode 100644
> index 0000000..22b6f19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/truly,nt35597.txt
> @@ -0,0 +1,47 @@
> +Truly model NT35597 1440x2560 DSI Panel
> +
> +Required properties:
> +- compatible: should be "truly,nt35597"
> +- vdda-supply: phandle of the regulator that provides the supply voltage
> +  Power IC supply
> +- vdispp-supply: phandle of the regulator that provides the supply voltage
> +  for positive LCD bias
> +- vdispn-supply: phandle of the regulator that provides the supply voltage
> +  for negative 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
> +  This should be low for dual DSI and high for single DSI mode
> +- display-timings: Node for the Panel timings
> +

You need to specify the of-graph bindings here. Especially, what port #
corresponds to master DSI, and what # for slave DSI.

> +Example:
> +
> +	dsi@ae94000 {
> +		panel@0 {
> +			compatible = "truly,nt35597";
> +			reg = <0>;
> +			vdda-supply = <&pm8998_l14>;
> +			vdispp-supply = <&lab_regulator>;
> +			vdispn-supply = <&ibb_regulator>;
> +			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>;
> +					vactie = <2560>;
> +					hfront-porch = <200>;
> +					hback-porch = <64>;
> +					hsync-len = <32>;
> +					vfront-porch = <8>;
> +					vback-porch = <7>;
> +					vsync-len = <1>;
> +				};
> +			};
> +		};

The example should specify the port too.

> +	};
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 988048e..9f0c490 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_NT35597_WQXGA
> +	tristate "Truly WQXGA"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	help
> +	  Say Y here if you want to enable support for Truly NT35597 WQXGA Dual DSI
> +	  Video Mode panel
>   endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 3d2a88d..b5b4b60 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_NT35597_WQXGA) += panel-truly-nt35597.o
> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> new file mode 100644
> index 0000000..5bd5fdc
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> @@ -0,0 +1,597 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* 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/of_graph.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>
> +
> +static const char * const regulator_names[] = {
> +	"vdda",
> +	"vdispp",
> +	"vdispn"
> +};
> +
> +static unsigned long regulator_enable_loads[] = {
> +	62000,
> +	100000,
> +	100000};

coding style nit: the closing bracket should come on the next line.
> +
> +static unsigned long regulator_disable_loads[] = {
> +	80,
> +	100,
> +	100};
> +

Same here.

> +struct truly_wqxga {
> +	struct device *dev;
> +	struct drm_panel panel;
> +
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
> +
> +	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, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
> +		ret = regulator_set_load(ctx->supplies[i].consumer,
> +				regulator_enable_loads[i]);

coding style nit: the func argument should be aligned with the opening
parenthesis.

> +		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);

Accoridng to Documentation/timers/timers-howto.txt, you're supposed to
use msleep if the delays are >= 20 ms.

> +
> +	/* dual port */
> +	gpiod_set_value(ctx->mode_gpio, 0);

Maybe we can set this just once during probe, since we don't support
single channel DSI?

> +
> +	return 0;
> +}
> +
> +static int truly_wqxga_power_off(struct truly_wqxga *ctx)
> +{
> +	int ret, i;
> +
> +	gpiod_set_value(ctx->reset_gpio, 0);
> +
> +	for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
> +		ret = regulator_set_load(ctx->supplies[i].consumer,
> +				regulator_disable_loads[i]);
> +		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);

Could you cross check if we need these sleeps for unprepare too?

> +
> +	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 },
> +};
> +
> +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) {
> +				dev_err(ctx->dev, "failed to cmd no %d, err: %d\n",
> +						j, ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +

Unnecessary extra blank 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, i;
> +
> +	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 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;
> +	}

This might be unnecessary, I think this may be done in
drivers/base/dd.c. I'm not a 100% sure though.

> +
> +	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);
> +}
> +
> +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;

We don't need to set it to NULL here.

> +	struct device_node *dsi1;
> +	struct mipi_dsi_host *dsi1_host;
> +	int ret = 0;
> +	const struct mipi_dsi_device_info info = {.type = "trulynt35597",
> +		.channel = 0,
> +		.node = NULL,
> +	};
> +
> +	/* This device represents itself as one with
> +	 * two input ports which are fed by the output
> +	 * ports of the two DSI controllers . The DSI0
> +	 * is the master controller and has most of the
> +	 * panel related info in its child node.
> +	 */
> +
> +	/* configure master DSI device */
> +	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;

Could we move the master DSI settings just before we call
mipi_dsi_attach() on it?

> +
> +	/* get the dsi1 output port node */
> +	dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
> +	if (!dsi1) {
> +		dev_err(dev, "failed to get remote node\n")You could mention 'remote node for secondary DSI' for the sake of
clarity.
> +		return -ENODEV;
> +	}
> +
> +	dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
> +	if (!dsi1_host) {
> +		dev_err(dev, "failed to find dsi host\n");
> +		ret = -EPROBE_DEFER;
> +		goto err_host;
> +	}
> +
> +	/* register the second DSI device */
> +	secondary = mipi_dsi_device_register_full(dsi1_host,
> +		&info);

coding style nit: needs to be aligned with opening parenthesis.

> +
> +	if (IS_ERR(secondary)) {
> +		dev_err(dev, "failed to create dsi device\n");
> +		ret = PTR_ERR(dsi);
> +		goto err_dsi_device;
> +	}
> +
> +	/* configure secondary DSI device */
> +	secondary->lanes = 4;
> +	secondary->format = MIPI_DSI_FMT_RGB888;
> +	secondary->mode_flags = MIPI_DSI_MODE_VIDEO |
> +		MIPI_DSI_CLOCK_NON_CONTINUOUS |
> +		MIPI_DSI_MODE_LPM;

This could be similarly moved just before calling mipi_dsi_attach()
on the secondary DSI device.

> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +

Allocating context is generally the first thing done in probe. It should
simplify the error handling a bit too.

> +	if (!ctx) {
> +		ret = -ENOMEM;
> +		goto err_dsi_ctx;
> +	}
> +
> +	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) {
> +		dev_err(dev, "failed to add panel\n");
> +		goto err_panel_add;
> +	}
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "master dsi attach failed\n");
> +		goto err_dsi_attach;
> +	}
> +
> +	ret = mipi_dsi_attach(secondary);
> +	if (ret < 0) {
> +		dev_err(dev, "mipi_dsi_attach on secondary failed\n");
> +		goto err_dsi_attach_sec;
> +	}
> +
> +	of_node_put(dsi1);

This should be done just after we call of_find_mipi_dsi_host_by_node()
for clarity.

> +
> +	return 0;
> +
> +err_dsi_attach_sec:
> +	mipi_dsi_detach(ctx->dsi[0]);
> +err_dsi_attach:
> +	truly_wqxga_panel_del(ctx);
> +err_panel_add:
> +	mipi_dsi_device_unregister(secondary);
> +err_dsi_ctx:
> +err_dsi_device:
> +err_host:
> +	of_node_put(dsi1);
> +	return ret;
> +}
> +
> +static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct truly_wqxga *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	if (ctx) {

I don't think we need the check above.
> +		if (ctx->dsi[0])
> +			mipi_dsi_detach(ctx->dsi[0]);
> +		if (ctx->dsi[1]) {
> +			mipi_dsi_detach(ctx->dsi[1]);
> +			mipi_dsi_device_unregister(ctx->dsi[1]);
> +		}
> +		truly_wqxga_panel_del(ctx);
> +		kfree(ctx);

You shouldn't free stuff that's allocated using devm_ funcs.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id truly_wqxga_of_match[] = {
> +	{ .compatible = "truly,nt35597", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, truly_wqxga_of_match);
> +
> +static struct mipi_dsi_driver truly_wqxga_driver = {
> +	.driver = {
> +		.name = "panel_truly_nt35597",
> +		.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 nt35597 DSI Panel Driver");

s/truly nt35597/Truly NT35597

The driver looks in good shape now. Thanks for working on it.

Regards,
Archit

> +MODULE_LICENSE("GPL v2");
> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 1/2] drm/panel: Add Truly NT35597 panel

[abhinavk]

Thanks Archit for your comments.

I have addressed all of them in V3 and added Theirry Reading and Rob 
Herring for reviews as well.

Abhinav

On 2018-04-19 10:44, Archit Taneja wrote:
> Hi,
> 
> On Saturday 14 April 2018 12:55 PM, Abhinav Kumar wrote:
>> From: Archit Taneja <architt@codeaurora.org>
> 
> You can drop DPU from the subject. Also, you'd need to add
> Theirry Reading for panel related patches, and Rob Herring
> for an Ack on the DT bindings.
> 
> I think you can change the author to yourself. You've had to
> make plenty of changes to get this in upstream state. You
> can keep my Signed-off-by, though.
> 
>> 
>> Add support for Truly NT35597 panel used
>> in MSM reference platforms.
> 
> You can mention here that the panel supports both single
> and dual DSI modes, and that we support only dual-DSI
> mode for now.
> 
>> 
>> Changes in v2:
>> - Renamed panel to truly,nt35597
>> - Added SPDX license
>> - Used endpoints instead of custom node
>> - Renamed and cleaned up power supplies
>> 
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>   .../devicetree/bindings/display/truly,nt35597.txt  |  47 ++
>>   drivers/gpu/drm/panel/Kconfig                      |   7 +
>>   drivers/gpu/drm/panel/Makefile                     |   1 +
>>   drivers/gpu/drm/panel/panel-truly-nt35597.c        | 597 
>> +++++++++++++++++++++
>>   4 files changed, 652 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/display/truly,nt35597.txt
>>   create mode 100644 drivers/gpu/drm/panel/panel-truly-nt35597.c
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/display/truly,nt35597.txt 
>> b/Documentation/devicetree/bindings/display/truly,nt35597.txt
>> new file mode 100644
>> index 0000000..22b6f19
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/truly,nt35597.txt
>> @@ -0,0 +1,47 @@
>> +Truly model NT35597 1440x2560 DSI Panel
>> +
>> +Required properties:
>> +- compatible: should be "truly,nt35597"
>> +- vdda-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  Power IC supply
>> +- vdispp-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for positive LCD bias
>> +- vdispn-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for negative 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
>> +  This should be low for dual DSI and high for single DSI mode
>> +- display-timings: Node for the Panel timings
>> +
> 
> You need to specify the of-graph bindings here. Especially, what port #
> corresponds to master DSI, and what # for slave DSI.
> 
>> +Example:
>> +
>> +	dsi@ae94000 {
>> +		panel@0 {
>> +			compatible = "truly,nt35597";
>> +			reg = <0>;
>> +			vdda-supply = <&pm8998_l14>;
>> +			vdispp-supply = <&lab_regulator>;
>> +			vdispn-supply = <&ibb_regulator>;
>> +			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>;
>> +					vactie = <2560>;
>> +					hfront-porch = <200>;
>> +					hback-porch = <64>;
>> +					hsync-len = <32>;
>> +					vfront-porch = <8>;
>> +					vback-porch = <7>;
>> +					vsync-len = <1>;
>> +				};
>> +			};
>> +		};
> 
> The example should specify the port too.
> 
>> +	};
>> diff --git a/drivers/gpu/drm/panel/Kconfig 
>> b/drivers/gpu/drm/panel/Kconfig
>> index 988048e..9f0c490 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_NT35597_WQXGA
>> +	tristate "Truly WQXGA"
>> +	depends on OF
>> +	depends on DRM_MIPI_DSI
>> +	help
>> +	  Say Y here if you want to enable support for Truly NT35597 WQXGA 
>> Dual DSI
>> +	  Video Mode panel
>>   endmenu
>> diff --git a/drivers/gpu/drm/panel/Makefile 
>> b/drivers/gpu/drm/panel/Makefile
>> index 3d2a88d..b5b4b60 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_NT35597_WQXGA) += panel-truly-nt35597.o
>> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c 
>> b/drivers/gpu/drm/panel/panel-truly-nt35597.c
>> new file mode 100644
>> index 0000000..5bd5fdc
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
>> @@ -0,0 +1,597 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* 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/of_graph.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>
>> +
>> +static const char * const regulator_names[] = {
>> +	"vdda",
>> +	"vdispp",
>> +	"vdispn"
>> +};
>> +
>> +static unsigned long regulator_enable_loads[] = {
>> +	62000,
>> +	100000,
>> +	100000};
> 
> coding style nit: the closing bracket should come on the next line.
>> +
>> +static unsigned long regulator_disable_loads[] = {
>> +	80,
>> +	100,
>> +	100};
>> +
> 
> Same here.
> 
>> +struct truly_wqxga {
>> +	struct device *dev;
>> +	struct drm_panel panel;
>> +
>> +	struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
>> +
>> +	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, i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
>> +		ret = regulator_set_load(ctx->supplies[i].consumer,
>> +				regulator_enable_loads[i]);
> 
> coding style nit: the func argument should be aligned with the opening
> parenthesis.
> 
>> +		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);
> 
> Accoridng to Documentation/timers/timers-howto.txt, you're supposed to
> use msleep if the delays are >= 20 ms.
> 
>> +
>> +	/* dual port */
>> +	gpiod_set_value(ctx->mode_gpio, 0);
> 
> Maybe we can set this just once during probe, since we don't support
> single channel DSI?
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_power_off(struct truly_wqxga *ctx)
>> +{
>> +	int ret, i;
>> +
>> +	gpiod_set_value(ctx->reset_gpio, 0);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ctx->supplies); i++) {
>> +		ret = regulator_set_load(ctx->supplies[i].consumer,
>> +				regulator_disable_loads[i]);
>> +		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);
> 
> Could you cross check if we need these sleeps for unprepare too?
> 
>> +
>> +	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 },
>> +};
>> +
>> +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) {
>> +				dev_err(ctx->dev, "failed to cmd no %d, err: %d\n",
>> +						j, ret);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +
>> +
> 
> Unnecessary extra blank 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, i;
>> +
>> +	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 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;
>> +	}
> 
> This might be unnecessary, I think this may be done in
> drivers/base/dd.c. I'm not a 100% sure though.
> 
>> +
>> +	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);
>> +}
>> +
>> +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;
> 
> We don't need to set it to NULL here.
> 
>> +	struct device_node *dsi1;
>> +	struct mipi_dsi_host *dsi1_host;
>> +	int ret = 0;
>> +	const struct mipi_dsi_device_info info = {.type = "trulynt35597",
>> +		.channel = 0,
>> +		.node = NULL,
>> +	};
>> +
>> +	/* This device represents itself as one with
>> +	 * two input ports which are fed by the output
>> +	 * ports of the two DSI controllers . The DSI0
>> +	 * is the master controller and has most of the
>> +	 * panel related info in its child node.
>> +	 */
>> +
>> +	/* configure master DSI device */
>> +	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;
> 
> Could we move the master DSI settings just before we call
> mipi_dsi_attach() on it?
> 
>> +
>> +	/* get the dsi1 output port node */
>> +	dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
>> +	if (!dsi1) {
>> +		dev_err(dev, "failed to get remote node\n")You could mention 
>> 'remote node for secondary DSI' for the sake of
> clarity.
>> +		return -ENODEV;
>> +	}
>> +
>> +	dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
>> +	if (!dsi1_host) {
>> +		dev_err(dev, "failed to find dsi host\n");
>> +		ret = -EPROBE_DEFER;
>> +		goto err_host;
>> +	}
>> +
>> +	/* register the second DSI device */
>> +	secondary = mipi_dsi_device_register_full(dsi1_host,
>> +		&info);
> 
> coding style nit: needs to be aligned with opening parenthesis.
> 
>> +
>> +	if (IS_ERR(secondary)) {
>> +		dev_err(dev, "failed to create dsi device\n");
>> +		ret = PTR_ERR(dsi);
>> +		goto err_dsi_device;
>> +	}
>> +
>> +	/* configure secondary DSI device */
>> +	secondary->lanes = 4;
>> +	secondary->format = MIPI_DSI_FMT_RGB888;
>> +	secondary->mode_flags = MIPI_DSI_MODE_VIDEO |
>> +		MIPI_DSI_CLOCK_NON_CONTINUOUS |
>> +		MIPI_DSI_MODE_LPM;
> 
> This could be similarly moved just before calling mipi_dsi_attach()
> on the secondary DSI device.
> 
>> +
>> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> +
> 
> Allocating context is generally the first thing done in probe. It 
> should
> simplify the error handling a bit too.
> 
>> +	if (!ctx) {
>> +		ret = -ENOMEM;
>> +		goto err_dsi_ctx;
>> +	}
>> +
>> +	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) {
>> +		dev_err(dev, "failed to add panel\n");
>> +		goto err_panel_add;
>> +	}
>> +
>> +	ret = mipi_dsi_attach(dsi);
>> +	if (ret < 0) {
>> +		dev_err(dev, "master dsi attach failed\n");
>> +		goto err_dsi_attach;
>> +	}
>> +
>> +	ret = mipi_dsi_attach(secondary);
>> +	if (ret < 0) {
>> +		dev_err(dev, "mipi_dsi_attach on secondary failed\n");
>> +		goto err_dsi_attach_sec;
>> +	}
>> +
>> +	of_node_put(dsi1);
> 
> This should be done just after we call of_find_mipi_dsi_host_by_node()
> for clarity.
> 
>> +
>> +	return 0;
>> +
>> +err_dsi_attach_sec:
>> +	mipi_dsi_detach(ctx->dsi[0]);
>> +err_dsi_attach:
>> +	truly_wqxga_panel_del(ctx);
>> +err_panel_add:
>> +	mipi_dsi_device_unregister(secondary);
>> +err_dsi_ctx:
>> +err_dsi_device:
>> +err_host:
>> +	of_node_put(dsi1);
>> +	return ret;
>> +}
>> +
>> +static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct truly_wqxga *ctx = mipi_dsi_get_drvdata(dsi);
>> +
>> +	if (ctx) {
> 
> I don't think we need the check above.
>> +		if (ctx->dsi[0])
>> +			mipi_dsi_detach(ctx->dsi[0]);
>> +		if (ctx->dsi[1]) {
>> +			mipi_dsi_detach(ctx->dsi[1]);
>> +			mipi_dsi_device_unregister(ctx->dsi[1]);
>> +		}
>> +		truly_wqxga_panel_del(ctx);
>> +		kfree(ctx);
> 
> You shouldn't free stuff that's allocated using devm_ funcs.
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id truly_wqxga_of_match[] = {
>> +	{ .compatible = "truly,nt35597", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, truly_wqxga_of_match);
>> +
>> +static struct mipi_dsi_driver truly_wqxga_driver = {
>> +	.driver = {
>> +		.name = "panel_truly_nt35597",
>> +		.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 nt35597 DSI Panel Driver");
> 
> s/truly nt35597/Truly NT35597
> 
> The driver looks in good shape now. Thanks for working on it.
> 
> Regards,
> Archit
> 
>> +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 v2 2/2] drm/panel: add backlight control support for truly panel

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

Hi Bjorn

Thanks for the comments.

I will upload a new patch for this which will be dependent on 
https://patchwork.kernel.org/patch/10377537/ series.

This series registers the backlight device we need.

Thanks

Abhinav

On 2018-04-18 21:42, Bjorn Andersson wrote:
> On Wed 18 Apr 21:23 PDT 2018, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
> 
> Hi Abhinav,
> 
>> Thanks very much for the detailed response.
>> 
> 
> You're welcome.
> 
>> Yes, we decided that userspace hardcoding this node name is not a
>> strong enough reason to register as another backlight device.
>> 
>> Had one follow up question though.
>> 
>> The QC WLED driver, drivers/leds/leds-qpnp-wled.c is not registering 
>> itself
>> as a backlight device.
>> 
>> It only registers as a led device.
>> 
>> In that case, we cannot invoke of_find_backlight_by_node to get a 
>> handle on
>> it.
>> 
>> One question we have is , is it required that every WLED driver 
>> register
>> itself as a backlight device too?
>> 
>> In this case since it is not doing so, but we need to trigger it for 
>> the
>> backlight.
>> 
>> Is there any way we can do this?
>> 
> 
> It seems I answered this in private, so let me summarize my answer for
> the record.
> 
> The downstream driver for the Qualcomm WLED registers a LED, but the
> WLED driver should register a backlight device. There is a driver
> (drivers/video/backlight/pm8941-wled.c) that does that for the WLED
> version found in PM8941, so the appropriate solution to this problem is
> to extend that to support the PMIC you're looking at.
> 
>> OR shall we just abandon the backlight control out of this driver 
>> entirely.
>> 
> 
> The backlight handling in the panel driver serves the purpose of
> blanking and unblanking the associated backlight device, given the
> status of the panel. And this is still considered valuable.
> 
> It does sounds like a reasonable idea to extend this to also cover
> brightness management, but as you might guess this becomes a larger and
> completely generic issue.
> 
> Regards,
> Bjorn
> 
>> Thanks
>> 
>> Abhinav
>> 
>> On 2018-04-18 21:00, Bjorn Andersson wrote:
>> > On Tue 17 Apr 17:04 PDT 2018, abhinavk@codeaurora.org wrote:
>> >
>> > > Hi Bjorn
>> > >
>> > > Apologies if the prev reply wasnt clear.
>> > >
>> > > Hope this one is.
>> > >
>> >
>> > Much better, now we can discuss the actual issues :)
>> >
>> > > reply inline.
>> > >
>> > > On 2018-04-17 14:29, Bjorn Andersson wrote:
>> > > > On Tue 17 Apr 11:21 PDT 2018, abhinavk@codeaurora.org wrote:
>> > > > > On 2018-04-16 23:13, Bjorn Andersson wrote:
>> > > > [..]
>> > > > > > If the panel isn't actually a piece of backlight hardware then it should
>> > > > > > not register a backlight device. Why do you need your own sysfs?
>> > > > > >
>> > > > > > Regards,
>> > > > > > Bjorn
>> > > > > [Abhinav] This particular panel isnt a piece of backlight hardware.
>> > > > > But, we want to have our own sysfs to give flexibility to our
>> > > > > userspace
>> > > > > to write and read stuff for its proprietary uses.
>> > > >
>> > > > Please be more specific in your replies, no one will accept code that
>> > > > "does stuff" and expecting a reviewer to spend time guessing or pulling
>> > > > the information out of you is a sure way to get your patches ignored.
>> > > >
>> > > > Exactly what kind of stuff are you talking about here and exactly which
>> > > > problem are you solving.
>> > > >
>> > > > > Thats how our downstream has been working so far and hence to maintain
>> > > > > the compatibility would like to have it.
>> > > >
>> > > > Make your proprietary code work with the upstream kernel and you
>> > > > shouldn't ever have to modify it.
>> > > >
>> > > > Regards,
>> > > > Bjorn
>> > >
>> > > [Abhinav] We have a few userspace clients today for the backlight
>> > > sysfs node
>> > > which read/write directly to
>> > > "/sys/class/backlight/panel0-backlight/brightness"
>> > > When I said "stuff" I was only referring to the brightness value.
>> > > This separate sysfs node allows us to validate those userspace
>> > > features of ours which directly edit the backlight value on our
>> > > reference platform.
>> >
>> > That's nice, but you're enforcing that either all panel drivers must
>> > implement this backlight wrapper or that your customers must modify
>> > their user space to match their backlight implementation.
>> >
>> > > Since our reference platform uses this panel,hence having our own
>> > > sysfs alias helps.  Otherwise, whenever we try to use this panel with
>> > > upstream code we will have to end up changing all those places in our
>> > > userspace/framework to change the sysfs path.
>> >
>> > The actual problem comes down to "how does user space know the name of
>> > the backlight instance associated with the panel" and this is a valid
>> > question to pursue.
>> >
>> > But given your current design you could just scan for the one and only
>> > backlight device available in the system; as your use of the static name
>> > "panel0-backlight" doesn't allow multiple backlights anyway.
>> >
>> >
>> > If your goal is simply to ship your reference with something that you
>> > can show work, then just replace the hard coded panel0-backlight with
>> > the name of the wled backlight device. Customers can change panels as
>> > they wish, but in the event that they plug in a different backlight
>> > controller they would need to modify the code.
>> >
>> > > Hence we wanted to preserve that sysfs node name.
>> > > The wled device is not created under /sys/class/backlight but under
>> > > /sys/class/leds/wled.
>> > > So we will have to change the name of this node across all our
>> > > userspace.
>> > >
>> >
>> > Hard coding /sys/class/backlight/panel0-backlight in your user space
>> > instead of /sys/class/leds/wled is hardly a gain, in particular since
>> > the cost is 94 insertions - per panel driver.
>> >
>> > Regards,
>> > Bjorn
>> > _______________________________________________
>> > Freedreno mailing list
>> > Freedreno@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/freedreno
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno