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

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

[...]
> +static int tinydrm_register(struct tinydrm_device *tdev)
> +{
> +	struct drm_device *drm = tdev->drm;
> +	int bpp = drm->mode_config.preferred_depth;
> +	struct drm_fbdev_cma *fbdev;
> +	int ret;
> +
> +	ret = drm_dev_register(tdev->drm, 0);
> +	if (ret)
> +		return ret;
> +
> +	fbdev = drm_fbdev_cma_init_with_funcs(drm, bpp ? bpp : 32,

Does this make sense? The helpers and the driver later in this series
suggest that these panels will usually be 16-bit. Wouldn't that be a
more sensible default?

> +static enum drm_connector_status
> +tinydrm_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	if (drm_device_is_unplugged(connector->dev))
> +		return connector_status_disconnected;

Is this really what you wanted? drm_device_is_unplugged() returns true
if the device is in the process of being removed. It only really makes
sense if you also call drm_device_is_unplugged() somewhere in the
driver, usually via drm_unplug_dev(), but I don't see that in the code
which means this will always return false anyway.

Thierry

Attachment: signature.asc
Description: PGP signature