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

[Thierry Reding]

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