Re: [PATCH v7 2/8] drm/panel: support for BOE tv101wum-nl6 wuxga dsi video mode panel

From: Sam Ravnborg <sam@ravnborg.org>
To: Jitao Shi <jitao.shi@mediatek.com>
Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	srv_heupstream@mediatek.com, yingjoe.chen@mediatek.com,
	eddie.huang@mediatek.com, cawa.cheng@mediatek.com,
	bibby.hsieh@mediatek.com, linux-mediatek@lists.infradead.org,
	ck.hu@mediatek.com, stonea168@163.com
Subject: Re: [PATCH v7 2/8] drm/panel: support for BOE tv101wum-nl6 wuxga dsi video mode panel
Date: Sun, 5 Jan 2020 12:23:10 +0100	[thread overview]
Message-ID: <20200105112310.GB3309@ravnborg.org> (raw)
In-Reply-To: <20191012030720.27127-3-jitao.shi@mediatek.com>

Hi Jitao.

On Sat, Oct 12, 2019 at 11:07:14AM +0800, Jitao Shi wrote:
> Add driver for BOE tv101wum-nl6 panel is a 10.1" 1200x1920 panel.
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> ---

> +
> +struct boe_panel {
> +	struct drm_panel base;
> +	struct mipi_dsi_device *dsi;
> +
> +	const struct panel_desc *desc;
> +
> +	struct backlight_device *backlight;

drm_panel has gained backlight support in drm-misc-next.
Please use this.

> +	struct regulator *pp1800;
> +	struct regulator *avee;
> +	struct regulator *avdd;
> +	struct gpio_desc *enable_gpio;
> +
> +	bool prepared;
> +	bool enabled;
This flag can be dropped when using drm_panel backlight support.

> +
> +	const struct drm_display_mode *mode;
> +};

> +
> +static int boe_panel_disable(struct drm_panel *panel)
> +{
> +	struct boe_panel *boe = to_boe_panel(panel);
> +
> +	if (!boe->enabled)
> +		return 0;
> +
> +	backlight_disable(boe->backlight);
> +
> +	boe->enabled = false;
> +
> +	return 0;
> +}
This function can be dropped when using the drm_panel backlight support.

> +
> +static int boe_panel_enable(struct drm_panel *panel)
> +{
> +	struct boe_panel *boe = to_boe_panel(panel);
> +	int ret;
> +
> +	if (boe->enabled)
> +		return 0;
> +
> +	ret = backlight_enable(boe->backlight);
> +	if (ret) {
> +		dev_err(panel->dev, "Failed to enable backlight %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	boe->enabled = true;
> +
> +	return 0;
> +}
This function can be dropped when using the drm_panel backlight support.

> +
> +static const struct drm_display_mode boe_tv101wum_nl6_default_mode = {
> +	.clock = 159425,
> +	.hdisplay = 1200,
> +	.hsync_start = 1200 + 100,
> +	.hsync_end = 1200 + 100 + 40,
> +	.htotal = 1200 + 100 + 40 + 24,
> +	.vdisplay = 1920,
> +	.vsync_start = 1920 + 10,
> +	.vsync_end = 1920 + 10 + 14,
> +	.vtotal = 1920 + 10 + 14 + 4,
> +	.vrefresh = 60,
> +};
> +
> +static const struct panel_desc boe_tv101wum_nl6_desc = {
> +	.modes = &boe_tv101wum_nl6_default_mode,
> +	.bpc = 8,
> +	.size = {
> +		.width_mm = 135,
> +		.height_mm = 216,
> +	},
> +	.lanes = 4,
> +	.format = MIPI_DSI_FMT_RGB888,
> +	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> +		      MIPI_DSI_MODE_LPM,
> +	.init_cmds = boe_init_cmd,
> +};
> +
> +static int boe_panel_get_modes(struct drm_panel *panel)
This function now takes drm_connector as second argument.
See drm-misc-next.
> +{
> +	struct boe_panel *boe = to_boe_panel(panel);
> +	const struct drm_display_mode *m = boe->desc->modes;
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, m);
Here you will need to use connector->dev to get the drm_device.

> +	if (!mode) {
> +		dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> +			m->hdisplay, m->vdisplay, m->vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_set_name(mode);
> +	drm_mode_probed_add(panel->connector, mode);
> +
> +	panel->connector->display_info.width_mm = boe->desc->size.width_mm;
> +	panel->connector->display_info.height_mm = boe->desc->size.height_mm;
> +	panel->connector->display_info.bpc = boe->desc->bpc;
There is no connector in drm_panel today.
Use the connector in supplied as second argument.

> +
> +	return 1;
> +}
> +
> +	gpiod_set_value(boe->enable_gpio, 0);
> +
> +	boe->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(boe->backlight))
> +		return PTR_ERR(boe->backlight);
Move this after drm_panel_init and use drm_panel_of_backlight()

The binding say that backlight is mandatory. This is really not checked
here. And I *think* backlight is optional.

> +
> +	drm_panel_init(&boe->base);
Here you need to specify connector_type as second argument.

> +
> +	boe->base.funcs = &boe_panel_funcs;
> +	boe->base.dev = &boe->dsi->dev;
> +
> +	return drm_panel_add(&boe->base);
> +}
> +
> +}
> +
> +static int boe_panel_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct boe_panel *boe = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = boe_panel_disable(&boe->base);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
> +
> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> +
> +	if (boe->base.dev)
> +		drm_panel_remove(&boe->base);
> +
> +	return 0;
> +}
This should just be:

	boe_panel_shutdown();
	ret = mipi_dsi_detach();
	if (ret < 0)
		...
	drm_panel_remove();

> +
> +static void boe_panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct boe_panel *boe = mipi_dsi_get_drvdata(dsi);
> +
> +	boe_panel_disable(&boe->base);
> +}
Please use drm_panel variants here.
And then I had expected to see:
	drm_panel_disable();
	drm_panel_unprepare();

> +
> +static const struct of_device_id boe_of_match[] = {
> +	{ .compatible = "boe,tv101wum-nl6",
> +	  .data = &boe_tv101wum_nl6_desc
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, boe_of_match);
> +
> +static struct mipi_dsi_driver boe_panel_driver = {
> +	.driver = {
> +		.name = "panel-boe-tv101wum-nl6",
> +		.of_match_table = boe_of_match,
> +	},
> +	.probe = boe_panel_probe,
> +	.remove = boe_panel_remove,
> +	.shutdown = boe_panel_shutdown,
> +};
> +module_mipi_dsi_driver(boe_panel_driver);
> +
> +MODULE_AUTHOR("Jitao Shi <jitao.shi@mediatek.com>");
> +MODULE_DESCRIPTION("BOE tv101wum-nl6 1200x1920 video mode panel driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.21.0