Re: [PATCH v3 1/7] drm: Add DRM support for tiny LCD displays

[Daniel Vetter]

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 :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch