Hi,
On Wed, Apr 10, 2024 at 12:15 AM Cong Yang
<[email protected]> 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.
> Signed-off-by: Cong Yang <[email protected]>
> ---
> .../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. ;-)
> 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.
> + _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.
> +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.
-Doug
Hi,
Doug Anderson <[email protected]> 于2024年4月11日周四 15:48写道：
>
> Hi,
>
> On Wed, Apr 10, 2024 at 12:15 AM Cong Yang
> <[email protected]> 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 <[email protected]>
> > ---
> > .../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
On Thu, Apr 11, 2024 at 9:40 AM Doug Anderson <[email protected]> wrote:
> On Wed, Apr 10, 2024 at 12:15 AM Cong Yang
> <[email protected]> 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.
I think at a minimum we need to split out any identifiable display controllers
to their own drivers.
Then what developers see is that the code sequence is very similar
between two completely different display controllers so they have this
urge to shoehorn several displays into the same driver for this
reason.
The latter is not good code reuse, what we need to do here is to split
out a sequencing library, like if we had
drivers/gpu/drm/panel/cmd-seqence-lib.c|.h with a bool Kconfig and
some helpful symbols to do the same seqences in different drivers,
so the same order can be obtained in different display controller
drivers that would be great.
I'm thinking something along the line of
panel_seq_exit_sleep_mode(unsigned int delay_after_exit_sleep,
u8 *cmd_seq_after_exit_sleep,
unsigned int delay_after_cmd_seq,
unsigned int delay_after_set_display_on);
That will call mipi_dsi_dcs_exit_sleep_mode(), delay, send
command sequence, delay, call mipi_dsi_dcs_set_display_on()
and delay where any delay can be 0.
This achieves the same goal without messing up the whole place,
but requires some tinkering with how to pass a sequence the right
way etc.
Are Google & partners interested in the job? ;)
Yours,
Linus Walleij