mbox series

[v3,0/4] drm/panel: boe-tv101wum-nl6: Support enabling a 3.3V rail

Message ID 20210827082407.101053-1-yangcong5@huaqin.corp-partner.google.com
Headers show
Series drm/panel: boe-tv101wum-nl6: Support enabling a 3.3V rail | expand

Message

Cong Yang Aug. 27, 2021, 8:24 a.m. UTC
Compared to v2, support for BOE tv1110c9m-ll3 and Inx hj110iz-01a 
video mode panel.

yangcong (4):
  drm/panel: boe-tv101wum-nl6: Support enabling a 3.3V rail
  dt-bindings: drm/panel: boe-tv101wum-nl6: Support enabling a 3.3V rail
  drm/panel: support for BOE and INX video mode panel
  dt-bindngs: display: panel: Add BOE and INX panel bindings

 .../display/panel/boe,tv101wum-nl6.yaml       |   7 +
 .../gpu/drm/panel/panel-boe-tv101wum-nl6.c    | 926 +++++++++++++++++-
 2 files changed, 930 insertions(+), 3 deletions(-)

Comments

Doug Anderson Aug. 27, 2021, 1:40 p.m. UTC | #1
Hi,

On Fri, Aug 27, 2021 at 1:24 AM yangcong
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> The auo,b101uan08.3 panel (already supported by this driver) has
> a 3.3V rail that needs to be turned on. For previous users of
> this panel this voltage was directly output by pmic. On a new
> user (the not-yet-upstream sc7180-trogdor-mrbland board) we need
> to turn the 3.3V rail on. Add support in the driver for this.
>
> Signed-off-by: yangcong <yangcong5@huaqin.corp-partner.google.com>
> ---
>  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

There were no differences between this and the previous version [1]. I
added my Reviewed-by tag on the previous version, so you should have
included it in this new version.

[1] https://lore.kernel.org/r/20210820070113.45191-2-yangcong5@huaqin.corp-partner.google.com

In any case:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Doug Anderson Aug. 27, 2021, 2:42 p.m. UTC | #2
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