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

[Emil Velikov]

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