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

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
> 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?

Self-correct, there never was a version with drm_panel. tbh I think that's
perfectly fine, tinydrm is aimed at simple panels behind spi/i2c buses
(where also the entire video data is uploaded through spi/i2c, not just
control information). Not changing anything like I recommend seems like
the right action still (well, shuffling the regulator into
simple_pipe->enable/disable like I think it should be).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel