Re: [PATCH 4/4] drm/panel: Add OSD101T2587-53TS driver

[Sam Ravnborg <sam@ravnborg.org>]

Hi Peter.

Good with more panel drivers.
Some comments in the following, please do not blindly follow them but
check that this is OK.

	Sam

On Fri, Feb 15, 2019 at 04:03:15PM +0200, Peter Ujfalusi via dri-devel wrote:
> The panel is similar to OSD101T2045-53TS (which is handled by panel-simple)
> with one big difference: osd101t2587-53ts needs MIPI_DSI_TURN_ON_PERIPHERAL
> message to be sent from the host to be operational and thus can not be
> handled by panel-simple.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> +++ b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com
> + *  Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
Please do not use drmP.h in new drivers - we try to get rid of this file.

> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +struct osd101t2587_panel {
> +	struct drm_panel base;
> +	struct mipi_dsi_device *dsi;
> +
> +	struct backlight_device *backlight;
> +	struct regulator *supply;
> +
> +	bool prepared;
> +	bool enabled;
> +
> +	ktime_t earliest_wake;
earliest_wake is not used, and can be deleted.

> +
> +	const struct drm_display_mode *default_mode;
> +	const struct drm_display_mode *mode;
Assigned but never used. Drop the assignment and the variable?

> +};
> +
> +static inline struct osd101t2587_panel *to_osd101t2587_panel(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct osd101t2587_panel, base);
> +}
> +
> +static int osd101t2587_panel_disable(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +	int ret;
> +
> +	if (!osd101t2587->enabled)
> +		return 0;
> +
> +	if (osd101t2587->backlight) {
> +		osd101t2587->backlight->props.power = FB_BLANK_POWERDOWN;
> +		osd101t2587->backlight->props.state |= BL_CORE_FBBLANK;
> +		backlight_update_status(osd101t2587->backlight);
> +	}
backlight_disable(osd101t2587->backlight);


> +
> +	ret = mipi_dsi_shutdown_peripheral(osd101t2587->dsi);
> +
> +	osd101t2587->enabled = false;
> +
> +	return ret;
> +}
> +
> +static int osd101t2587_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +
> +	if (!osd101t2587->prepared)
> +		return 0;
> +
> +	regulator_disable(osd101t2587->supply);
> +	osd101t2587->prepared = false;
> +
> +	return 0;
> +}
> +
> +static int osd101t2587_panel_prepare(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +	int ret;
> +
> +	if (osd101t2587->prepared)
> +		return 0;
> +
> +	ret = regulator_enable(osd101t2587->supply);
> +	if (!ret)
> +		osd101t2587->prepared = true;
> +
> +	return ret;
> +}
> +
> +static int osd101t2587_panel_enable(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +	int ret;
> +
> +	if (osd101t2587->enabled)
> +		return 0;
> +
> +	ret = mipi_dsi_turn_on_peripheral(osd101t2587->dsi);
> +	if (ret)
> +		return ret;
> +
> +	if (osd101t2587->backlight) {
> +		osd101t2587->backlight->props.power = FB_BLANK_UNBLANK;
> +		osd101t2587->backlight->props.state &= ~BL_CORE_FBBLANK;
> +		backlight_update_status(osd101t2587->backlight);
> +	}
backlight_enable(osd101t2587->backlight);
can be used here.

> +
> +	osd101t2587->enabled = true;
> +
> +	return ret;
> +}
> +
> +static const struct drm_display_mode default_mode_osd101t2587 = {
> +	.clock = 164400,
> +	.hdisplay = 1920,
> +	.hsync_start = 1920 + 152,
> +	.hsync_end = 1920 + 152 + 52,
> +	.htotal = 1920 + 152 + 52 + 20,
> +	.vdisplay = 1200,
> +	.vsync_start = 1200 + 24,
> +	.vsync_end = 1200 + 24 + 6,
> +	.vtotal = 1200 + 24 + 6 + 48,
> +	.vrefresh = 60,
> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +};
> +
> +static int osd101t2587_panel_get_modes(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, osd101t2587->default_mode);
> +	if (!mode) {
> +		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
> +			osd101t2587->default_mode->hdisplay,
> +			osd101t2587->default_mode->vdisplay,
> +			osd101t2587->default_mode->vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	drm_mode_probed_add(panel->connector, mode);
> +
> +	panel->connector->display_info.width_mm = 217;
> +	panel->connector->display_info.height_mm = 136;
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs osd101t2587_panel_funcs = {
> +	.disable = osd101t2587_panel_disable,
> +	.unprepare = osd101t2587_panel_unprepare,
> +	.prepare = osd101t2587_panel_prepare,
> +	.enable = osd101t2587_panel_enable,
> +	.get_modes = osd101t2587_panel_get_modes,
> +};
> +
> +static const struct of_device_id osd101t2587_of_match[] = {
> +	{
> +		.compatible = "osd,osd101t2587-53ts",
> +		.data = &default_mode_osd101t2587,
> +	}, { }
Move { } to a separate line and use { /* sentinel */ } to make it look like other drivers.
 

> +};
> +MODULE_DEVICE_TABLE(of, osd101t2587_of_match);
> +
> +static int osd101t2587_panel_add(struct osd101t2587_panel *osd101t2587)
> +{
> +	struct device *dev = &osd101t2587->dsi->dev;
> +	struct device_node *np;
> +	int ret;
> +
> +	osd101t2587->mode = osd101t2587->default_mode;
> +
> +	osd101t2587->supply = devm_regulator_get(dev, "power");
> +	if (IS_ERR(osd101t2587->supply))
> +		return PTR_ERR(osd101t2587->supply);
> +
> +	np = of_parse_phandle(dev->of_node, "backlight", 0);
> +	if (np) {
> +		osd101t2587->backlight = of_find_backlight_by_node(np);
> +		of_node_put(np);
> +
> +		if (!osd101t2587->backlight)
> +			return -EPROBE_DEFER;
> +	}
Use devm_of_find_backlight() - this results in simpler code.

> +
> +	drm_panel_init(&osd101t2587->base);
> +	osd101t2587->base.funcs = &osd101t2587_panel_funcs;
> +	osd101t2587->base.dev = &osd101t2587->dsi->dev;
> +
> +	ret = drm_panel_add(&osd101t2587->base);
> +	if (ret < 0)
> +		goto put_backlight;
> +
> +	return 0;
> +
> +put_backlight:
> +	if (osd101t2587->backlight)
> +		put_device(&osd101t2587->backlight->dev);
When using devm_of_find_backlight() this is not needed.

> +
> +	return ret;
> +}
> +
> +static void osd101t2587_panel_del(struct osd101t2587_panel *osd101t2587)
> +{
> +	if (osd101t2587->base.dev)
> +		drm_panel_remove(&osd101t2587->base);
> +
> +	if (osd101t2587->backlight)
> +		put_device(&osd101t2587->backlight->dev);
When you use devm_of_find_backlight() then this is done for you, so the above can be deleted.

> +}
> +
> +static int osd101t2587_panel_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct osd101t2587_panel *osd101t2587;
> +	const struct of_device_id *id;
> +	int ret;
> +
> +	id = of_match_node(osd101t2587_of_match, dsi->dev.of_node);
> +	if (!id)
> +		return -ENODEV;
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> +			  MIPI_DSI_MODE_VIDEO_BURST |
> +			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> +			  MIPI_DSI_MODE_EOT_PACKET;
> +
> +	osd101t2587 = devm_kzalloc(&dsi->dev, sizeof(*osd101t2587), GFP_KERNEL);
> +	if (!osd101t2587)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, osd101t2587);
> +
> +	osd101t2587->dsi = dsi;
> +	osd101t2587->default_mode = id->data;
> +
> +	ret = osd101t2587_panel_add(osd101t2587);
> +	if (ret < 0)
> +		return ret;
> +
> +	return mipi_dsi_attach(dsi);
> +}
> +
> +static int osd101t2587_panel_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = osd101t2587_panel_disable(&osd101t2587->base);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
> +
To trun off power supply call osd101t2587_panel_unprepare() here?

> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> +
> +	osd101t2587_panel_del(osd101t2587);
Replace this with
	drm_panel_remove(osd101t2587->base);
This should be fine and no need to disable backlight, this is done in osd101t2587_panel_disable()

> +
> +	return 0;
> +}
> +
> +static void osd101t2587_panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
> +
Maybe call osd101t2587_panel_unprepare() here to turn off power supply?

> +	osd101t2587_panel_disable(&osd101t2587->base);
> +}
> +
> +static struct mipi_dsi_driver osd101t2587_panel_driver = {
> +	.driver = {
> +		.name = "panel-osd-osd101t2587-53ts",
> +		.of_match_table = osd101t2587_of_match,
> +	},
> +	.probe = osd101t2587_panel_probe,
> +	.remove = osd101t2587_panel_remove,
> +	.shutdown = osd101t2587_panel_shutdown,
> +};
> +module_mipi_dsi_driver(osd101t2587_panel_driver);
> +
> +MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
> +MODULE_DESCRIPTION("OSD101T2587-53TS DSI panel");
> +MODULE_LICENSE("GPL v2");
> -- 
> Peter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel