Re: [PATCH 2/3] drm/panel: add Kingdisplay kd097d04 panel driver

From: Sean Paul
Date: Wed Sep 20 2017 - 15:18:27 EST


On Mon, Sep 18, 2017 at 04:40:01PM +0800, Nickey Yang wrote:
> Support Kingdisplay kd097d04 9.7" 1536x2048 TFT LCD panel,
> it is a MIPI DSI panel.
>
> Signed-off-by: Nickey Yang <nickey.yang@xxxxxxxxxxxxxx>

Hi Nickey,
This patch doesn't apply cleanly to drm-misc-next and the encoding is set to 'y'
instead of 'UTF-8'.

> ---
> drivers/gpu/drm/panel/Kconfig | 11 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c | 485 +++++++++++++++++++++
> 3 files changed, 497 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index d84a031..8b1f0bd 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -50,6 +50,17 @@ config DRM_PANEL_JDI_LT070ME05000
> The panel has a 1200(RGB)×1920 (WUXGA) resolution and uses
> 24 bit per pixel.
>
> +config DRM_PANEL_KINGDISPLAY_KD097D04
> + tristate "Kingdisplay kd097d04 panel"
> + depends on OF
> + depends on DRM_MIPI_DSI
> + depends on BACKLIGHT_CLASS_DEVICE
> + help
> + Say Y here if you want to enable support for Kingdisplay kd097d04
> + TFT-LCD modules. The panel has a 1536x2048 resolution and uses
> + 24 bit RGB per pixel. It provides a MIPI DSI interface to
> + the host and has a built-in LED backlight.
> +
> config DRM_PANEL_SAMSUNG_LD9040
> tristate "Samsung LD9040 RGB/SPI panel"
> depends on OF && SPI
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 9f6610d..2548931 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
> obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
> obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
> obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
> +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
> obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
> obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
> obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
> diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c
> new file mode 100644
> index 0000000..359cd34
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c
> @@ -0,0 +1,485 @@
> +/*
> + * Copyright (c) 2017, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +struct kingdisplay_panel {
> + struct drm_panel base;
> + struct mipi_dsi_device *link;
> +
> + struct backlight_device *backlight;
> + struct regulator *supply;
> + struct gpio_desc *enable_gpio;
> +
> + bool prepared;
> + bool enabled;

I really wish we didn't need to keep track of prepared/enabled here. Is this
just to protect the regulator refcount from unbalanced unprepare/prepare or
enabled/disable calls?

The reason I don't like it is it pretty much guarantees we won't be able to
recover from error paths since prepared/enabled are only flipped at the end of
the various hooks.

> +};
> +
> +struct kingdisplay_pannel_cmd {

s/pannel/panel/

> + char cmd;
> + char data;
> +};
> +
> +static const struct kingdisplay_pannel_cmd init_code[] = {

It doesn't look like these are standard DCS operations? Are these documented
anywhere? It would be great if you could #define some of these registers/values
such that they're useful for people without access to the datasheet (like
myself).

> + /* voltage setting */
> + { 0xB0, 0x00 },
> + { 0xB2, 0x02 },
> + { 0xB3, 0x11 },
> + { 0xB4, 0x00 },
> + { 0xB6, 0x80 },
> + /* VCOM disable */
> + { 0xB8, 0x80 },
> + { 0xBA, 0x43 },
> + /* VCOM setting */
> + { 0xBB, 0x53 },
> + /* VSP setting */
> + { 0xBC, 0x0A },
> + /* VSN setting */
> + { 0xBD, 0x4A },
> + /* VGH setting */
> + { 0xBE, 0x2F },
> + /* VGL setting */
> + { 0xBF, 0x1A },
> + { 0xF0, 0x39 },
> + { 0xF1, 0x21 },
> + /* Gamma setting */
> + { 0xB0, 0x02 },
> + { 0xC0, 0x00 },
> + { 0xC1, 0x01 },
> + { 0xC2, 0x0B },
> + { 0xC3, 0x15 },
> + { 0xC4, 0x22 },
> + { 0xC5, 0x11 },
> + { 0xC6, 0x15 },
> + { 0xC7, 0x19 },
> + { 0xC8, 0x1A },
> + { 0xC9, 0x16 },
> + { 0xCA, 0x18 },
> + { 0xCB, 0x13 },
> + { 0xCC, 0x18 },
> + { 0xCD, 0x13 },
> + { 0xCE, 0x1C },
> + { 0xCF, 0x19 },
> + { 0xD0, 0x21 },
> + { 0xD1, 0x2C },
> + { 0xD2, 0x2F },
> + { 0xD3, 0x30 },
> + { 0xD4, 0x19 },
> + { 0xD5, 0x1F },
> + { 0xD6, 0x00 },
> + { 0xD7, 0x01 },
> + { 0xD8, 0x0B },
> + { 0xD9, 0x15 },
> + { 0xDA, 0x22 },
> + { 0xDB, 0x11 },
> + { 0xDC, 0x15 },
> + { 0xDD, 0x19 },
> + { 0xDE, 0x1A },
> + { 0xDF, 0x16 },
> + { 0xE0, 0x18 },
> + { 0xE1, 0x13 },
> + { 0xE2, 0x18 },
> + { 0xE3, 0x13 },
> + { 0xE4, 0x1C },
> + { 0xE5, 0x19 },
> + { 0xE6, 0x21 },
> + { 0xE7, 0x2C },
> + { 0xE8, 0x2F },
> + { 0xE9, 0x30 },
> + { 0xEA, 0x19 },
> + { 0xEB, 0x1F },
> + /* GOA MUX setting */
> + { 0xB0, 0x01 },
> + { 0xC0, 0x10 },
> + { 0xC1, 0x0F },
> + { 0xC2, 0x0E },
> + { 0xC3, 0x0D },
> + { 0xC4, 0x0C },
> + { 0xC5, 0x0B },
> + { 0xC6, 0x0A },
> + { 0xC7, 0x09 },
> + { 0xC8, 0x08 },
> + { 0xC9, 0x07 },
> + { 0xCA, 0x06 },
> + { 0xCB, 0x05 },
> + { 0xCC, 0x00 },
> + { 0xCD, 0x01 },
> + { 0xCE, 0x02 },
> + { 0xCF, 0x03 },
> + { 0xD0, 0x04 },
> + { 0xD6, 0x10 },
> + { 0xD7, 0x0F },
> + { 0xD8, 0x0E },
> + { 0xD9, 0x0D },
> + { 0xDA, 0x0C },
> + { 0xDB, 0x0B },
> + { 0xDC, 0x0A },
> + { 0xDD, 0x09 },
> + { 0xDE, 0x08 },
> + { 0xDF, 0x07 },
> + { 0xE0, 0x06 },
> + { 0xE1, 0x05 },
> + { 0xE2, 0x00 },
> + { 0xE3, 0x01 },
> + { 0xE4, 0x02 },
> + { 0xE5, 0x03 },
> + { 0xE6, 0x04 },
> + { 0xE7, 0x00 },
> + { 0xEC, 0xC0 },
> + /* GOA timing setting */
> + { 0xB0, 0x03 },
> + { 0xC0, 0x01 },
> + { 0xC2, 0x6F },
> + { 0xC3, 0x6F },
> + { 0xC5, 0x36 },
> + { 0xC8, 0x08 },
> + { 0xC9, 0x04 },
> + { 0xCA, 0x41 },
> + { 0xCC, 0x43 },
> + { 0xCF, 0x60 },
> + { 0xD2, 0x04 },
> + { 0xD3, 0x04 },
> + { 0xD4, 0x03 },
> + { 0xD5, 0x02 },
> + { 0xD6, 0x01 },
> + { 0xD7, 0x00 },
> + { 0xDB, 0x01 },
> + { 0xDE, 0x36 },
> + { 0xE6, 0x6F },
> + { 0xE7, 0x6F },
> + /* GOE setting */
> + { 0xB0, 0x06 },
> + { 0xB8, 0xA5 },
> + { 0xC0, 0xA5 },
> + { 0xD5, 0x3F },
> +};
> +
> +static inline struct kingdisplay_panel *to_kingdisplay_panel(struct drm_panel *panel)

>80 chars


> +{
> + return container_of(panel, struct kingdisplay_panel, base);
> +}
> +
> +static int kingdisplay_panel_disable(struct drm_panel *panel)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> + int err;
> +
> + if (!kingdisplay->enabled)
> + return 0;
> +
> + kingdisplay->backlight->props.power = FB_BLANK_POWERDOWN;
> + backlight_update_status(kingdisplay->backlight);
> +
> + err = mipi_dsi_dcs_set_display_off(kingdisplay->link);
> + if (err < 0)
> + DRM_DEV_ERROR(panel->dev, "failed to set display off: %d\n",
> + err);
> +
> + kingdisplay->enabled = false;
> +
> + return 0;
> +}
> +
> +static int kingdisplay_panel_unprepare(struct drm_panel *panel)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> + int err;
> +
> + if (!kingdisplay->prepared)
> + return 0;
> +
> + err = mipi_dsi_dcs_enter_sleep_mode(kingdisplay->link);
> + if (err < 0) {
> + DRM_DEV_ERROR(panel->dev, "failed to enter sleep mode: %d\n",
> + err);
> + return err;
> + }
> +
> + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0);
> +
> + /* T8: 80ms - 1000ms */
> + msleep(80);
> +
> + err = regulator_disable(kingdisplay->supply);
> + if (err < 0)

Log an error here.

> + return err;
> +
> + kingdisplay->prepared = false;
> +
> + return 0;
> +}
> +
> +static int kingdisplay_panel_prepare(struct drm_panel *panel)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> + int err, regulator_err;
> + int i;
> +
> + if (kingdisplay->prepared)
> + return 0;
> +
> + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0);
> +
> + err = regulator_enable(kingdisplay->supply);
> + if (err < 0)
> + return err;
> +
> + /* T2: 15ms - 1000ms */
> + usleep_range(15000, 16000);
> +
> + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 1);
> +
> + /* T4: 15ms - 1000ms */
> + usleep_range(15000, 16000);
> +
> + for (i = 0; i < ARRAY_SIZE(init_code); i++)
> + mipi_dsi_generic_write(kingdisplay->link, &init_code[i],
> + sizeof(struct kingdisplay_pannel_cmd));

Doing each command as a short write isn't exactly optimal. Any chance you can
compress multiple commands into a few long writes?

> +
> + err = mipi_dsi_dcs_exit_sleep_mode(kingdisplay->link);
> + if (err < 0) {
> + DRM_DEV_ERROR(panel->dev, "failed to exit sleep mode: %d\n",
> + err);
> + goto poweroff;
> + }
> +
> + /* T6: 120ms - 1000ms*/
> + msleep(120);
> +
> + err = mipi_dsi_dcs_set_display_on(kingdisplay->link);
> + if (err < 0) {
> + DRM_DEV_ERROR(panel->dev, "failed to set display on: %d\n",
> + err);
> + goto poweroff;
> + }
> +
> + /* T7: 10ms */
> + usleep_range(10000, 10000);
> +
> + kingdisplay->prepared = true;
> +
> + return 0;
> +
> +poweroff:
> + regulator_err = regulator_disable(kingdisplay->supply);
> + if (regulator_err)
> + DRM_DEV_ERROR(panel->dev, "failed to disable regulator: %d\n",
> + regulator_err);
> +
> + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0);
> + return err;
> +}
> +
> +static int kingdisplay_panel_enable(struct drm_panel *panel)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> + int ret;
> +
> + if (kingdisplay->enabled)
> + return 0;
> +
> + kingdisplay->backlight->props.power = FB_BLANK_UNBLANK;
> + ret = backlight_update_status(kingdisplay->backlight);
> + if (ret) {
> + DRM_DEV_ERROR(panel->drm->dev,
> + "Failed to enable backlight %d\n", ret);
> + return ret;
> + }
> +
> + kingdisplay->enabled = true;
> +
> + return 0;
> +}
> +
> +static const struct drm_display_mode default_mode = {
> + .clock = 213000,
> + .hdisplay = 1536,
> + .hsync_start = 1536 + 70,
> + .hsync_end = 1536 + 70 + 24,
> + .htotal = 1536 + 70 + 24 + 70,
> + .vdisplay = 2048,
> + .vsync_start = 2048 + 17,
> + .vsync_end = 2048 + 17 + 2,
> + .vtotal = 2048 + 17 + 2 + 23,
> + .vrefresh = 60,
> +};
> +
> +static int kingdisplay_panel_get_modes(struct drm_panel *panel)
> +{
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(panel->drm, &default_mode);
> + if (!mode) {
> + DRM_DEV_ERROR(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
> + default_mode.hdisplay, default_mode.vdisplay,
> + default_mode.vrefresh);
> + return -ENOMEM;
> + }
> +
> + drm_mode_set_name(mode);
> +
> + drm_mode_probed_add(panel->connector, mode);
> +
> + panel->connector->display_info.width_mm = 147;
> + panel->connector->display_info.height_mm = 196;
> + panel->connector->display_info.bpc = 8;
> +
> + return 1;
> +}
> +
> +static const struct drm_panel_funcs kingdisplay_panel_funcs = {
> + .disable = kingdisplay_panel_disable,
> + .unprepare = kingdisplay_panel_unprepare,
> + .prepare = kingdisplay_panel_prepare,
> + .enable = kingdisplay_panel_enable,
> + .get_modes = kingdisplay_panel_get_modes,
> +};
> +
> +static const struct of_device_id kingdisplay_of_match[] = {
> + { .compatible = "kingdisplay,kd097d04", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, kingdisplay_of_match);
> +
> +static int kingdisplay_panel_add(struct kingdisplay_panel *kingdisplay)
> +{
> + struct device *dev = &kingdisplay->link->dev;
> + struct device_node *np;
> + int err;
> +
> + kingdisplay->supply = devm_regulator_get(dev, "power");
> + if (IS_ERR(kingdisplay->supply))
> + return PTR_ERR(kingdisplay->supply);
> +
> + kingdisplay->enable_gpio = devm_gpiod_get_optional(dev, "enable",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(kingdisplay->enable_gpio)) {
> + err = PTR_ERR(kingdisplay->enable_gpio);
> + dev_dbg(dev, "failed to get enable gpio: %d\n", err);
> + kingdisplay->enable_gpio = NULL;
> + }
> +
> + np = of_parse_phandle(dev->of_node, "backlight", 0);
> + if (np) {
> + kingdisplay->backlight = of_find_backlight_by_node(np);
> + of_node_put(np);
> +
> + if (!kingdisplay->backlight)
> + return -EPROBE_DEFER;
> + }
> +
> + drm_panel_init(&kingdisplay->base);
> + kingdisplay->base.funcs = &kingdisplay_panel_funcs;
> + kingdisplay->base.dev = &kingdisplay->link->dev;
> +
> + err = drm_panel_add(&kingdisplay->base);
> + if (err < 0)
> + goto put_backlight;
> +
> + return 0;
> +
> +put_backlight:
> + put_device(&kingdisplay->backlight->dev);
> +
> + return err;
> +}
> +
> +static void kingdisplay_panel_del(struct kingdisplay_panel *kingdisplay)
> +{
> + if (kingdisplay->base.dev)
> + drm_panel_remove(&kingdisplay->base);
> +
> + put_device(&kingdisplay->backlight->dev);
> +}
> +
> +static int kingdisplay_panel_probe(struct mipi_dsi_device *dsi)
> +{
> + struct kingdisplay_panel *kingdisplay;
> + int err;
> +
> + dsi->lanes = 8;
> + dsi->format = MIPI_DSI_FMT_RGB888;
> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> + MIPI_DSI_MODE_LPM;
> +
> + kingdisplay = devm_kzalloc(&dsi->dev, sizeof(*kingdisplay), GFP_KERNEL);
> + if (!kingdisplay)
> + return -ENOMEM;
> +
> + mipi_dsi_set_drvdata(dsi, kingdisplay);
> +
> + kingdisplay->link = dsi;
> +
> + err = kingdisplay_panel_add(kingdisplay);
> + if (err < 0)
> + return err;
> +
> + err = mipi_dsi_attach(dsi);
> + return err;

Let's log some errors for failures in this function

> +}
> +
> +static int kingdisplay_panel_remove(struct mipi_dsi_device *dsi)
> +{
> + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
> + int err;
> +
> + err = kingdisplay_panel_unprepare(&kingdisplay->base);
> + if (err < 0)
> + DRM_DEV_ERROR(&dsi->dev, "failed to unprepare panel: %d\n",
> + err);
> +
> + err = kingdisplay_panel_disable(&kingdisplay->base);
> + if (err < 0)
> + DRM_DEV_ERROR(&dsi->dev, "failed to disable panel: %d\n", err);
> +
> + err = mipi_dsi_detach(dsi);
> + if (err < 0)
> + DRM_DEV_ERROR(&dsi->dev, "failed to detach from DSI host: %d\n",
> + err);
> +
> + drm_panel_detach(&kingdisplay->base);
> + kingdisplay_panel_del(kingdisplay);
> +
> + return 0;
> +}
> +
> +static void kingdisplay_panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
> +
> + kingdisplay_panel_unprepare(&kingdisplay->base);
> + kingdisplay_panel_disable(&kingdisplay->base);
> +}
> +
> +static struct mipi_dsi_driver kingdisplay_panel_driver = {
> + .driver = {
> + .name = "panel-kingdisplay-kd097d04",
> + .of_match_table = kingdisplay_of_match,
> + },
> + .probe = kingdisplay_panel_probe,
> + .remove = kingdisplay_panel_remove,
> + .shutdown = kingdisplay_panel_shutdown,
> +};
> +module_mipi_dsi_driver(kingdisplay_panel_driver);
> +
> +MODULE_AUTHOR("Chris Zhong <zyw@xxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Nickey Yang <nickey.yang@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("kingdisplay KD097D04 panel driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
>

--
Sean Paul, Software Engineer, Google / Chromium OS