Re: [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel

From: Emil Velikov
Date: Wed Mar 14 2018 - 08:16:50 EST


On 14 March 2018 at 09:12, Lin Huang <hl@xxxxxxxxxxxxxx> wrote:
> Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel, it reuse
> the Innolux P079ZCA panel driver.
>
> Change-Id: I97923aa3735f707332681691b0231c9421b427d0
> Signed-off-by: Lin Huang <hl@xxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - download panel initial code
>
> drivers/gpu/drm/panel/panel-innolux-p079zca.c | 192 +++++++++++++++++++++++++-
> 1 file changed, 187 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 2075a9d..883b279 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -20,6 +20,15 @@
>
> #include <video/mipi_display.h>
>
> +struct panel_init_cmd {
> + int len;
> + const char *data;
> +};
> +
> +#define _INIT_CMD(...) { \
> + .len = sizeof((char[]){__VA_ARGS__}), \
> + .data = (char[]){__VA_ARGS__} }
> +
> struct panel_desc {
> const struct drm_display_mode *modes;
> unsigned int bpc;
> @@ -30,6 +39,7 @@ struct panel_desc {
>
> unsigned long flags;
> enum mipi_dsi_pixel_format format;
> + const struct panel_init_cmd *init_cmds;
> unsigned int lanes;
> };
>
> @@ -88,9 +98,12 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
> return err;
> }
>
> + /* p097pfg: t15 */
> + msleep(100);
> +
Based on the comment this is not needed for p079zca, yet still executed.

> gpiod_set_value_cansleep(innolux->enable_gpio, 0);
>
> - /* T8: 80ms - 1000ms */
> + /* p079zca: t8*/
> msleep(80);
>
And this is seem the opposite - zca only, yet executed on pfg.

One way to to avoid these and use the appropriate sleep throughout is
to store have the numbers in the respective panel_desc entries.



> regulator_disable(innolux->avee);
> @@ -124,13 +137,43 @@ static int innolux_panel_prepare(struct drm_panel *panel)
> if (err < 0)
> goto disable_avdd;
>
> - /* T2: 15ms - 1000ms */
> - usleep_range(15000, 16000);
> + /* p079zca: t2 (20ms), p097pfg: t4 (15ms) */
> + usleep_range(20000, 21000);
>
> gpiod_set_value_cansleep(innolux->enable_gpio, 1);
>
> - /* T4: 15ms - 1000ms */
> - usleep_range(15000, 16000);
> + /* p079zca: t4, p097pfg: t5 */
> + usleep_range(20000, 21000);
> +
> + if (innolux->desc->init_cmds) {
> + const struct panel_init_cmd *cmds =
> + innolux->desc->init_cmds;
> + int i;
> +
> + for (i = 0; cmds[i].len != 0; i++) {
> + const struct panel_init_cmd *cmd = &cmds[i];
> +
> + err = mipi_dsi_generic_write(innolux->link, cmd->data,
> + cmd->len);
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to write command %d\n", i);
> + goto poweroff;
> + }
> +
> + /*
> + * Included by random guessing, because without this
> + * (or at least, some delay), the panel sometimes
> + * didn't appear to pick up the command sequence.
> + */
This seems a bit hackish. Adding a reference to a bugreport/mailing
list discussion would be a nice move.
It'll save you/next person a lot of time searching for the specifics
of the problem.

> + err = mipi_dsi_dcs_nop(innolux->link);
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to send DCS nop: %d\n", err);
> + goto poweroff;
> + }
> + }
> + }
>
> err = mipi_dsi_dcs_exit_sleep_mode(innolux->link);
> if (err < 0) {
> @@ -213,6 +256,142 @@ static const struct panel_desc innolux_p079zca_panel_desc = {
> .lanes = 4,
> };
>
> +static const struct drm_display_mode innolux_p097pfg_mode = {
> + .clock = 229000,
> + .hdisplay = 1536,
> + .hsync_start = 1536 + 100,
> + .hsync_end = 1536 + 100 + 24,
> + .htotal = 1536 + 100 + 24 + 100,
> + .vdisplay = 2048,
> + .vsync_start = 2048 + 100,
> + .vsync_end = 2048 + 100 + 2,
> + .vtotal = 2048 + 100 + 2 + 18,
> + .vrefresh = 60,
> +};
> +
> +static const struct panel_init_cmd innolux_p097pfg_init_cmds[] = {

A lot of magic numbers :-(

Please reference the source of these - say XX spec. vY.
We could get a vY+1, which makes the nop workaround obsolete.

HTH
Emil