On Tue, Feb 07, 2017 at 07:58:52AM +0100, Daniel Vetter wrote: > On Mon, Feb 06, 2017 at 08:23:36PM +0100, Noralf Trønnes wrote: > > > > Den 06.02.2017 10.17, skrev Thierry Reding: > > > On Tue, Jan 31, 2017 at 05:03:13PM +0100, Noralf Trønnes wrote: > > > > tinydrm provides helpers for very simple displays that can use > > > > CMA backed framebuffers and need flushing on changes. > > > > > > > > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx> > > > > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > > > --- > > > > > > > > Changes since version 2: > > > > - Remove fbdev after drm unregister, not before. > > > > > > > > Changes since version 1: > > > > - Add tinydrm.rst > > > > - Set tdev->fbdev_cma=NULL on unregister (lastclose is called after that). > > > > - Remove some DRM_DEBUG*() > > > > > > > > Documentation/gpu/index.rst | 1 + > > > > Documentation/gpu/tinydrm.rst | 21 ++ > > > > drivers/gpu/drm/Kconfig | 2 + > > > > drivers/gpu/drm/Makefile | 1 + > > > > drivers/gpu/drm/tinydrm/Kconfig | 8 + > > > > drivers/gpu/drm/tinydrm/Makefile | 1 + > > > > drivers/gpu/drm/tinydrm/core/Makefile | 3 + > > > > drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 377 ++++++++++++++++++++++++++++ > > > > drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 234 +++++++++++++++++ > > > > include/drm/tinydrm/tinydrm.h | 115 +++++++++ > > > > 10 files changed, 763 insertions(+) > > > > create mode 100644 Documentation/gpu/tinydrm.rst > > > > create mode 100644 drivers/gpu/drm/tinydrm/Kconfig > > > > create mode 100644 drivers/gpu/drm/tinydrm/Makefile > > > > create mode 100644 drivers/gpu/drm/tinydrm/core/Makefile > > > > create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-core.c > > > > create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > > > > create mode 100644 include/drm/tinydrm/tinydrm.h > > > I realize this is totally subjective, but this feels somewhat too much > > > of a separation. Given the helper nature of TinyDRM, I think it would be > > > more appropriate to move the helpers themselves into drm_tiny.[ch] and > > > then maybe add a subdirectory drivers/gpu/drm/tiny that contains all the > > > drivers that use the helpers. > > > > > > The separation above further shows in subsequent patches where helpers > > > are added to tinydrm that aren't specific to TinyDRM. So this make the > > > new helpers appear as more of a subsystem in DRM rather than a helper > > > library. It also makes things somewhat inconsistent with existing > > > infrastructure. > > > > What I have done with tinydrm is to do as little as possible in the > > core helper. The minimum required to pull together > > drm_simple_display_pipe, cma helper + fbdev and framebuffer flushing. > > > > Then I have added a set of functions that ease the writing of drivers > > for RGB565 displays with optional backlight and regulator. > > > > Added to that is a controller library that handles register access (will > > use regmap for non-mipi) and framebuffer flushing (set the windowing > > registers and write the buffer to the pixel register). > > > > And at last there's the display driver that initializes the controller > > to match a particular panel. > > > > Maybe I should narrow tinydrm to _just_ support "RGB565 displays with > > optional backlight and regulator". It looks like I split it up to much, > > unless someone sees a need for the core of tinydrm elsewhere. > > > > I think it's hard to avoid the subsystem smell here. These displays are > > really simple, fbdev is more than enough to cover their needs. > > But fbdev is closed. > > > > I'm glad you pick on this, as getting the architecture right will save > > me maintenance down the line. I did paint me into a corner with > > staging/fbtft and I'm not keen on repeating that (to my defence it was > > my first C code since university and I had 2 displays that had some > > similarities which ended up as fbtft). > > tbh I'm not sure either whether the tinydrm midlayer is good or not. But > then it is what it says on the tin, i.e. tiny, so not much work to > demidlayer if we notice a clear need for that change. I wouldn't worry > about this for now, but good to keep in mind. In the end, good design is > design that can be changed, because you'll only get it right until the > next unforseen thing happens :-) Agreed, there's nothing in this that couldn't easily be tweaked later on. It's clearly drm-misc material, too, so even cross-tree dependencies wouldn't be an issue. Thierry
Attachment:
signature.asc
Description: PGP signature
