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@tronnes.org Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
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 :-) -Daniel