Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com>
 <20230519080136.4058243-1-yangcong5@huaqin.corp-partner.google.com> <20230519080136.4058243-2-yangcong5@huaqin.corp-partner.google.com>
In-Reply-To: <20230519080136.4058243-2-yangcong5@huaqin.corp-partner.google.com>
From:   Doug Anderson <dianders@google.com>
Date:   Fri, 19 May 2023 10:17:19 -0700
Message-ID: <CAD=FV=Wjmff00acDU3JxwSNYW8ctdz+qi=2fXSGFRt+Bnese4Q@mail.gmail.com>
Subject: Re: [v1 1/2] drm/panel: Support for Starry-himax83102-j02 TDDI
 MIPI-DSI panel
To:     Cong Yang <yangcong5@huaqin.corp-partner.google.com>
Cc:     sam@ravnborg.org, neil.armstrong@linaro.org, daniel@ffwll.ch,
        hsinyi@google.com, dri-devel@lists.freedesktop.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Fri, May 19, 2023 at 1:02=E2=80=AFAM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> The Starry-himax83102-j02 panel is a TDDI IC. From the datasheet[1],
> it seems that the touch can communicate successfully only when the RST
> signal is high. Since i2c_hid_core_probe comes after boe_panel_prepare
> let's set the default high for RST at boe_panel_add.

No, that doesn't work. There are no guarantees about the ordering of
the probe of the i2c_hid device and the panel and the order could
change from version to version of Linux. Also: deasserting this reset
early like this (before regulators are turned on) can cause leakage
since that will make the signal go high and the touchscreen can suck
current out of that line.

Is it possible to change the hardware to fix this and have separate
reset lines for the touchscreen and the panel?

For a long time, I have felt like we needed a better solution in Linux
for stuff like this, but I've never found a clean way to do it. We
really want the touchscreen to power on and off together with the
panel, where the panel is in charge and the touchscreen always powers
on after the panel and powers off before the panel. I can't promise
anything, but I can see if I can find some time to whip up a
prototype.


> @@ -1698,6 +1768,34 @@ static const struct panel_desc starry_qfh032011_53=
g_desc =3D {
>         .init_cmds =3D starry_qfh032011_53g_init_cmd,
>  };
>
> +static const struct drm_display_mode starry_himax83102_j02_default_mode =
=3D {
> +       .clock =3D 161600,
> +       .hdisplay =3D 1200,
> +       .hsync_start =3D 1200 + 40,
> +       .hsync_end =3D 1200 + 40 + 20,
> +       .htotal =3D 1200 + 40 + 20 + 40,
> +       .vdisplay =3D 1920,
> +       .vsync_start =3D 1920 + 116,
> +       .vsync_end =3D 1920 + 116 + 8,
> +       .vtotal =3D 1920 + 116 + 8 + 12,
> +       .type =3D DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +};
> +
> +static const struct panel_desc starry_himax83102_j02_desc =3D {
> +       .modes =3D &starry_himax83102_j02_default_mode,
> +       .bpc =3D 8,
> +       .size =3D {
> +               .width_mm =3D 141,
> +               .height_mm =3D 226,
> +       },
> +       .lanes =3D 4,
> +       .format =3D MIPI_DSI_FMT_RGB888,
> +       .mode_flags =3D MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PU=
LSE |
> +                     MIPI_DSI_MODE_LPM,
> +       .init_cmds =3D starry_himax83102_j02_init_cmd,
> +       .enable_gpio_init_value =3D 1,
> +       .lp11_before_reset =3D true,
> +};
>  static int boe_panel_get_modes(struct drm_panel *panel,

nit: put a blank line above.


> @@ -1871,6 +1969,9 @@ static const struct of_device_id boe_of_match[] =3D=
 {
>         { .compatible =3D "starry,2081101qfh032011-53g",
>           .data =3D &starry_qfh032011_53g_desc
>         },
> +       { .compatible =3D "starry,himax83102-j02",
> +         .data =3D &starry_himax83102_j02_desc

You need device tree bindings for the above compatible string.