[U-Boot] [PATCH 3/5] arm: tegra30: video: integrate display driver for t30

[Thierry Reding]

On Fri, Aug 21, 2015 at 06:37:37PM -0600, Simon Glass wrote:
[...]
> I have serious doubts about the wisdom of requiring a contributor to
> completely re-architect the existing display system in U-Boot. It's a
> big job. Perhaps we can settle for following along the same lines and
> not making things worse?

I didn't suggest re-architecting the display system in U-Boot. What I
was suggesting was a way to architect Tegra-specific display driver code
to make it reusable rather than duplicate display controller programming
for each new generation, while the hardware has remained mostly the
same.

> > Perhaps something as simple as:
> >
> >         struct tegra_dc {
> >                 ...
> >                 int (*enable)(struct tegra_dc *dc, const struct display_mode *mode);
> >                 void (*disable)(struct tegra_dc *dc);
> >                 ...
> >         };
> >
> >         struct tegra_output {
> >                 ...
> >                 struct tegra_dc *dc;
> >                 ...
> >                 int (*enable)(struct tegra_output *output, const struct display_mode *mode);
> >                 void (*disable)(struct tegra_output *output);
> >                 ...
> >         };
> >
> > would work fine. That's roughly how drivers are implemented in the
> > kernel. Setting up display on an output would be done by determining the
> > mode (typically by parsing EDID if available, or using a hard-coded mode
> > otherwise) and then calling:
> >
> >         output->dc = dc;
> >         dc->enable(dc, mode);
> >         output->enable(output, mode);
> >
> > You might want to add in an abstraction for panels as well to make sure
> > you have enough flexibility to enable and disable those, too. In that
> > case you'd probably want to complement the above sequence with:
> >
> >         panel->enable(panel);
> 
> Please don't add function points to structures on an ad-hoc basis.
> These should use driver model. There is a uclass for display port but
> not for LCD panels or SOR. You could add a very simple one for a panel
> if you like. Please take a look at tegra124's display driver for an
> example.

I don't think the driver model is a good fit here. Abstracting a display
port isn't very useful in itself because users don't really care about
the type of display, they only care about it being a display. So if you
want to usefully abstract you'd do it at a higher level, such as display
or screen. Then you have a generic object which users can use to put up
a framebuffer onto a physical screen.

SOR is an even worse abstraction because it's completely Tegra-specific
and other SoCs will have completely different ways of providing the same
types of output. You'll end up with a uclass containing a single
implementation.

So, to reiterate, the above wasn't meant to be a generic abstraction for
a U-Boot-wide display framework, but rather a suggestion on how the
Tegra driver could internally be structured in order to avoid code
duplication.

> > Which should work for everything, except maybe DSI, where you may need
> > some sort of inbetween step for panels that need additional setup using
> > DCS commands or the like. But I suspect that's a bridge that can be
> > crossed when we get to it.
> >
> > That said, I don't forsee myself having any time to devote to this, but
> > if anyone ends up spending work on this, feel free to Cc me on patches
> > or ask if you have questions about the display hardware or the framework
> > design. I'm sure I can find the time to provide feedback.
> 
> In which case I suggest we limit the amount of rewrite we ask for in
> this case...

People asked for my opinion, so I shared. If you prefer code duplication
over a properly architected driver that's of course your prerogative.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150824/550a0a6f/attachment.sig>