[v1,0/2] Add suppport for rm69299 Visionox panel driver and add devicetree bindings for visionox panel.

Current patchset adds support for rm69299 visionox panel driver used in MSM reference platforms.
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.

Changes in v1:
	-add devicetree bindings for visionox panel.
	-Split out panel driver patch from dsi config changes(Rob Clark).
        -Remove unrelated code(Stephen Boyd).
        -Remove static arrays to make regulator setup
         open coded in probe(Stephen Boyd).
        -Remove pre-assigning variables(Stephen Boyd).
        -Inline panel_add function into probe(Stephen Boyd).
        -Use mipi_dsi_dcs_write directly(Rob Clark).
	-Remove qcom_rm69299_1080p_panel_magic_cmds array(Rob Clark).

Harigovindan P (2):
  dt-bindings: display: add sc7180 panel variant
  drm/panel: add support for rm69299 visionox panel driver

 .../bindings/display/visionox,rm69299.txt          |  68 ++++
 drivers/gpu/drm/panel/Kconfig                      |   9 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 drivers/gpu/drm/panel/panel-visionox-rm69299.c     | 412 +++++++++++++++++++++
 4 files changed, 490 insertions(+)
 create mode 100755 Documentation/devicetree/bindings/display/visionox,rm69299.txt
 create mode 100755 drivers/gpu/drm/panel/panel-visionox-rm69299.c

Hi Harigovindan.

Thanks for re-submitting this driver.
There is several more or less trivial comments below.
Please fix and send a v3.

Thanks,

	Sam

On Fri, Nov 29, 2019 at 12:25:45PM +0530, Harigovindan P wrote:
> Add support for Visionox panel driver.
> 
> Changes in v1:
> 	-Split out panel driver patch from dsi config changes(Rob Clark).
>         -Remove unrelated code(Stephen Boyd).
>         -Remove static arrays to make regulator setup open coded
> 	 in probe(Stephen Boyd).
>         -Remove pre-assigning variables(Stephen Boyd).
>         -Inline panel_add function into probe(Stephen Boyd).
>         -Use mipi_dsi_dcs_write directly(Rob Clark).
> 	-Remove qcom_rm69299_1080p_panel_magic_cmds array(Rob Clark).
> 
> 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 | 412 +++++++++++++++++++++++++
>  3 files changed, 422 insertions(+)
>  create mode 100755 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.
Drop redundant space.

> +
>  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 100755
> index 0000000..da86714
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
> @@ -0,0 +1,412 @@
> +// 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_print.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/backlight.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +
> +#include <video/mipi_display.h>

Please use following structure for include files:
#include <linux/*>

#include <video/*>

#include <drm/*>

#include ""

And within each block sort the include lines alphabetically.

> +
> +struct rm69299_config {
> +	unsigned long width_mm;
> +	unsigned long height_mm;
> +	const char *panel_name;
> +	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[2];
> +
> +	struct gpio_desc *reset_gpio;
> +
> +	struct backlight_device *backlight;
backlight is never assigned - drop it.
Your binding does not mention backligth so
this seems to be correct to drop it.

> +
> +	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 int visionox_35597_power_on(struct visionox_rm69299 *ctx)
> +{
> +	int ret;
> +
> +	ret = regulator_set_load(ctx->supplies[0].consumer, 32000);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_set_load(ctx->supplies[1].consumer, 13200);
> +	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;
> +
> +	gpiod_set_value(ctx->reset_gpio, 0);
> +
> +	ret = regulator_set_load(ctx->supplies[0].consumer, 80);
> +
> +	if (ret) {
> +		DRM_DEV_ERROR(ctx->dev,
> +			"regulator_set_load failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_set_load(ctx->supplies[1].consumer, 80);

It looks wrong that you have to change regulator_set_load()
just before the regulators are disabled.
> +
> +	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;
> +
> +	ret = backlight_disable(ctx->backlight);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(ctx->dev, "backlight disable failed %d\n",
> +		ret);
> +
> +	ctx->enabled = false;
> +	return 0;
> +}
With backlight dropped this function is no logner needed.

> +
> +static int visionox_rm69299_unprepare(struct drm_panel *panel)
> +{
> +	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +	int ret;
> +
> +	if (!ctx->prepared)
> +		return 0;
> +
> +	ctx->dsi->mode_flags = 0;
> +
> +	ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_SET_DISPLAY_OFF, NULL, 0);
> +	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 = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_ENTER_SLEEP_MODE, NULL, 0);
> +	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);
visionox_rm69299_power_off() already printed an error message. No need
for two errors messages.
> +
> +	ctx->prepared = false;
> +	return ret;
> +}
> +
> +static int visionox_rm69299_prepare(struct drm_panel *panel)
> +{
> +	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
> +	int ret;
> +	const struct rm69299_config *config;
> +
> +	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;
> +
> +	ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0xfe, 0x00 }, 2);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +		"cmd set tx 0 failed, ret = %d\n",
> +		ret);
> +		goto power_off;
> +	}
> +
> +	ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0xc2, 0x08 }, 2);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +		"cmd set tx 1 failed, ret = %d\n",
> +		ret);
> +		goto power_off;
> +	}
> +
> +	ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0x35, 0x00 }, 2);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +		"cmd set tx 2 failed, ret = %d\n",
> +		ret);
> +		goto power_off;
> +	}
> +
> +	ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0x51, 0xff }, 2);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +		"cmd set tx 3 failed, ret = %d\n",
> +		ret);
> +		goto power_off;
> +	}
> +
> +	ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_EXIT_SLEEP_MODE, NULL, 0);
> +	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 = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);
> +	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");
No need for two error messages. visionox_rm69299_power_off() alredy
printed an error message.

> +	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;
> +
> +	ret = backlight_enable(ctx->backlight);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(ctx->dev, "backlight enable failed %d\n",
> +		ret);
> +
> +	ctx->enabled = true;
> +
> +	return 0;
> +}
With backlight dropped there is no need for this function.

> +
> +static int visionox_rm69299_get_modes(struct drm_panel *panel)
In drm-misc-next this function takes drm_connector as an argument.
Please update.

> +{
> +	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 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,
> +};
> +
> +static int visionox_rm69299_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct device *device;
> +	struct visionox_rm69299 *ctx;
> +	const struct rm69299_config *config;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->config = 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;
> +
> +	device = ctx->dev;
> +
> +	config = ctx->config;
> +	ctx->supplies[0].supply = "vdda";
> +	ctx->supplies[1].supply = "vdd3p3";
> +
> +	ret = devm_regulator_bulk_get(device, ARRAY_SIZE(ctx->supplies),
> +								ctx->supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	ctx->reset_gpio = devm_gpiod_get(device, "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);
> +	}
> +
> +	drm_panel_init(&ctx->panel);
> +	ctx->panel.dev = device;
> +	ctx->panel.funcs = &visionox_rm69299_drm_funcs;
> +	drm_panel_add(&ctx->panel);
> +
> +	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);
> +	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",
Must be updated to the new binding as per Rob's comments.

> +		.data = &rm69299_dir,
> +	},
> +	{ }
Use { /* sentinel */ },


> +};
> +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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel