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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
