Message-ID: <CAHwB_NJMWR883kjshtfBFuDBcM9Av87Fx2Jf4d=3_5LLnhUFXg@mail.gmail.com>
Date: Tue, 7 May 2024 21:44:35 +0800
From: cong yang <yangcong5@...qin.corp-partner.google.com>
To: Doug Anderson <dianders@...omium.org>
Cc: sam@...nborg.org, neil.armstrong@...aro.org, daniel@...ll.ch, 
	linus.walleij@...aro.org, krzysztof.kozlowski+dt@...aro.org, 
	robh+dt@...nel.org, conor+dt@...nel.org, airlied@...il.com, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, xuxinxiong@...qin.corp-partner.google.com
Subject: Re: [PATCH v3 2/7] drm/panel: himax-hx83102: Break out as separate driver

Hi,
 Thanks for review.

Doug Anderson <dianders@...omium.org> 于2024年5月1日周三 03:19写道：

>
> Hi,
>
> On Tue, Apr 23, 2024 at 7:30 PM Cong Yang
> <yangcong5@...qin.corp-partner.google.com> wrote:
> >
> > The Starry HX83102 based mipi panel should never have been part of the boe
> > tv101wum driver. Discussion with Doug and Linus in V1 [1], we need a
> > separate driver to enable the hx83102 controller.
> >
> > In hx83102 driver, add DSI commands as macros. So it can add some panels
> > with same control model in the future.
> >
> > [1]: https://lore.kernel.org/all/CACRpkdbzYZAS0=zBQJUC4CB2wj4s1h6n6aSAZQvdMV95r3zRUw@mail.gmail.com
> >
> > Signed-off-by: Cong Yang <yangcong5@...qin.corp-partner.google.com>
> > ---
> > Chage since V3:
> >
> > -  Drop excess flags and function, inital cmds use lowercasehex.
> >
> > V2: https://lore.kernel.org/all/20240422090310.3311429-3-yangcong5@huaqin.corp-partner.google.com
> >
> > ---
> >  drivers/gpu/drm/panel/Kconfig                 |   9 +
> >  drivers/gpu/drm/panel/Makefile                |   1 +
> >  .../gpu/drm/panel/panel-boe-tv101wum-nl6.c    |  99 ----
> >  drivers/gpu/drm/panel/panel-himax-hx83102.c   | 525 ++++++++++++++++++
> >  4 files changed, 535 insertions(+), 99 deletions(-)
>
> It probably makes sense to base your series upon mine that reduces
> bloat / introduces a better way to do these init sequences. I'm going
> to wait one more day in case anyone else has any more comments on my
> v2 and then I'll post my v3. So far everyone has been on-board with
> the overall goal and so all we need to do is iron out the small
> details, so I don't expect it to take too long.
>
> If you want to wait a day or two and then post your patches atop my v3
> (once I post it) then that would be OK by me.
>
>
> > @@ -0,0 +1,525 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for panels based on Himax HX83102 controller, such as:
> > + *
> > + * - Starry 10.51" WUXGA MIPI-DSI panel
> > + *
> > + * Based on drivers/gpu/drm/panel/panel-boe-tv101wum.c
>
> The above file doesn't exist? Maybe you forgot the "-nl6" suffix? I
> would also say that this driver appears to be more similar to
> `panel-himax-hx8394.c` even if the data came from
> `panel-boe-tv101wum-nl6.c`.
>
> ...also, since this is based on `panel-himax-hx8394.c`, it seems like
> you're making pretty significant changes here. For instance, when this
> code was part of `panel-boe-tv101wum-nl6.c` it used to do the "init
> commands" as part of prepare. With the new driver it does it as part
> of "enable". IMO even if the new code based on `panel-himax-hx8394.c`
> is more correct, I'd rather see you change that in a separate change.
> In this change, which is supposed to be more about code refactoring, I
> think you should focus on keeping the behavior before and after your
> patch identical.

Ok,modified to panel-himax-hx8394.c` in V4 version.

>
>
> > +/* Manufacturer specific DSI commands */
> > +#define HX83102_SETPOWER       0xb1
> > +#define HX83102_SETDISP                0xb2
> > +#define HX83102_SETCYC         0xb4
> > +#define HX83102_SETEXTC                0xb9
> > +#define HX83102_SETMIPI                0xba
> > +#define HX83102_SETVDC         0xbc
> > +#define HX83102_SETBANK                0xbd
> > +#define HX83102_UNKNOWN1       0xbe
>
> I'm not sure that the "unknown" define helps much, but I guess it's
> fine. One nit would be to call this UNKNOWN_BE based on the address so
> that if we can later replace some of the unknowns then there won't be
> gaps in the numbering.

Got it. Thanks.

>
>
> > +#define HX83102_SETPTBA                0xbf
> > +#define HX83102_SETSTBA                0xc0
> > +#define HX83102_SETTCON                0xc7
> > +#define HX83102_SETRAMDMY      0xc8
> > +#define HX83102_SETPWM         0xc9
> > +#define HX83102_SETCLOCK       0xcb
> > +#define HX83102_SETPANEL       0xcc
> > +#define HX83102_SETCASCADE     0xd0
> > +#define HX83102_SETPCTRL       0xd1
> > +#define HX83102_UNKNOWN2       0xd2
> > +#define HX83102_SETGIP0                0xd3
> > +#define HX83102_SETGIP1                0xd5
> > +#define HX83102_UNKNOWN3       0xd6
>
> Given everything surrounding it and given a datasheet I have for a
> similar panel, I'm going to guess UNKNOWN3 is "GIP2".

Got it. Thanks.

>
>
> > +#define HX83102_SETGIP3                0xd8
> > +#define HX83102_UNKNOWN4       0xe0
>
> I think UNKNOWN4 is SETGMA to set the gamma curve.

Got it. Thanks.

>
>
> > +static int starry_init_cmd(struct hx83102 *ctx)
> > +{
> > +       struct mipi_dsi_device *dsi = ctx->dsi;
> > +
> > +       mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
>
> As far as I can tell from looking at a different (but similar)
> datasheet, the above means "enable extended command set". Assuming I'm
> correct, maybe you could abstract out:
>
> static int hx83102_enable_extended_cmds(struct hx83102 *ctx, bool enable)
> {
>   if (enable)
>     mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
>   else
>     mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x00, 0x00, 0x00);
> }
>
> Then your panel-specific init functions could call that?

Got it. Thanks.

>
> Speaking of which, some of the panels that you add to this driver seem
> to disable extended commands at the end of their init sequence, but
> this one doesn't. Should it?

I  have confirmed with himax that disable extended commands  used
at the end to prevent accidental writing. And our inital code has been
confirmed by himax before FSI. Considering that this has been on the
market for a long time and there are no feedback issues, I think it should
remain the same as `panel-boe-tv101wum-nl6.c`.  What do you think?

>
>
> > +       mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2c, 0xb5, 0xb5, 0x31, 0xf1, 0x31, 0xd7, 0x2f,
> > +                                                 0x36, 0x36, 0x36, 0x36, 0x1a, 0x8b, 0x11, 0x65, 0x00, 0x88, 0xfa, 0xff,
> > +                                                 0xff, 0x8f, 0xff, 0x08, 0x74, 0x33);
>
> nit: Can you make your indentation cleaner? Right now when a function
> call extends to multiple lines the subsequent lines are indented a
> somewhat random amount of space that probably has to do with how much
> they needed to be indented before.
>
> ...though if you base on my v3 series that I'll send out tomorrow then
> probably you'd just move this over and the indentation would be right.

Got it. Fix in V4.

>
>
> > +
> > +       mipi_dsi_dcs_write_seq(dsi, HX83102_SETDISP, 0x00, 0x47, 0xb0, 0x80, 0x00, 0x12, 0x72, 0x3c,
> > +                                                 0xa3, 0x03, 0x03, 0x00, 0x00, 0x88, 0xf5);
> > +
> > +       mipi_dsi_dcs_write_seq(dsi, HX83102_SETCYC, 0x76, 0x76, 0x76, 0x76, 0x76, 0x76, 0x63, 0x5c,
> > +                                                 0x63, 0x5c, 0x01, 0x9e);
>
> nit: I wouldn't have put a blank line between every function call.

Got it. Fix in V4.

>
>
> > +static int hx83102_enable(struct drm_panel *panel)
> > +{
> > +       struct hx83102 *ctx = panel_to_hx83102(panel);
> > +       struct mipi_dsi_device *dsi = ctx->dsi;
> > +       struct device *dev = &dsi->dev;
> > +       int ret;
> > +
> > +       ret = ctx->desc->init_cmds(ctx);
> > +       if (ret) {
> > +               dev_err(dev, "Panel init cmds failed: %d\n", ret);
> > +               return ret;
> > +       }
>
> nit: don't call it "init_cmds" now that it's not a cmdlist anymore. In
> my patch series converting things I called it "init". Seems like
> "hx8394.c" calls it "init_sequence". Either would be fine.

Got it. Fix in V4.

>
> nit: the init function already prints errors so you don't need to.
> Just return the error without printing.
>
>
> > +       ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       msleep(120);
> > +
> > +       ret = mipi_dsi_dcs_set_display_on(dsi);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to turn on the display: %d\n", ret);
> > +               goto sleep_in;
> > +       }
> > +
> > +       msleep(130);
> > +
> > +       return 0;
> > +
> > +sleep_in:
> > +       ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> > +       if (!ret)
> > +               msleep(50);
>
> The above is broken (but it's also broken in the driver that you
> copied this from). Specifically imagine that the call to
> mipi_dsi_dcs_set_display_on() above failed. "ret" will have an error
> code and you'll jump to "sleep_in". Now, imagine that
> mipi_dsi_dcs_enter_sleep_mode() _didn't_ fail. Since you store the
> result in the same variable "ret" you'll clobber the error code that
> mipi_dsi_dcs_set_display_on() returned and you'll return "0" (no
> error) from this function. That's not right.

Thank you for your explanation, remove  'goto sleep_in;' in V4.

>
>
> > +static int hx83102_panel_add(struct hx83102 *ctx)
> > +{
> > +       struct device *dev = &ctx->dsi->dev;
> > +       int err;
> > +
> > +       ctx->avdd = devm_regulator_get(dev, "avdd");
> > +       if (IS_ERR(ctx->avdd))
> > +               return PTR_ERR(ctx->avdd);
> > +
> > +       ctx->avee = devm_regulator_get(dev, "avee");
> > +       if (IS_ERR(ctx->avee))
> > +               return PTR_ERR(ctx->avee);
> > +
> > +       ctx->pp1800 = devm_regulator_get(dev, "pp1800");
> > +       if (IS_ERR(ctx->pp1800))
> > +               return PTR_ERR(ctx->pp1800);
> > +
> > +       ctx->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> > +       if (IS_ERR(ctx->enable_gpio)) {
> > +               return dev_err_probe(dev, PTR_ERR(ctx->enable_gpio), "Cannot get enable GPIO\n");
> > +       }
>
> nit: remove braces since the above is now one line.
Got it. Thanks.

Powered by blists - more mailing lists