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

[Noralf TrÃnnes]

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).


Noralf.