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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
