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? > > > 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. A large chunk of the tinydrm functions should probably be moved into relevant existin drm helpers, e.g. - tinydrm_lastclose could be drm_fb_helper_lastclose. Only thing we need for that is to store the drm_fb_helper pointer somewhere in drm_device->mode_config. And thenc we could roll that out to all the drivers. - tinydrm_gem_cma_prime_import_sg_table should probably go into the cma helpers, as a _vmapped variant (since not every driver needs the vmap). And tinydrm_gem_cma_free_object could the be merged into drm_gem_cma_free_object(). - tinydrm_fb_create we could move into drm_simple_pipe, only need to add the fb_create hook there, which would again simplify a bunch of things (since it gives you a one-stop vfunc for simple drivers). - Quick aside: The unregister devm stuff is kinda getting the lifetimes of a drm_device wrong. Doesn't matter, since everyone else gets it wrong too :-) - With the fbdev pointer in dev->mode_config we could also make suspend/resume helpers entirely generic, at least if we add a dev->mode_config.suspend_state. Just a bunch of ideas. If you don't feel like doing those, ok if I add them to todo.rst as tinydrm refactorings? -Daniel > > 2. Add tons more panel drivers. Personally I'd do that first :-) > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- 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
