[PATCH v1 0/2] Add suppport for rm69299 Visionox panel driver and add DSI config to support DSI version

[PATCH v1 0/2] Add suppport for rm69299 Visionox panel driver and add DSI config to support DSI version

[Harigovindan P]

Current patchset adds support for rm69299 visionox panel driver used in MSM reference platforms 
and also adds DSI config that supports the respective DSI version.

The visionox panel driver supports a resolution of 1080x2248 with 4 lanes and supports only single DSI mode.

Current patchset is tested on actual panel.

Harigovindan P (2):
  drm/panel: add support for rm69299 visionox panel driver
  drm/msm: add DSI config changes to support DSI version

 drivers/gpu/drm/msm/dsi/dsi_cfg.c              |  21 ++
 drivers/gpu/drm/msm/dsi/dsi_cfg.h              |   1 +
 drivers/gpu/drm/panel/Kconfig                  |   9 +
 drivers/gpu/drm/panel/Makefile                 |   1 +
 drivers/gpu/drm/panel/panel-visionox-rm69299.c | 478 +++++++++++++++++++++++++
 5 files changed, 510 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c

-- 
2.7.4

[PATCH v1 1/2] drm/panel: add support for rm69299 visionox panel driver

[Harigovindan P]

Add support for Visionox panel driver.

Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
---
 drivers/gpu/drm/panel/Kconfig                  |   9 +
 drivers/gpu/drm/panel/Makefile                 |   1 +
 drivers/gpu/drm/panel/panel-visionox-rm69299.c | 478 +++++++++++++++++++++++++
 3 files changed, 488 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index f152bc4..c06c403 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -355,4 +355,13 @@ config DRM_PANEL_TRULY_NT35597_WQXGA
 	help
 	  Say Y here if you want to enable support for Truly NT35597 WQXGA Dual DSI
 	  Video Mode panel
+
+config DRM_PANEL_VISIONOX_RM69299
+	tristate "Visionox RM69299"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for Visionox
+	  RM69299  DSI Video Mode panel.
+
 endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index b6cd39f..6f1e4c6 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_PANEL_TPO_TD028TTEC1) += panel-tpo-td028ttec1.o
 obj-$(CONFIG_DRM_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o
 obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o
 obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
+obj-$(CONFIG_DRM_PANEL_VISIONOX_RM69299) += panel-visionox-rm69299.o
diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
new file mode 100644
index 0000000..faf6d05
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
@@ -0,0 +1,478 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include <drm/drm_panel.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+
+#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/backlight.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+
+#include <video/mipi_display.h>
+
+static const char * const regulator_names[] = {
+	"vdda",
+	"vdd3p3",
+};
+
+static unsigned long const regulator_enable_loads[] = {
+	32000,
+	13200,
+};
+
+static unsigned long const regulator_disable_loads[] = {
+	80,
+	80,
+};
+
+struct cmd_set {
+	u8 commands[4];
+	u8 size;
+};
+
+struct rm69299_config {
+	u32 width_mm;
+	u32 height_mm;
+	const char *panel_name;
+	const struct cmd_set *panel_on_cmds;
+	u32 num_on_cmds;
+	const struct drm_display_mode *dm;
+};
+
+struct visionox_rm69299 {
+	struct device *dev;
+	struct drm_panel panel;
+
+	struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
+
+	struct gpio_desc *reset_gpio;
+
+	struct backlight_device *backlight;
+
+	struct mipi_dsi_device *dsi;
+	const struct rm69299_config *config;
+	bool prepared;
+	bool enabled;
+};
+
+static inline struct visionox_rm69299 *panel_to_ctx(struct drm_panel *panel)
+{
+	return container_of(panel, struct visionox_rm69299, panel);
+}
+
+static const struct cmd_set qcom_rm69299_1080p_panel_magic_cmds[] = {
+	{ { 0xfe, 0x00 }, 2 },
+	{ { 0xc2, 0x08 }, 2 },
+	{ { 0x35, 0x00 }, 2 },
+	{ { 0x51, 0xff }, 2 },
+};
+
+static int visionox_dcs_write(struct drm_panel *panel, u32 command)
+{
+	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+	int i = 0, ret;
+
+	ret = mipi_dsi_dcs_write(ctx->dsi, command, NULL, 0);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev,
+			"cmd 0x%x failed for dsi = %d\n",
+			command, i);
+	}
+
+	return ret;
+}
+
+static int visionox_dcs_write_buf(struct drm_panel *panel,
+	u32 size, const u8 *buf)
+{
+	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+	int ret = 0;
+	int i = 0;
+
+	ret = mipi_dsi_dcs_write_buffer(ctx->dsi, buf, size);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev,
+			"failed to tx cmd [%d], err: %d\n", i, ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int visionox_35597_power_on(struct visionox_rm69299 *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;
+
+	/*
+	 * Reset sequence of visionox panel requires the panel to be
+	 * out of reset for 10ms, followed by being held in reset
+	 * for 10ms and then out again
+	 */
+	gpiod_set_value(ctx->reset_gpio, 1);
+	usleep_range(10000, 20000);
+	gpiod_set_value(ctx->reset_gpio, 0);
+	usleep_range(10000, 20000);
+	gpiod_set_value(ctx->reset_gpio, 1);
+	usleep_range(10000, 20000);
+
+	return 0;
+}
+
+static int visionox_rm69299_power_off(struct visionox_rm69299 *ctx)
+{
+	int ret = 0;
+	int 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) {
+			DRM_DEV_ERROR(ctx->dev,
+				"regulator_set_load failed %d\n", ret);
+			return ret;
+		}
+	}
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (ret) {
+		DRM_DEV_ERROR(ctx->dev,
+			"regulator_bulk_disable failed %d\n", ret);
+	}
+	return ret;
+}
+
+static int visionox_rm69299_disable(struct drm_panel *panel)
+{
+	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+	int ret;
+
+	if (!ctx->enabled)
+		return 0;
+
+	if (ctx->backlight) {
+		ret = backlight_disable(ctx->backlight);
+		if (ret < 0)
+			DRM_DEV_ERROR(ctx->dev, "backlight disable failed %d\n",
+				ret);
+	}
+
+	ctx->enabled = false;
+	return 0;
+}
+
+static int visionox_rm69299_unprepare(struct drm_panel *panel)
+{
+	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+	int ret = 0;
+
+	if (!ctx->prepared)
+		return 0;
+
+	ctx->dsi->mode_flags = 0;
+
+	ret = visionox_dcs_write(panel, MIPI_DCS_SET_DISPLAY_OFF);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev,
+			"set_display_off cmd failed ret = %d\n",
+			ret);
+	}
+
+	/* 120ms delay required here as per DCS spec */
+	msleep(120);
+
+	ret = visionox_dcs_write(panel, MIPI_DCS_ENTER_SLEEP_MODE);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev,
+			"enter_sleep cmd failed ret = %d\n", ret);
+	}
+
+	ret = visionox_rm69299_power_off(ctx);
+	if (ret < 0)
+		DRM_DEV_ERROR(ctx->dev, "power_off failed ret = %d\n", ret);
+
+	ctx->prepared = false;
+	return ret;
+}
+
+static int visionox_rm69299_prepare(struct drm_panel *panel)
+{
+	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+	int ret;
+	int i;
+	const struct cmd_set *panel_on_cmds;
+	const struct rm69299_config *config;
+	u32 num_cmds;
+
+	if (ctx->prepared)
+		return 0;
+
+	ret = visionox_35597_power_on(ctx);
+	if (ret < 0)
+		return ret;
+
+	ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	config = ctx->config;
+	panel_on_cmds = config->panel_on_cmds;
+	num_cmds = config->num_on_cmds;
+
+	for (i = 0; i < num_cmds; i++) {
+		ret = visionox_dcs_write_buf(panel,
+				panel_on_cmds[i].size,
+					panel_on_cmds[i].commands);
+		if (ret < 0) {
+			DRM_DEV_ERROR(ctx->dev,
+				"cmd set tx failed i = %d ret = %d\n",
+					i, ret);
+			goto power_off;
+		}
+	}
+
+	ret = visionox_dcs_write(panel, MIPI_DCS_EXIT_SLEEP_MODE);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev,
+			"exit_sleep_mode cmd failed ret = %d\n",
+			ret);
+		goto power_off;
+	}
+
+	/* Per DSI spec wait 120ms after sending exit sleep DCS command */
+	msleep(120);
+
+	ret = visionox_dcs_write(panel, MIPI_DCS_SET_DISPLAY_ON);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev,
+			"set_display_on cmd failed ret = %d\n", ret);
+		goto power_off;
+	}
+
+	/* Per DSI spec wait 120ms after sending set_display_on DCS command */
+	msleep(120);
+
+	ctx->prepared = true;
+
+	return 0;
+
+power_off:
+	if (visionox_rm69299_power_off(ctx))
+		DRM_DEV_ERROR(ctx->dev, "power_off failed\n");
+	return ret;
+}
+
+static int visionox_rm69299_enable(struct drm_panel *panel)
+{
+	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+	int ret;
+
+	if (ctx->enabled)
+		return 0;
+
+	if (ctx->backlight) {
+		ret = backlight_enable(ctx->backlight);
+		if (ret < 0)
+			DRM_DEV_ERROR(ctx->dev, "backlight enable failed %d\n",
+						  ret);
+	}
+
+	ctx->enabled = true;
+
+	return 0;
+}
+
+static int visionox_rm69299_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+	struct drm_display_mode *mode;
+	const struct rm69299_config *config;
+
+	config = ctx->config;
+	mode = drm_mode_create(connector->dev);
+	if (!mode) {
+		DRM_DEV_ERROR(ctx->dev,
+			"failed to create a new display mode\n");
+		return 0;
+	}
+
+	connector->display_info.width_mm = config->width_mm;
+	connector->display_info.height_mm = config->height_mm;
+	drm_mode_copy(mode, config->dm);
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs visionox_rm69299_drm_funcs = {
+	.disable = visionox_rm69299_disable,
+	.unprepare = visionox_rm69299_unprepare,
+	.prepare = visionox_rm69299_prepare,
+	.enable = visionox_rm69299_enable,
+	.get_modes = visionox_rm69299_get_modes,
+};
+
+static int visionox_rm69299_panel_add(struct visionox_rm69299 *ctx)
+{
+	struct device *dev = ctx->dev;
+	int ret, i;
+	const struct rm69299_config *config;
+
+	config = ctx->config;
+	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_LOW);
+	if (IS_ERR(ctx->reset_gpio)) {
+		DRM_DEV_ERROR(dev, "cannot get reset gpio %ld\n",
+			PTR_ERR(ctx->reset_gpio));
+		return PTR_ERR(ctx->reset_gpio);
+	}
+
+	ret = gpiod_direction_output(ctx->reset_gpio, 0);
+	if(ret < 0) {
+		pr_err("direction output failed \n");
+	}
+
+	drm_panel_init(&ctx->panel);
+	ctx->panel.dev = dev;
+	ctx->panel.funcs = &visionox_rm69299_drm_funcs;
+	drm_panel_add(&ctx->panel);
+
+	return 0;
+}
+
+static const struct drm_display_mode qcom_sc7180_mtp_1080p_mode = {
+	.name = "1080x2248",
+	.clock = 158695,
+	.hdisplay = 1080,
+	.hsync_start = 1080 + 26,
+	.hsync_end = 1080 + 26 + 2,
+	.htotal = 1080 + 26 + 2 + 36,
+	.vdisplay = 2248,
+	.vsync_start = 2248 + 56,
+	.vsync_end = 2248 + 56 + 4,
+	.vtotal = 2248 + 56 + 4 + 4,
+	.vrefresh = 60,
+	.flags = 0,
+};
+
+static const struct rm69299_config rm69299_dir = {
+	.width_mm = 74,
+	.height_mm = 131,
+	.panel_name = "qcom_sc7180_mtp_1080p_panel",
+	.dm = &qcom_sc7180_mtp_1080p_mode,
+	.panel_on_cmds = qcom_rm69299_1080p_panel_magic_cmds,
+	.num_on_cmds = ARRAY_SIZE(qcom_rm69299_1080p_panel_magic_cmds),
+};
+
+static int visionox_rm69299_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct visionox_rm69299 *ctx;
+	int ret = 0;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->config = of_device_get_match_data(dev);
+
+	if (!ctx->config) {
+		dev_err(dev, "missing device configuration\n");
+		return -ENODEV;
+	}
+
+	mipi_dsi_set_drvdata(dsi, ctx);
+
+	ctx->dev = dev;
+	ctx->dsi = dsi;
+
+	ret = visionox_rm69299_panel_add(ctx);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "failed to add panel\n");
+		goto err_panel_add;
+	}
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_LPM |
+		MIPI_DSI_CLOCK_NON_CONTINUOUS;
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev,
+			"dsi attach failed ret = %d\n", ret);
+		goto err_dsi_attach;
+	}
+
+	return 0;
+
+err_dsi_attach:
+	drm_panel_remove(&ctx->panel);
+err_panel_add:
+	mipi_dsi_device_unregister(dsi);
+	return ret;
+}
+
+static int visionox_rm69299_remove(struct mipi_dsi_device *dsi)
+{
+	struct visionox_rm69299 *ctx = mipi_dsi_get_drvdata(dsi);
+
+	if (ctx->dsi) {
+		mipi_dsi_detach(ctx->dsi);
+		mipi_dsi_device_unregister(ctx->dsi);
+	}
+
+	drm_panel_remove(&ctx->panel);
+	return 0;
+}
+
+static const struct of_device_id visionox_rm69299_of_match[] = {
+	{
+		.compatible = "visionox,rm69299-1080p-display",
+		.data = &rm69299_dir,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, visionox_rm69299_of_match);
+
+static struct mipi_dsi_driver visionox_rm69299_driver = {
+	.driver = {
+		.name = "panel-visionox-rm69299",
+		.of_match_table = visionox_rm69299_of_match,
+	},
+	.probe = visionox_rm69299_probe,
+	.remove = visionox_rm69299_remove,
+};
+module_mipi_dsi_driver(visionox_rm69299_driver);
+
+MODULE_DESCRIPTION("VISIONOX RM69299 DSI Panel Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH v1 2/2] drm/msm: add DSI config changes to support DSI version

[Harigovindan P]

Add DSI config changes to support DSI version.

Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi_cfg.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/dsi_cfg.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index b7b7c1a..d2c4592 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -133,6 +133,10 @@ static const char * const dsi_sdm845_bus_clk_names[] = {
 	"iface", "bus",
 };
 
+static const char * const dsi_sc7180_bus_clk_names[] = {
+        "iface", "bus",
+};
+
 static const struct msm_dsi_config sdm845_dsi_cfg = {
 	.io_offset = DSI_6G_REG_SHIFT,
 	.reg_cfg = {
@@ -147,6 +151,20 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
 	.num_dsi = 2,
 };
 
+static const struct msm_dsi_config sc7180_dsi_cfg = {
+	.io_offset = DSI_6G_REG_SHIFT,
+	.reg_cfg = {
+		.num = 1,
+		.regs = {
+			{"vdda", 21800, 4 },	/* 1.2 V */
+		},
+	},
+	.bus_clk_names = dsi_sc7180_bus_clk_names,
+	.num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
+	.io_start = { 0xae94000 },
+	.num_dsi = 1,
+};
+
 const static struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
 	.link_clk_enable = dsi_link_clk_enable_v2,
 	.link_clk_disable = dsi_link_clk_disable_v2,
@@ -201,6 +219,9 @@ static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
 		&msm8998_dsi_cfg, &msm_dsi_6g_v2_host_ops},
 	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1,
 		&sdm845_dsi_cfg, &msm_dsi_6g_v2_host_ops},
+	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_4_1,
+		&sc7180_dsi_cfg, &msm_dsi_6g_v2_host_ops},
+
 };
 
 const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
index e2b7a7d..9919536 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
@@ -19,6 +19,7 @@
 #define MSM_DSI_6G_VER_MINOR_V1_4_1	0x10040001
 #define MSM_DSI_6G_VER_MINOR_V2_2_0	0x20000000
 #define MSM_DSI_6G_VER_MINOR_V2_2_1	0x20020001
+#define MSM_DSI_6G_VER_MINOR_V2_4_1	0x20040001
 
 #define MSM_DSI_V2_VER_MINOR_8064	0x0
 
-- 
2.7.4

Re: [PATCH v1 1/2] drm/panel: add support for rm69299 visionox panel driver

[Stephen Boyd]

Quoting Harigovindan P (2019-11-14 02:16:27)
> diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
> new file mode 100644
> index 0000000..faf6d05
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
> @@ -0,0 +1,478 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <drm/drm_panel.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>

This is included twice. It's really a panel!

> +#include <drm/drm_print.h>
> +
> +#include <linux/of_gpio.h>

Is this include used?

> +#include <linux/gpio/consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/of_graph.h>

Is this include used?

> +#include <linux/pinctrl/consumer.h>

Is this include used?

> +#include <linux/regulator/consumer.h>
> +#include <linux/backlight.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +
> +#include <video/mipi_display.h>
> +
> +static const char * const regulator_names[] = {
> +       "vdda",
> +       "vdd3p3",
> +};
> +
> +static unsigned long const regulator_enable_loads[] = {
> +       32000,
> +       13200,
> +};
> +
> +static unsigned long const regulator_disable_loads[] = {
> +       80,
> +       80,
> +};

Why do we need these static arrays for two regulators? Is the panel
going to have additional regulators added in the future? I'd prefer
these arrays go away and the regulator setup is open coded in probe.
Maybe that means dropping the use of bulk APIs, I'm not sure.

> +
> +struct cmd_set {
> +       u8 commands[4];
> +       u8 size;
> +};
> +
> +struct rm69299_config {
> +       u32 width_mm;
> +       u32 height_mm;

Why u32 for these types? Do they need to be 32 bits wide or can they be
more generic like unsigned long types?

> +       const char *panel_name;
> +       const struct cmd_set *panel_on_cmds;
> +       u32 num_on_cmds;
> +       const struct drm_display_mode *dm;
> +};
> +
> +struct visionox_rm69299 {
> +       struct device *dev;
> +       struct drm_panel panel;
> +
> +       struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
> +
> +       struct gpio_desc *reset_gpio;
> +
> +       struct backlight_device *backlight;
> +
> +       struct mipi_dsi_device *dsi;
> +       const struct rm69299_config *config;
> +       bool prepared;
> +       bool enabled;
> +};
> +
> +static inline struct visionox_rm69299 *panel_to_ctx(struct drm_panel *panel)
> +{
> +       return container_of(panel, struct visionox_rm69299, panel);
> +}
> +
> +static const struct cmd_set qcom_rm69299_1080p_panel_magic_cmds[] = {

I like magic.

> +       { { 0xfe, 0x00 }, 2 },
> +       { { 0xc2, 0x08 }, 2 },
> +       { { 0x35, 0x00 }, 2 },
> +       { { 0x51, 0xff }, 2 },
> +};
> +
> +static int visionox_dcs_write(struct drm_panel *panel, u32 command)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int i = 0, ret;
> +
> +       ret = mipi_dsi_dcs_write(ctx->dsi, command, NULL, 0);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "cmd 0x%x failed for dsi = %d\n",
> +                       command, i);
> +       }
> +
> +       return ret;
> +}
> +
> +static int visionox_dcs_write_buf(struct drm_panel *panel,
> +       u32 size, const u8 *buf)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret = 0;
> +       int i = 0;

Please don't assign variables by default and then reassign them without
testing them in between. Also, i is never used so it's confusing that it
isn't just hardcoded into the printk message as '0'.

> +
> +       ret = mipi_dsi_dcs_write_buffer(ctx->dsi, buf, size);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "failed to tx cmd [%d], err: %d\n", i, ret);
> +               return ret;
> +       }
> +
> +       return ret;
> +}
> +
> +static int visionox_35597_power_on(struct visionox_rm69299 *ctx)
> +{
> +       int ret, i;

Nitpick: Add a newline between declarations and statements.

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

We forgot to drop the load... O well. Probably best to just never drop
the load anyway, see below.

> +
> +       /*
> +        * Reset sequence of visionox panel requires the panel to be
> +        * out of reset for 10ms, followed by being held in reset
> +        * for 10ms and then out again
> +        */
> +       gpiod_set_value(ctx->reset_gpio, 1);
> +       usleep_range(10000, 20000);
> +       gpiod_set_value(ctx->reset_gpio, 0);
> +       usleep_range(10000, 20000);
> +       gpiod_set_value(ctx->reset_gpio, 1);
> +       usleep_range(10000, 20000);
> +
> +       return 0;
> +}
> +
> +static int visionox_rm69299_power_off(struct visionox_rm69299 *ctx)
> +{
> +       int ret = 0;

Ugh. Please stop pre-assigning ret.

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

Do we need to drop the load? I thought that disabling the regulator
would make the regulator core stop considering the load from these
consumers.

> +               if (ret) {
> +                       DRM_DEV_ERROR(ctx->dev,
> +                               "regulator_set_load failed %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +       if (ret) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "regulator_bulk_disable failed %d\n", ret);
> +       }
> +       return ret;
> +}
> +
> +static int visionox_rm69299_disable(struct drm_panel *panel)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret;
> +
> +       if (!ctx->enabled)
> +               return 0;
> +
> +       if (ctx->backlight) {

backlight_disable() and backlight_enable() already check for NULL so
these NULL checks can go away.

> +               ret = backlight_disable(ctx->backlight);
> +               if (ret < 0)
> +                       DRM_DEV_ERROR(ctx->dev, "backlight disable failed %d\n",
> +                               ret);
> +       }
> +
> +       ctx->enabled = false;
> +       return 0;
> +}
> +
> +static int visionox_rm69299_unprepare(struct drm_panel *panel)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret = 0;
> +
> +       if (!ctx->prepared)
> +               return 0;
> +
> +       ctx->dsi->mode_flags = 0;
> +
> +       ret = visionox_dcs_write(panel, MIPI_DCS_SET_DISPLAY_OFF);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "set_display_off cmd failed ret = %d\n",
> +                       ret);

There will be double error prints in this case. Can we just have one
print instead?

> +       }
> +
> +       /* 120ms delay required here as per DCS spec */
> +       msleep(120);
> +
> +       ret = visionox_dcs_write(panel, MIPI_DCS_ENTER_SLEEP_MODE);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "enter_sleep cmd failed ret = %d\n", ret);
> +       }
> +
> +       ret = visionox_rm69299_power_off(ctx);
> +       if (ret < 0)
> +               DRM_DEV_ERROR(ctx->dev, "power_off failed ret = %d\n", ret);
> +
> +       ctx->prepared = false;
> +       return ret;
> +}
> +
> +static int visionox_rm69299_prepare(struct drm_panel *panel)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret;
> +       int i;
> +       const struct cmd_set *panel_on_cmds;
> +       const struct rm69299_config *config;
> +       u32 num_cmds;
> +
> +       if (ctx->prepared)
> +               return 0;
> +
> +       ret = visionox_35597_power_on(ctx);
> +       if (ret < 0)
> +               return ret;
> +
> +       ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +       config = ctx->config;
> +       panel_on_cmds = config->panel_on_cmds;
> +       num_cmds = config->num_on_cmds;
> +
> +       for (i = 0; i < num_cmds; i++) {
> +               ret = visionox_dcs_write_buf(panel,
> +                               panel_on_cmds[i].size,
> +                                       panel_on_cmds[i].commands);
> +               if (ret < 0) {
> +                       DRM_DEV_ERROR(ctx->dev,
> +                               "cmd set tx failed i = %d ret = %d\n",
> +                                       i, ret);
> +                       goto power_off;
> +               }
> +       }
> +
> +       ret = visionox_dcs_write(panel, MIPI_DCS_EXIT_SLEEP_MODE);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "exit_sleep_mode cmd failed ret = %d\n",
> +                       ret);
> +               goto power_off;
> +       }
> +
> +       /* Per DSI spec wait 120ms after sending exit sleep DCS command */
> +       msleep(120);
> +
> +       ret = visionox_dcs_write(panel, MIPI_DCS_SET_DISPLAY_ON);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "set_display_on cmd failed ret = %d\n", ret);
> +               goto power_off;
> +       }
> +
> +       /* Per DSI spec wait 120ms after sending set_display_on DCS command */
> +       msleep(120);
> +
> +       ctx->prepared = true;
> +
> +       return 0;
> +
> +power_off:
> +       if (visionox_rm69299_power_off(ctx))
> +               DRM_DEV_ERROR(ctx->dev, "power_off failed\n");
> +       return ret;
> +}
> +
> +static int visionox_rm69299_enable(struct drm_panel *panel)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret;
> +
> +       if (ctx->enabled)
> +               return 0;
> +
> +       if (ctx->backlight) {
> +               ret = backlight_enable(ctx->backlight);
> +               if (ret < 0)
> +                       DRM_DEV_ERROR(ctx->dev, "backlight enable failed %d\n",
> +                                                 ret);
> +       }
> +
> +       ctx->enabled = true;
> +
> +       return 0;
> +}
> +
> +static int visionox_rm69299_get_modes(struct drm_panel *panel)
> +{
> +       struct drm_connector *connector = panel->connector;
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       struct drm_display_mode *mode;
> +       const struct rm69299_config *config;
> +
> +       config = ctx->config;
> +       mode = drm_mode_create(connector->dev);
> +       if (!mode) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "failed to create a new display mode\n");
> +               return 0;
> +       }
> +
> +       connector->display_info.width_mm = config->width_mm;
> +       connector->display_info.height_mm = config->height_mm;
> +       drm_mode_copy(mode, config->dm);
> +       mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +       drm_mode_probed_add(connector, mode);
> +
> +       return 1;
> +}
> +
> +static const struct drm_panel_funcs visionox_rm69299_drm_funcs = {
> +       .disable = visionox_rm69299_disable,
> +       .unprepare = visionox_rm69299_unprepare,
> +       .prepare = visionox_rm69299_prepare,
> +       .enable = visionox_rm69299_enable,
> +       .get_modes = visionox_rm69299_get_modes,
> +};
> +
> +static int visionox_rm69299_panel_add(struct visionox_rm69299 *ctx)

Please inline this function into probe. It's not helping much to be
split out.

> +{
> +       struct device *dev = ctx->dev;
> +       int ret, i;
> +       const struct rm69299_config *config;
> +
> +       config = ctx->config;
> +       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_LOW);
> +       if (IS_ERR(ctx->reset_gpio)) {
> +               DRM_DEV_ERROR(dev, "cannot get reset gpio %ld\n",
> +                       PTR_ERR(ctx->reset_gpio));
> +               return PTR_ERR(ctx->reset_gpio);
> +       }
> +
> +       ret = gpiod_direction_output(ctx->reset_gpio, 0);

Didn't the devm_gpiod_get() call above make the gpio use output
direction already? Why call this again?

> +       if(ret < 0) {
> +               pr_err("direction output failed \n");
> +       }
> +
> +       drm_panel_init(&ctx->panel);
> +       ctx->panel.dev = dev;
> +       ctx->panel.funcs = &visionox_rm69299_drm_funcs;
> +       drm_panel_add(&ctx->panel);
> +
> +       return 0;
> +}
> +
> +static const struct drm_display_mode qcom_sc7180_mtp_1080p_mode = {
> +       .name = "1080x2248",
> +       .clock = 158695,
> +       .hdisplay = 1080,
> +       .hsync_start = 1080 + 26,
> +       .hsync_end = 1080 + 26 + 2,
> +       .htotal = 1080 + 26 + 2 + 36,
> +       .vdisplay = 2248,
> +       .vsync_start = 2248 + 56,
> +       .vsync_end = 2248 + 56 + 4,
> +       .vtotal = 2248 + 56 + 4 + 4,
> +       .vrefresh = 60,
> +       .flags = 0,
> +};
> +
> +static const struct rm69299_config rm69299_dir = {
> +       .width_mm = 74,
> +       .height_mm = 131,
> +       .panel_name = "qcom_sc7180_mtp_1080p_panel",
> +       .dm = &qcom_sc7180_mtp_1080p_mode,
> +       .panel_on_cmds = qcom_rm69299_1080p_panel_magic_cmds,
> +       .num_on_cmds = ARRAY_SIZE(qcom_rm69299_1080p_panel_magic_cmds),
> +};
> +
> +static int visionox_rm69299_probe(struct mipi_dsi_device *dsi)
> +{
> +       struct device *dev = &dsi->dev;
> +       struct visionox_rm69299 *ctx;
> +       int ret = 0;
> +
> +       ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +
> +       if (!ctx)
> +               return -ENOMEM;
> +
> +       ctx->config = of_device_get_match_data(dev);

Call device_get_match_data() instead?

> +
> +       if (!ctx->config) {
> +               dev_err(dev, "missing device configuration\n");
> +               return -ENODEV;
> +       }
> +

Re: [PATCH v1 0/2] Add suppport for rm69299 Visionox panel driver and add DSI config to support DSI version

[Stephen Boyd]

Quoting Harigovindan P (2019-11-14 02:16:26)
> Current patchset adds support for rm69299 visionox panel driver used in MSM reference platforms 
> and also adds DSI config that supports the respective DSI version.
> 
> The visionox panel driver supports a resolution of 1080x2248 with 4 lanes and supports only single DSI mode.
> 
> Current patchset is tested on actual panel.

Do you have the DT binding patch?

Re: [PATCH v1 2/2] drm/msm: add DSI config changes to support DSI version

[Rob Clark]

On Thu, Nov 14, 2019 at 2:16 AM Harigovindan P <harigovi@codeaurora.org> wrote:
>
> Add DSI config changes to support DSI version.
>
> Signed-off-by: Harigovindan P <harigovi@codeaurora.org>

Reviewed-by: Rob Clark <robdclark@gmail.com>

For patch 1/2 with the panel driver, probably best to split that out
into a different patch(set), since panel drivers are merged into
drm-next via a different tree

BR,
-R

> ---
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 21 +++++++++++++++++++++
>  drivers/gpu/drm/msm/dsi/dsi_cfg.h |  1 +
>  2 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index b7b7c1a..d2c4592 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -133,6 +133,10 @@ static const char * const dsi_sdm845_bus_clk_names[] = {
>         "iface", "bus",
>  };
>
> +static const char * const dsi_sc7180_bus_clk_names[] = {
> +        "iface", "bus",
> +};
> +
>  static const struct msm_dsi_config sdm845_dsi_cfg = {
>         .io_offset = DSI_6G_REG_SHIFT,
>         .reg_cfg = {
> @@ -147,6 +151,20 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
>         .num_dsi = 2,
>  };
>
> +static const struct msm_dsi_config sc7180_dsi_cfg = {
> +       .io_offset = DSI_6G_REG_SHIFT,
> +       .reg_cfg = {
> +               .num = 1,
> +               .regs = {
> +                       {"vdda", 21800, 4 },    /* 1.2 V */
> +               },
> +       },
> +       .bus_clk_names = dsi_sc7180_bus_clk_names,
> +       .num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
> +       .io_start = { 0xae94000 },
> +       .num_dsi = 1,
> +};
> +
>  const static struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
>         .link_clk_enable = dsi_link_clk_enable_v2,
>         .link_clk_disable = dsi_link_clk_disable_v2,
> @@ -201,6 +219,9 @@ static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
>                 &msm8998_dsi_cfg, &msm_dsi_6g_v2_host_ops},
>         {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1,
>                 &sdm845_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> +       {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_4_1,
> +               &sc7180_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> +
>  };
>
>  const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> index e2b7a7d..9919536 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> @@ -19,6 +19,7 @@
>  #define MSM_DSI_6G_VER_MINOR_V1_4_1    0x10040001
>  #define MSM_DSI_6G_VER_MINOR_V2_2_0    0x20000000
>  #define MSM_DSI_6G_VER_MINOR_V2_2_1    0x20020001
> +#define MSM_DSI_6G_VER_MINOR_V2_4_1    0x20040001
>
>  #define MSM_DSI_V2_VER_MINOR_8064      0x0
>
> --
> 2.7.4
>

Re: [Freedreno] [PATCH v1 2/2] drm/msm: add DSI config changes to support DSI version

[Jeffrey Hugo]

On Thu, Nov 14, 2019 at 10:47 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Thu, Nov 14, 2019 at 2:16 AM Harigovindan P <harigovi@codeaurora.org> wrote:
> >
> > Add DSI config changes to support DSI version.
> >
> > Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
>
> Reviewed-by: Rob Clark <robdclark@gmail.com>

Can we fix the subject/commit text for this to indicate what DSI
version and/or SoC this is for?  This is a SoC enablement patch, but
at first I thought this was some bug fix or something.

>
> For patch 1/2 with the panel driver, probably best to split that out
> into a different patch(set), since panel drivers are merged into
> drm-next via a different tree
>
> BR,
> -R
>
> > ---
> >  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 21 +++++++++++++++++++++
> >  drivers/gpu/drm/msm/dsi/dsi_cfg.h |  1 +
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> > index b7b7c1a..d2c4592 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> > @@ -133,6 +133,10 @@ static const char * const dsi_sdm845_bus_clk_names[] = {
> >         "iface", "bus",
> >  };
> >
> > +static const char * const dsi_sc7180_bus_clk_names[] = {
> > +        "iface", "bus",
> > +};
> > +
> >  static const struct msm_dsi_config sdm845_dsi_cfg = {
> >         .io_offset = DSI_6G_REG_SHIFT,
> >         .reg_cfg = {
> > @@ -147,6 +151,20 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
> >         .num_dsi = 2,
> >  };
> >
> > +static const struct msm_dsi_config sc7180_dsi_cfg = {
> > +       .io_offset = DSI_6G_REG_SHIFT,
> > +       .reg_cfg = {
> > +               .num = 1,
> > +               .regs = {
> > +                       {"vdda", 21800, 4 },    /* 1.2 V */
> > +               },
> > +       },
> > +       .bus_clk_names = dsi_sc7180_bus_clk_names,
> > +       .num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
> > +       .io_start = { 0xae94000 },
> > +       .num_dsi = 1,
> > +};
> > +
> >  const static struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
> >         .link_clk_enable = dsi_link_clk_enable_v2,
> >         .link_clk_disable = dsi_link_clk_disable_v2,
> > @@ -201,6 +219,9 @@ static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
> >                 &msm8998_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> >         {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1,
> >                 &sdm845_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> > +       {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_4_1,
> > +               &sc7180_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> > +
> >  };
> >
> >  const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> > index e2b7a7d..9919536 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> > @@ -19,6 +19,7 @@
> >  #define MSM_DSI_6G_VER_MINOR_V1_4_1    0x10040001
> >  #define MSM_DSI_6G_VER_MINOR_V2_2_0    0x20000000
> >  #define MSM_DSI_6G_VER_MINOR_V2_2_1    0x20020001
> > +#define MSM_DSI_6G_VER_MINOR_V2_4_1    0x20040001
> >
> >  #define MSM_DSI_V2_VER_MINOR_8064      0x0
> >
> > --
> > 2.7.4
> >
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v1 1/2] drm/panel: add support for rm69299 visionox panel driver

[Rob Clark]

On Thu, Nov 14, 2019 at 2:17 AM Harigovindan P <harigovi@codeaurora.org> wrote:
>
> Add support for Visionox panel driver.
>
> Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
> ---
>  drivers/gpu/drm/panel/Kconfig                  |   9 +
>  drivers/gpu/drm/panel/Makefile                 |   1 +
>  drivers/gpu/drm/panel/panel-visionox-rm69299.c | 478 +++++++++++++++++++++++++
>  3 files changed, 488 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index f152bc4..c06c403 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -355,4 +355,13 @@ config DRM_PANEL_TRULY_NT35597_WQXGA
>         help
>           Say Y here if you want to enable support for Truly NT35597 WQXGA Dual DSI
>           Video Mode panel
> +
> +config DRM_PANEL_VISIONOX_RM69299
> +       tristate "Visionox RM69299"
> +       depends on OF
> +       depends on DRM_MIPI_DSI
> +       help
> +         Say Y here if you want to enable support for Visionox
> +         RM69299  DSI Video Mode panel.
> +
>  endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index b6cd39f..6f1e4c6 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_DRM_PANEL_TPO_TD028TTEC1) += panel-tpo-td028ttec1.o
>  obj-$(CONFIG_DRM_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o
>  obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o
>  obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
> +obj-$(CONFIG_DRM_PANEL_VISIONOX_RM69299) += panel-visionox-rm69299.o
> diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
> new file mode 100644
> index 0000000..faf6d05
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
> @@ -0,0 +1,478 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <drm/drm_panel.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +
> +#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/of_graph.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/backlight.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +
> +#include <video/mipi_display.h>
> +
> +static const char * const regulator_names[] = {
> +       "vdda",
> +       "vdd3p3",
> +};
> +
> +static unsigned long const regulator_enable_loads[] = {
> +       32000,
> +       13200,
> +};
> +
> +static unsigned long const regulator_disable_loads[] = {
> +       80,
> +       80,
> +};
> +
> +struct cmd_set {
> +       u8 commands[4];
> +       u8 size;
> +};
> +
> +struct rm69299_config {
> +       u32 width_mm;
> +       u32 height_mm;
> +       const char *panel_name;
> +       const struct cmd_set *panel_on_cmds;
> +       u32 num_on_cmds;
> +       const struct drm_display_mode *dm;
> +};
> +
> +struct visionox_rm69299 {
> +       struct device *dev;
> +       struct drm_panel panel;
> +
> +       struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
> +
> +       struct gpio_desc *reset_gpio;
> +
> +       struct backlight_device *backlight;
> +
> +       struct mipi_dsi_device *dsi;
> +       const struct rm69299_config *config;
> +       bool prepared;
> +       bool enabled;
> +};
> +
> +static inline struct visionox_rm69299 *panel_to_ctx(struct drm_panel *panel)
> +{
> +       return container_of(panel, struct visionox_rm69299, panel);
> +}
> +
> +static const struct cmd_set qcom_rm69299_1080p_panel_magic_cmds[] = {
> +       { { 0xfe, 0x00 }, 2 },
> +       { { 0xc2, 0x08 }, 2 },
> +       { { 0x35, 0x00 }, 2 },
> +       { { 0x51, 0xff }, 2 },
> +};

I'm not really seeing the value of a table of cmds, vs just a sequence
of mipi_dsi_dcs_write_buffer() calls.. it is only used in one place,
so it just seems like a level of indirection that doesn't add value,
compared to something like:

  mipi_dsi_dsc_write_buffer(dsi, (u8[]){ 0xfe, 0x00 }, 2);

(otoh, some of the other panel so maybe Thierry is less picky)

> +
> +static int visionox_dcs_write(struct drm_panel *panel, u32 command)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int i = 0, ret;
> +
> +       ret = mipi_dsi_dcs_write(ctx->dsi, command, NULL, 0);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "cmd 0x%x failed for dsi = %d\n",
> +                       command, i);
> +       }
> +
> +       return ret;
> +}
> +
> +static int visionox_dcs_write_buf(struct drm_panel *panel,
> +       u32 size, const u8 *buf)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret = 0;
> +       int i = 0;
> +
> +       ret = mipi_dsi_dcs_write_buffer(ctx->dsi, buf, size);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "failed to tx cmd [%d], err: %d\n", i, ret);
> +               return ret;
> +       }
> +
> +       return ret;
> +}

drop these two and just use mipi_dsi_dcs_write(_buffer)() directly..
the only extra thing these seem to do is error reporting, which is
already done at the call-sites of these.

> +
> +static int visionox_35597_power_on(struct visionox_rm69299 *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;
> +
> +       /*
> +        * Reset sequence of visionox panel requires the panel to be
> +        * out of reset for 10ms, followed by being held in reset
> +        * for 10ms and then out again
> +        */
> +       gpiod_set_value(ctx->reset_gpio, 1);
> +       usleep_range(10000, 20000);
> +       gpiod_set_value(ctx->reset_gpio, 0);
> +       usleep_range(10000, 20000);
> +       gpiod_set_value(ctx->reset_gpio, 1);
> +       usleep_range(10000, 20000);
> +
> +       return 0;
> +}
> +
> +static int visionox_rm69299_power_off(struct visionox_rm69299 *ctx)
> +{
> +       int ret = 0;
> +       int 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) {
> +                       DRM_DEV_ERROR(ctx->dev,
> +                               "regulator_set_load failed %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +       if (ret) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "regulator_bulk_disable failed %d\n", ret);
> +       }
> +       return ret;
> +}
> +
> +static int visionox_rm69299_disable(struct drm_panel *panel)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret;
> +
> +       if (!ctx->enabled)
> +               return 0;
> +
> +       if (ctx->backlight) {
> +               ret = backlight_disable(ctx->backlight);
> +               if (ret < 0)
> +                       DRM_DEV_ERROR(ctx->dev, "backlight disable failed %d\n",
> +                               ret);
> +       }
> +
> +       ctx->enabled = false;
> +       return 0;
> +}
> +
> +static int visionox_rm69299_unprepare(struct drm_panel *panel)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret = 0;
> +
> +       if (!ctx->prepared)
> +               return 0;
> +
> +       ctx->dsi->mode_flags = 0;
> +
> +       ret = visionox_dcs_write(panel, MIPI_DCS_SET_DISPLAY_OFF);

mipi_dsi_dcs_set_display_off()

> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "set_display_off cmd failed ret = %d\n",
> +                       ret);
> +       }
> +
> +       /* 120ms delay required here as per DCS spec */
> +       msleep(120);
> +
> +       ret = visionox_dcs_write(panel, MIPI_DCS_ENTER_SLEEP_MODE);

mipi_dsi_dcs_enter_sleep_mode()

(same comment at a few points below as well)

BR,
-R


> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "enter_sleep cmd failed ret = %d\n", ret);
> +       }
> +
> +       ret = visionox_rm69299_power_off(ctx);
> +       if (ret < 0)
> +               DRM_DEV_ERROR(ctx->dev, "power_off failed ret = %d\n", ret);
> +
> +       ctx->prepared = false;
> +       return ret;
> +}
> +
> +static int visionox_rm69299_prepare(struct drm_panel *panel)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret;
> +       int i;
> +       const struct cmd_set *panel_on_cmds;
> +       const struct rm69299_config *config;
> +       u32 num_cmds;
> +
> +       if (ctx->prepared)
> +               return 0;
> +
> +       ret = visionox_35597_power_on(ctx);
> +       if (ret < 0)
> +               return ret;
> +
> +       ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +       config = ctx->config;
> +       panel_on_cmds = config->panel_on_cmds;
> +       num_cmds = config->num_on_cmds;
> +
> +       for (i = 0; i < num_cmds; i++) {
> +               ret = visionox_dcs_write_buf(panel,
> +                               panel_on_cmds[i].size,
> +                                       panel_on_cmds[i].commands);
> +               if (ret < 0) {
> +                       DRM_DEV_ERROR(ctx->dev,
> +                               "cmd set tx failed i = %d ret = %d\n",
> +                                       i, ret);
> +                       goto power_off;
> +               }
> +       }
> +
> +       ret = visionox_dcs_write(panel, MIPI_DCS_EXIT_SLEEP_MODE);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "exit_sleep_mode cmd failed ret = %d\n",
> +                       ret);
> +               goto power_off;
> +       }
> +
> +       /* Per DSI spec wait 120ms after sending exit sleep DCS command */
> +       msleep(120);
> +
> +       ret = visionox_dcs_write(panel, MIPI_DCS_SET_DISPLAY_ON);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "set_display_on cmd failed ret = %d\n", ret);
> +               goto power_off;
> +       }
> +
> +       /* Per DSI spec wait 120ms after sending set_display_on DCS command */
> +       msleep(120);
> +
> +       ctx->prepared = true;
> +
> +       return 0;
> +
> +power_off:
> +       if (visionox_rm69299_power_off(ctx))
> +               DRM_DEV_ERROR(ctx->dev, "power_off failed\n");
> +       return ret;
> +}
> +
> +static int visionox_rm69299_enable(struct drm_panel *panel)
> +{
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       int ret;
> +
> +       if (ctx->enabled)
> +               return 0;
> +
> +       if (ctx->backlight) {
> +               ret = backlight_enable(ctx->backlight);
> +               if (ret < 0)
> +                       DRM_DEV_ERROR(ctx->dev, "backlight enable failed %d\n",
> +                                                 ret);
> +       }
> +
> +       ctx->enabled = true;
> +
> +       return 0;
> +}
> +
> +static int visionox_rm69299_get_modes(struct drm_panel *panel)
> +{
> +       struct drm_connector *connector = panel->connector;
> +       struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +       struct drm_display_mode *mode;
> +       const struct rm69299_config *config;
> +
> +       config = ctx->config;
> +       mode = drm_mode_create(connector->dev);
> +       if (!mode) {
> +               DRM_DEV_ERROR(ctx->dev,
> +                       "failed to create a new display mode\n");
> +               return 0;
> +       }
> +
> +       connector->display_info.width_mm = config->width_mm;
> +       connector->display_info.height_mm = config->height_mm;
> +       drm_mode_copy(mode, config->dm);
> +       mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +       drm_mode_probed_add(connector, mode);
> +
> +       return 1;
> +}
> +
> +static const struct drm_panel_funcs visionox_rm69299_drm_funcs = {
> +       .disable = visionox_rm69299_disable,
> +       .unprepare = visionox_rm69299_unprepare,
> +       .prepare = visionox_rm69299_prepare,
> +       .enable = visionox_rm69299_enable,
> +       .get_modes = visionox_rm69299_get_modes,
> +};
> +
> +static int visionox_rm69299_panel_add(struct visionox_rm69299 *ctx)
> +{
> +       struct device *dev = ctx->dev;
> +       int ret, i;
> +       const struct rm69299_config *config;
> +
> +       config = ctx->config;
> +       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_LOW);
> +       if (IS_ERR(ctx->reset_gpio)) {
> +               DRM_DEV_ERROR(dev, "cannot get reset gpio %ld\n",
> +                       PTR_ERR(ctx->reset_gpio));
> +               return PTR_ERR(ctx->reset_gpio);
> +       }
> +
> +       ret = gpiod_direction_output(ctx->reset_gpio, 0);
> +       if(ret < 0) {
> +               pr_err("direction output failed \n");
> +       }
> +
> +       drm_panel_init(&ctx->panel);
> +       ctx->panel.dev = dev;
> +       ctx->panel.funcs = &visionox_rm69299_drm_funcs;
> +       drm_panel_add(&ctx->panel);
> +
> +       return 0;
> +}
> +
> +static const struct drm_display_mode qcom_sc7180_mtp_1080p_mode = {
> +       .name = "1080x2248",
> +       .clock = 158695,
> +       .hdisplay = 1080,
> +       .hsync_start = 1080 + 26,
> +       .hsync_end = 1080 + 26 + 2,
> +       .htotal = 1080 + 26 + 2 + 36,
> +       .vdisplay = 2248,
> +       .vsync_start = 2248 + 56,
> +       .vsync_end = 2248 + 56 + 4,
> +       .vtotal = 2248 + 56 + 4 + 4,
> +       .vrefresh = 60,
> +       .flags = 0,
> +};
> +
> +static const struct rm69299_config rm69299_dir = {
> +       .width_mm = 74,
> +       .height_mm = 131,
> +       .panel_name = "qcom_sc7180_mtp_1080p_panel",
> +       .dm = &qcom_sc7180_mtp_1080p_mode,
> +       .panel_on_cmds = qcom_rm69299_1080p_panel_magic_cmds,
> +       .num_on_cmds = ARRAY_SIZE(qcom_rm69299_1080p_panel_magic_cmds),
> +};
> +
> +static int visionox_rm69299_probe(struct mipi_dsi_device *dsi)
> +{
> +       struct device *dev = &dsi->dev;
> +       struct visionox_rm69299 *ctx;
> +       int ret = 0;
> +
> +       ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +
> +       if (!ctx)
> +               return -ENOMEM;
> +
> +       ctx->config = of_device_get_match_data(dev);
> +
> +       if (!ctx->config) {
> +               dev_err(dev, "missing device configuration\n");
> +               return -ENODEV;
> +       }
> +
> +       mipi_dsi_set_drvdata(dsi, ctx);
> +
> +       ctx->dev = dev;
> +       ctx->dsi = dsi;
> +
> +       ret = visionox_rm69299_panel_add(ctx);
> +       if (ret) {
> +               DRM_DEV_ERROR(dev, "failed to add panel\n");
> +               goto err_panel_add;
> +       }
> +
> +       dsi->lanes = 4;
> +       dsi->format = MIPI_DSI_FMT_RGB888;
> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_LPM |
> +               MIPI_DSI_CLOCK_NON_CONTINUOUS;
> +       ret = mipi_dsi_attach(dsi);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(dev,
> +                       "dsi attach failed ret = %d\n", ret);
> +               goto err_dsi_attach;
> +       }
> +
> +       return 0;
> +
> +err_dsi_attach:
> +       drm_panel_remove(&ctx->panel);
> +err_panel_add:
> +       mipi_dsi_device_unregister(dsi);
> +       return ret;
> +}
> +
> +static int visionox_rm69299_remove(struct mipi_dsi_device *dsi)
> +{
> +       struct visionox_rm69299 *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +       if (ctx->dsi) {
> +               mipi_dsi_detach(ctx->dsi);
> +               mipi_dsi_device_unregister(ctx->dsi);
> +       }
> +
> +       drm_panel_remove(&ctx->panel);
> +       return 0;
> +}
> +
> +static const struct of_device_id visionox_rm69299_of_match[] = {
> +       {
> +               .compatible = "visionox,rm69299-1080p-display",
> +               .data = &rm69299_dir,
> +       },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, visionox_rm69299_of_match);
> +
> +static struct mipi_dsi_driver visionox_rm69299_driver = {
> +       .driver = {
> +               .name = "panel-visionox-rm69299",
> +               .of_match_table = visionox_rm69299_of_match,
> +       },
> +       .probe = visionox_rm69299_probe,
> +       .remove = visionox_rm69299_remove,
> +};
> +module_mipi_dsi_driver(visionox_rm69299_driver);
> +
> +MODULE_DESCRIPTION("VISIONOX RM69299 DSI Panel Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>