On Mon, Mar 13, 2017 at 04:12:37PM +0100, Noralf Trønnes wrote:
Den 12.03.2017 21.40, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 09:17:00PM +0100, Noralf Trønnes wrote:
Den 12.03.2017 20.16, skrev Daniel Vetter:
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).
I have looked at the emails, and I used drm_panel in the first RFC, but I got the impression that Thierry didn't like it so it was dropped in RFC v2.
Hm, I thought I checked all the old versions of your example tinydrm submissions and didn't find any with drm_panel. Do you have a link to archives? I'd like to read Thierry's aguments, in case I'm oblivious to a bad corner case :-)
I used drm_panel in the first tinydrm RFC in March 2016: https://lists.freedesktop.org/archives/dri-devel/2016-March/102903.html
struct tinydrm_device { struct drm_device *base; struct drm_panel panel; ... };
Then you commented: https://lists.freedesktop.org/archives/dri-devel/2016-March/102921.html
In the case of tinydrm I think that means we should have a bunch of new drm helpers, or extensions for existing ones:
<snip> > - A helper to create a simple drm_connector from a drm_panel (the > get_modes hooks you have here), maybe also in drm_simple_kms_helper.c.
So I made: [PATCH 4/4] drm/panel: Add helper for simple panel connector https://lists.freedesktop.org/archives/dri-devel/2016-May/106890.html
Thierry replied: https://lists.freedesktop.org/archives/dri-devel/2016-May/107023.html
Ugh, that's bad, review has come full circle :(
Given that I'd say let's not bother more with panel frameworks for now, but just add the simple panel drivers as-is. Once we have someone who wants to reuse a panel (probably a mipi-dbi one), but where the dbi bus isn't exposed to software but behind some fancy hw encoder, then we can ponder whether/how (and probably just for the affected drivers) we should re-introduce drm_panel into tinydrm.
I'll try and collect everything we've discussed here into a tinydrm todo.rst entry. -Daniel