[v3,0/4] Fix up the boe-tv101wum-nl6 panel driver

Message

Linus Walleij July 3, 2023, 1:21 p.m. UTC

This is two patches fixing things I would normally complain about
in reviews, but alas I missed this one, so I go in and fix it up
myself.

Discovering that a completely unrelated driver has been merged
into this panel driver I had to bite the bullet and break it out.
I am pretty suspicious of the other recently added panel as well.

I am surprised that contributors from manufacturers do not seem
to have datasheets for the display controllers embedded in the
panels of their products. Can you take a second look?

Signed-off-by: Linus Walleij <[e-mail]>
---
Changes in v3:
- Rebase on drm-misc-next
- Convert the two newly added Starry panels as well.
- Break out the obvious ILI9882t-based panel into its own driver.
- Link to v2: https://lore.kernel.org/r/[e-mail]

Changes in v2:
- Fix a missed static keyword
- Link to v1: https://lore.kernel.org/r/[e-mail]

---
Linus Walleij (4):
      drm/panel: boe-tv101wum-nl6: Drop macros and open code sequences
      drm/panel: boe-tv101wum-nl6: Drop surplus prepare tracking
      drm/panel: ili9882t: Break out as separate driver
      drm/panel: ili9882t: Break out function for switching page

 drivers/gpu/drm/panel/Kconfig                  |    9 +
 drivers/gpu/drm/panel/Makefile                 |    1 +
 drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 3037 ++++++++++--------------
 drivers/gpu/drm/panel/panel-ilitek-ili9882t.c  |  759 ++++++
 4 files changed, 2067 insertions(+), 1739 deletions(-)
---
base-commit: 14806c6415820b1c4bc317655c40784d050a2edb
change-id: 20230615-fix-boe-tv101wum-nl6-6aa3fab22b44

Best regards,

Comments

Cong Yang July 4, 2023, 10:03 a.m. UTC | #1

Hi,Linus Walleij

On Mon, Jul 3, 2023 at 9:21 PM Linus Walleij <[e-mail]> wrote:
>
> This is two patches fixing things I would normally complain about
> in reviews, but alas I missed this one, so I go in and fix it up
> myself.
>
> Discovering that a completely unrelated driver has been merged
> into this panel driver I had to bite the bullet and break it out.
> I am pretty suspicious of the other recently added panel as well.

Do you think the starry,himax83102-j02 panel needs to break it out? Thanks.

>
> I am surprised that contributors from manufacturers do not seem
> to have datasheets for the display controllers embedded in the
> panels of their products. Can you take a second look?

Sorry, this panel datasheet is not open source, I can't share this datasheet.
>
> Signed-off-by: Linus Walleij <[e-mail]>
> ---
> Changes in v3:
> - Rebase on drm-misc-next
> - Convert the two newly added Starry panels as well.
> - Break out the obvious ILI9882t-based panel into its own driver.
> - Link to v2: https://lore.kernel.org/r/[e-mail]
>
> Changes in v2:
> - Fix a missed static keyword
> - Link to v1: https://lore.kernel.org/r/[e-mail]
>
> ---
> Linus Walleij (4):
>       drm/panel: boe-tv101wum-nl6: Drop macros and open code sequences
>       drm/panel: boe-tv101wum-nl6: Drop surplus prepare tracking
>       drm/panel: ili9882t: Break out as separate driver
>       drm/panel: ili9882t: Break out function for switching page
>
>  drivers/gpu/drm/panel/Kconfig                  |    9 +
>  drivers/gpu/drm/panel/Makefile                 |    1 +
>  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 3037 ++++++++++--------------
>  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c  |  759 ++++++
>  4 files changed, 2067 insertions(+), 1739 deletions(-)
> ---
> base-commit: 14806c6415820b1c4bc317655c40784d050a2edb
> change-id: 20230615-fix-boe-tv101wum-nl6-6aa3fab22b44
>
> Best regards,
> --
> Linus Walleij <[e-mail]>
>

Linus Walleij July 4, 2023, 10:16 a.m. UTC | #2

On Tue, Jul 4, 2023 at 12:04 PM cong yang
<[e-mail]> wrote:
> On Mon, Jul 3, 2023 at 9:21 PM Linus Walleij <[e-mail]> wrote:

> > I am surprised that contributors from manufacturers do not seem
> > to have datasheets for the display controllers embedded in the
> > panels of their products. Can you take a second look?
>
> Sorry, this panel datasheet is not open source, I can't share this datasheet.

Perhaps not, but you can use the knowledge in the datasheet to
name the commands and give better structure to the members of
the driver, if you know what commands mean then provide
#define statements to them so sequences become understandable.
See for example patch 4/4.

Yours,
Linus Walleij

Cong Yang July 4, 2023, 11:14 a.m. UTC | #3

Hi,

On Tue, Jul 4, 2023 at 6:16 PM Linus Walleij <[e-mail]> wrote:
>
> On Tue, Jul 4, 2023 at 12:04 PM cong yang
> <[e-mail]> wrote:
> > On Mon, Jul 3, 2023 at 9:21 PM Linus Walleij <[e-mail]> wrote:
>
> > > I am surprised that contributors from manufacturers do not seem
> > > to have datasheets for the display controllers embedded in the
> > > panels of their products. Can you take a second look?
> >
> > Sorry, this panel datasheet is not open source, I can't share this datasheet.
>
> Perhaps not, but you can use the knowledge in the datasheet to
> name the commands and give better structure to the members of
> the driver, if you know what commands mean then provide
> #define statements to them so sequences become understandable.
> See for example patch 4/4.

Patch 4/4 LGTM, from the datasheet  0XFF is EXTC Command Set Enable .
Description: Set the register, 1 Parameter = 98h, 2 Parameter = 82h, 3
Parameter = Page value to enable “page command set” available.
00h = Page 0 ,01h = Page 1... 0Eh = Page 14.

Thank you for you patch.
>
> Yours,
> Linus Walleij

Doug Anderson Sept. 18, 2023, 4:19 p.m. UTC | #4

Hi,

On Mon, Jul 3, 2023 at 6:21 AM Linus Walleij <[e-mail]> wrote:
>
> This is two patches fixing things I would normally complain about
> in reviews, but alas I missed this one, so I go in and fix it up
> myself.
>
> Discovering that a completely unrelated driver has been merged
> into this panel driver I had to bite the bullet and break it out.
> I am pretty suspicious of the other recently added panel as well.
>
> I am surprised that contributors from manufacturers do not seem
> to have datasheets for the display controllers embedded in the
> panels of their products. Can you take a second look?
>
> Signed-off-by: Linus Walleij <[e-mail]>
> ---
> Changes in v3:
> - Rebase on drm-misc-next
> - Convert the two newly added Starry panels as well.
> - Break out the obvious ILI9882t-based panel into its own driver.
> - Link to v2: https://lore.kernel.org/r/[e-mail]
>
> Changes in v2:
> - Fix a missed static keyword
> - Link to v1: https://lore.kernel.org/r/[e-mail]
>
> ---
> Linus Walleij (4):
>       drm/panel: boe-tv101wum-nl6: Drop macros and open code sequences
>       drm/panel: boe-tv101wum-nl6: Drop surplus prepare tracking
>       drm/panel: ili9882t: Break out as separate driver
>       drm/panel: ili9882t: Break out function for switching page
>
>  drivers/gpu/drm/panel/Kconfig                  |    9 +
>  drivers/gpu/drm/panel/Makefile                 |    1 +
>  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 3037 ++++++++++--------------
>  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c  |  759 ++++++
>  4 files changed, 2067 insertions(+), 1739 deletions(-)

I'm curious what the latest on this patch series is. Is it abandoned,
or is it still on your list to move forward with it? If it's
abandoned, does that mean we've abandoned the idea of breaking
ili9882t into a separate driver?

From looking at things that have landed downstream in the ChromeOS
kernel trees it looks as if additional fixes are getting blocked from
being posted/landed because of the limbo state that this is in.

-Doug

Doug Anderson Sept. 26, 2023, 9:49 p.m. UTC | #5

Hi,

On Mon, Sep 18, 2023 at 9:19 AM Doug Anderson <[e-mail]> wrote:
>
> Hi,
>
> On Mon, Jul 3, 2023 at 6:21 AM Linus Walleij <[e-mail]> wrote:
> >
> > This is two patches fixing things I would normally complain about
> > in reviews, but alas I missed this one, so I go in and fix it up
> > myself.
> >
> > Discovering that a completely unrelated driver has been merged
> > into this panel driver I had to bite the bullet and break it out.
> > I am pretty suspicious of the other recently added panel as well.
> >
> > I am surprised that contributors from manufacturers do not seem
> > to have datasheets for the display controllers embedded in the
> > panels of their products. Can you take a second look?
> >
> > Signed-off-by: Linus Walleij <[e-mail]>
> > ---
> > Changes in v3:
> > - Rebase on drm-misc-next
> > - Convert the two newly added Starry panels as well.
> > - Break out the obvious ILI9882t-based panel into its own driver.
> > - Link to v2: https://lore.kernel.org/r/[e-mail]
> >
> > Changes in v2:
> > - Fix a missed static keyword
> > - Link to v1: https://lore.kernel.org/r/[e-mail]
> >
> > ---
> > Linus Walleij (4):
> >       drm/panel: boe-tv101wum-nl6: Drop macros and open code sequences
> >       drm/panel: boe-tv101wum-nl6: Drop surplus prepare tracking
> >       drm/panel: ili9882t: Break out as separate driver
> >       drm/panel: ili9882t: Break out function for switching page
> >
> >  drivers/gpu/drm/panel/Kconfig                  |    9 +
> >  drivers/gpu/drm/panel/Makefile                 |    1 +
> >  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 3037 ++++++++++--------------
> >  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c  |  759 ++++++
> >  4 files changed, 2067 insertions(+), 1739 deletions(-)
>
> I'm curious what the latest on this patch series is. Is it abandoned,
> or is it still on your list to move forward with it? If it's
> abandoned, does that mean we've abandoned the idea of breaking
> ili9882t into a separate driver?
>
> From looking at things that have landed downstream in the ChromeOS
> kernel trees it looks as if additional fixes are getting blocked from
> being posted/landed because of the limbo state that this is in.

I presume Linus is busy or otherwise indisposed.

So I guess we have two options here:

a) Cong Yang can post any relevant fixes to the existing "monolithic"
panel driver so that we can get them landed and at least get things
fixed.

- or -

b) Cong Yang could take over all or some of Linus's series and post
new versions of it, addressing feedback.

I would tend to say we should go with "a)" because I think Linus needs
to be involved in some of the cleanup discussions.

-Doug

Linus Walleij Sept. 28, 2023, 9:42 p.m. UTC | #6

On Tue, Sep 26, 2023 at 11:49 PM Doug Anderson <[e-mail]> wrote:

> > I'm curious what the latest on this patch series is. Is it abandoned,
> > or is it still on your list to move forward with it? If it's
> > abandoned, does that mean we've abandoned the idea of breaking
> > ili9882t into a separate driver?
> >
> > From looking at things that have landed downstream in the ChromeOS
> > kernel trees it looks as if additional fixes are getting blocked from
> > being posted/landed because of the limbo state that this is in.
>
> I presume Linus is busy or otherwise indisposed.

Sorry I was looking for the branch with my patches and I have it
somewhere not ordinary :/

Originally I shelved it because I got requests to do additional
patches to the driver:
https://lore.kernel.org/dri-devel/CAD=FV=[e-mail]/

To do measurements about binary code size in object files, and if it does,
then I need to invent new sequence macros (IIUC):
https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=[e-mail]/

So I just didn't have time for that extensive rework of the driver.

It's good feedback, but I just wanted to make the situation a little
bit better, and perfect is the enemy of good (TM).

> So I guess we have two options here:
>
> a) Cong Yang can post any relevant fixes to the existing "monolithic"
> panel driver so that we can get them landed and at least get things
> fixed.
>
> - or -
>
> b) Cong Yang could take over all or some of Linus's series and post
> new versions of it, addressing feedback.

Either works for me, I would prefer b), Cong is welcome to adopt
the patches if he/his employer want to. Go ahead!

We can't really let this one-size-fits-all driver go on like this.

My main concern with the "boe-tv101wum-nl6" driver is that it can
be renamed "cromeos-hackfest" at this point because it becomes
hard for any other system to reuse the panel drivers, the typical
example would be a system using say ili9882t but with
a different init sequence or something, why would they want
support for 9 unrelated panels compiled in? The condition that
these drivers should be related to the original panel that gave
name to the file has seemingly been dropped long ago.

It looks like the drivers only share the power lines (avdd, avee, pp3300,
pp1800) then this can be broken out to a helper library. But I am
sceptical about that too. I doubt that the vastly different panels
actually have exactly these these supply line names, I think it is
actually names of the rails on the chrome machine board. And that is
not how these regulators should be named, they should be named after
the input name on the component. This is really hard to catch in reviews when
we don't have datasheets so I'm not blaming anyone, but is this something
that even needs fixing in the device tree bindings? (By deprecation
and addition...) can we look into this?

I would say can we at least agree that before we merge one more
driver into this file, break out to subdrivers those that clearly have
an identifiable display controller and is thus reusable? From my
point of view I can just see the ili9882t so that's a good start.

Yours,
Linus Walleij

Doug Anderson Sept. 29, 2023, 2:17 p.m. UTC | #7

Hi,

On Thu, Sep 28, 2023 at 2:42 PM Linus Walleij <[e-mail]> wrote:
>
> On Tue, Sep 26, 2023 at 11:49 PM Doug Anderson <[e-mail]> wrote:
>
> > > I'm curious what the latest on this patch series is. Is it abandoned,
> > > or is it still on your list to move forward with it? If it's
> > > abandoned, does that mean we've abandoned the idea of breaking
> > > ili9882t into a separate driver?
> > >
> > > From looking at things that have landed downstream in the ChromeOS
> > > kernel trees it looks as if additional fixes are getting blocked from
> > > being posted/landed because of the limbo state that this is in.
> >
> > I presume Linus is busy or otherwise indisposed.
>
> Sorry I was looking for the branch with my patches and I have it
> somewhere not ordinary :/
>
> Originally I shelved it because I got requests to do additional
> patches to the driver:
> https://lore.kernel.org/dri-devel/CAD=FV=[e-mail]/
>
> To do measurements about binary code size in object files, and if it does,
> then I need to invent new sequence macros (IIUC):
> https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=[e-mail]/
>
> So I just didn't have time for that extensive rework of the driver.
>
> It's good feedback, but I just wanted to make the situation a little
> bit better, and perfect is the enemy of good (TM).
>
> > So I guess we have two options here:
> >
> > a) Cong Yang can post any relevant fixes to the existing "monolithic"
> > panel driver so that we can get them landed and at least get things
> > fixed.
> >
> > - or -
> >
> > b) Cong Yang could take over all or some of Linus's series and post
> > new versions of it, addressing feedback.
>
> Either works for me, I would prefer b), Cong is welcome to adopt
> the patches if he/his employer want to. Go ahead!
>
> We can't really let this one-size-fits-all driver go on like this.
>
> My main concern with the "boe-tv101wum-nl6" driver is that it can
> be renamed "cromeos-hackfest" at this point because it becomes
> hard for any other system to reuse the panel drivers, the typical
> example would be a system using say ili9882t but with
> a different init sequence or something, why would they want
> support for 9 unrelated panels compiled in? The condition that
> these drivers should be related to the original panel that gave
> name to the file has seemingly been dropped long ago.
>
> It looks like the drivers only share the power lines (avdd, avee, pp3300,
> pp1800) then this can be broken out to a helper library. But I am
> sceptical about that too. I doubt that the vastly different panels
> actually have exactly these these supply line names, I think it is
> actually names of the rails on the chrome machine board. And that is
> not how these regulators should be named, they should be named after
> the input name on the component. This is really hard to catch in reviews when
> we don't have datasheets so I'm not blaming anyone, but is this something
> that even needs fixing in the device tree bindings? (By deprecation
> and addition...) can we look into this?
>
> I would say can we at least agree that before we merge one more
> driver into this file, break out to subdrivers those that clearly have
> an identifiable display controller and is thus reusable? From my
> point of view I can just see the ili9882t so that's a good start.

This sounds like a reasonable plan to me. What if Cong posted patches
that broke this up into a separate driver for the distinct controller
but otherwise didn't substantially reorganize it? In other words both
the old driver and the new one would keep the "struct panel_init_cmd"
until we get some resolution about the binary size issue. That would
at least let us move forward...

-Doug