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

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