All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sam Ravnborg <sam@ravnborg.org>
To: Icenowy Zheng <icenowy@aosc.io>
Cc: Rob Herring <robh+dt@kernel.org>,
	Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-sunxi@googlegroups.com
Subject: Re: [PATCH 3/5] drm/panel: Add Feixin K101 IM2BA02 panel
Date: Fri, 10 Jan 2020 18:46:03 +0100	[thread overview]
Message-ID: <20200110174603.GC25099@ravnborg.org> (raw)
In-Reply-To: <20200110155225.1051749-4-icenowy@aosc.io>

Hi Icenowy.

On Fri, Jan 10, 2020 at 11:52:23PM +0800, Icenowy Zheng wrote:
> Feixin K101 IM2BA02 is a 800x1280 4-lane MIPI-DSI LCD panel.
> 
> Add a panel driver for it.
Thanks, driver looks really good.
Only a few comments/nits below.

	Sam
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/gpu/drm/panel/Kconfig                 |   9 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../gpu/drm/panel/panel-feixin-k101-im2ba02.c | 548 ++++++++++++++++++
>  4 files changed, 564 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c
> 

> new file mode 100644
> index 000000000000..45a62bdabb3f
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c
> @@ -0,0 +1,548 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019-2020 Icenowy Zheng <icenowy@aosc.io>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
Sorted and in the right order - good.

> +
> +static int k101_im2ba02_prepare(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	struct mipi_dsi_device *dsi = ctx->dsi;
> +	unsigned int i;
> +	int ret;
> +
> +	ret = regulator_enable(ctx->avdd);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ret = regulator_enable(ctx->dvdd);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ret = regulator_enable(ctx->cvdd);
> +	if (ret)
> +		return ret;

Unless the msleep() between is regulator_enable() really is needed,
this change this to use regulator_bulk_enable().

If the above is kept then please do proper error handling
where all enabled regulators are disable in case of errors.

> +
> +	msleep(30);
> +
> +	gpiod_set_value(ctx->reset, 1);
> +	msleep(50);
> +
> +	gpiod_set_value(ctx->reset, 0);
> +	msleep(50);
> +
> +	gpiod_set_value(ctx->reset, 1);
> +	msleep(200);
> +
> +	for (i = 0; i < ARRAY_SIZE(k101_im2ba02_init_cmds); i++) {
> +		const struct k101_im2ba02_init_cmd *cmd = &k101_im2ba02_init_cmds[i];
> +
> +		ret = mipi_dsi_dcs_write_buffer(dsi, cmd->data, K101_IM2BA02_INIT_CMD_LEN);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int k101_im2ba02_enable(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	const struct k101_im2ba02_init_cmd *cmd = &timed_cmds[1];
> +	int ret;
> +
> +	msleep(150);
> +
> +	ret = mipi_dsi_dcs_set_display_on(ctx->dsi);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(50);
> +
> +	return mipi_dsi_dcs_write_buffer(ctx->dsi, cmd->data, K101_IM2BA02_INIT_CMD_LEN);
> +	if (ret < 0)
> +		return ret;
The last two lines can be deleted - they are never reached.

> +}
> +
> +static int k101_im2ba02_disable(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +
> +	return mipi_dsi_dcs_set_display_off(ctx->dsi);
> +}
> +
> +static int k101_im2ba02_unprepare(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	int ret;
> +
> +	ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(panel->dev, "failed to set display off: %d\n",
> +			      ret);
> +
> +	ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(panel->dev, "failed to enter sleep mode: %d\n",
> +			      ret);
> +
> +	msleep(200);
> +
> +	gpiod_set_value(ctx->reset, 0);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->cvdd);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->dvdd);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->avdd);
Can we get rid of a few empty lines here?
If the 20 msecs are not really needed then use regulator_bulk_disable()


> +	return 0;
> +}
> +
> +static const struct drm_display_mode k101_im2ba02_default_mode = {
> +	.clock = 70000,
> +	.vrefresh = 60,
> +
> +	.hdisplay = 800,
> +	.hsync_start = 800 + 20,
> +	.hsync_end = 800 + 20 + 20,
> +	.htotal = 800 + 20 + 20 + 20,
> +
> +	.vdisplay = 1280,
> +	.vsync_start = 1280 + 16,
> +	.vsync_end = 1280 + 16 + 4,
> +	.vtotal = 1280 + 16 + 4 + 4,
> +
> +	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +	.width_mm	= 136,
> +	.height_mm	= 217,
> +};
> +
> +static int k101_im2ba02_get_modes(struct drm_panel *panel,
> +				  struct drm_connector *connector)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(connector->dev, &k101_im2ba02_default_mode);
> +	if (!mode) {
> +		DRM_DEV_ERROR(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
> +			      k101_im2ba02_default_mode.hdisplay,
> +			      k101_im2ba02_default_mode.vdisplay,
> +			      k101_im2ba02_default_mode.vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	connector->display_info.width_mm = mode->width_mm;
> +	connector->display_info.height_mm = mode->height_mm;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs k101_im2ba02_funcs = {
> +	.disable = k101_im2ba02_disable,
> +	.unprepare = k101_im2ba02_unprepare,
> +	.prepare = k101_im2ba02_prepare,
> +	.enable = k101_im2ba02_enable,
> +	.get_modes = k101_im2ba02_get_modes,
> +};
> +
> +static int k101_im2ba02_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct k101_im2ba02 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +	ctx->dsi = dsi;
> +
> +	ctx->dvdd = devm_regulator_get(&dsi->dev, "dvdd");
> +	if (IS_ERR(ctx->dvdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get dvdd regulator\n");
> +		return PTR_ERR(ctx->dvdd);
> +	}
> +
> +	ctx->avdd = devm_regulator_get(&dsi->dev, "avdd");
> +	if (IS_ERR(ctx->avdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get avdd regulator\n");
> +		return PTR_ERR(ctx->avdd);
> +	}
> +
> +	ctx->cvdd = devm_regulator_get(&dsi->dev, "cvdd");
> +	if (IS_ERR(ctx->cvdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get cvdd regulator\n");
> +		return PTR_ERR(ctx->cvdd);
> +	}
> +
> +	ctx->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get our reset GPIO\n");
> +		return PTR_ERR(ctx->reset);
> +	}
> +
> +	drm_panel_init(&ctx->panel, &dsi->dev, &k101_im2ba02_funcs,
> +		       DRM_MODE_CONNECTOR_DSI);
> +
> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_panel_add(&ctx->panel);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->lanes = 4;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		drm_panel_remove(&ctx->panel);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int k101_im2ba02_dsi_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct k101_im2ba02 *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&ctx->panel);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id k101_im2ba02_of_match[] = {
> +	{ .compatible = "feixin,k101-im2ba02", },
> +	{ }
Use { /* sentinel */ },

> +};
> +MODULE_DEVICE_TABLE(of, k101_im2ba02_of_match);
> +
> +static struct mipi_dsi_driver k101_im2ba02_driver = {
> +	.probe = k101_im2ba02_dsi_probe,
> +	.remove = k101_im2ba02_dsi_remove,
> +	.driver = {
> +		.name = "feixin-k101-im2ba02",
> +		.of_match_table = k101_im2ba02_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(k101_im2ba02_driver);
> +
> +MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
> +MODULE_DESCRIPTION("Feixin K101 IM2BA02 MIPI-DSI LCD panel");
> +MODULE_LICENSE("GPL");
> -- 
> 2.23.0

WARNING: multiple messages have this Message-ID
From: Sam Ravnborg <sam@ravnborg.org>
To: Icenowy Zheng <icenowy@aosc.io>
Cc: devicetree@vger.kernel.org, David Airlie <airlied@linux.ie>,
	linux-sunxi@googlegroups.com, linux-kernel@vger.kernel.org,
	Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Rob Herring <robh+dt@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	dri-devel@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/5] drm/panel: Add Feixin K101 IM2BA02 panel
Date: Fri, 10 Jan 2020 18:46:03 +0100	[thread overview]
Message-ID: <20200110174603.GC25099@ravnborg.org> (raw)
In-Reply-To: <20200110155225.1051749-4-icenowy@aosc.io>

Hi Icenowy.

On Fri, Jan 10, 2020 at 11:52:23PM +0800, Icenowy Zheng wrote:
> Feixin K101 IM2BA02 is a 800x1280 4-lane MIPI-DSI LCD panel.
> 
> Add a panel driver for it.
Thanks, driver looks really good.
Only a few comments/nits below.

	Sam
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/gpu/drm/panel/Kconfig                 |   9 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../gpu/drm/panel/panel-feixin-k101-im2ba02.c | 548 ++++++++++++++++++
>  4 files changed, 564 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c
> 

> new file mode 100644
> index 000000000000..45a62bdabb3f
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c
> @@ -0,0 +1,548 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019-2020 Icenowy Zheng <icenowy@aosc.io>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
Sorted and in the right order - good.

> +
> +static int k101_im2ba02_prepare(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	struct mipi_dsi_device *dsi = ctx->dsi;
> +	unsigned int i;
> +	int ret;
> +
> +	ret = regulator_enable(ctx->avdd);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ret = regulator_enable(ctx->dvdd);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ret = regulator_enable(ctx->cvdd);
> +	if (ret)
> +		return ret;

Unless the msleep() between is regulator_enable() really is needed,
this change this to use regulator_bulk_enable().

If the above is kept then please do proper error handling
where all enabled regulators are disable in case of errors.

> +
> +	msleep(30);
> +
> +	gpiod_set_value(ctx->reset, 1);
> +	msleep(50);
> +
> +	gpiod_set_value(ctx->reset, 0);
> +	msleep(50);
> +
> +	gpiod_set_value(ctx->reset, 1);
> +	msleep(200);
> +
> +	for (i = 0; i < ARRAY_SIZE(k101_im2ba02_init_cmds); i++) {
> +		const struct k101_im2ba02_init_cmd *cmd = &k101_im2ba02_init_cmds[i];
> +
> +		ret = mipi_dsi_dcs_write_buffer(dsi, cmd->data, K101_IM2BA02_INIT_CMD_LEN);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int k101_im2ba02_enable(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	const struct k101_im2ba02_init_cmd *cmd = &timed_cmds[1];
> +	int ret;
> +
> +	msleep(150);
> +
> +	ret = mipi_dsi_dcs_set_display_on(ctx->dsi);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(50);
> +
> +	return mipi_dsi_dcs_write_buffer(ctx->dsi, cmd->data, K101_IM2BA02_INIT_CMD_LEN);
> +	if (ret < 0)
> +		return ret;
The last two lines can be deleted - they are never reached.

> +}
> +
> +static int k101_im2ba02_disable(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +
> +	return mipi_dsi_dcs_set_display_off(ctx->dsi);
> +}
> +
> +static int k101_im2ba02_unprepare(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	int ret;
> +
> +	ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(panel->dev, "failed to set display off: %d\n",
> +			      ret);
> +
> +	ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(panel->dev, "failed to enter sleep mode: %d\n",
> +			      ret);
> +
> +	msleep(200);
> +
> +	gpiod_set_value(ctx->reset, 0);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->cvdd);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->dvdd);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->avdd);
Can we get rid of a few empty lines here?
If the 20 msecs are not really needed then use regulator_bulk_disable()


> +	return 0;
> +}
> +
> +static const struct drm_display_mode k101_im2ba02_default_mode = {
> +	.clock = 70000,
> +	.vrefresh = 60,
> +
> +	.hdisplay = 800,
> +	.hsync_start = 800 + 20,
> +	.hsync_end = 800 + 20 + 20,
> +	.htotal = 800 + 20 + 20 + 20,
> +
> +	.vdisplay = 1280,
> +	.vsync_start = 1280 + 16,
> +	.vsync_end = 1280 + 16 + 4,
> +	.vtotal = 1280 + 16 + 4 + 4,
> +
> +	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +	.width_mm	= 136,
> +	.height_mm	= 217,
> +};
> +
> +static int k101_im2ba02_get_modes(struct drm_panel *panel,
> +				  struct drm_connector *connector)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(connector->dev, &k101_im2ba02_default_mode);
> +	if (!mode) {
> +		DRM_DEV_ERROR(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
> +			      k101_im2ba02_default_mode.hdisplay,
> +			      k101_im2ba02_default_mode.vdisplay,
> +			      k101_im2ba02_default_mode.vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	connector->display_info.width_mm = mode->width_mm;
> +	connector->display_info.height_mm = mode->height_mm;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs k101_im2ba02_funcs = {
> +	.disable = k101_im2ba02_disable,
> +	.unprepare = k101_im2ba02_unprepare,
> +	.prepare = k101_im2ba02_prepare,
> +	.enable = k101_im2ba02_enable,
> +	.get_modes = k101_im2ba02_get_modes,
> +};
> +
> +static int k101_im2ba02_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct k101_im2ba02 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +	ctx->dsi = dsi;
> +
> +	ctx->dvdd = devm_regulator_get(&dsi->dev, "dvdd");
> +	if (IS_ERR(ctx->dvdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get dvdd regulator\n");
> +		return PTR_ERR(ctx->dvdd);
> +	}
> +
> +	ctx->avdd = devm_regulator_get(&dsi->dev, "avdd");
> +	if (IS_ERR(ctx->avdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get avdd regulator\n");
> +		return PTR_ERR(ctx->avdd);
> +	}
> +
> +	ctx->cvdd = devm_regulator_get(&dsi->dev, "cvdd");
> +	if (IS_ERR(ctx->cvdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get cvdd regulator\n");
> +		return PTR_ERR(ctx->cvdd);
> +	}
> +
> +	ctx->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get our reset GPIO\n");
> +		return PTR_ERR(ctx->reset);
> +	}
> +
> +	drm_panel_init(&ctx->panel, &dsi->dev, &k101_im2ba02_funcs,
> +		       DRM_MODE_CONNECTOR_DSI);
> +
> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_panel_add(&ctx->panel);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->lanes = 4;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		drm_panel_remove(&ctx->panel);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int k101_im2ba02_dsi_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct k101_im2ba02 *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&ctx->panel);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id k101_im2ba02_of_match[] = {
> +	{ .compatible = "feixin,k101-im2ba02", },
> +	{ }
Use { /* sentinel */ },

> +};
> +MODULE_DEVICE_TABLE(of, k101_im2ba02_of_match);
> +
> +static struct mipi_dsi_driver k101_im2ba02_driver = {
> +	.probe = k101_im2ba02_dsi_probe,
> +	.remove = k101_im2ba02_dsi_remove,
> +	.driver = {
> +		.name = "feixin-k101-im2ba02",
> +		.of_match_table = k101_im2ba02_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(k101_im2ba02_driver);
> +
> +MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
> +MODULE_DESCRIPTION("Feixin K101 IM2BA02 MIPI-DSI LCD panel");
> +MODULE_LICENSE("GPL");
> -- 
> 2.23.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID
From: Sam Ravnborg <sam@ravnborg.org>
To: Icenowy Zheng <icenowy@aosc.io>
Cc: devicetree@vger.kernel.org, David Airlie <airlied@linux.ie>,
	linux-sunxi@googlegroups.com, linux-kernel@vger.kernel.org,
	Chen-Yu Tsai <wens@csie.org>, Rob Herring <robh+dt@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/5] drm/panel: Add Feixin K101 IM2BA02 panel
Date: Fri, 10 Jan 2020 18:46:03 +0100	[thread overview]
Message-ID: <20200110174603.GC25099@ravnborg.org> (raw)
In-Reply-To: <20200110155225.1051749-4-icenowy@aosc.io>

Hi Icenowy.

On Fri, Jan 10, 2020 at 11:52:23PM +0800, Icenowy Zheng wrote:
> Feixin K101 IM2BA02 is a 800x1280 4-lane MIPI-DSI LCD panel.
> 
> Add a panel driver for it.
Thanks, driver looks really good.
Only a few comments/nits below.

	Sam
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/gpu/drm/panel/Kconfig                 |   9 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../gpu/drm/panel/panel-feixin-k101-im2ba02.c | 548 ++++++++++++++++++
>  4 files changed, 564 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c
> 

> new file mode 100644
> index 000000000000..45a62bdabb3f
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c
> @@ -0,0 +1,548 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019-2020 Icenowy Zheng <icenowy@aosc.io>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
Sorted and in the right order - good.

> +
> +static int k101_im2ba02_prepare(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	struct mipi_dsi_device *dsi = ctx->dsi;
> +	unsigned int i;
> +	int ret;
> +
> +	ret = regulator_enable(ctx->avdd);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ret = regulator_enable(ctx->dvdd);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ret = regulator_enable(ctx->cvdd);
> +	if (ret)
> +		return ret;

Unless the msleep() between is regulator_enable() really is needed,
this change this to use regulator_bulk_enable().

If the above is kept then please do proper error handling
where all enabled regulators are disable in case of errors.

> +
> +	msleep(30);
> +
> +	gpiod_set_value(ctx->reset, 1);
> +	msleep(50);
> +
> +	gpiod_set_value(ctx->reset, 0);
> +	msleep(50);
> +
> +	gpiod_set_value(ctx->reset, 1);
> +	msleep(200);
> +
> +	for (i = 0; i < ARRAY_SIZE(k101_im2ba02_init_cmds); i++) {
> +		const struct k101_im2ba02_init_cmd *cmd = &k101_im2ba02_init_cmds[i];
> +
> +		ret = mipi_dsi_dcs_write_buffer(dsi, cmd->data, K101_IM2BA02_INIT_CMD_LEN);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int k101_im2ba02_enable(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	const struct k101_im2ba02_init_cmd *cmd = &timed_cmds[1];
> +	int ret;
> +
> +	msleep(150);
> +
> +	ret = mipi_dsi_dcs_set_display_on(ctx->dsi);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(50);
> +
> +	return mipi_dsi_dcs_write_buffer(ctx->dsi, cmd->data, K101_IM2BA02_INIT_CMD_LEN);
> +	if (ret < 0)
> +		return ret;
The last two lines can be deleted - they are never reached.

> +}
> +
> +static int k101_im2ba02_disable(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +
> +	return mipi_dsi_dcs_set_display_off(ctx->dsi);
> +}
> +
> +static int k101_im2ba02_unprepare(struct drm_panel *panel)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	int ret;
> +
> +	ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(panel->dev, "failed to set display off: %d\n",
> +			      ret);
> +
> +	ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(panel->dev, "failed to enter sleep mode: %d\n",
> +			      ret);
> +
> +	msleep(200);
> +
> +	gpiod_set_value(ctx->reset, 0);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->cvdd);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->dvdd);
> +
> +	msleep(20);
> +
> +	regulator_disable(ctx->avdd);
Can we get rid of a few empty lines here?
If the 20 msecs are not really needed then use regulator_bulk_disable()


> +	return 0;
> +}
> +
> +static const struct drm_display_mode k101_im2ba02_default_mode = {
> +	.clock = 70000,
> +	.vrefresh = 60,
> +
> +	.hdisplay = 800,
> +	.hsync_start = 800 + 20,
> +	.hsync_end = 800 + 20 + 20,
> +	.htotal = 800 + 20 + 20 + 20,
> +
> +	.vdisplay = 1280,
> +	.vsync_start = 1280 + 16,
> +	.vsync_end = 1280 + 16 + 4,
> +	.vtotal = 1280 + 16 + 4 + 4,
> +
> +	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +	.width_mm	= 136,
> +	.height_mm	= 217,
> +};
> +
> +static int k101_im2ba02_get_modes(struct drm_panel *panel,
> +				  struct drm_connector *connector)
> +{
> +	struct k101_im2ba02 *ctx = panel_to_k101_im2ba02(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(connector->dev, &k101_im2ba02_default_mode);
> +	if (!mode) {
> +		DRM_DEV_ERROR(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
> +			      k101_im2ba02_default_mode.hdisplay,
> +			      k101_im2ba02_default_mode.vdisplay,
> +			      k101_im2ba02_default_mode.vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	connector->display_info.width_mm = mode->width_mm;
> +	connector->display_info.height_mm = mode->height_mm;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs k101_im2ba02_funcs = {
> +	.disable = k101_im2ba02_disable,
> +	.unprepare = k101_im2ba02_unprepare,
> +	.prepare = k101_im2ba02_prepare,
> +	.enable = k101_im2ba02_enable,
> +	.get_modes = k101_im2ba02_get_modes,
> +};
> +
> +static int k101_im2ba02_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct k101_im2ba02 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +	ctx->dsi = dsi;
> +
> +	ctx->dvdd = devm_regulator_get(&dsi->dev, "dvdd");
> +	if (IS_ERR(ctx->dvdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get dvdd regulator\n");
> +		return PTR_ERR(ctx->dvdd);
> +	}
> +
> +	ctx->avdd = devm_regulator_get(&dsi->dev, "avdd");
> +	if (IS_ERR(ctx->avdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get avdd regulator\n");
> +		return PTR_ERR(ctx->avdd);
> +	}
> +
> +	ctx->cvdd = devm_regulator_get(&dsi->dev, "cvdd");
> +	if (IS_ERR(ctx->cvdd)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get cvdd regulator\n");
> +		return PTR_ERR(ctx->cvdd);
> +	}
> +
> +	ctx->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset)) {
> +		DRM_DEV_ERROR(&dsi->dev, "Couldn't get our reset GPIO\n");
> +		return PTR_ERR(ctx->reset);
> +	}
> +
> +	drm_panel_init(&ctx->panel, &dsi->dev, &k101_im2ba02_funcs,
> +		       DRM_MODE_CONNECTOR_DSI);
> +
> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_panel_add(&ctx->panel);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->lanes = 4;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		drm_panel_remove(&ctx->panel);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int k101_im2ba02_dsi_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct k101_im2ba02 *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&ctx->panel);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id k101_im2ba02_of_match[] = {
> +	{ .compatible = "feixin,k101-im2ba02", },
> +	{ }
Use { /* sentinel */ },

> +};
> +MODULE_DEVICE_TABLE(of, k101_im2ba02_of_match);
> +
> +static struct mipi_dsi_driver k101_im2ba02_driver = {
> +	.probe = k101_im2ba02_dsi_probe,
> +	.remove = k101_im2ba02_dsi_remove,
> +	.driver = {
> +		.name = "feixin-k101-im2ba02",
> +		.of_match_table = k101_im2ba02_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(k101_im2ba02_driver);
> +
> +MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
> +MODULE_DESCRIPTION("Feixin K101 IM2BA02 MIPI-DSI LCD panel");
> +MODULE_LICENSE("GPL");
> -- 
> 2.23.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2020-01-10 17:46 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 15:52 [PATCH 0/5] Add support for Pine64 PineTab Icenowy Zheng
2020-01-10 15:52 ` Icenowy Zheng
2020-01-10 15:52 ` Icenowy Zheng
2020-01-10 15:52 ` [PATCH 1/5] dt-bindings: vendor-prefix: add Shenzhen Feixin Photoelectics Co., Ltd Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 17:07   ` Sam Ravnborg
2020-01-10 17:07     ` Sam Ravnborg
2020-01-10 17:07     ` Sam Ravnborg
2020-01-15 15:22   ` Rob Herring
2020-01-15 15:22     ` Rob Herring
2020-01-15 15:22     ` Rob Herring
2020-01-10 15:52 ` [PATCH 2/5] dt-bindings: panel: add Feixin K101 IM2BA02 MIPI-DSI panel Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 17:12   ` Sam Ravnborg
2020-01-10 17:12     ` Sam Ravnborg
2020-01-10 17:12     ` Sam Ravnborg
2020-01-10 15:52 ` [PATCH 3/5] drm/panel: Add Feixin K101 IM2BA02 panel Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 17:46   ` Sam Ravnborg [this message]
2020-01-10 17:46     ` Sam Ravnborg
2020-01-10 17:46     ` Sam Ravnborg
2020-01-10 15:52 ` [PATCH 4/5] dt-bindings: arm: sunxi: add binding for PineTab tablet Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-15 15:24   ` Rob Herring
2020-01-15 15:24     ` Rob Herring
2020-01-15 15:24     ` Rob Herring
2020-01-10 15:52 ` [PATCH 5/5] arm64: dts: allwinner: a64: add support for PineTab Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 15:52   ` Icenowy Zheng
2020-01-10 17:23   ` Maxime Ripard
2020-01-10 17:23     ` Maxime Ripard
2020-01-10 17:23     ` Maxime Ripard
2020-01-10 18:12   ` Sam Ravnborg
2020-01-10 18:12     ` Sam Ravnborg
2020-01-10 18:12     ` Sam Ravnborg
2020-01-10 18:51   ` [linux-sunxi] " Vasily Khoruzhick
2020-01-10 18:51     ` Vasily Khoruzhick
2020-01-10 18:51     ` Vasily Khoruzhick

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200110174603.GC25099@ravnborg.org \
    --to=sam@ravnborg.org \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=icenowy@aosc.io \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=mripard@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.