Re: [RESEND v2 1/2] drm/panel: Add inx Himax8279d MIPI-DSI LCD panel driver

[Linus Walleij <linus.walleij@linaro.org>]

On Thu, Apr 14, 2022 at 10:19 AM xiazhengqiao
<xiazhengqiao@huaqin.corp-partner.google.com> wrote:

> Add STARRY 2081101QFH032011-53G 10.1" WUXGA TFT LCD panel
>
> Signed-off-by: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Reviewed-by: Hsin-Yi Wang <hsinyi@chromium.org>

OK cool... Do you know the name of the display controller
used in this panel? We tend to name the implementation, Kconfig
symbols etc after the display controller such as panel-ilitek-*
panel-truly-* panel-novatek-* etc. This would be panel-innolux-xyznnnn.c
in that case. We already have:
panel-innolux-ej030na.c
panel-innolux-p079zca.c

> +struct panel_desc {

Name it after the panel. struct inx_config to reflect other code
in this subsystem.

> +       const struct drm_display_mode *modes;
> +       unsigned int bpc;
> +
> +       /**
> +        * @width_mm: width of the panel's active display area
> +        * @height_mm: height of the panel's active display area
> +        */
> +       struct {
> +               unsigned int width_mm;
> +               unsigned int height_mm;
> +       } size;

This seems a bit overdesigned, why not just put these two unsigned in the
strict as is without an anonymous struct in the struct?

> +       unsigned long mode_flags;
> +       enum mipi_dsi_pixel_format format;
> +       const struct panel_init_cmd *init_cmds;

OK so a sequence of commands per panel type I get it.

> +       unsigned int lanes;
> +       bool discharge_on_disable;
> +};
> +
> +struct inx_panel {
> +       struct drm_panel base;
> +       struct mipi_dsi_device *dsi;
> +
> +       const struct panel_desc *desc;
> +
> +       enum drm_panel_orientation orientation;
> +       struct regulator *pp1800;
> +       struct regulator *avee;
> +       struct regulator *avdd;

These match the pin names on the chip? (Just checking.)

> +       struct gpio_desc *enable_gpio;
> +
> +       bool prepared;
> +};
> +
> +enum dsi_cmd_type {
> +       INIT_DCS_CMD,
> +       DELAY_CMD,
> +};

I've seen this comand/delay sequencing before. Maybe something
we should generalize as it keeps popping up?

> +struct panel_init_cmd {
> +       enum dsi_cmd_type type;
> +       size_t len;
> +       const char *data;
> +};
> +
> +#define _INIT_DCS_CMD(...) { \
> +       .type = INIT_DCS_CMD, \
> +       .len = sizeof((char[]){__VA_ARGS__}), \
> +       .data = (char[]){__VA_ARGS__} }
> +
> +#define _INIT_DELAY_CMD(...) { \
> +       .type = DELAY_CMD,\
> +       .len = sizeof((char[]){__VA_ARGS__}), \
> +       .data = (char[]){__VA_ARGS__} }

These defines are massive overkill, especially .len, if you look
below it is clear that all of them have ... len = 2 and two that have
len = 1.

If these macros are just for this driver ... drop them, just do something
like:

struct inx_init_tuple {
    u8 cmd;
    u8 val;
};

static const struct inx_tuple inx_init_seq[] = {
    { .cmd = ..., .val = ...}
   ...
};

Then iterate over this array of structs until you reach
ARRAY_SIZE(inx_init_seq).

Just hardcode the delays etc in the end.

> +static const struct panel_init_cmd starry_qfh032011_53g_init_cmd[] = {
> +       _INIT_DCS_CMD(0xB0, 0x01),

All of these opaque 0xB0 etc, create a proper #define for that parameter
using the name in the datasheet. It's especially helpful if the datasheet is
not public which is usually the case, but I think that you have it?
It's really hard for the next system using this to alter things if they have no
idea what the different parameters are.

> +       _INIT_DCS_CMD(0xC3, 0x4F),
> +       _INIT_DCS_CMD(0xC4, 0x40),
> +       _INIT_DCS_CMD(0xC5, 0x40),
> +       _INIT_DCS_CMD(0xC6, 0x40),
> +       _INIT_DCS_CMD(0xC7, 0x40),
> +       _INIT_DCS_CMD(0xC8, 0x4D),
> +       _INIT_DCS_CMD(0xC9, 0x52),
> +       _INIT_DCS_CMD(0xCA, 0x51),
> +       _INIT_DCS_CMD(0xCD, 0x5D),
> +       _INIT_DCS_CMD(0xCE, 0x5B),
> +       _INIT_DCS_CMD(0xCF, 0x4B),
> +       _INIT_DCS_CMD(0xD0, 0x49),
> +       _INIT_DCS_CMD(0xD1, 0x47),
> +       _INIT_DCS_CMD(0xD2, 0x45),
> +       _INIT_DCS_CMD(0xD3, 0x41),
> +       _INIT_DCS_CMD(0xD7, 0x50),
> +       _INIT_DCS_CMD(0xD8, 0x40),
> +       _INIT_DCS_CMD(0xD9, 0x40),
> +       _INIT_DCS_CMD(0xDA, 0x40),
> +       _INIT_DCS_CMD(0xDB, 0x40),
> +       _INIT_DCS_CMD(0xDC, 0x4E),
> +       _INIT_DCS_CMD(0xDD, 0x52),
> +       _INIT_DCS_CMD(0xDE, 0x51),
> +       _INIT_DCS_CMD(0xE1, 0x5E),
> +       _INIT_DCS_CMD(0xE2, 0x5C),
> +       _INIT_DCS_CMD(0xE3, 0x4C),
> +       _INIT_DCS_CMD(0xE4, 0x4A),
> +       _INIT_DCS_CMD(0xE5, 0x48),
> +       _INIT_DCS_CMD(0xE6, 0x46),
> +       _INIT_DCS_CMD(0xE7, 0x42),
> +       _INIT_DCS_CMD(0xB0, 0x03),
> +       _INIT_DCS_CMD(0xBE, 0x03),
> +       _INIT_DCS_CMD(0xCC, 0x44),
> +       _INIT_DCS_CMD(0xC8, 0x07),
> +       _INIT_DCS_CMD(0xC9, 0x05),
> +       _INIT_DCS_CMD(0xCA, 0x42),
> +       _INIT_DCS_CMD(0xCD, 0x3E),
> +       _INIT_DCS_CMD(0xCF, 0x60),
> +       _INIT_DCS_CMD(0xD2, 0x04),
> +       _INIT_DCS_CMD(0xD3, 0x04),
> +       _INIT_DCS_CMD(0xD4, 0x01),
> +       _INIT_DCS_CMD(0xD5, 0x00),
> +       _INIT_DCS_CMD(0xD6, 0x03),
> +       _INIT_DCS_CMD(0xD7, 0x04),
> +       _INIT_DCS_CMD(0xD9, 0x01),
> +       _INIT_DCS_CMD(0xDB, 0x01),
> +       _INIT_DCS_CMD(0xE4, 0xF0),
> +       _INIT_DCS_CMD(0xE5, 0x0A),
> +       _INIT_DCS_CMD(0xB0, 0x00),
> +       _INIT_DCS_CMD(0xCC, 0x08),
> +       _INIT_DCS_CMD(0xC2, 0x08),
> +       _INIT_DCS_CMD(0xC4, 0x10),
> +       _INIT_DCS_CMD(0xB0, 0x02),
> +       _INIT_DCS_CMD(0xC0, 0x00),
> +       _INIT_DCS_CMD(0xC1, 0x0A),
> +       _INIT_DCS_CMD(0xC2, 0x20),
> +       _INIT_DCS_CMD(0xC3, 0x24),
> +       _INIT_DCS_CMD(0xC4, 0x23),
> +       _INIT_DCS_CMD(0xC5, 0x29),
> +       _INIT_DCS_CMD(0xC6, 0x23),
> +       _INIT_DCS_CMD(0xC7, 0x1C),
> +       _INIT_DCS_CMD(0xC8, 0x19),
> +       _INIT_DCS_CMD(0xC9, 0x17),
> +       _INIT_DCS_CMD(0xCA, 0x17),
> +       _INIT_DCS_CMD(0xCB, 0x18),
> +       _INIT_DCS_CMD(0xCC, 0x1A),
> +       _INIT_DCS_CMD(0xCD, 0x1E),
> +       _INIT_DCS_CMD(0xCE, 0x20),
> +       _INIT_DCS_CMD(0xCF, 0x23),
> +       _INIT_DCS_CMD(0xD0, 0x07),
> +       _INIT_DCS_CMD(0xD1, 0x00),
> +       _INIT_DCS_CMD(0xD2, 0x00),
> +       _INIT_DCS_CMD(0xD3, 0x0A),
> +       _INIT_DCS_CMD(0xD4, 0x13),
> +       _INIT_DCS_CMD(0xD5, 0x1C),
> +       _INIT_DCS_CMD(0xD6, 0x1A),
> +       _INIT_DCS_CMD(0xD7, 0x13),
> +       _INIT_DCS_CMD(0xD8, 0x17),
> +       _INIT_DCS_CMD(0xD9, 0x1C),
> +       _INIT_DCS_CMD(0xDA, 0x19),
> +       _INIT_DCS_CMD(0xDB, 0x17),
> +       _INIT_DCS_CMD(0xDC, 0x17),
> +       _INIT_DCS_CMD(0xDD, 0x18),
> +       _INIT_DCS_CMD(0xDE, 0x1A),
> +       _INIT_DCS_CMD(0xDF, 0x1E),
> +       _INIT_DCS_CMD(0xE0, 0x20),
> +       _INIT_DCS_CMD(0xE1, 0x23),
> +       _INIT_DCS_CMD(0xE2, 0x07),

So make all of this use that struct then hardcode the rest.

> +       _INIT_DCS_CMD(0X11),

Just call mipi_dsi_dcs_exit_sleep_mode(dsi) which sends exactly
this command over DSI.

> +       _INIT_DELAY_CMD(120),

Just harcode this.

> +       _INIT_DCS_CMD(0X29),

This command is a bit weird since it means
MIPI_DSI_GENERIC_LONG_WRITE and it is strange to have
this and nothing else at the end. At least use the define
from <video/mipi_display.h>

> +       _INIT_DELAY_CMD(80),

Just hardcode this.

> +static int inx_panel_enter_sleep_mode(struct inx_panel *inx)
> +{
> +       struct mipi_dsi_device *dsi = inx->dsi;
> +       int ret;
> +
> +       dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +       ret = mipi_dsi_dcs_set_display_off(dsi);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
> +}

This looks good.

> +static int inx_panel_prepare(struct drm_panel *panel)
> +{
> +       struct inx_panel *inx = to_inx_panel(panel);
> +       int ret;
> +
> +       if (inx->prepared)
> +               return 0;

This is unnecessary. Trust the framework to keep track of
this and drop that prepared member of inx.

> +       gpiod_set_value(inx->enable_gpio, 0);
> +       usleep_range(1000, 1500);
> +
> +       ret = regulator_enable(inx->pp1800);
> +       if (ret < 0)
> +               return ret;
> +
> +       usleep_range(3000, 5000);
> +
> +       ret = regulator_enable(inx->avdd);> +
> +static struct mipi_dsi_driver inx_panel_driver = {
> +       .driver = {
> +               .name = "panel-innolux-himax8279d",
> +               .of_match_table = inx_of_match,
> +       },
> +       .probe = inx_panel_probe,
> +       .remove = inx_panel_remove,
> +       .shutdown = inx_panel_shutdown,
> +};
> +module_mipi_dsi_driver(inx_panel_driver);
> +
> +MODULE_AUTHOR("Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>");
> +MODULE_DESCRIPTION("INNOLUX HIMAX8279D 1200x1920 video mode panel driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1

> +       if (ret < 0)
> +               goto poweroff1v8;
> +       ret = regulator_enable(inx->avee);
> +       if (ret < 0)
> +               goto poweroffavdd;
> +
> +       usleep_range(5000, 10000);
> +
> +       gpiod_set_value(inx->enable_gpio, 1);
> +       usleep_range(1000, 2000);
> +       gpiod_set_value(inx->enable_gpio, 0);
> +       usleep_range(1000, 2000);
> +       gpiod_set_value(inx->enable_gpio, 1);
> +       usleep_range(6000, 10000);

Care to explain what is happening here? Add a comment
about this wire shake. Honestly this looks more like
a RESET_N signal (active low reset) than an enable, so
in that case rename it reset_gpiod?

If this is an active low reset, invert the values and tag the
reset line with GPIO_ACTIVE_LOW in the device tree and
mention this in the DT bindings.

> +static int inx_panel_enable(struct drm_panel *panel)
> +{
> +       msleep(130);
> +       return 0;
> +}

This looks pretty pointless. Can't you just stick this msleep(130)
and the end of .prepare() and skip this?

> +       inx->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);

Suspect this is reset_gpio and should be flagged active low
in the device tree and requested GPIOD_OUT_HIGH instead.

> +       if (IS_ERR(inx->enable_gpio)) {
> +               dev_err(dev, "cannot get reset-gpios %ld\n",

You even say it  is reset-gpios ... :P> +
> +static struct mipi_dsi_driver inx_panel_driver = {
> +       .driver = {
> +               .name = "panel-innolux-himax8279d",
> +               .of_match_table = inx_of_match,
> +       },
> +       .probe = inx_panel_probe,
> +       .remove = inx_panel_remove,
> +       .shutdown = inx_panel_shutdown,
> +};
> +module_mipi_dsi_driver(inx_panel_driver);
> +
> +MODULE_AUTHOR("Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>");
> +MODULE_DESCRIPTION("INNOLUX HIMAX8279D 1200x1920 video mode panel driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1

> +static const struct of_device_id inx_of_match[] = {
> +       { .compatible = "starry,2081101qfh032011-53g",
> +         .data = &starry_qfh032011_53g_desc
> +       },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, inx_of_match);

It's really nice that you make it possible to use more displays with the
same display controller!

Yours,
Linus Walleij