Re[2]: [PATCH 4/4] drm/panel: Add Rondo RB070D30 panel

From: "Konstantin Sudakov" <k.sudakov@integrasources.com>
To: "Sam Ravnborg" <sam@ravnborg.org>
Cc: bbrezillon@kernel.org,
	"Maxime Ripard" <maxime.ripard@bootlin.com>,
	dri-devel@lists.freedesktop.org,
	"Paul Kocialkowski" <paul.kocialkowski@bootlin.com>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Sean Paul" <seanpaul@chromium.org>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Jagan Teki" <jagan@openedev.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re[2]: [PATCH 4/4] drm/panel: Add Rondo RB070D30 panel
Date: Sun, 27 Jan 2019 08:26:55 +0300	[thread overview]
Message-ID: <1548566815.202603076@f339.i.mail.ru> (raw)
In-Reply-To: <20190126153946.GB17756@ravnborg.org>

[-- Attachment #1.1: Type: text/plain, Size: 12185 bytes --]

Hello, Sam!
Thank you for the comments.

>> +#include <drm/drmP.h>
> Please do not use drmP.h in new drivers. We are trying to get rid of it.
It can be replaced by these headers:

#include <drm/drm_connector.h>
#include <drm/drm_modes.h>
#include <uapi/linux/media-bus-format.h>

> Use DRM_DEV_ERROR(...) to have consistent error message for the drm drivers.
> This is general for the whole file.
Good idea, I think. So, this header is needed:

#include <drm/drm_print.h>

>> + /* Reset */
>> + msleep(20);
>> + gpiod_set_value(ctx->gpios.power, 1);
>> + msleep(20);
>> + gpiod_set_value(ctx->gpios.reset, 1);
>> + msleep(20);
>To verify the above pointer to a datasheet would be nice.
I looked the datasheet again, looks like the reset should be first. But I didn't find information about timings. It's needed to be checked on hardware. The power pin is named "STBYB" originally.

> height and width missing here. Seems better to add them here than hiding in code below.
> Is it on purpose that no flags are specified?
I sure, this algorithm was copied from the ilitek driver. The default_mode (drm_display_mode) variable and panel->connector->display_info (drm_display_info) variable has a different types, I'm not sure about this point. The bpc is not not applicable for the drm_display_mode.

>> + mode = drm_mode_duplicate(panel->drm, &default_mode);
>> + if (!mode) {
>> + dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
>> + default_mode.hdisplay, default_mode.vdisplay,
>> + default_mode.vrefresh);
> Use"
> DRM_DEV_ERROR(&ctx->dsi->dev,
> "failed to add mode" DRM_MODE_FMT,
> DRM_MODE_ARG(mode));
Not sure because the mode is NULL. May the default_mode be used for print?

>> + ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW);
>> + if (IS_ERR(ctx->gpios.updn)) {
>> + dev_err(&dsi->dev, "Couldn't get our updn GPIO\n");
>> + return PTR_ERR(ctx->gpios.updn);
>> + }
> This gpio is never used, it is only read from DT
The gpio is initialized with low state. The state may be inverted by DT. The same for the "shlr".
It is a vertical / horizontal inversion.

> Use devm_of_find_backlight()
I agree with you.

The lastly, I think the rondo should be replaced by ronbo, because the company name is Ronbo Electronics Ltd.
http://www.ronboe.com/about.html  

>Суббота, 26 января 2019, 22:39 +07:00 от Sam Ravnborg <sam@ravnborg.org>:
>
>Hi Maxime / Konstantin.
>
>Nice welstructured and small driver.
>Please see a few comments below
>
>Some of the comments in the following apply to a lot of
>the existing panel drivers as well.
>But lets see if we can get new drivers to be even better.
>
>Sam 
>
>On Wed, Jan 23, 2019 at 04:54:24PM +0100, Maxime Ripard wrote:
>> From: Konstantin Sudakov < k.sudakov@integrasources.com >
>> 
>> The Rondo RB070D30 panel is a MIPI-DSI panel based on a Fitipower EK79007
>> controller and a 1024x600 panel.
>> 
>> Signed-off-by: Konstantin Sudakov < k.sudakov@integrasources.com >
>> Signed-off-by: Maxime Ripard < maxime.ripard@bootlin.com >
>> ---
>>  drivers/gpu/drm/panel/Kconfig                |   9 +-
>>  drivers/gpu/drm/panel/Makefile               |   1 +-
>>  drivers/gpu/drm/panel/panel-rondo-rb070d30.c | 258 ++++++++++++++++++++-
>>  3 files changed, 268 insertions(+)
>>  create mode 100644 drivers/gpu/drm/panel/panel-rondo-rb070d30.c
>> 
>> diff --git a/drivers/gpu/drm/panel/panel-rondo-rb070d30.c b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c
>> new file mode 100644
>> index 000000000000..4f8e63f367b1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c
>> @@ -0,0 +1,258 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018, Bridge Systems BV
>> + * Copyright (C) 2018, Bootlin
>> + * Copyright (C) 2017, Free Electrons
>> + *
>> + * This file based on panel-ilitek-ili9881c.c
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/fb.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <drm/drm_mipi_dsi.h>
>> +#include <drm/drmP.h>
>> +#include <drm/drm_panel.h>
>Please do not use drmP.h in new drivers. We are trying to get rid of it.
>
>> +
>> +#include <video/mipi_display.h>
>> +#include <video/of_display_timing.h>
>> +#include <video/videomode.h>
>> +
>> +struct rb070d30_panel {
>> +	struct drm_panel panel;
>> +	struct mipi_dsi_device *dsi;
>> +	struct backlight_device *backlight;
>> +	struct regulator *supply;
>> +
>> +	struct {
>> +		struct gpio_desc *power;
>> +		struct gpio_desc *reset;
>> +		struct gpio_desc *updn;
>> +		struct gpio_desc *shlr;
>> +	} gpios;
>> +};
>> +
>> +static inline struct rb070d30_panel *panel_to_rb070d30_panel(struct drm_panel *panel)
>> +{
>> +	return container_of(panel, struct rb070d30_panel, panel);
>> +}
>> +
>> +static int rb070d30_panel_prepare(struct drm_panel *panel)
>> +{
>> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
>> +	int ret;
>> +
>> +	ret = regulator_enable(ctx->supply);
>> +	if (ret < 0) {
>> +		dev_err(&ctx->dsi->dev, "Failed to enable supply: %d\n", ret);
>Use DRM_DEV_ERROR(...) to have consistent error message for the drm drivers.
>This is general for the whole file.
>
>> +		return ret;
>> +	}
>> +
>> +	/* Reset */
>> +	msleep(20);
>> +	gpiod_set_value(ctx->gpios.power, 1);
>> +	msleep(20);
>> +	gpiod_set_value(ctx->gpios.reset, 1);
>> +	msleep(20);
>> +	return 0;
>> +}
>To verify the above pointer to a datasheet would be nice.
>
>
>> +
>> +static int rb070d30_panel_unprepare(struct drm_panel *panel)
>> +{
>> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
>> +
>> +	gpiod_set_value(ctx->gpios.power, 0);
>> +	gpiod_set_value(ctx->gpios.reset, 0);
>> +	regulator_disable(ctx->supply);
>> +
>> +	return 0;
>> +}
>There is sometimes timing constrains after deassert reset..
>The order is not strictly reverse of prepare - is that on purpose?
>
>
>> +
>> +static int rb070d30_panel_enable(struct drm_panel *panel)
>> +{
>> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
>> +	int ret;
>> +
>> +	ret = mipi_dsi_dcs_exit_sleep_mode(ctx->dsi);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = backlight_enable(ctx->backlight);
>> +	if (ret)
>> +		goto out;
>> +
>> +	return 0;
>> +
>> +out:
>> +	mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
>> +	return ret;
>> +}
>> +
>> +static int rb070d30_panel_disable(struct drm_panel *panel)
>> +{
>> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
>> +
>> +	backlight_disable(ctx->backlight);
>> +	return mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
>> +}
>> +
>> +/* Default timings */
>> +static const struct drm_display_mode default_mode = {
>> +	.clock		= 51206,
>> +	.hdisplay	= 1024,
>> +	.hsync_start	= 1024 + 160,
>> +	.hsync_end	= 1024 + 160 + 80,
>> +	.htotal		= 1024 + 160 + 80 + 80,
>> +	.vdisplay	= 600,
>> +	.vsync_start	= 600 + 12,
>> +	.vsync_end	= 600 + 12 + 10,
>> +	.vtotal		= 600 + 12 + 10 + 13,
>> +	.vrefresh	= 60,
>> +};
>height and width missing here. Seems better to add them here than hiding in code below.
>Is it on purpose that no flags are specified?
>
>> +
>> +static int rb070d30_panel_get_modes(struct drm_panel *panel)
>> +{
>> +	struct drm_connector *connector = panel->connector;
>> +	struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel);
>> +	struct drm_display_mode *mode;
>> +	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>> +
>> +	mode = drm_mode_duplicate(panel->drm, &default_mode);
>> +	if (!mode) {
>> +		dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
>> +			default_mode.hdisplay, default_mode.vdisplay,
>> +			default_mode.vrefresh);
>Use"
>DRM_DEV_ERROR(&ctx->dsi->dev,
>"failed to add mode" DRM_MODE_FMT,
>DRM_MODE_ARG(mode));
>> +		return -EINVAL;
>> +	}
>> +
>> +	drm_mode_set_name(mode);
>> +
>> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>> +	drm_mode_probed_add(connector, mode);
>> +
>> +	panel->connector->display_info.bpc = 8;
>> +	panel->connector->display_info.width_mm = 154;
>> +	panel->connector->display_info.height_mm = 85;
>See comment on height above.
>Same goes for bpc
>
>> +	drm_display_info_set_bus_formats(&connector->display_info,
>> +					 &bus_format, 1);
>> +
>> +	return 1;
>> +}
>> +
>> +static const struct drm_panel_funcs rb070d30_panel_funcs = {
>> +	.get_modes	= rb070d30_panel_get_modes,
>> +	.prepare	= rb070d30_panel_prepare,
>> +	.enable		= rb070d30_panel_enable,
>> +	.disable	= rb070d30_panel_disable,
>> +	.unprepare	= rb070d30_panel_unprepare,
>> +};
>> +
>> +static int rb070d30_panel_dsi_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct device_node *np;
>> +	struct rb070d30_panel *ctx;
>> +	int ret;
>> +
>> +	ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
>> +	if (!ctx)
>> +		return -ENOMEM;
>> +
>> +	ctx->supply = devm_regulator_get(&dsi->dev, "vcc-lcd");
>> +	if (IS_ERR(ctx->supply))
>> +		return PTR_ERR(ctx->supply);
>> +
>> +	mipi_dsi_set_drvdata(dsi, ctx);
>> +	ctx->dsi = dsi;
>> +
>> +	drm_panel_init(&ctx->panel);
>> +	ctx->panel.dev = &dsi->dev;
>> +	ctx->panel.funcs = &rb070d30_panel_funcs;
>> +
>> +	ctx->gpios.reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ctx->gpios.reset)) {
>> +		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
>> +		return PTR_ERR(ctx->gpios.reset);
>> +	}
>> +
>> +	ctx->gpios.power = devm_gpiod_get(&dsi->dev, "power", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ctx->gpios.power)) {
>> +		dev_err(&dsi->dev, "Couldn't get our power GPIO\n");
>> +		return PTR_ERR(ctx->gpios.power);
>> +	}
>> +
>> +	ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ctx->gpios.updn)) {
>> +		dev_err(&dsi->dev, "Couldn't get our updn GPIO\n");
>> +		return PTR_ERR(ctx->gpios.updn);
>> +	}
>This gpio is never used, it is only read from DT
>
>
>> +
>> +	ctx->gpios.shlr = devm_gpiod_get(&dsi->dev, "shlr", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ctx->gpios.shlr)) {
>> +		dev_err(&dsi->dev, "Couldn't get our shlr GPIO\n");
>> +		return PTR_ERR(ctx->gpios.shlr);
>> +	}
>This gpio is never used, it is only read from DT
>
>> +
>> +	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
>> +	if (np) {
>> +		ctx->backlight = of_find_backlight_by_node(np);
>> +		of_node_put(np);
>> +
>> +		if (!ctx->backlight)
>> +			return -EPROBE_DEFER;
>> +	}
>Use devm_of_find_backlight()
>
>> +
>> +	ret = drm_panel_add(&ctx->panel);
>> +	if (ret < 0)
>> +		goto free_backlight;
>> +
>> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_LPM;
>> +	dsi->format = MIPI_DSI_FMT_RGB888;
>> +	dsi->lanes = 4;
>> +
>> +	return mipi_dsi_attach(dsi);
>> +
>> +free_backlight:
>> +	backlight_put(ctx->backlight);
>If devm_of_find_backlight() is used this can go.
>
>> +	return ret;
>> +}
>> +
>> +static int rb070d30_panel_dsi_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct rb070d30_panel *ctx = mipi_dsi_get_drvdata(dsi);
>> +
>> +	mipi_dsi_detach(dsi);
>> +	drm_panel_remove(&ctx->panel);
>> +	backlight_put(ctx->backlight);
>If devm_of_find_backlight() is used this can go.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id rb070d30_panel_of_match[] = {
>> +	{ .compatible = "rondo,rb070d30" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, rb070d30_panel_of_match);
>> +
>> +static struct mipi_dsi_driver rb070d30_panel_driver = {
>> +	.probe = rb070d30_panel_dsi_probe,
>> +	.remove = rb070d30_panel_dsi_remove,
>> +	.driver = {
>> +		.name = "panel-rondo-rb070d30",
>> +		.of_match_table	= rb070d30_panel_of_match,
>> +	},
>> +};
>> +module_mipi_dsi_driver(rb070d30_panel_driver);
>> +
>> +MODULE_AUTHOR("Boris Brezillon < boris.brezillon@bootlin.com >");
>> +MODULE_AUTHOR("Konstantin Sudakov < k.sudakov@integrasources.com >");
>> +MODULE_DESCRIPTION("Rondo RB070D30 Panel Driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> git-series 0.9.1
>> _______________________________________________
>> dri-devel mailing list
>>  dri-devel@lists.freedesktop.org
>>  https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Konstantin Sudakov

[-- Attachment #1.2: Type: text/html, Size: 15816 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel