| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACRpkdYRbPtir7a3rv4VhKTwSU2O7Em1CGFDfQ68M0r0T7QjwA@mail.gmail.com>
Date: Wed, 20 Apr 2022 22:41:27 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: xiazhengqiao <xiazhengqiao@...qin.corp-partner.google.com>
Cc: thierry.reding@...il.com, sam@...nborg.org, airlied@...ux.ie,
daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RESEND v2 1/2] drm/panel: Add inx Himax8279d MIPI-DSI LCD panel driver
On Thu, Apr 14, 2022 at 10:19 AM xiazhengqiao
<xiazhengqiao@...qin.corp-partner.google.com> wrote:
> Add STARRY 2081101QFH032011-53G 10.1" WUXGA TFT LCD panel
>
> Signed-off-by: xiazhengqiao <xiazhengqiao@...qin.corp-partner.google.com>
> Tested-by: Hsin-Yi Wang <hsinyi@...omium.org>
> Reviewed-by: Hsin-Yi Wang <hsinyi@...omium.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@...qin.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@...qin.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
Powered by blists - more mailing lists