Hi,
Doug Anderson <dianders@xxxxxxxxxxxx> 于2024年4月11日周四 15:48写道：
>
> Hi,
>
> On Wed, Apr 10, 2024 at 12:15 AM Cong Yang
> <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > The BOE nv110wum-l60 is a 11.0" WUXGA TFT LCD panel, which fits in nicely
> > with the existing panel-boe-tv101wum-nl6 driver. Hence, we add a new
> > compatible with panel specific config.
>
> I guess we have the same question we've had with this driver in the
> past: do we add more tables here, or do we break this out into a
> separate driver like we ended up doing with "ili9882t". I guess the
> question is: what is the display controller used with this panel and
> is it the same (or nearly the same) display controller as other panels
> in this driver or is it a completely different display controller.
> Maybe you could provide this information in the commit message to help
> reviewers understand.
okay, I will add detailed information in V2 patch.Thanks.
>
>
> > Signed-off-by: Cong Yang <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > .../gpu/drm/panel/panel-boe-tv101wum-nl6.c | 115 ++++++++++++++++++
> > 1 file changed, 115 insertions(+)
>
> Maybe add Linus W to your patches since he has had opinions on this
> driver in the past. I've added him as CC here but you should make sure
> to CC him on future versions unless he says not to. ;-)
Got it,thanks.
>
>
> > diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> > index 0ffe8f8c01de..f91827e1548c 100644
> > --- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> > +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> > @@ -1368,6 +1368,91 @@ static const struct panel_init_cmd starry_himax83102_j02_init_cmd[] = {
> > {},
> > };
> >
> > +static const struct panel_init_cmd boe_nv110wum_init_cmd[] = {
> > + _INIT_DELAY_CMD(60),
> > + _INIT_DCS_CMD(0xB9, 0x83, 0x10, 0x21, 0x55, 0x00),
>
> Given that the first command of "(0xB9, 0x83, 0x10, 0x21, 0x55, 0x00)"
> seems to be the same as "starry_himax83102_j02" maybe those two are
> the same controller? I'm just guessing, but if those are the same
> controller as the two new ones you're adding in this series, maybe all
> 3 of them should be in their own driver? Maybe we can do something to
> make more sense of some of these commands too? There certainly seem to
> be a lot of commonalities in the init sequences of all 3 and if we can
> define the init sequence more logically then we can share more of the
> code between the different panels and we don't have a giant duplicated
> blob.
Yes, your guess is correct. boe_nv110wum and ivo_t109nw41 and
starry_himax83102_j02
are the same controller (himax83102). They are equipped with different
glass panels (BOE/IVO/starry),
so there will be some differences in initial code and porch.
>
>
> > + _INIT_DCS_CMD(0xB9, 0x00, 0x00, 0x00),
> > + _INIT_DELAY_CMD(50),
> > + _INIT_DCS_CMD(0x11),
> > + _INIT_DELAY_CMD(110),
> > + _INIT_DCS_CMD(0x29),
> > + _INIT_DELAY_CMD(25),
> > + {},
> > +};
> > static inline struct boe_panel *to_boe_panel(struct drm_panel *panel)
>
> nit: should have a blank line between the end of your struct and the
> next function.
Got it,thanks.
>
>
> > +static const struct panel_desc boe_nv110wum_desc = {
> > + .modes = &boe_tv110wum_default_mode,
> > + .bpc = 8,
> > + .size = {
> > + .width_mm = 147,
> > + .height_mm = 235,
> > + },
> > + .lanes = 4,
> > + .format = MIPI_DSI_FMT_RGB888,
> > + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > + MIPI_DSI_MODE_LPM,
> > + .init_cmds = boe_nv110wum_init_cmd,
> > + .lp11_before_reset = true,
> > +};
> > static int boe_panel_get_modes(struct drm_panel *panel,
> > struct drm_connector *connector)
>
> nit: should have a blank line between the end of your struct and the
> next function.
>
>
> > @@ -1973,6 +2085,9 @@ static const struct of_device_id boe_of_match[] = {
> > { .compatible = "starry,himax83102-j02",
> > .data = &starry_himax83102_j02_desc
> > },
> > + { .compatible = "boe,nv110wum-l60",
> > + .data = &boe_nv110wum_desc
> > + },
>
> nit: the existing panels that are supported are sorted alphabetically.
> Please sort things alphabetically throughout your patch series.
Got it, fx net patch. Thanks.
>
> -Doug
