Received-SPF: pass (google.com: domain of linux-kernel+bounces-157797-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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> <CAHwB_N+e4E8uZe3YpfNyemPyW-Rj1RLR5kKA3SDiBygOPYrmVQ@mail.gmail.com>
In-Reply-To: <CAHwB_N+e4E8uZe3YpfNyemPyW-Rj1RLR5kKA3SDiBygOPYrmVQ@mail.gmail.com>
From: Doug Anderson <dianders@chromium.org>
Date: Wed, 24 Apr 2024 15:43:10 -0700
Message-ID: <CAD=FV=XHmkVbU7RqvOPLpknWWTPAMWu_-EApWdcuPRRDMOim8g@mail.gmail.com>
Subject: Re: [PATCH v2 2/7] drm/panel: himax-hx83102: Break out as separate driver
To: cong yang <yangcong5@huaqin.corp-partner.google.com>
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, 
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Tue, Apr 23, 2024 at 7:42=E2=80=AFPM cong yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> 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, =
0x21, 0x55, 0x00);
> > > > > +
> > > > > +       mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5,=
 0xB5, 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> > > > > +                                                 0x36, 0x36, 0x3=
6, 0x36, 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> > > > > +                                                 0xFF, 0x8F, 0xF=
F, 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 bloat=
s
> > > > the code so much. I think we need to stick with something more tabl=
e
> > > > 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 tabl=
e,
> > > > but since those always seem to be at the beginning or the end it se=
ems
> > > > like we could still have the majority of the code as table based. D=
o
> > > > 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 muc=
h.
> >
> > Sure, I can, though maybe you want to give it a shot with the above des=
cription?
>
> Sorry, I still don't seem to understand. How to encapsulate the parameter=
s of
> "HX83102_SETDISP, HX83102_SETCYC,....."? Different parameters for each pa=
nel.
> I have sent my V3 but it does not contain the patch you want.

It sounds as if Dmitry has come up with a solution that allows us to
keep using the functions like you've been using but avoid the code
bloat problems. ...so let's go with that. It sounds as if he's going
to send a patch before too long and then it should be pretty easy to
convert your patches over to use his new function.

[1] https://lore.kernel.org/r/CAA8EJprv3qBd1hfdWHrfhY=3DS0w2O70dZnYb6TVsS6A=
GRPxsYdw@mail.gmail.com