Re: [v3 3/4] drm/panel: support for BOE and INX video mode panel

[cong yang <yangcong5@huaqin.corp-partner.google.com>]

[-- Attachment #1: Type: text/plain, Size: 4027 bytes --]

Hi,Doug
  Thanks for your help. I had send v4 patch
https://patchwork.kernel.org/project/dri-devel/patch/20210830023849.258839-4-yangcong5@huaqin.corp-partner.google.com/.
Please help to review.

yangcong

On Fri, Aug 27, 2021 at 10:42 PM Doug Anderson <dianders@google.com> wrote:

> Hi,
>
> On Fri, Aug 27, 2021 at 1:24 AM yangcong
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > Add driver for BOE tv110c9m-ll3 and Inx hj110iz-01a panel
> > both of those are 10.95" 1200x2000 panel.
>
> Your commit message would be a good place to note design choices you
> made in your patch. Maybe you might say:
>
> Support for these two panels fits in nicely with the existing
> panel-boe-tv101wum-nl6 driver as suggested by Sam [1]. The main things
> we needed to handle were:
> a) These panels need slightly longer delays in two places. Since these
> new delays aren't much longer, let's just unconditionally increase
> them for the driver.
> b) One of these two panels doesn't support DSI HS mode so this patch
> adds a flag for a panel to disable that.
>
> [1] https://lore.kernel.org/r/YSPAseE6WD8dDRuz@ravnborg.org/
>
> If you send a new version, maybe you could include prose similar to that?
>
> > +       _INIT_DCS_CMD(0x4D, 0x21),
> > +       _INIT_DCS_CMD(0x4E, 0x43),
> > +       _INIT_DCS_CMD(0x51, 0x12),
> > +       _INIT_DCS_CMD(0x52, 0x34),
> > +       _INIT_DCS_CMD(0x55, 0x82, 0x02),
> > +       _INIT_DCS_CMD(0x56, 0x04),
> > +       _INIT_DCS_CMD(0x58, 0x21),
> > +       _INIT_DCS_CMD(0x59, 0x30),
> > +       _INIT_DCS_CMD(0x5A, 0xBA),      //9A
>
> nit: the "//9A" above seems like it's leftover from something. Remove?
>
> > +       _INIT_DCS_CMD(0x1F, 0xBA),//9A
> > +       _INIT_DCS_CMD(0x20, 0xA0),
> > +
> > +       _INIT_DCS_CMD(0x26, 0xBA),//9A
> > +       _INIT_DCS_CMD(0x27, 0xA0),
> > +
> > +       _INIT_DCS_CMD(0x33, 0xBA),//9A
> > +       _INIT_DCS_CMD(0x34, 0xA0),
> > +
> > +       _INIT_DCS_CMD(0x3F, 0xE0),
> > +
> > +       _INIT_DCS_CMD(0x40, 0x00),
> > +
> > +       _INIT_DCS_CMD(0x44, 0x00),
> > +       _INIT_DCS_CMD(0x45, 0x40),
> > +
> > +       _INIT_DCS_CMD(0x48, 0xBA),//9A
> > +       _INIT_DCS_CMD(0x49, 0xA0),
> > +
> > +       _INIT_DCS_CMD(0x5B, 0x00),
> > +       _INIT_DCS_CMD(0x5C, 0x00),
> > +       _INIT_DCS_CMD(0x5D, 0x00),
> > +       _INIT_DCS_CMD(0x5E, 0xD0),
> > +
> > +       _INIT_DCS_CMD(0x61, 0xBA),//9A
> > +       _INIT_DCS_CMD(0x62, 0xA0),
>
> More random //9A to remove above?
>
>
> > @@ -515,7 +1363,7 @@ static int boe_panel_unprepare(struct drm_panel
> *panel)
> >                 regulator_disable(boe->pp3300);
> >         } else {
> >                 gpiod_set_value(boe->enable_gpio, 0);
> > -               usleep_range(500, 1000);
> > +               usleep_range(1000, 2000);
> >                 regulator_disable(boe->avee);
> >                 regulator_disable(boe->avdd);
> >                 usleep_range(5000, 7000);
> > @@ -556,7 +1404,7 @@ static int boe_panel_prepare(struct drm_panel
> *panel)
> >         if (ret < 0)
> >                 goto poweroffavdd;
> >
> > -       usleep_range(5000, 10000);
> > +       usleep_range(10000, 15000);
>
> nit: how about using the range 10000, 11000? Last I looked at
> usleep_range() it almost always ended up at the longer of the two
> times, so that will shave 4 ms off and get us nearly to where we were
> without your change. The whole point of the range is to make the
> system more power efficient for frequent operations (wakeup
> combining), but that really doesn't matter for something as infrequent
> as turning on a LCD.
>
> Other than nits this looks fine to me and I'd be happy to add my
> Reviewed-by to a version with nits fixed. I'm not really an expert on
> MIPI panels but the convention of a big stream of binary commands
> seems to match what other panels in this driver do, even if their
> table of binary data isn't quite as long as yours (are all of yours
> actually needed?). I'm happy to land this in drm-misc-next with Sam or
> Thierry's Ack, too.
>
>
> -Doug
>

[-- Attachment #2: Type: text/html, Size: 5400 bytes --]