[PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Daniel Vetter]

Hi Noralf,

On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
> Add support for displays that have a register interface and can be
> operated using a simple vtable.
> 
> I have looked through the staging/fbtft drivers and it seems that,
> except the MIPI controllers, most if not all controllers are operated
> through a register. And since most controllers have more than one bus
> interface option, regmap seems like a good choice to describe the
> interface (tested[1,2]).
> MIPI DCS can't be represented using regmap since some commands doesn't
> have a parameter. That would be like a register without a value, which
> doesn't make sense.
> 
> In my second RFC of tinydrm I used drm_panel to decribe the panels
> since it was a good match to the fbtft displays. I was then told that
> drm_panel wasn't supposed to used like that, so I dropped it and have
> tried to use the drm_simple_display_pipe_funcs vtable directly. This
> hasn't been all successful, since I ended up using devm_add_action() to
> power down the controller at the right time. Thierry Reding wasn't
> happy with this and suggested "to add an explicit callback somewhere".
> My solution has been to copy the drm_panel_funcs vtable.
> Since I now have a vtable, I also added a callback to flush the
> framebuffer. So presumably all the fbtft drivers can now be operated
> through the tinydrm_panel_funcs vtable.

Ehrm, what? I admit I didn't follow the discussion in-depth, but if
drm_panel can't be used to describe a panel, it's not fit for the job and
needs to be extended. Adding even more abstraction, matroschka-style,
isn't a solution.

Personally I think tinydrm itself is already a bit much wrapping, but I
see that for really simple drivers it has it's uses. But more layers feels
like going in the wrong direction.

For the callback you're looking for (i.e. the regulator_disable call) I
think the correct place is to enable/disable the regulator in the
enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in
their equivalent in drm_panel (well, probably pre_enable and post_disable,
since I guess you need that regulator to driver anything). Same for _init,
if the display is completely off there's no point in keeping the hw
running. Enabling/disabling the entire hw is pretty much what ->enable and
->disable are for. This also gives you proper runtime pm for almost for
free ...

Also, since the regulator is actually stored in struct mipi_dbi, it's
probably best to handle it in the mipi_dbi helpers too. You do that
already with the backlight anyway.

I noticed that the version of tinydrm that landed doesn't use drm_panel
anymore, I thought that was the case once, and for the version I acked?

> After having done this the question arises:
> Why not extend tinydrm_device instead of subclassing it?
> 
> The benefit of subclassing is that it keeps the door open for drivers
> that can use tinydrm_device, but not tinydrm_panel. But I don't know of
> such a driver now, then again who knows what the future brings.
> Something that might or might not happen isn't a good reason, so it
> seems that I want it this way because I just like it. And it's easy to
> merge the two should it be that no one uses tinydrm_device directly
> three years down the line. But I'm actually not sure what's best.

As a rule of thumb, never design code for future use that you don't know
yet will happen. No one can reliably predict the future, which means
you'll get it wrong, and in the future we'll have to do 2x the work: Once
do unto the code resulting from the wrong prediction, then redoing it the
way we need to. Not including the on-going burden of maintaining unused
functionally.

So let's pls merge first, split later when there's a clean need.

> To recap:
> 
> tinydrm_device
> - Combines drm_simple_display_pipe with CMA backed framebuffer and fbdev.
> - Optional pipe setup with a connector with one mode, but the driver
>   can do it's own.
> 
> tinydrm_panel
> - All drm operations are distilled down to tinydrm_panel_funcs.
> - Some common driver properties

So overal sorry I'm shredding this a bit, but I don't see the point. Imo
much more useful would be:

1. Extract the non-tinydrm specific helpers from tinydrm and put them into
the right places. I think from our discussions this was:

- backlight helpers, probably best to put them into a new drm_backlight.c.
  This is because drivers/video is defactor unmaintained. We could also
  move drivers/video/backlight to drivers/gpu/backlight and take it all
  over within drm-misc, but that's more work.

- spi helpers, probably best put into spi core/helper code. Thierry said
  the spi maintainer is fast&reactive, so shouldn't be a big issue.

- extract the mipi-dbi helper (well, the non-tinydrm specific parts at
  least) into a separate helper, like we have for mipi-dsi already.

2. Add tons more panel drivers. Personally I'd do that first :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch