Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210913105920.3719525-1-yangcong5@huaqin.corp-partner.google.com>
 <20210913105920.3719525-6-yangcong5@huaqin.corp-partner.google.com>
In-Reply-To: <20210913105920.3719525-6-yangcong5@huaqin.corp-partner.google.com>
From:   Doug Anderson <dianders@google.com>
Date:   Mon, 13 Sep 2021 12:23:25 -0700
Message-ID: <CAD=FV=XFApy_RxCRzrQrQ1-xua+tBpZN60B1TzyEvDNm3LnD7Q@mail.gmail.com>
Subject: Re: [v5 5/5] drm/panel: Update BOE and INX init code to support touch function
To:     yangcong <yangcong5@huaqin.corp-partner.google.com>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Sep 13, 2021 at 3:59 AM yangcong
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> This is an incell IC, TDDI use time division multiplexing.
> Init code effect touch sensing.
> Support touch function we needed to handle were:
> a) Update init code for the panel driver, adjust the porch value.
> b) After update init code these two panels can support DSI HS mode.
>
> Signed-off-by: yangcong <yangcong5@huaqin.corp-partner.google.com>
> ---
>  .../gpu/drm/panel/panel-boe-tv101wum-nl6.c    | 399 +++++-------------
>  1 file changed, 110 insertions(+), 289 deletions(-)

Please squash this with patch #3 in the series. Even though you have
landed patch #3 in the series in a Chrome OS tree it doesn't matter.
Since patch #3 hasn't landed upstream there is no reason to post the
"wrong" code and then fix it up in the same series. The Chrome OS tree
can always take a revert of the old version and a re-pick of the new
version.


> @@ -36,7 +36,6 @@ struct panel_desc {
>         const struct panel_init_cmd *init_cmds;
>         unsigned int lanes;
>         bool discharge_on_disable;
> -       bool unsupport_dsi_hs_mode;

Not that I'm complaining, but how did this suddenly get supported?


> @@ -84,6 +83,8 @@ static const struct panel_init_cmd boe_tv110c9m_init_cmd[] = {
>         _INIT_DCS_CMD(0x07, 0x78),
>         _INIT_DCS_CMD(0x08, 0x5A),
>         _INIT_DCS_CMD(0x0D, 0x63),
> +       _INIT_DCS_CMD(0x0E, 0x91),
> +       _INIT_DCS_CMD(0x0F, 0x73),

I _really_ don't like this opaque list of commands and the fact that
apparently they aren't fixed but need to change depending on how
you're configuring the panel, but I won't personally block this patch
just because of it since it matches what other panels in this driver
are doing.

If someone more familiar with MIPI panels wants to chime in though,
I'm all ears.


> @@ -1442,15 +1260,15 @@ static int boe_panel_enable(struct drm_panel *panel)
>  }
>
>  static const struct drm_display_mode boe_tv110c9m_default_mode = {
> -       .clock = 162383,
> +       .clock = 166594,
>         .hdisplay = 1200,
>         .hsync_start = 1200 + 40,
>         .hsync_end = 1200 + 40 + 8,
>         .htotal = 1200 + 40 + 8 + 28,
>         .vdisplay = 2000,
>         .vsync_start = 2000 + 26,
> -       .vsync_end = 2000 + 26 + 1,
> -       .vtotal = 2000 + 26 + 1 + 94,
> +       .vsync_end = 2000 + 26 + 2,
> +       .vtotal = 2000 + 26 + 2 + 148,
>         .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
>  };
>
> @@ -1463,14 +1281,15 @@ static const struct panel_desc boe_tv110c9m_desc = {
>         },
>         .lanes = 4,
>         .format = MIPI_DSI_FMT_RGB888,
> -       .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> -                     MIPI_DSI_MODE_LPM,
> +       .mode_flags = MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_VIDEO
> +                       | MIPI_DSI_MODE_VIDEO_HSE
> +                       | MIPI_DSI_CLOCK_NON_CONTINUOUS
> +                       | MIPI_DSI_MODE_VIDEO_BURST,
>         .init_cmds = boe_tv110c9m_init_cmd,
> -       .unsupport_dsi_hs_mode = true,
>  };
>
>  static const struct drm_display_mode inx_hj110iz_default_mode = {
> -       .clock = 162383,
> +       .clock = 166594,
>         .hdisplay = 1200,
>         .hsync_start = 1200 + 40,
>         .hsync_end = 1200 + 40 + 8,
> @@ -1478,7 +1297,7 @@ static const struct drm_display_mode inx_hj110iz_default_mode = {
>         .vdisplay = 2000,
>         .vsync_start = 2000 + 26,
>         .vsync_end = 2000 + 26 + 1,
> -       .vtotal = 2000 + 26 + 1 + 94,
> +       .vtotal = 2000 + 26 + 1 + 149,

It seems really odd that the two panels have _exactly_ the same timing
except that one of them now has:

.vsync_end = 2000 + 26 + 2,

...and the other:

.vsync_end = 2000 + 26 + 1,

Do they really need to be different?

-Doug