[PATCH v3] video: sunxi_display: Convert to DM_VIDEO

[Simon Glass <sjg@chromium.org>]

Hi Andre,

On Sun, 7 Feb 2021 at 18:37, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sun, 7 Feb 2021 07:37:34 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
> Hi Simon,
>
> > On Thu, 4 Feb 2021 at 18:08, Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > From: Jagan Teki <jagan@amarulasolutions.com>
> > >
> > > DM_VIDEO migration deadline is already expired, but around
> > > 80 Allwinner boards are still using video in a legacy way.
> > >
> > > ===================== WARNING ======================
> > > This board does not use CONFIG_DM_VIDEO Please update
> > > the board to use CONFIG_DM_VIDEO before the v2019.07 release.
> > > Failure to update by the deadline may result in board removal.
> > > See doc/driver-model/migration.rst for more info.
> > > ====================================================
> > >
> > > Convert the legacy video driver over to the DM_VIDEO framework. This is
> > > a minimal conversion: it doesn't use the DT for finding its resources,
> > > nor does it use DM clocks or DM devices for the outputs (LCD, HDMI, CVBS).
> > >
> > > Tested in Bananapi M1+ Plus 1920x1200 HDMI out. (Jagan)
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > [Andre: rebase and smaller fixes]
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > > Hi,
> > >
> > > I picked this one up to get rid of the warnings. I dropped the BMP
> > > support for now (v2 1/3 and v2 2/3), I need to have a closer look, as
> > > I am not convinced this was the right solution.
> > >
> > > Cheers,
> > > Andre
> > >
> > > Changelog v2 .. v3:
> > > - rebase against master, fixing up renamed variables and structs
> > > - replace enum with #define
> > > - remove BMP from Kconfig
> > > - fix memory node size calculation in simplefb setup
> > >
> > >  arch/arm/mach-sunxi/Kconfig         |   9 +-
> > >  drivers/video/sunxi/sunxi_display.c | 262 ++++++++++++++++------------
> > >  include/configs/sunxi-common.h      |  17 --
> > >  scripts/config_whitelist.txt        |   1 -
> > >  4 files changed, 157 insertions(+), 132 deletions(-)
> > >
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Some thoughts below
>
> Many thanks for having a thorough look, much appreciated.
>
> I will have a closer look at your comments which I didn't reply to
> below.
>
> For the other points:
> To be honest, I haven't checked every line in this patch, my goal was
> merely to get it merged (this time), to prevent feature removal and
> drop the nasty buildman warnings.
> I see quite some cleanup potential here (some I already mentioned in
> the commit message above), but wanted to get the main DM_VIDEO
> conversion done first (as I think last time some discussion already
> prevented a merge).
> So my plan was to queue this for next ASAP, then send more cleanup patches
> on top, before the next merge window. I understand that it's typically
> done the other way around, but given this is dragging on for a while,
> is long overdue and works for me (TM) as it is, I would prefer a
> functional patch first, with cleanups on top.

That's fine...you have my review tag. My comments are just suggestions
for improvement and much of it relates to the existing code.

>
> >
> > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > > index 0135575ca1e..a29d11505aa 100644
> > > --- a/arch/arm/mach-sunxi/Kconfig
> > > +++ b/arch/arm/mach-sunxi/Kconfig
> > > @@ -816,13 +816,14 @@ config VIDEO_SUNXI
> > >         depends on !MACH_SUN9I
> > >         depends on !MACH_SUN50I
> > >         depends on !SUN50I_GEN_H6
> > > -       select VIDEO
> > > +       select DM_VIDEO
> >
> > I wonder whether instead VIDEO_SUNXI should depend on DM_VIDEO ?
>
> So I was always wondering about the logic behind that.
> My understanding would be: This driver is (now) implementing the
> DM_VIDEO interface. Any user (at board or SoC level) would just be
> selecting this driver, without caring about which driver interface it
> uses.
> So as this driver is (now) DM_VIDEO only, it would signal this via this
> select line.
>
> If that is not how it's meant to be, can you explain the the idea behind
> this, please?

That sounds fine to me, but what happens when someone selects a DM and
non-DM driver? That has always been the intent - to ensure that people
are selecting DM for a subsystem on purpose.

Of course these days most stuff is DM, so perhaps that approach is
obsolete and we should do what you say. I'm OK with that.

>
> Also: with CONFIG_VIDEO now dying, CONFIG_DM_VIDEO somewhat loses its
> purpose, doesn't it?

All of the CONFIG_DM... things should be removed at some point, yet.
Just waiting for the mythical deadlines...

[...]

Regards,
Simon