[PATCH] [v2, 2/2] drm/panel: Add Boe Himax8279d MIPI-DSI LCD panel
Sam Ravnborg
sam at ravnborg.org
Sun Mar 3 19:47:38 UTC 2019
Hi Jerry.
Thanks for this nice patch.
It triggered a few review comments, please see below.
In general it is considered good style to prefix all local function,
to avoid name clash with global function and to make it obvious
when the local function is called direct.
So
panel_unprepare() becomes boe_panel_unprepare() etc.
(boe_ is just a suggestion)
Sam
On Fri, Mar 01, 2019 at 04:54:12PM +0800, Jerry Han wrote:
> Add panel driver for it.
>
> Signed-off-by: Jerry Han <hanxu5 at huaqin.corp-partner.google.com>
> Cc: Jitao Shi <jitao.shi at mediatek.com>
> Cc: Nick Sanders <nsanders at google.com>
> Cc: YH Lin <yueherngl at chromium.org>
> Cc: Rock wang <rock_wang at himax.com.cn>
> ---
> MAINTAINERS | 6 +
> drivers/gpu/drm/panel/Kconfig | 11 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-boe-himax8279d.c | 1074 ++++++++++++++++++++++++++
> 4 files changed, 1092 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-boe-himax8279d.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b2f710e..38e2881 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4636,6 +4636,12 @@ S: Maintained
> F: drivers/gpu/drm/tinydrm/ili9225.c
> F: Documentation/devicetree/bindings/display/ilitek,ili9225.txt
>
> +DRM DRIVER FOR BOE HIMAX8279D PANELS
> +M: Jerry Han <hanxu5 at huaqin.corp-partner.google.com>
> +S: Maintained
> +F: drivers/gpu/drm/panel/panel-boe-himax8279d.c
> +F: Documentation/devicetree/bindings/display/panel/boe,himax8279d.txt
> +
Please add new entries in alphabetic order.
> DRM DRIVER FOR INTEL I810 VIDEO CARDS
> S: Orphan / Obsolete
> F: drivers/gpu/drm/i810/
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 6020c30..e5dcfd1 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -186,4 +186,15 @@ config DRM_PANEL_SITRONIX_ST7789V
> Say Y here if you want to enable support for the Sitronix
> ST7789V controller for 240x320 LCD panels
>
> +config DRM_PANEL_BOE_HIMAX8279D
> + tristate "Boe Himax8279d 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 Same type
The help text talks about a "Same" TFT-LCD module, but the driver
is named boe. Is the help text correct?
> + TFT-LCD modules. The panel has a 1200x1920 resolution and uses
> + 24 bit RGB per pixel. It provides a MIPI DSI interface to
> + the host and has a built-in LED backlight.
> +
> endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 5ccaaa9..8a36543 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
> obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> +obj-$(CONFIG_DRM_PANEL_BOE_HIMAX8279D) += panel-boe-himax8279d.o
Entries in the makefile must be sorted alphabetically.
> diff --git a/drivers/gpu/drm/panel/panel-boe-himax8279d.c b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
> new file mode 100644
> index 0000000..836a9cb
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
> @@ -0,0 +1,1074 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Huaqin Telecom Technology Co., Ltd
> + *
> + * Author: Jerry Han <hanxu5 at huaqin.corp-partner.google.com>
> + *
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/fb.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_modes.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_print.h>
Please sort include lines alphabetically.
But keep the nice grouping.
> +
> +#include <video/mipi_display.h>
> +
> +struct panel_cmd {
> + size_t len;
> + const char *data;
> +};
> +
> +#define _INIT_CMD(...) { \
> + .len = sizeof((char[]){__VA_ARGS__}), \
> + .data = (char[]){__VA_ARGS__} }
> +
> +struct panel_desc {
> + const struct drm_display_mode *display_mode;
> + unsigned int bpc;
> + unsigned int width;
> + unsigned int height;
Use width_mm, height_mm like in display_info - this is less confusing.
> +
> + unsigned int delay_t1;
> + unsigned int reset_delay_t2;
> + unsigned int reset_delay_t3;
> + unsigned int reset_delay_t4;
> +
> + unsigned long flags;
In struct mipi_dsi_device this member is named mode_flags,
and all other members have same name. Consider renaming
this to match struct mipi_dsi_device too.
> + enum mipi_dsi_pixel_format format;
> + unsigned int lanes;
> + const struct panel_cmd *on_cmds;
> + const struct panel_cmd *off_cmds;
> +};
> +
> +struct panel_info {
> + struct drm_panel base;
> + struct mipi_dsi_device *link;
> + const struct panel_desc *desc;
> +
> + struct backlight_device *backlight;
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *pp33_gpio;
> + struct gpio_desc *pp18_gpio;
> +
> + bool prepared;
> + bool enabled;
> +};
> +
> +void set_gpios(struct panel_info *pinfo, int enable)
> +{
> + gpiod_set_value(pinfo->reset_gpio, enable);
> + gpiod_set_value(pinfo->pp33_gpio, enable);
> + gpiod_set_value(pinfo->pp18_gpio, enable);
> +}
> +
> +static inline struct panel_info *to_panel_info(struct drm_panel *panel)
> +{
> + return container_of(panel, struct panel_info, base);
> +}
> +
> +static int panel_disable(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> +
> + backlight_disable(pinfo->backlight);
> +
> + pinfo->enabled = false;
> +
> + return 0;
> +}
> +
> +static int panel_unprepare(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> + int err;
> +
> + if (!pinfo->prepared)
> + return 0;
> +
> + /* send off code */
> + if (pinfo->desc->off_cmds) {
> + const struct panel_cmd *cmds = pinfo->desc->off_cmds;
> + unsigned int i;
> +
> + for (i = 0; cmds[i].len != 0; i++) {
> + const struct panel_cmd *cmd = &cmds[i];
> +
> + if (cmd->len == 2)
> + err = mipi_dsi_dcs_write(pinfo->link,
> + cmd->data[1], NULL, 0);
> + else
> + err = mipi_dsi_dcs_write(pinfo->link,
> + cmd->data[1], cmd->data + 2,
> + cmd->len - 2);
> +
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to send DCS Off Code: %d\n",
> + err);
> + goto poweroff;
> + }
> + usleep_range((cmd->data[0]) * 1000,
> + (1 + cmd->data[0]) * 1000);
> + }
Sending off_cmds and on_cmds looks very similar.
Can you add a helper function to avoid duplicating the code?
> + }
> +
> + usleep_range(1000, 1000);
> + set_gpios(pinfo, 0);
> +
> + pinfo->prepared = false;
> +
> + return 0;
> +
> +poweroff:
> + set_gpios(pinfo, 0);
> + return err;
> +}
> +
> +static int panel_prepare(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> + const struct panel_desc *desc = pinfo->desc;
> + int err;
> +
> + if (pinfo->prepared)
> + return 0;
> +
> + gpiod_set_value(pinfo->pp18_gpio, 1);
> + /* T1 (> 5ms) */
> + usleep_range(desc->delay_t1, 1000 + desc->delay_t1);
> + gpiod_set_value(pinfo->pp33_gpio, 1);
> +
> + /* reset sequence */
> + // T2 (> 14ms)
> + usleep_range(desc->reset_delay_t2, 1000 + desc->reset_delay_t2);
> +
> + // T3 (>= 0ms)
> + if (desc->reset_delay_t3) {
> + gpiod_set_value(pinfo->reset_gpio, 1);
> + usleep_range(desc->reset_delay_t3, 1000 + desc->reset_delay_t3);
> + gpiod_set_value(pinfo->reset_gpio, 0);
> + }
> +
> + // T4 (> 20us)
> + usleep_range(desc->reset_delay_t4, 1000 + desc->reset_delay_t4);
> + gpiod_set_value(pinfo->reset_gpio, 1);
> +
> + /* send init code */
> + if (pinfo->desc->on_cmds) {
> + const struct panel_cmd *cmds = pinfo->desc->on_cmds;
> + unsigned int i;
> +
> + for (i = 0; cmds[i].len != 0; i++) {
> + const struct panel_cmd *cmd = &cmds[i];
> +
> + if (cmd->len == 2)
> + err = mipi_dsi_dcs_write(pinfo->link,
> + cmd->data[1], NULL, 0);
> + else
> + err = mipi_dsi_dcs_write(pinfo->link,
> + cmd->data[1], cmd->data + 2,
> + cmd->len - 2);
> +
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to send DCS Init Code: %d\n",
> + err);
> + goto poweroff;
> + }
> + usleep_range(cmd->data[0] * 1000,
> + (1 + cmd->data[0]) * 1000);
> + }
> + }
> +
> + pinfo->prepared = true;
> +
> + return 0;
> +
> +poweroff:
> + set_gpios(pinfo, 0);
> + return err;
> +}
> +
> +static int panel_enable(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> + int ret;
> +
> + if (pinfo->enabled)
> + return 0;
> +
> + ret = backlight_enable(pinfo->backlight);
> + if (ret) {
> + DRM_DEV_ERROR(panel->drm->dev,
> + "Failed to enable backlight %d\n", ret);
> + return ret;
> + }
> +
> + pinfo->enabled = true;
> +
> + return 0;
> +}
> +
> +static int panel_get_modes(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> + const struct drm_display_mode *m = pinfo->desc->display_mode;
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(panel->drm, m);
> + if (!mode) {
> + DRM_DEV_ERROR(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
> + m->hdisplay, m->vdisplay, m->vrefresh);
An extra "x" have sneaked in here.
%ux%u@%u should be the right way to express this.
> + return -ENOMEM;
> + }
> +
> + drm_mode_set_name(mode);
> +
> + drm_mode_probed_add(panel->connector, mode);
> +
> + panel->connector->display_info.width_mm = pinfo->desc->width;
> + panel->connector->display_info.height_mm = pinfo->desc->height;
> + panel->connector->display_info.bpc = pinfo->desc->bpc;
> +
> + return 1;
> +}
> +
> +static const struct drm_panel_funcs panel_funcs = {
> + .disable = panel_disable,
> + .unprepare = panel_unprepare,
> + .prepare = panel_prepare,
> + .enable = panel_enable,
> + .get_modes = panel_get_modes,
> +};
> +
> +/* 8 inch */
> +static const struct drm_display_mode boe_himax8279d8p_display_mode = {
> + .clock = 159420,
> + .hdisplay = 1200,
> + .hsync_start = 1200 + 80,
> + .hsync_end = 1200 + 80 + 60,
> + .htotal = 1200 + 80 + 60 + 24,
> + .vdisplay = 1920,
> + .vsync_start = 1920 + 10,
> + .vsync_end = 1920 + 10 + 14,
> + .vtotal = 1920 + 10 + 14 + 4,
> + .vrefresh = 60,
> +};
> +
> +static const struct panel_cmd boe_himax8279d8p_on_cmds[] = {
> + _INIT_CMD(0x22, 0x10),
...
> + _INIT_CMD(0x14, 0x29),
> +
> + {},
> +};
> +
> +static const struct panel_cmd boe_himax8279d8p_off_cmds[] = {
> + _INIT_CMD(0x00, 0x28),
> + _INIT_CMD(0x00, 0x10),
> +
> + {},
> +};
> +
> +static const struct panel_desc boe_himax8279d8p_panel_desc = {
> + .display_mode = &boe_himax8279d8p_display_mode,
> + .bpc = 8,
> + .width = 107,
> + .height = 172,
> + .delay_t1 = 5000,
> + .reset_delay_t2 = 14000,
> + .reset_delay_t3 = 1000,
> + .reset_delay_t4 = 1000,
> + .flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> + MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_LPM,
> + .format = MIPI_DSI_FMT_RGB888,
> + .lanes = 4,
> + .on_cmds = boe_himax8279d8p_on_cmds,
> + .off_cmds = boe_himax8279d8p_off_cmds,
> +};
> +
> +/* 10 inch */
> +static const struct drm_display_mode boe_himax8279d10p_display_mode = {
> + .clock = 159420,
> + .hdisplay = 1200,
> + .hsync_start = 1200 + 80,
> + .hsync_end = 1200 + 80 + 60,
> + .htotal = 1200 + 80 + 60 + 24,
> + .vdisplay = 1920,
> + .vsync_start = 1920 + 10,
> + .vsync_end = 1920 + 10 + 14,
> + .vtotal = 1920 + 10 + 14 + 4,
> + .vrefresh = 60,
> +};
> +
> +static const struct panel_cmd boe_himax8279d10p_on_cmds[] = {
> + _INIT_CMD(0x00, 0xB0, 0x05),
...
> +
> + {},
> +};
> +
> +static const struct panel_cmd boe_himax8279d10p_off_cmds[] = {
> + _INIT_CMD(0x00, 0x28),
> + _INIT_CMD(0x00, 0x10),
> +
> + {},
> +};
> +
> +static const struct panel_desc boe_himax8279d10p_panel_desc = {
> + .display_mode = &boe_himax8279d10p_display_mode,
> + .bpc = 8,
> + .width = 135,
> + .height = 216,
> + .delay_t1 = 5000,
> + .reset_delay_t2 = 14000,
> + .reset_delay_t3 = 1000,
> + .reset_delay_t4 = 1000,
> + .flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> + MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_LPM,
> + .format = MIPI_DSI_FMT_RGB888,
> + .lanes = 4,
> + .on_cmds = boe_himax8279d10p_on_cmds,
> + .off_cmds = boe_himax8279d10p_off_cmds,
> +};
> +
> +static const struct of_device_id panel_of_match[] = {
> + { .compatible = "boe,himax8279d8p",
> + .data = &boe_himax8279d8p_panel_desc
> + },
> + { .compatible = "boe,himax8279d10p",
> + .data = &boe_himax8279d10p_panel_desc
> + },
> + { }
Use { /* sentinel */ }, like most other drivers do.
> +};
> +MODULE_DEVICE_TABLE(of, panel_of_match);
> +
> +static int panel_add(struct panel_info *pinfo)
> +{
> + struct device *dev = &pinfo->link->dev;
> + int err;
> +
> + pinfo->pp18_gpio = devm_gpiod_get_optional(dev, "pp18", GPIOD_OUT_LOW);
> + if (IS_ERR(pinfo->pp18_gpio)) {
> + err = PTR_ERR(pinfo->pp18_gpio);
> + dev_err(dev, "failed to get pp18 gpio: %d\n", err);
> + pinfo->pp18_gpio = NULL;
> + }
> +
> + pinfo->pp33_gpio = devm_gpiod_get_optional(dev, "pp33", GPIOD_OUT_LOW);
> + if (IS_ERR(pinfo->pp33_gpio)) {
> + err = PTR_ERR(pinfo->pp33_gpio);
> + dev_err(dev, "failed to get pp33 gpio: %d\n", err);
> + pinfo->pp33_gpio = NULL;
> + }
> +
> + pinfo->reset_gpio = devm_gpiod_get_optional(dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(pinfo->reset_gpio)) {
> + err = PTR_ERR(pinfo->reset_gpio);
> + dev_err(dev, "failed to get enable gpio: %d\n", err);
> + pinfo->reset_gpio = NULL;
> + }
> +
> + pinfo->backlight = devm_of_find_backlight(dev);
> + if (IS_ERR(pinfo->backlight))
> + return PTR_ERR(pinfo->backlight);
> +
> + drm_panel_init(&pinfo->base);
> + pinfo->base.funcs = &panel_funcs;
> + pinfo->base.dev = &pinfo->link->dev;
> +
> + err = drm_panel_add(&pinfo->base);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +
> +static void panel_del(struct panel_info *pinfo)
> +{
> + if (pinfo->base.dev)
> + drm_panel_remove(&pinfo->base);
> +}
> +
> +static int panel_probe(struct mipi_dsi_device *dsi)
> +{
> + struct panel_info *pinfo;
> + int err;
> + const struct panel_desc *desc;
> +
> + pinfo = devm_kzalloc(&dsi->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;
> +
> + desc = of_device_get_match_data(&dsi->dev);
> + dsi->mode_flags = desc->flags;
> + dsi->format = desc->format;
> + dsi->lanes = desc->lanes;
> + pinfo->desc = desc;
> +
> + pinfo->link = dsi;
> + mipi_dsi_set_drvdata(dsi, pinfo);
> +
> + err = panel_add(pinfo);
> + if (err < 0)
> + return err;
> +
> + return mipi_dsi_attach(dsi);
> +}
> +
> +static int panel_remove(struct mipi_dsi_device *dsi)
> +{
> + struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
> + int err;
> +
> + err = panel_unprepare(&pinfo->base);
> + if (err < 0)
> + DRM_DEV_ERROR(&dsi->dev, "failed to unprepare panel: %d\n",
> + err);
> +
> + err = panel_disable(&pinfo->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(&pinfo->base);
> + panel_del(pinfo);
> +
> + return 0;
> +}
> +
> +static void panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> + struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
> +
> + panel_disable(&pinfo->base);
> + panel_unprepare(&pinfo->base);
> +}
> +
> +static struct mipi_dsi_driver panel_driver = {
> + .driver = {
> + .name = "panel-boe-himax8279d",
> + .of_match_table = panel_of_match,
> + },
> + .probe = panel_probe,
> + .remove = panel_remove,
> + .shutdown = panel_shutdown,
> +};
> +module_mipi_dsi_driver(panel_driver);
> +
> +MODULE_AUTHOR("Jerry Han <hanxu5 at huaqin.corp-partner.google.com>");
> +MODULE_DESCRIPTION("Boe Himax8279d driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list