Received-SPF: pass (google.com: domain of linux-kernel+bounces-156151-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240422090310.3311429-1-yangcong5@huaqin.corp-partner.google.com>
 <20240422090310.3311429-3-yangcong5@huaqin.corp-partner.google.com>
 <CAD=FV=V2O2aFDVn5CjbXfgcOLkmNp-G3ChVqQKouB2mDB+NZug@mail.gmail.com>
 <CAHwB_NJsDsTc=gjP8TJ+6ipo10uMYFLmuf+tKGVgxnznhuAcUQ@mail.gmail.com> <CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com>
In-Reply-To: <CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com>
From: cong yang <yangcong5@huaqin.corp-partner.google.com>
Date: Wed, 24 Apr 2024 10:42:29 +0800
Message-ID: <CAHwB_N+e4E8uZe3YpfNyemPyW-Rj1RLR5kKA3SDiBygOPYrmVQ@mail.gmail.com>
Subject: Re: [PATCH v2 2/7] drm/panel: himax-hx83102: Break out as separate driver
To: Doug Anderson <dianders@chromium.org>
Cc: sam@ravnborg.org, neil.armstrong@linaro.org, daniel@ffwll.ch, 
	linus.walleij@linaro.org, krzysztof.kozlowski+dt@linaro.org, 
	robh+dt@kernel.org, conor+dt@kernel.org, airlied@gmail.com, 
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, xuxinxiong@huaqin.corp-partner.google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,
 Thanks reply.

Doug Anderson <dianders@chromium.org> =E4=BA=8E2024=E5=B9=B44=E6=9C=8824=E6=
=97=A5=E5=91=A8=E4=B8=89 00:26=E5=86=99=E9=81=93=EF=BC=9A
>
> Hi,
>
> On Tue, Apr 23, 2024 at 2:37=E2=80=AFAM cong yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > > > +static int starry_init_cmd(struct hx83102 *ctx)
> > > > +{
> > > > +       struct mipi_dsi_device *dsi =3D ctx->dsi;
> > > > +
> > > > +       mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x=
21, 0x55, 0x00);
> > > > +
> > > > +       mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 0=
xB5, 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> > > > +                                                 0x36, 0x36, 0x36,=
 0x36, 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> > > > +                                                 0xFF, 0x8F, 0xFF,=
 0x08, 0x74, 0x33);
> > >
> > > I know this is a sticking point between Linus W. and me, but I'm
> > > really not a fan of all the hardcoded function calls since it bloats
> > > the code so much. I think we need to stick with something more table
> > > based at least for the majority of the commands. If I understand
> > > correctly, Linus was OK w/ something table based as long as it was in
> > > common code [1]. I think he also wanted the "delay" out of the table,
> > > but since those always seem to be at the beginning or the end it seem=
s
> > > like we could still have the majority of the code as table based. Do
> > > you want to make an attempt at that? If not I can try to find some
> > > time to write up a patch in the next week or so.
> >
> > Do you mean not add "delay" in the table?  However, the delay
> > required by each panel may be different. How should this be handled?
>
> In the case of the "himax-hx83102" driver, it looks as if all the
> delays are at the beginning or end of the init sequence. That means
> you could just make those extra parameters that are set per-panel and
> you're back to having a simple sequence without delays.

Do you mean add msleep  in hx83102_enable()?

@@ -612,12 +604,15 @@ static int hx83102_enable(struct drm_panel *panel)
        struct device *dev =3D &dsi->dev;
        int ret;

+       msleep(60);
        ret =3D ctx->desc->init_cmds(ctx);
        if (ret) {
                dev_err(dev, "Panel init cmds failed: %d\n", ret);
                return ret;
        }

+       msleep(60);
+
        ret =3D mipi_dsi_dcs_exit_sleep_mode(dsi);

>
> If you had panels that needed delays in a more complicated way, you
> could keep the per-panel functions but just make the bulk of the
> function calls apply a sequence. For instance:
>
> static int my_panel_init_cmd(...)
> {
>   ret =3D mipi_dsi_dcs_write_cmd_seq(dsi, my_panel_init_cmd_seq);
>   if (ret)
>     return ret;
>   mdelay(100);
>   ret =3D mipi_dsi_dcs_write(dsi, ...);
>   if (ret)
>     return ret;
>   mdelay(50);
>   ret =3D mipi_dsi_dcs_write_cmd_seq(dsi, ...);
>   if (ret)
>     return ret;
> }
>
> The vast majority of the work is still table driven so it doesn't
> bloat the code, but you don't have the "delay" in the command sequence
> since Linus didn't like it. I think something like the above would
> make Linus happy and I'd be OK w/ it as well. Ideally you should still
> make your command sequence as easy to understand as possible, kind of
> like how we did with _INIT_SWITCH_PAGE_CMD() in
> "panel-ilitek-ili9882t.c"
>
> As part of this, you'd have to add a patch to create
> mipi_dsi_dcs_write_cmd_seq(), but hopefully that shouldn't be too
> complicated?
>
>
> > It would be great if you could help provide a patch. Thank you so much.
>
> Sure, I can, though maybe you want to give it a shot with the above descr=
iption?

Sorry, I still don't seem to understand. How to encapsulate the parameters =
of
"HX83102_SETDISP, HX83102_SETCYC,....."? Different parameters for each pane=
l.
I have sent my V3 but it does not contain the patch you want.


>
> -Doug