[PATCH v3 0/7] drm: Add support for tiny LCD displays

[PATCH v3 0/7] drm: Add support for tiny LCD displays

[Noralf Trønnes]

drm: Add support for tiny LCD displays

This is an attempt at providing a DRM version of drivers/staging/fbtft.

The tinydrm library provides a very simplified view of DRM in particular
for tiny displays that has onboard video memory and is connected through
a slow bus like SPI/I2C.

The main change this time is adding 'rotation' as a common display
Device Tree property.


Noralf.

Changes since version 2:
- Remove fbdev after drm unregister, not before.
- Added Documentation/devicetree/bindings/display/display.txt

Changes since version 1:
- Add tinydrm.rst
- Set tdev->fbdev_cma=NULL on unregister (lastclose is called after that).
- Remove some DRM_DEBUG*()
- Write-combined memory has uncached reads, so speed up by copying/buffering
  one pixel line before conversion.

Changes since RFC v2:
- Rebased on new core helpers
- Don't use drm_panel
- Flush when the framebuffer is changed on the plane
- Add devm_tinydrm_init()
- Fix PRIME support, set vaddr
- Use atomic helpers in suspend/resume
- Add a tinydrm_connector with one display mode
- Set mode_config.preferred_depth and use it for fbdev
- Subclass tinydrm_device in drivers instead of bloating the structure
- The PiTFT display uses a MI0283QT panel, write driver for that instead.
- Drop homegrown lcdreg module, it ended up as a collection of special
  cases.
- Add more documentation

Changes since RFC v1:
- Add fb_deferred_io support to drm_fb_helper and drm_fb_cma_helper,
  and use drm_fb_cma_helper instead.
- Move display pipeline code to drm_simple_kms_helper.
- Don't use (struct drm_driver *)->load().
- Make tinydrm more like a library, exporting the internals.
- Move the struct drm_driver definition from the tinydrm module to the
  driver using a helper macro: TINYDRM_DRM_DRIVER.
- Remove dirtyfb() async code.
- Added support for partial display updates.


Noralf Trønnes (7):
  drm: Add DRM support for tiny LCD displays
  drm/tinydrm: Add helper functions
  drm/tinydrm: Add MIPI DBI support
  of: Add vendor prefix for Multi-Inno
  dt-bindings: display: Add common rotation property
  dt-bindings: Add Multi-Inno MI0283QT binding
  drm/tinydrm: Add support for Multi-Inno MI0283QT display

 .../devicetree/bindings/display/display.txt        |    4 +
 .../bindings/display/multi-inno,mi0283qt.txt       |   27 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 Documentation/gpu/index.rst                        |    1 +
 Documentation/gpu/tinydrm.rst                      |   42 +
 MAINTAINERS                                        |    6 +
 drivers/gpu/drm/Kconfig                            |    2 +
 drivers/gpu/drm/Makefile                           |    1 +
 drivers/gpu/drm/tinydrm/Kconfig                    |   19 +
 drivers/gpu/drm/tinydrm/Makefile                   |    7 +
 drivers/gpu/drm/tinydrm/core/Makefile              |    3 +
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c        |  377 ++++++++
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c     |  462 +++++++++
 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c        |  234 +++++
 drivers/gpu/drm/tinydrm/mi0283qt.c                 |  279 ++++++
 drivers/gpu/drm/tinydrm/mipi-dbi.c                 | 1005 ++++++++++++++++++++
 include/drm/tinydrm/ili9341.h                      |   54 ++
 include/drm/tinydrm/mipi-dbi.h                     |  107 +++
 include/drm/tinydrm/tinydrm-helpers.h              |  100 ++
 include/drm/tinydrm/tinydrm.h                      |  115 +++
 20 files changed, 2846 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/display.txt
 create mode 100644 Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
 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-helpers.c
 create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
 create mode 100644 drivers/gpu/drm/tinydrm/mi0283qt.c
 create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
 create mode 100644 include/drm/tinydrm/ili9341.h
 create mode 100644 include/drm/tinydrm/mipi-dbi.h
 create mode 100644 include/drm/tinydrm/tinydrm-helpers.h
 create mode 100644 include/drm/tinydrm/tinydrm.h

--
2.10.2

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

[Noralf Trønnes]

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

diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst
index 367d7c3..f81278a 100644
--- a/Documentation/gpu/index.rst
+++ b/Documentation/gpu/index.rst
@@ -11,6 +11,7 @@ Linux GPU Driver Developer's Guide
    drm-kms-helpers
    drm-uapi
    i915
+   tinydrm
    vga-switcheroo
    vgaarbiter

diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst
new file mode 100644
index 0000000..ec4a20d
--- /dev/null
+++ b/Documentation/gpu/tinydrm.rst
@@ -0,0 +1,21 @@
+==========================
+drm/tinydrm Driver library
+==========================
+
+.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-core.c
+   :doc: overview
+
+Core functionality
+==================
+
+.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-core.c
+   :doc: core
+
+.. kernel-doc:: include/drm/tinydrm/tinydrm.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-core.c
+   :export:
+
+.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
+   :export:
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 90bc65d..88e01e08e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -263,6 +263,8 @@ source "drivers/gpu/drm/mxsfb/Kconfig"

 source "drivers/gpu/drm/meson/Kconfig"

+source "drivers/gpu/drm/tinydrm/Kconfig"
+
 # Keep legacy drivers last

 menuconfig DRM_LEGACY
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 92de399..3ee9579 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -94,3 +94,4 @@ obj-$(CONFIG_DRM_ARCPGU)+= arc/
 obj-y			+= hisilicon/
 obj-$(CONFIG_DRM_ZTE)	+= zte/
 obj-$(CONFIG_DRM_MXSFB)	+= mxsfb/
+obj-$(CONFIG_DRM_TINYDRM) += tinydrm/
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
new file mode 100644
index 0000000..ffb873f
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -0,0 +1,8 @@
+menuconfig DRM_TINYDRM
+	tristate "Support for simple displays"
+	depends on DRM
+	select DRM_KMS_HELPER
+	select DRM_KMS_CMA_HELPER
+	help
+	  Choose this option if you have a tinydrm supported display.
+	  If M is selected the module will be called tinydrm.
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
new file mode 100644
index 0000000..7476ed1
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_DRM_TINYDRM)		+= core/
diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile
new file mode 100644
index 0000000..4f14a0f
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/core/Makefile
@@ -0,0 +1,3 @@
+tinydrm-y := tinydrm-core.o tinydrm-pipe.o
+
+obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
new file mode 100644
index 0000000..c0a2eb6
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
@@ -0,0 +1,377 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/tinydrm/tinydrm.h>
+#include <linux/device.h>
+#include <linux/dma-buf.h>
+
+/**
+ * DOC: overview
+ *
+ * This library provides driver helpers for very simple display hardware.
+ *
+ * It is based on &drm_simple_display_pipe coupled with a &drm_connector which
+ * has only one fixed &drm_display_mode. The framebuffers are backed by the
+ * cma helper and have support for framebuffer flushing (dirty).
+ * fbdev support is also included.
+ *
+ */
+
+/**
+ * DOC: core
+ *
+ * The driver allocates &tinydrm_device, initializes it using
+ * devm_tinydrm_init(), sets up the pipeline using tinydrm_display_pipe_init()
+ * and registers the DRM device using devm_tinydrm_register().
+ */
+
+/**
+ * tinydrm_lastclose - DRM lastclose helper
+ * @drm: DRM device
+ *
+ * This function ensures that fbdev is restored when drm_lastclose() is called
+ * on the last drm_release(). Drivers can use this as their
+ * &drm_driver->lastclose callback.
+ */
+void tinydrm_lastclose(struct drm_device *drm)
+{
+	struct tinydrm_device *tdev = drm->dev_private;
+
+	DRM_DEBUG_KMS("\n");
+	drm_fbdev_cma_restore_mode(tdev->fbdev_cma);
+}
+EXPORT_SYMBOL(tinydrm_lastclose);
+
+/**
+ * tinydrm_gem_cma_prime_import_sg_table - Produce a CMA GEM object from
+ *     another driver's scatter/gather table of pinned pages
+ * @drm: DRM device to import into
+ * @attach: DMA-BUF attachment
+ * @sgt: Scatter/gather table of pinned pages
+ *
+ * This function imports a scatter/gather table exported via DMA-BUF by
+ * another driver using drm_gem_cma_prime_import_sg_table(). It sets the
+ * kernel virtual address on the CMA object. Drivers should use this as their
+ * &drm_driver->gem_prime_import_sg_table callback if they need the virtual
+ * address. tinydrm_gem_cma_free_object() should be used in combination with
+ * this function.
+ *
+ * Returns:
+ * A pointer to a newly created GEM object or an ERR_PTR-encoded negative
+ * error code on failure.
+ */
+struct drm_gem_object *
+tinydrm_gem_cma_prime_import_sg_table(struct drm_device *drm,
+				      struct dma_buf_attachment *attach,
+				      struct sg_table *sgt)
+{
+	struct drm_gem_cma_object *cma_obj;
+	struct drm_gem_object *obj;
+	void *vaddr;
+
+	vaddr = dma_buf_vmap(attach->dmabuf);
+	if (!vaddr) {
+		DRM_ERROR("Failed to vmap PRIME buffer\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	obj = drm_gem_cma_prime_import_sg_table(drm, attach, sgt);
+	if (IS_ERR(obj)) {
+		dma_buf_vunmap(attach->dmabuf, vaddr);
+		return obj;
+	}
+
+	cma_obj = to_drm_gem_cma_obj(obj);
+	cma_obj->vaddr = vaddr;
+
+	return obj;
+}
+EXPORT_SYMBOL(tinydrm_gem_cma_prime_import_sg_table);
+
+/**
+ * tinydrm_gem_cma_free_object - Free resources associated with a CMA GEM
+ *                               object
+ * @gem_obj: GEM object to free
+ *
+ * This function frees the backing memory of the CMA GEM object, cleans up the
+ * GEM object state and frees the memory used to store the object itself using
+ * drm_gem_cma_free_object(). It also handles PRIME buffers which has the kernel
+ * virtual address set by tinydrm_gem_cma_prime_import_sg_table(). Drivers
+ * can use this as their &drm_driver->gem_free_object callback.
+ */
+void tinydrm_gem_cma_free_object(struct drm_gem_object *gem_obj)
+{
+	if (gem_obj->import_attach) {
+		struct drm_gem_cma_object *cma_obj;
+
+		cma_obj = to_drm_gem_cma_obj(gem_obj);
+		dma_buf_vunmap(gem_obj->import_attach->dmabuf, cma_obj->vaddr);
+		cma_obj->vaddr = NULL;
+	}
+
+	drm_gem_cma_free_object(gem_obj);
+}
+EXPORT_SYMBOL_GPL(tinydrm_gem_cma_free_object);
+
+const struct file_operations tinydrm_fops = {
+	.owner		= THIS_MODULE,
+	.open		= drm_open,
+	.release	= drm_release,
+	.unlocked_ioctl	= drm_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= drm_compat_ioctl,
+#endif
+	.poll		= drm_poll,
+	.read		= drm_read,
+	.llseek		= no_llseek,
+	.mmap		= drm_gem_cma_mmap,
+};
+EXPORT_SYMBOL(tinydrm_fops);
+
+static struct drm_framebuffer *
+tinydrm_fb_create(struct drm_device *drm, struct drm_file *file_priv,
+		  const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	struct tinydrm_device *tdev = drm->dev_private;
+
+	return drm_fb_cma_create_with_funcs(drm, file_priv, mode_cmd,
+					    tdev->fb_funcs);
+}
+
+static const struct drm_mode_config_funcs tinydrm_mode_config_funcs = {
+	.fb_create = tinydrm_fb_create,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
+			const struct drm_framebuffer_funcs *fb_funcs,
+			struct drm_driver *driver)
+{
+	struct drm_device *drm;
+
+	mutex_init(&tdev->dirty_lock);
+	tdev->fb_funcs = fb_funcs;
+
+	/*
+	 * We don't embed drm_device, because that prevent us from using
+	 * devm_kzalloc() to allocate tinydrm_device in the driver since
+	 * drm_dev_unref() frees the structure. The devm_ functions provide
+	 * for easy error handling.
+	 */
+	drm = drm_dev_alloc(driver, parent);
+	if (IS_ERR(drm))
+		return PTR_ERR(drm);
+
+	tdev->drm = drm;
+	drm->dev_private = tdev;
+	drm_mode_config_init(drm);
+	drm->mode_config.funcs = &tinydrm_mode_config_funcs;
+
+	return 0;
+}
+
+static void tinydrm_fini(struct tinydrm_device *tdev)
+{
+	drm_mode_config_cleanup(tdev->drm);
+	mutex_destroy(&tdev->dirty_lock);
+	tdev->drm->dev_private = NULL;
+	drm_dev_unref(tdev->drm);
+}
+
+static void devm_tinydrm_release(void *data)
+{
+	tinydrm_fini(data);
+}
+
+/**
+ * devm_tinydrm_init - Initialize tinydrm device
+ * @parent: Parent device object
+ * @tdev: tinydrm device
+ * @fb_funcs: Framebuffer functions
+ * @driver: DRM driver
+ *
+ * This function initializes @tdev, the underlying DRM device and it's
+ * mode_config. Resources will be automatically freed on driver detach (devres)
+ * using drm_mode_config_cleanup() and drm_dev_unref().
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
+		      const struct drm_framebuffer_funcs *fb_funcs,
+		      struct drm_driver *driver)
+{
+	int ret;
+
+	ret = tinydrm_init(parent, tdev, fb_funcs, driver);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action(parent, devm_tinydrm_release, tdev);
+	if (ret)
+		tinydrm_fini(tdev);
+
+	return ret;
+}
+EXPORT_SYMBOL(devm_tinydrm_init);
+
+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,
+					      drm->mode_config.num_crtc,
+					      drm->mode_config.num_connector,
+					      tdev->fb_funcs);
+	if (IS_ERR(fbdev))
+		DRM_ERROR("Failed to initialize fbdev: %ld\n", PTR_ERR(fbdev));
+	else
+		tdev->fbdev_cma = fbdev;
+
+	return 0;
+}
+
+static void tinydrm_unregister(struct tinydrm_device *tdev)
+{
+	struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;
+
+	drm_crtc_force_disable_all(tdev->drm);
+	/* don't restore fbdev in lastclose, keep pipeline disabled */
+	tdev->fbdev_cma = NULL;
+	drm_dev_unregister(tdev->drm);
+	if (fbdev_cma)
+		drm_fbdev_cma_fini(fbdev_cma);
+}
+
+static void devm_tinydrm_register_release(void *data)
+{
+	tinydrm_unregister(data);
+}
+
+/**
+ * devm_tinydrm_register - Register tinydrm device
+ * @tdev: tinydrm device
+ *
+ * This function registers the underlying DRM device and fbdev.
+ * These resources will be automatically unregistered on driver detach (devres)
+ * and the display pipeline will be disabled.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int devm_tinydrm_register(struct tinydrm_device *tdev)
+{
+	struct device *dev = tdev->drm->dev;
+	int ret;
+
+	ret = tinydrm_register(tdev);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action(dev, devm_tinydrm_register_release, tdev);
+	if (ret)
+		tinydrm_unregister(tdev);
+
+	return ret;
+}
+EXPORT_SYMBOL(devm_tinydrm_register);
+
+/**
+ * tinydrm_shutdown - Shutdown tinydrm
+ * @tdev: tinydrm device
+ *
+ * This function makes sure that the display pipeline is disabled.
+ * Used by drivers in their shutdown callback to turn off the display
+ * on machine shutdown and reboot.
+ */
+void tinydrm_shutdown(struct tinydrm_device *tdev)
+{
+	drm_crtc_force_disable_all(tdev->drm);
+}
+EXPORT_SYMBOL(tinydrm_shutdown);
+
+/**
+ * tinydrm_suspend - Suspend tinydrm
+ * @tdev: tinydrm device
+ *
+ * Used in driver PM operations to suspend tinydrm.
+ * Suspends fbdev and DRM.
+ * Resume with tinydrm_resume().
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int tinydrm_suspend(struct tinydrm_device *tdev)
+{
+	struct drm_atomic_state *state;
+
+	if (tdev->suspend_state) {
+		DRM_ERROR("Failed to suspend: state already set\n");
+		return -EINVAL;
+	}
+
+	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
+	state = drm_atomic_helper_suspend(tdev->drm);
+	if (IS_ERR(state)) {
+		drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
+		return PTR_ERR(state);
+	}
+
+	tdev->suspend_state = state;
+
+	return 0;
+}
+EXPORT_SYMBOL(tinydrm_suspend);
+
+/**
+ * tinydrm_resume - Resume tinydrm
+ * @tdev: tinydrm device
+ *
+ * Used in driver PM operations to resume tinydrm.
+ * Suspend with tinydrm_suspend().
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int tinydrm_resume(struct tinydrm_device *tdev)
+{
+	struct drm_atomic_state *state = tdev->suspend_state;
+	int ret;
+
+	if (!state) {
+		DRM_ERROR("Failed to resume: state is not set\n");
+		return -EINVAL;
+	}
+
+	tdev->suspend_state = NULL;
+
+	ret = drm_atomic_helper_resume(tdev->drm, state);
+	if (ret) {
+		DRM_ERROR("Error resuming state: %d\n", ret);
+		return ret;
+	}
+
+	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
+
+	return 0;
+}
+EXPORT_SYMBOL(tinydrm_resume);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
new file mode 100644
index 0000000..ec43fb7
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
@@ -0,0 +1,234 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_modes.h>
+#include <drm/tinydrm/tinydrm.h>
+
+struct tinydrm_connector {
+	struct drm_connector base;
+	const struct drm_display_mode *mode;
+};
+
+static inline struct tinydrm_connector *
+to_tinydrm_connector(struct drm_connector *connector)
+{
+	return container_of(connector, struct tinydrm_connector, base);
+}
+
+static int tinydrm_connector_get_modes(struct drm_connector *connector)
+{
+	struct tinydrm_connector *tconn = to_tinydrm_connector(connector);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(connector->dev, tconn->mode);
+	if (!mode) {
+		DRM_ERROR("Failed to duplicate mode\n");
+		return 0;
+	}
+
+	if (mode->name[0] == '\0')
+		drm_mode_set_name(mode);
+
+	mode->type |= DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	if (mode->width_mm) {
+		connector->display_info.width_mm = mode->width_mm;
+		connector->display_info.height_mm = mode->height_mm;
+	}
+
+	return 1;
+}
+
+static const struct drm_connector_helper_funcs tinydrm_connector_hfuncs = {
+	.get_modes = tinydrm_connector_get_modes,
+	.best_encoder = drm_atomic_helper_best_encoder,
+};
+
+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;
+
+	return connector->status;
+}
+
+static void tinydrm_connector_destroy(struct drm_connector *connector)
+{
+	struct tinydrm_connector *tconn = to_tinydrm_connector(connector);
+
+	drm_connector_cleanup(connector);
+	kfree(tconn);
+}
+
+static const struct drm_connector_funcs tinydrm_connector_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
+	.detect = tinydrm_connector_detect,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = tinydrm_connector_destroy,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+struct drm_connector *
+tinydrm_connector_create(struct drm_device *drm,
+			 const struct drm_display_mode *mode,
+			 int connector_type)
+{
+	struct tinydrm_connector *tconn;
+	struct drm_connector *connector;
+	int ret;
+
+	tconn = kzalloc(sizeof(*tconn), GFP_KERNEL);
+	if (!tconn)
+		return ERR_PTR(-ENOMEM);
+
+	tconn->mode = mode;
+	connector = &tconn->base;
+
+	drm_connector_helper_add(connector, &tinydrm_connector_hfuncs);
+	ret = drm_connector_init(drm, connector, &tinydrm_connector_funcs,
+				 connector_type);
+	if (ret) {
+		kfree(tconn);
+		return ERR_PTR(ret);
+	}
+
+	connector->status = connector_status_connected;
+
+	return connector;
+}
+
+/**
+ * tinydrm_display_pipe_update - Display pipe update helper
+ * @pipe: Simple display pipe
+ * @old_state: Old plane state
+ *
+ * This function does a full framebuffer flush if the plane framebuffer
+ * has changed. It also handles vblank events. Drivers can use this as their
+ * &drm_simple_display_pipe_funcs->update callback.
+ */
+void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
+				 struct drm_plane_state *old_state)
+{
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct drm_framebuffer *fb = pipe->plane.state->fb;
+	struct drm_crtc *crtc = &tdev->pipe.crtc;
+
+	if (fb && (fb != old_state->fb)) {
+		pipe->plane.fb = fb;
+		if (fb->funcs->dirty)
+			fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+	}
+
+	if (crtc->state->event) {
+		spin_lock_irq(&crtc->dev->event_lock);
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		spin_unlock_irq(&crtc->dev->event_lock);
+		crtc->state->event = NULL;
+	}
+}
+EXPORT_SYMBOL(tinydrm_display_pipe_update);
+
+/**
+ * tinydrm_display_pipe_prepare_fb - Display pipe prepare_fb helper
+ * @pipe: Simple display pipe
+ * @plane_state: Plane state
+ *
+ * This function uses drm_fb_cma_prepare_fb() to check if the plane FB has an
+ * dma-buf attached, extracts the exclusive fence and attaches it to plane
+ * state for the atomic helper to wait on. Drivers can use this as their
+ * &drm_simple_display_pipe_funcs->prepare_fb callback.
+ */
+int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
+				    struct drm_plane_state *plane_state)
+{
+	return drm_fb_cma_prepare_fb(&pipe->plane, plane_state);
+}
+EXPORT_SYMBOL(tinydrm_display_pipe_prepare_fb);
+
+static int tinydrm_rotate_mode(struct drm_display_mode *mode,
+			       unsigned int rotation)
+{
+	if (rotation == 0 || rotation == 180) {
+		return 0;
+	} else if (rotation == 90 || rotation == 270) {
+		swap(mode->hdisplay, mode->vdisplay);
+		swap(mode->hsync_start, mode->vsync_start);
+		swap(mode->hsync_end, mode->vsync_end);
+		swap(mode->htotal, mode->vtotal);
+		swap(mode->width_mm, mode->height_mm);
+		return 0;
+	} else {
+		return -EINVAL;
+	}
+}
+
+/**
+ * tinydrm_display_pipe_init - Initialize display pipe
+ * @tdev: tinydrm device
+ * @funcs: Display pipe functions
+ * @connector_type: Connector type
+ * @formats: Array of supported formats (DRM_FORMAT\_\*)
+ * @format_count: Number of elements in @formats
+ * @mode: Supported mode
+ * @rotation: Initial @mode rotation in degrees Counter Clock Wise
+ *
+ * This function sets up a &drm_simple_display_pipe with a &drm_connector that
+ * has one fixed &drm_display_mode which is rotated according to @rotation.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int
+tinydrm_display_pipe_init(struct tinydrm_device *tdev,
+			  const struct drm_simple_display_pipe_funcs *funcs,
+			  int connector_type,
+			  const uint32_t *formats,
+			  unsigned int format_count,
+			  const struct drm_display_mode *mode,
+			  unsigned int rotation)
+{
+	struct drm_device *drm = tdev->drm;
+	struct drm_display_mode *mode_copy;
+	struct drm_connector *connector;
+	int ret;
+
+	mode_copy = devm_kmalloc(drm->dev, sizeof(*mode_copy), GFP_KERNEL);
+	if (!mode_copy)
+		return -ENOMEM;
+
+	*mode_copy = *mode;
+	ret = tinydrm_rotate_mode(mode_copy, rotation);
+	if (ret) {
+		DRM_ERROR("Illegal rotation value %u\n", rotation);
+		return -EINVAL;
+	}
+
+	drm->mode_config.min_width = mode_copy->hdisplay;
+	drm->mode_config.max_width = mode_copy->hdisplay;
+	drm->mode_config.min_height = mode_copy->vdisplay;
+	drm->mode_config.max_height = mode_copy->vdisplay;
+
+	connector = tinydrm_connector_create(drm, mode_copy, connector_type);
+	if (IS_ERR(connector))
+		return PTR_ERR(connector);
+
+	ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats,
+					   format_count, connector);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(tinydrm_display_pipe_init);
diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
new file mode 100644
index 0000000..cf9ca20
--- /dev/null
+++ b/include/drm/tinydrm/tinydrm.h
@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TINYDRM_H
+#define __LINUX_TINYDRM_H
+
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+/**
+ * struct tinydrm_device - tinydrm device
+ * @drm: DRM device
+ * @pipe: Display pipe structure
+ * @dirty_lock: Serializes framebuffer flushing
+ * @fbdev_cma: CMA fbdev structure
+ * @suspend_state: Atomic state when suspended
+ * @fb_funcs: Framebuffer functions used when creating framebuffers
+ */
+struct tinydrm_device {
+	struct drm_device *drm;
+	struct drm_simple_display_pipe pipe;
+	struct mutex dirty_lock;
+	struct drm_fbdev_cma *fbdev_cma;
+	struct drm_atomic_state *suspend_state;
+	const struct drm_framebuffer_funcs *fb_funcs;
+};
+
+static inline struct tinydrm_device *
+pipe_to_tinydrm(struct drm_simple_display_pipe *pipe)
+{
+	return container_of(pipe, struct tinydrm_device, pipe);
+}
+
+/**
+ * TINYDRM_GEM_DRIVER_OPS - default tinydrm gem operations
+ *
+ * This macro provides a shortcut for setting the tinydrm GEM operations in
+ * the &drm_driver structure.
+ */
+#define TINYDRM_GEM_DRIVER_OPS \
+	.gem_free_object	= tinydrm_gem_cma_free_object, \
+	.gem_vm_ops		= &drm_gem_cma_vm_ops, \
+	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd, \
+	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle, \
+	.gem_prime_import	= drm_gem_prime_import, \
+	.gem_prime_export	= drm_gem_prime_export, \
+	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table, \
+	.gem_prime_import_sg_table = tinydrm_gem_cma_prime_import_sg_table, \
+	.gem_prime_vmap		= drm_gem_cma_prime_vmap, \
+	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap, \
+	.gem_prime_mmap		= drm_gem_cma_prime_mmap, \
+	.dumb_create		= drm_gem_cma_dumb_create, \
+	.dumb_map_offset	= drm_gem_cma_dumb_map_offset, \
+	.dumb_destroy		= drm_gem_dumb_destroy, \
+	.fops			= &tinydrm_fops
+
+/**
+ * TINYDRM_MODE - tinydrm display mode
+ * @hd: Horizontal resolution, width
+ * @vd: Vertical resolution, height
+ * @hd_mm: Display width in millimeters
+ * @vd_mm: Display height in millimeters
+ *
+ * This macro creates a &drm_display_mode for use with tinydrm.
+ */
+#define TINYDRM_MODE(hd, vd, hd_mm, vd_mm) \
+	.hdisplay = (hd), \
+	.hsync_start = (hd), \
+	.hsync_end = (hd), \
+	.htotal = (hd), \
+	.vdisplay = (vd), \
+	.vsync_start = (vd), \
+	.vsync_end = (vd), \
+	.vtotal = (vd), \
+	.width_mm = (hd_mm), \
+	.height_mm = (vd_mm), \
+	.type = DRM_MODE_TYPE_DRIVER, \
+	.clock = 1 /* pass validation */
+
+extern const struct file_operations tinydrm_fops;
+void tinydrm_lastclose(struct drm_device *drm);
+void tinydrm_gem_cma_free_object(struct drm_gem_object *gem_obj);
+struct drm_gem_object *
+tinydrm_gem_cma_prime_import_sg_table(struct drm_device *drm,
+				      struct dma_buf_attachment *attach,
+				      struct sg_table *sgt);
+int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
+		      const struct drm_framebuffer_funcs *fb_funcs,
+		      struct drm_driver *driver);
+int devm_tinydrm_register(struct tinydrm_device *tdev);
+void tinydrm_shutdown(struct tinydrm_device *tdev);
+int tinydrm_suspend(struct tinydrm_device *tdev);
+int tinydrm_resume(struct tinydrm_device *tdev);
+
+void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
+				 struct drm_plane_state *old_state);
+int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
+				    struct drm_plane_state *plane_state);
+int
+tinydrm_display_pipe_init(struct tinydrm_device *tdev,
+			  const struct drm_simple_display_pipe_funcs *funcs,
+			  int connector_type,
+			  const uint32_t *formats,
+			  unsigned int format_count,
+			  const struct drm_display_mode *mode,
+			  unsigned int rotation);
+
+#endif /* __LINUX_TINYDRM_H */
--
2.10.2

[PATCH v3 2/7] drm/tinydrm: Add helper functions

[Noralf Trønnes]

Add common functionality needed by many tinydrm drivers.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/gpu/tinydrm.rst                  |   9 +
 drivers/gpu/drm/tinydrm/core/Makefile          |   2 +-
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 462 +++++++++++++++++++++++++
 include/drm/tinydrm/tinydrm-helpers.h          | 100 ++++++
 4 files changed, 572 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
 create mode 100644 include/drm/tinydrm/tinydrm-helpers.h

diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst
index ec4a20d..fb256d2 100644
--- a/Documentation/gpu/tinydrm.rst
+++ b/Documentation/gpu/tinydrm.rst
@@ -19,3 +19,12 @@ Core functionality
 
 .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
    :export:
+
+Additional helpers
+==================
+
+.. kernel-doc:: include/drm/tinydrm/tinydrm-helpers.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+   :export:
diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile
index 4f14a0f..fb221e6 100644
--- a/drivers/gpu/drm/tinydrm/core/Makefile
+++ b/drivers/gpu/drm/tinydrm/core/Makefile
@@ -1,3 +1,3 @@
-tinydrm-y := tinydrm-core.o tinydrm-pipe.o
+tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o
 
 obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
new file mode 100644
index 0000000..75e4e54
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -0,0 +1,462 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <drm/tinydrm/tinydrm.h>
+#include <drm/tinydrm/tinydrm-helpers.h>
+#include <linux/backlight.h>
+#include <linux/pm.h>
+#include <linux/spi/spi.h>
+#include <linux/swab.h>
+
+static unsigned int spi_max;
+module_param(spi_max, uint, 0400);
+MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
+
+/**
+ * tinydrm_merge_clips - Merge clip rectangles
+ * @dst: Destination clip rectangle
+ * @src: Source clip rectangle(s)
+ * @num_clips: Number of @src clip rectangles
+ * @flags: Dirty fb ioctl flags
+ * @max_width: Maximum width of @dst
+ * @max_height: Maximum height of @dst
+ *
+ * This function merges @src clip rectangle(s) into @dst. If @src is NULL,
+ * @max_width and @min_width is used to set a full @dst clip rectangle.
+ *
+ * Returns:
+ * true if it's a full clip, false otherwise
+ */
+bool tinydrm_merge_clips(struct drm_clip_rect *dst,
+			 struct drm_clip_rect *src, unsigned int num_clips,
+			 unsigned int flags, u32 max_width, u32 max_height)
+{
+	unsigned int i;
+
+	if (!src || !num_clips) {
+		dst->x1 = 0;
+		dst->x2 = max_width;
+		dst->y1 = 0;
+		dst->y2 = max_height;
+		return true;
+	}
+
+	dst->x1 = ~0;
+	dst->y1 = ~0;
+	dst->x2 = 0;
+	dst->y2 = 0;
+
+	for (i = 0; i < num_clips; i++) {
+		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
+			i++;
+		dst->x1 = min(dst->x1, src[i].x1);
+		dst->x2 = max(dst->x2, src[i].x2);
+		dst->y1 = min(dst->y1, src[i].y1);
+		dst->y2 = max(dst->y2, src[i].y2);
+	}
+
+	if (dst->x2 > max_width || dst->y2 > max_height ||
+	    dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
+		DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
+			      dst->x1, dst->x2, dst->y1, dst->y2);
+		dst->x1 = 0;
+		dst->y1 = 0;
+		dst->x2 = max_width;
+		dst->y2 = max_height;
+	}
+
+	return (dst->x2 - dst->x1) == max_width &&
+	       (dst->y2 - dst->y1) == max_height;
+}
+EXPORT_SYMBOL(tinydrm_merge_clips);
+
+/**
+ * tinydrm_memcpy - Copy clip buffer
+ * @dst: Destination buffer
+ * @vaddr: Source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ */
+void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb,
+		    struct drm_clip_rect *clip)
+{
+	unsigned int cpp = drm_format_plane_cpp(fb->format->format, 0);
+	unsigned int pitch = fb->pitches[0];
+	void *src = vaddr + (clip->y1 * pitch) + (clip->x1 * cpp);
+	size_t len = (clip->x2 - clip->x1) * cpp;
+	unsigned int y;
+
+	for (y = clip->y1; y < clip->y2; y++) {
+		memcpy(dst, src, len);
+		src += pitch;
+		dst += len;
+	}
+}
+EXPORT_SYMBOL(tinydrm_memcpy);
+
+/**
+ * tinydrm_swab16 - Swap bytes into clip buffer
+ * @dst: RGB565 destination buffer
+ * @vaddr: RGB565 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ */
+void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
+		    struct drm_clip_rect *clip)
+{
+	size_t len = (clip->x2 - clip->x1) * sizeof(u16);
+	unsigned int x, y;
+	u16 *src, *buf;
+
+	/*
+	 * The cma memory is write-combined so reads are uncached.
+	 * Speed up by fetching one line at a time.
+	 */
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	for (y = clip->y1; y < clip->y2; y++) {
+		src = vaddr + (y * fb->pitches[0]);
+		src += clip->x1;
+		memcpy(buf, src, len);
+		src = buf;
+		for (x = clip->x1; x < clip->x2; x++)
+			*dst++ = swab16(*src++);
+	}
+
+	kfree(buf);
+}
+EXPORT_SYMBOL(tinydrm_swab16);
+
+/**
+ * tinydrm_xrgb8888_to_rgb565 - Convert XRGB8888 to RGB565 clip buffer
+ * @dst: RGB565 destination buffer
+ * @vaddr: XRGB8888 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ * @swap: Swap bytes
+ *
+ * Drivers can use this function for RGB565 devices that don't natively
+ * support XRGB8888.
+ */
+void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
+				struct drm_framebuffer *fb,
+				struct drm_clip_rect *clip, bool swap)
+{
+	size_t len = (clip->x2 - clip->x1) * sizeof(u32);
+	unsigned int x, y;
+	u32 *src, *buf;
+	u16 val16;
+
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	for (y = clip->y1; y < clip->y2; y++) {
+		src = vaddr + (y * fb->pitches[0]);
+		src += clip->x1;
+		memcpy(buf, src, len);
+		src = buf;
+		for (x = clip->x1; x < clip->x2; x++) {
+			val16 = ((*src & 0x00F80000) >> 8) |
+				((*src & 0x0000FC00) >> 5) |
+				((*src & 0x000000F8) >> 3);
+			src++;
+			if (swap)
+				*dst++ = swab16(val16);
+			else
+				*dst++ = val16;
+		}
+	}
+
+	kfree(buf);
+}
+EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
+
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
+/**
+ * tinydrm_of_find_backlight - Find backlight device in device-tree
+ * @dev: Device
+ *
+ * This function looks for a DT node pointed to by a property named 'backlight'
+ * and uses of_find_backlight_by_node() to get the backlight device.
+ * Additionally if the brightness property is zero, it is set to
+ * max_brightness.
+ *
+ * Returns:
+ * NULL if there's no backlight property.
+ * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
+ * is found.
+ * If the backlight device is found, a pointer to the structure is returned.
+ */
+struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
+{
+	struct backlight_device *backlight;
+	struct device_node *np;
+
+	np = of_parse_phandle(dev->of_node, "backlight", 0);
+	if (!np)
+		return NULL;
+
+	backlight = of_find_backlight_by_node(np);
+	of_node_put(np);
+
+	if (!backlight)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (!backlight->props.brightness) {
+		backlight->props.brightness = backlight->props.max_brightness;
+		DRM_DEBUG_KMS("Backlight brightness set to %d\n",
+			      backlight->props.brightness);
+	}
+
+	return backlight;
+}
+EXPORT_SYMBOL(tinydrm_of_find_backlight);
+
+/**
+ * tinydrm_enable_backlight - Enable backlight helper
+ * @backlight: Backlight device
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int tinydrm_enable_backlight(struct backlight_device *backlight)
+{
+	unsigned int old_state;
+	int ret;
+
+	if (!backlight)
+		return 0;
+
+	old_state = backlight->props.state;
+	backlight->props.state &= ~BL_CORE_FBBLANK;
+	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
+		      backlight->props.state);
+
+	ret = backlight_update_status(backlight);
+	if (ret)
+		DRM_ERROR("Failed to enable backlight %d\n", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL(tinydrm_enable_backlight);
+
+/**
+ * tinydrm_disable_backlight - Disable backlight helper
+ * @backlight: Backlight device
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int tinydrm_disable_backlight(struct backlight_device *backlight)
+{
+	unsigned int old_state;
+	int ret;
+
+	if (!backlight)
+		return 0;
+
+	old_state = backlight->props.state;
+	backlight->props.state |= BL_CORE_FBBLANK;
+	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
+		      backlight->props.state);
+	ret = backlight_update_status(backlight);
+	if (ret)
+		DRM_ERROR("Failed to disable backlight %d\n", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL(tinydrm_disable_backlight);
+#endif
+
+#ifdef CONFIG_SPI
+
+/**
+ * tinydrm_spi_max_transfer_size - Determine max SPI transfer size
+ * @spi: SPI device
+ * @max_len: Maximum buffer size needed (optional)
+ *
+ * This function returns the maximum size to use for SPI transfers. It checks
+ * the SPI master, the optional @max_len and the module parameter spi_max and
+ * returns the smallest.
+ *
+ * Returns:
+ * Maximum size for SPI transfers
+ */
+size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len)
+{
+	size_t ret;
+
+	ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len);
+	if (max_len)
+		ret = min(ret, max_len);
+	if (spi_max)
+		ret = min_t(size_t, ret, spi_max);
+	ret &= ~0x3;
+	if (ret < 4)
+		ret = 4;
+
+	return ret;
+}
+EXPORT_SYMBOL(tinydrm_spi_max_transfer_size);
+
+/**
+ * tinydrm_spi_bpw_supported - Check if bits per word is supported
+ * @spi: SPI device
+ * @bpw: Bits per word
+ *
+ * This function checks to see if the SPI master driver supports @bpw.
+ *
+ * Returns:
+ * True if @bpw is supported, false otherwise.
+ */
+bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw)
+{
+	u32 bpw_mask = spi->master->bits_per_word_mask;
+
+	if (bpw == 8)
+		return true;
+
+	if (!bpw_mask) {
+		dev_warn_once(&spi->dev,
+			      "bits_per_word_mask not set, assume 8-bit only\n");
+		return false;
+	}
+
+	if (bpw_mask & SPI_BPW_MASK(bpw))
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(tinydrm_spi_bpw_supported);
+
+static void
+tinydrm_dbg_spi_print(struct spi_device *spi, struct spi_transfer *tr,
+		      const void *buf, int idx, bool tx)
+{
+	u32 speed_hz = tr->speed_hz ? tr->speed_hz : spi->max_speed_hz;
+	char linebuf[3 * 32];
+
+	hex_dump_to_buffer(buf, tr->len, 16,
+			   DIV_ROUND_UP(tr->bits_per_word, 8),
+			   linebuf, sizeof(linebuf), false);
+
+	printk(KERN_DEBUG
+	       "    tr(%i): speed=%u%s, bpw=%i, len=%u, %s_buf=[%s%s]\n", idx,
+	       speed_hz > 1000000 ? speed_hz / 1000000 : speed_hz / 1000,
+	       speed_hz > 1000000 ? "MHz" : "kHz", tr->bits_per_word, tr->len,
+	       tx ? "tx" : "rx", linebuf, tr->len > 16 ? " ..." : "");
+}
+
+/* called through tinydrm_dbg_spi_message() */
+void _tinydrm_dbg_spi_message(struct spi_device *spi, struct spi_message *m)
+{
+	struct spi_transfer *tmp;
+	struct list_head *pos;
+	int i = 0;
+
+	list_for_each(pos, &m->transfers) {
+		tmp = list_entry(pos, struct spi_transfer, transfer_list);
+
+		if (tmp->tx_buf)
+			tinydrm_dbg_spi_print(spi, tmp, tmp->tx_buf, i, true);
+		if (tmp->rx_buf)
+			tinydrm_dbg_spi_print(spi, tmp, tmp->rx_buf, i, false);
+		i++;
+	}
+}
+EXPORT_SYMBOL(_tinydrm_dbg_spi_message);
+
+/**
+ * tinydrm_spi_transfer - SPI transfer helper
+ * @spi: SPI device
+ * @speed_hz: Override speed (optional)
+ * @header: Optional header transfer
+ * @bpw: Bits per word
+ * @buf: Buffer to transfer
+ * @len: Buffer length
+ *
+ * This SPI transfer helper breaks up the transfer of @buf into chunks which
+ * the SPI master driver can handle. If the machine is Little Endian and the
+ * SPI master driver doesn't support 16 bits per word, it swaps the bytes and
+ * does a 8-bit transfer.
+ * If @header is set, it is prepended to each SPI message.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
+			 struct spi_transfer *header, u8 bpw, const void *buf,
+			 size_t len)
+{
+	struct spi_transfer tr = {
+		.bits_per_word = bpw,
+		.speed_hz = speed_hz,
+	};
+	struct spi_message m;
+	u16 *swap_buf = NULL;
+	size_t max_chunk;
+	size_t chunk;
+	int ret = 0;
+
+	if (WARN_ON_ONCE(bpw != 8 && bpw != 16))
+		return -EINVAL;
+
+	max_chunk = tinydrm_spi_max_transfer_size(spi, 0);
+
+	if (drm_debug & DRM_UT_DRIVER)
+		pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n",
+			 __func__, bpw, max_chunk);
+
+	if (bpw == 16 && !tinydrm_spi_bpw_supported(spi, 16)) {
+		tr.bits_per_word = 8;
+		if (tinydrm_machine_little_endian()) {
+			swap_buf = kmalloc(min(len, max_chunk), GFP_KERNEL);
+			if (!swap_buf)
+				return -ENOMEM;
+		}
+	}
+
+	spi_message_init(&m);
+	if (header)
+		spi_message_add_tail(header, &m);
+	spi_message_add_tail(&tr, &m);
+
+	while (len) {
+		chunk = min(len, max_chunk);
+
+		tr.tx_buf = buf;
+		tr.len = chunk;
+
+		if (swap_buf) {
+			const u16 *buf16 = buf;
+			unsigned int i;
+
+			for (i = 0; i < chunk / 2; i++)
+				swap_buf[i] = swab16(buf16[i]);
+
+			tr.tx_buf = swap_buf;
+		}
+
+		buf += chunk;
+		len -= chunk;
+
+		tinydrm_dbg_spi_message(spi, &m);
+		ret = spi_sync(spi, &m);
+		if (ret)
+			return ret;
+	};
+
+	return 0;
+}
+EXPORT_SYMBOL(tinydrm_spi_transfer);
+
+#endif /* CONFIG_SPI */
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
new file mode 100644
index 0000000..78175fe
--- /dev/null
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -0,0 +1,100 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TINYDRM_HELPERS_H
+#define __LINUX_TINYDRM_HELPERS_H
+
+struct backlight_device;
+struct tinydrm_device;
+struct drm_clip_rect;
+struct spi_transfer;
+struct spi_message;
+struct spi_device;
+struct device;
+
+/**
+ * tinydrm_machine_little_endian - Machine is little endian
+ *
+ * Returns:
+ * true if *defined(__LITTLE_ENDIAN)*, false otherwise
+ */
+static inline bool tinydrm_machine_little_endian(void)
+{
+#if defined(__LITTLE_ENDIAN)
+	return true;
+#else
+	return false;
+#endif
+}
+
+bool tinydrm_merge_clips(struct drm_clip_rect *dst,
+			 struct drm_clip_rect *src, unsigned int num_clips,
+			 unsigned int flags, u32 max_width, u32 max_height);
+void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb,
+		    struct drm_clip_rect *clip);
+void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
+		    struct drm_clip_rect *clip);
+void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
+				struct drm_framebuffer *fb,
+				struct drm_clip_rect *clip, bool swap);
+
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
+struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
+int tinydrm_enable_backlight(struct backlight_device *backlight);
+int tinydrm_disable_backlight(struct backlight_device *backlight);
+#else
+static inline struct backlight_device *
+tinydrm_of_find_backlight(struct device *dev)
+{
+	return NULL;
+}
+
+static inline int tinydrm_enable_backlight(struct backlight_device *backlight)
+{
+	return 0;
+}
+
+static inline int
+tinydrm_disable_backlight(struct backlight_device *backlight)
+{
+	return 0;
+}
+#endif
+
+size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
+bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
+int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
+			 struct spi_transfer *header, u8 bpw, const void *buf,
+			 size_t len);
+void _tinydrm_dbg_spi_message(struct spi_device *spi, struct spi_message *m);
+
+#ifdef DEBUG
+/**
+ * tinydrm_dbg_spi_message - Dump SPI message
+ * @spi: SPI device
+ * @m: SPI message
+ *
+ * Dumps info about the transfers in a SPI message including buffer content.
+ * DEBUG has to be defined for this function to be enabled alongside setting
+ * the DRM_UT_DRIVER bit of &drm_debug.
+ */
+static inline void tinydrm_dbg_spi_message(struct spi_device *spi,
+					   struct spi_message *m)
+{
+	if (drm_debug & DRM_UT_DRIVER)
+		_tinydrm_dbg_spi_message(spi, m);
+}
+#else
+static inline void tinydrm_dbg_spi_message(struct spi_device *spi,
+					   struct spi_message *m)
+{
+}
+#endif /* DEBUG */
+
+#endif /* __LINUX_TINYDRM_HELPERS_H */
-- 
2.10.2

[PATCH v3 3/7] drm/tinydrm: Add MIPI DBI support

[Noralf Trønnes]

Add support for MIPI DBI compatible controllers.
Interface type C option 1 and 3 are supported (SPI).

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/gpu/tinydrm.rst      |   12 +
 drivers/gpu/drm/tinydrm/Kconfig    |    3 +
 drivers/gpu/drm/tinydrm/Makefile   |    3 +
 drivers/gpu/drm/tinydrm/mipi-dbi.c | 1005 ++++++++++++++++++++++++++++++++++++
 include/drm/tinydrm/mipi-dbi.h     |  107 ++++
 5 files changed, 1130 insertions(+)
 create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
 create mode 100644 include/drm/tinydrm/mipi-dbi.h

diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst
index fb256d2..a913644 100644
--- a/Documentation/gpu/tinydrm.rst
+++ b/Documentation/gpu/tinydrm.rst
@@ -28,3 +28,15 @@ Additional helpers
 
 .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
    :export:
+
+MIPI DBI Compatible Controllers
+===============================
+
+.. kernel-doc:: drivers/gpu/drm/tinydrm/mipi-dbi.c
+   :doc: overview
+
+.. kernel-doc:: include/drm/tinydrm/mipi-dbi.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/tinydrm/mipi-dbi.c
+   :export:
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index ffb873f..128d2f3 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -6,3 +6,6 @@ menuconfig DRM_TINYDRM
 	help
 	  Choose this option if you have a tinydrm supported display.
 	  If M is selected the module will be called tinydrm.
+
+config TINYDRM_MIPI_DBI
+	tristate
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 7476ed1..fe5d4c6 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -1 +1,4 @@
 obj-$(CONFIG_DRM_TINYDRM)		+= core/
+
+# Controllers
+obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
new file mode 100644
index 0000000..5ded299
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -0,0 +1,1005 @@
+/*
+ * MIPI Display Bus Interface (DBI) LCD controller support
+ *
+ * Copyright 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <drm/tinydrm/mipi-dbi.h>
+#include <drm/tinydrm/tinydrm-helpers.h>
+#include <linux/debugfs.h>
+#include <linux/dma-buf.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <video/mipi_display.h>
+
+#define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */
+
+#define DCS_POWER_MODE_DISPLAY			BIT(2)
+#define DCS_POWER_MODE_DISPLAY_NORMAL_MODE	BIT(3)
+#define DCS_POWER_MODE_SLEEP_MODE		BIT(4)
+#define DCS_POWER_MODE_PARTIAL_MODE		BIT(5)
+#define DCS_POWER_MODE_IDLE_MODE		BIT(6)
+#define DCS_POWER_MODE_RESERVED_MASK		(BIT(0) | BIT(1) | BIT(7))
+
+/**
+ * DOC: overview
+ *
+ * This library provides helpers for MIPI Display Bus Interface (DBI)
+ * compatible display controllers.
+ *
+ * Many controllers for tiny lcd displays are MIPI compliant and can use this
+ * library. If a controller uses registers 0x2A and 0x2B to set the area to
+ * update and uses register 0x2C to write to frame memory, it is most likely
+ * MIPI compliant.
+ *
+ * Only MIPI Type 1 displays are supported since a full frame memory is needed.
+ *
+ * There are 3 MIPI DBI implementation types:
+ *
+ * A. Motorola 6800 type parallel bus
+ *
+ * B. Intel 8080 type parallel bus
+ *
+ * C. SPI type with 3 options:
+ *
+ *    1. 9-bit with the Data/Command signal as the ninth bit
+ *    2. Same as above except it's sent as 16 bits
+ *    3. 8-bit with the Data/Command signal as a separate D/CX pin
+ *
+ * Currently mipi_dbi only supports Type C options 1 and 3 with
+ * mipi_dbi_spi_init().
+ */
+
+#define MIPI_DBI_DEBUG_COMMAND(cmd, data, len) \
+({ \
+	if (!len) \
+		DRM_DEBUG_DRIVER("cmd=%02x\n", cmd); \
+	else if (len <= 32) \
+		DRM_DEBUG_DRIVER("cmd=%02x, par=%*ph\n", cmd, len, data); \
+	else \
+		DRM_DEBUG_DRIVER("cmd=%02x, len=%zu\n", cmd, len); \
+})
+
+static const u8 mipi_dbi_dcs_read_commands[] = {
+	MIPI_DCS_GET_DISPLAY_ID,
+	MIPI_DCS_GET_RED_CHANNEL,
+	MIPI_DCS_GET_GREEN_CHANNEL,
+	MIPI_DCS_GET_BLUE_CHANNEL,
+	MIPI_DCS_GET_DISPLAY_STATUS,
+	MIPI_DCS_GET_POWER_MODE,
+	MIPI_DCS_GET_ADDRESS_MODE,
+	MIPI_DCS_GET_PIXEL_FORMAT,
+	MIPI_DCS_GET_DISPLAY_MODE,
+	MIPI_DCS_GET_SIGNAL_MODE,
+	MIPI_DCS_GET_DIAGNOSTIC_RESULT,
+	MIPI_DCS_READ_MEMORY_START,
+	MIPI_DCS_READ_MEMORY_CONTINUE,
+	MIPI_DCS_GET_SCANLINE,
+	MIPI_DCS_GET_DISPLAY_BRIGHTNESS,
+	MIPI_DCS_GET_CONTROL_DISPLAY,
+	MIPI_DCS_GET_POWER_SAVE,
+	MIPI_DCS_GET_CABC_MIN_BRIGHTNESS,
+	MIPI_DCS_READ_DDB_START,
+	MIPI_DCS_READ_DDB_CONTINUE,
+	0, /* sentinel */
+};
+
+static bool mipi_dbi_command_is_read(struct mipi_dbi *mipi, u8 cmd)
+{
+	unsigned int i;
+
+	if (!mipi->read_commands)
+		return false;
+
+	for (i = 0; i < 0xff; i++) {
+		if (!mipi->read_commands[i])
+			return false;
+		if (cmd == mipi->read_commands[i])
+			return true;
+	}
+
+	return false;
+}
+
+/**
+ * mipi_dbi_command_read - MIPI DCS read command
+ * @mipi: MIPI structure
+ * @cmd: Command
+ * @val: Value read
+ *
+ * Send MIPI DCS read command to the controller.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val)
+{
+	if (!mipi->read_commands)
+		return -EACCES;
+
+	if (!mipi_dbi_command_is_read(mipi, cmd))
+		return -EINVAL;
+
+	return mipi_dbi_command_buf(mipi, cmd, val, 1);
+}
+EXPORT_SYMBOL(mipi_dbi_command_read);
+
+/**
+ * mipi_dbi_command_buf - MIPI DCS command with parameter(s) in an array
+ * @mipi: MIPI structure
+ * @cmd: Command
+ * @data: Parameter buffer
+ * @len: Buffer length
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
+{
+	int ret;
+
+	mutex_lock(&mipi->cmdlock);
+	ret = mipi->command(mipi, cmd, data, len);
+	mutex_unlock(&mipi->cmdlock);
+
+	return ret;
+}
+EXPORT_SYMBOL(mipi_dbi_command_buf);
+
+static int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
+				struct drm_clip_rect *clip, bool swap)
+{
+	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
+	struct drm_format_name_buf format_name;
+	void *src = cma_obj->vaddr;
+	int ret = 0;
+
+	if (import_attach) {
+		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
+					       DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
+	}
+
+	switch (fb->format->format) {
+	case DRM_FORMAT_RGB565:
+		if (swap)
+			tinydrm_swab16(dst, src, fb, clip);
+		else
+			tinydrm_memcpy(dst, src, fb, clip);
+		break;
+	case DRM_FORMAT_XRGB8888:
+		tinydrm_xrgb8888_to_rgb565(dst, src, fb, clip, swap);
+		break;
+	default:
+		dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
+			     drm_get_format_name(fb->format->format,
+						 &format_name));
+		return -EINVAL;
+	}
+
+	if (import_attach)
+		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
+					     DMA_FROM_DEVICE);
+	return ret;
+}
+
+static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
+			     struct drm_file *file_priv,
+			     unsigned int flags, unsigned int color,
+			     struct drm_clip_rect *clips,
+			     unsigned int num_clips)
+{
+	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	struct tinydrm_device *tdev = fb->dev->dev_private;
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	bool swap = mipi->swap_bytes;
+	struct drm_clip_rect clip;
+	int ret = 0;
+	bool full;
+	void *tr;
+
+	mutex_lock(&tdev->dirty_lock);
+
+	if (!mipi->enabled)
+		goto out_unlock;
+
+	/* fbdev can flush even when we're not interested */
+	if (tdev->pipe.plane.fb != fb)
+		goto out_unlock;
+
+	full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
+				   fb->width, fb->height);
+
+	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
+		  clip.x1, clip.x2, clip.y1, clip.y2);
+
+	if (!mipi->dc || !full || swap ||
+	    fb->format->format == DRM_FORMAT_XRGB8888) {
+		tr = mipi->tx_buf;
+		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap);
+		if (ret)
+			goto out_unlock;
+	} else {
+		tr = cma_obj->vaddr;
+	}
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
+			 (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF,
+			 (clip.x2 >> 8) & 0xFF, (clip.x2 - 1) & 0xFF);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
+			 (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
+			 (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
+
+	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, tr,
+				(clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2);
+
+out_unlock:
+	mutex_unlock(&tdev->dirty_lock);
+
+	if (ret)
+		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
+			     ret);
+
+	return ret;
+}
+
+static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = {
+	.destroy	= drm_fb_cma_destroy,
+	.create_handle	= drm_fb_cma_create_handle,
+	.dirty		= mipi_dbi_fb_dirty,
+};
+
+/**
+ * mipi_dbi_pipe_enable - MIPI DBI pipe enable helper
+ * @pipe: Display pipe
+ * @crtc_state: CRTC state
+ *
+ * This function enables backlight. Drivers can use this as their
+ * &drm_simple_display_pipe_funcs->enable callback.
+ */
+void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
+			  struct drm_crtc_state *crtc_state)
+{
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	struct drm_framebuffer *fb = pipe->plane.fb;
+
+	DRM_DEBUG_KMS("\n");
+
+	mipi->enabled = true;
+	if (fb)
+		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+
+	tinydrm_enable_backlight(mipi->backlight);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_enable);
+
+static void mipi_dbi_blank(struct mipi_dbi *mipi)
+{
+	struct drm_device *drm = mipi->tinydrm.drm;
+	u16 height = drm->mode_config.min_height;
+	u16 width = drm->mode_config.min_width;
+	size_t len = width * height * 2;
+
+	memset(mipi->tx_buf, 0, len);
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS, 0, 0,
+			 (width >> 8) & 0xFF, (width - 1) & 0xFF);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS, 0, 0,
+			 (height >> 8) & 0xFF, (height - 1) & 0xFF);
+	mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
+			     (u8 *)mipi->tx_buf, len);
+}
+
+/**
+ * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper
+ * @pipe: Display pipe
+ *
+ * This function disables backlight if present or if not the
+ * display memory is blanked. Drivers can use this as their
+ * &drm_simple_display_pipe_funcs->disable callback.
+ */
+void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+
+	DRM_DEBUG_KMS("\n");
+
+	mipi->enabled = false;
+
+	if (mipi->backlight)
+		tinydrm_disable_backlight(mipi->backlight);
+	else
+		mipi_dbi_blank(mipi);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_disable);
+
+static const uint32_t mipi_dbi_formats[] = {
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+};
+
+/**
+ * mipi_dbi_init - MIPI DBI initialization
+ * @dev: Parent device
+ * @mipi: &mipi_dbi structure to initialize
+ * @pipe_funcs: Display pipe functions
+ * @driver: DRM driver
+ * @mode: Display mode
+ * @rotation: Initial rotation in degrees Counter Clock Wise
+ *
+ * This function initializes a &mipi_dbi structure and it's underlying
+ * @tinydrm_device. It also sets up the display pipeline.
+ *
+ * Supported formats: Native RGB565 and emulated XRGB8888.
+ *
+ * Objects created by this function will be automatically freed on driver
+ * detach (devres).
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
+		  const struct drm_simple_display_pipe_funcs *pipe_funcs,
+		  struct drm_driver *driver,
+		  const struct drm_display_mode *mode, unsigned int rotation)
+{
+	size_t bufsize = mode->vdisplay * mode->hdisplay * sizeof(u16);
+	struct tinydrm_device *tdev = &mipi->tinydrm;
+	int ret;
+
+	if (!mipi->command)
+		return -EINVAL;
+
+	mutex_init(&mipi->cmdlock);
+
+	mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL);
+	if (!mipi->tx_buf)
+		return -ENOMEM;
+
+	ret = devm_tinydrm_init(dev, tdev, &mipi_dbi_fb_funcs, driver);
+	if (ret)
+		return ret;
+
+	/* TODO: Maybe add DRM_MODE_CONNECTOR_SPI */
+	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
+					DRM_MODE_CONNECTOR_VIRTUAL,
+					mipi_dbi_formats,
+					ARRAY_SIZE(mipi_dbi_formats), mode,
+					rotation);
+	if (ret)
+		return ret;
+
+	tdev->drm->mode_config.preferred_depth = 16;
+	mipi->rotation = rotation;
+
+	drm_mode_config_reset(tdev->drm);
+
+	DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",
+		      tdev->drm->mode_config.preferred_depth, rotation);
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dbi_init);
+
+/**
+ * mipi_dbi_hw_reset - Hardware reset of controller
+ * @mipi: MIPI DBI structure
+ *
+ * Reset controller if the &mipi_dbi->reset gpio is set.
+ */
+void mipi_dbi_hw_reset(struct mipi_dbi *mipi)
+{
+	if (!mipi->reset)
+		return;
+
+	gpiod_set_value_cansleep(mipi->reset, 0);
+	msleep(20);
+	gpiod_set_value_cansleep(mipi->reset, 1);
+	msleep(120);
+}
+EXPORT_SYMBOL(mipi_dbi_hw_reset);
+
+/**
+ * mipi_dbi_display_is_on - Check if display is on
+ * @mipi: MIPI DBI structure
+ *
+ * This function checks the Power Mode register (if readable) to see if
+ * display output is turned on. This can be used to see if the bootloader
+ * has already turned on the display avoiding flicker when the pipeline is
+ * enabled.
+ *
+ * Returns:
+ * true if the display can be verified to be on, false otherwise.
+ */
+bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
+{
+	u8 val;
+
+	if (mipi_dbi_command_read(mipi, MIPI_DCS_GET_POWER_MODE, &val))
+		return false;
+
+	val &= ~DCS_POWER_MODE_RESERVED_MASK;
+
+	if (val != (DCS_POWER_MODE_DISPLAY |
+	    DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE))
+		return false;
+
+	DRM_DEBUG_DRIVER("Display is ON\n");
+
+	return true;
+}
+EXPORT_SYMBOL(mipi_dbi_display_is_on);
+
+#ifdef CONFIG_SPI
+
+/*
+ * Many controllers have a max speed of 10MHz, but can be pushed way beyond
+ * that. Increase reliability by running pixel data at max speed and the rest
+ * at 10MHz, preventing transfer glitches from messing up the init settings.
+ */
+static u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len)
+{
+	if (len > 64)
+		return 0; /* use default */
+
+	return min_t(u32, 10000000, spi->max_speed_hz);
+}
+
+/*
+ * MIPI DBI Type C Option 1
+ *
+ * If the SPI controller doesn't have 9 bits per word support,
+ * use blocks of 9 bytes to send 8x 9-bit words using a 8-bit SPI transfer.
+ * Pad partial blocks with MIPI_DCS_NOP (zero).
+ * This is how the D/C bit (x) is added:
+ *     x7654321
+ *     0x765432
+ *     10x76543
+ *     210x7654
+ *     3210x765
+ *     43210x76
+ *     543210x7
+ *     6543210x
+ *     76543210
+ */
+
+static int mipi_dbi_spi1e_transfer(struct mipi_dbi *mipi, int dc,
+				   const void *buf, size_t len,
+				   unsigned int bpw)
+{
+	bool swap_bytes = (bpw == 16 && tinydrm_machine_little_endian());
+	size_t chunk, max_chunk = mipi->tx_buf9_len;
+	struct spi_device *spi = mipi->spi;
+	struct spi_transfer tr = {
+		.tx_buf = mipi->tx_buf9,
+		.bits_per_word = 8,
+	};
+	struct spi_message m;
+	const u8 *src = buf;
+	int i, ret;
+	u8 *dst;
+
+	if (drm_debug & DRM_UT_DRIVER)
+		pr_debug("[drm:%s] dc=%d, max_chunk=%zu, transfers:\n",
+			 __func__, dc, max_chunk);
+
+	tr.speed_hz = mipi_dbi_spi_cmd_max_speed(spi, len);
+	spi_message_init_with_transfers(&m, &tr, 1);
+
+	if (!dc) {
+		if (WARN_ON_ONCE(len != 1))
+			return -EINVAL;
+
+		/* Command: pad no-op's (zeroes) at beginning of block */
+		dst = mipi->tx_buf9;
+		memset(dst, 0, 9);
+		dst[8] = *src;
+		tr.len = 9;
+
+		tinydrm_dbg_spi_message(spi, &m);
+
+		return spi_sync(spi, &m);
+	}
+
+	/* max with room for adding one bit per byte */
+	max_chunk = max_chunk / 9 * 8;
+	/* but no bigger than len */
+	max_chunk = min(max_chunk, len);
+	/* 8 byte blocks */
+	max_chunk = max_t(size_t, 8, max_chunk & ~0x7);
+
+	while (len) {
+		size_t added = 0;
+
+		chunk = min(len, max_chunk);
+		len -= chunk;
+		dst = mipi->tx_buf9;
+
+		if (chunk < 8) {
+			u8 val, carry = 0;
+
+			/* Data: pad no-op's (zeroes) at end of block */
+			memset(dst, 0, 9);
+
+			if (swap_bytes) {
+				for (i = 1; i < (chunk + 1); i++) {
+					val = src[1];
+					*dst++ = carry | BIT(8 - i) | (val >> i);
+					carry = val << (8 - i);
+					i++;
+					val = src[0];
+					*dst++ = carry | BIT(8 - i) | (val >> i);
+					carry = val << (8 - i);
+					src += 2;
+				}
+				*dst++ = carry;
+			} else {
+				for (i = 1; i < (chunk + 1); i++) {
+					val = *src++;
+					*dst++ = carry | BIT(8 - i) | (val >> i);
+					carry = val << (8 - i);
+				}
+				*dst++ = carry;
+			}
+
+			chunk = 8;
+			added = 1;
+		} else {
+			for (i = 0; i < chunk; i += 8) {
+				if (swap_bytes) {
+					*dst++ =                 BIT(7) | (src[1] >> 1);
+					*dst++ = (src[1] << 7) | BIT(6) | (src[0] >> 2);
+					*dst++ = (src[0] << 6) | BIT(5) | (src[3] >> 3);
+					*dst++ = (src[3] << 5) | BIT(4) | (src[2] >> 4);
+					*dst++ = (src[2] << 4) | BIT(3) | (src[5] >> 5);
+					*dst++ = (src[5] << 3) | BIT(2) | (src[4] >> 6);
+					*dst++ = (src[4] << 2) | BIT(1) | (src[7] >> 7);
+					*dst++ = (src[7] << 1) | BIT(0);
+					*dst++ = src[6];
+				} else {
+					*dst++ =                 BIT(7) | (src[0] >> 1);
+					*dst++ = (src[0] << 7) | BIT(6) | (src[1] >> 2);
+					*dst++ = (src[1] << 6) | BIT(5) | (src[2] >> 3);
+					*dst++ = (src[2] << 5) | BIT(4) | (src[3] >> 4);
+					*dst++ = (src[3] << 4) | BIT(3) | (src[4] >> 5);
+					*dst++ = (src[4] << 3) | BIT(2) | (src[5] >> 6);
+					*dst++ = (src[5] << 2) | BIT(1) | (src[6] >> 7);
+					*dst++ = (src[6] << 1) | BIT(0);
+					*dst++ = src[7];
+				}
+
+				src += 8;
+				added++;
+			}
+		}
+
+		tr.len = chunk + added;
+
+		tinydrm_dbg_spi_message(spi, &m);
+		ret = spi_sync(spi, &m);
+		if (ret)
+			return ret;
+	};
+
+	return 0;
+}
+
+static int mipi_dbi_spi1_transfer(struct mipi_dbi *mipi, int dc,
+				  const void *buf, size_t len,
+				  unsigned int bpw)
+{
+	struct spi_device *spi = mipi->spi;
+	struct spi_transfer tr = {
+		.bits_per_word = 9,
+	};
+	const u16 *src16 = buf;
+	const u8 *src8 = buf;
+	struct spi_message m;
+	size_t max_chunk;
+	u16 *dst16;
+	int ret;
+
+	if (!tinydrm_spi_bpw_supported(spi, 9))
+		return mipi_dbi_spi1e_transfer(mipi, dc, buf, len, bpw);
+
+	tr.speed_hz = mipi_dbi_spi_cmd_max_speed(spi, len);
+	max_chunk = mipi->tx_buf9_len;
+	dst16 = mipi->tx_buf9;
+
+	if (drm_debug & DRM_UT_DRIVER)
+		pr_debug("[drm:%s] dc=%d, max_chunk=%zu, transfers:\n",
+			 __func__, dc, max_chunk);
+
+	max_chunk = min(max_chunk / 2, len);
+
+	spi_message_init_with_transfers(&m, &tr, 1);
+	tr.tx_buf = dst16;
+
+	while (len) {
+		size_t chunk = min(len, max_chunk);
+		unsigned int i;
+
+		if (bpw == 16 && tinydrm_machine_little_endian()) {
+			for (i = 0; i < (chunk * 2); i += 2) {
+				dst16[i]     = *src16 >> 8;
+				dst16[i + 1] = *src16++ & 0xFF;
+				if (dc) {
+					dst16[i]     |= 0x0100;
+					dst16[i + 1] |= 0x0100;
+				}
+			}
+		} else {
+			for (i = 0; i < chunk; i++) {
+				dst16[i] = *src8++;
+				if (dc)
+					dst16[i] |= 0x0100;
+			}
+		}
+
+		tr.len = chunk;
+		len -= chunk;
+
+		tinydrm_dbg_spi_message(spi, &m);
+		ret = spi_sync(spi, &m);
+		if (ret)
+			return ret;
+	};
+
+	return 0;
+}
+
+static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
+				   u8 *parameters, size_t num)
+{
+	unsigned int bpw = (cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
+	int ret;
+
+	if (mipi_dbi_command_is_read(mipi, cmd))
+		return -ENOTSUPP;
+
+	MIPI_DBI_DEBUG_COMMAND(cmd, parameters, num);
+
+	ret = mipi_dbi_spi1_transfer(mipi, 0, &cmd, 1, 8);
+	if (ret || !num)
+		return ret;
+
+	return mipi_dbi_spi1_transfer(mipi, 1, parameters, num, bpw);
+}
+
+/* MIPI DBI Type C Option 3 */
+
+static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
+					u8 *data, size_t len)
+{
+	struct spi_device *spi = mipi->spi;
+	u32 speed_hz = min_t(u32, MIPI_DBI_MAX_SPI_READ_SPEED,
+			     spi->max_speed_hz / 2);
+	struct spi_transfer tr[2] = {
+		{
+			.speed_hz = speed_hz,
+			.tx_buf = &cmd,
+			.len = 1,
+		}, {
+			.speed_hz = speed_hz,
+			.len = len,
+		},
+	};
+	struct spi_message m;
+	u8 *buf;
+	int ret;
+
+	if (!len)
+		return -EINVAL;
+
+	/*
+	 * Support non-standard 24-bit and 32-bit Nokia read commands which
+	 * start with a dummy clock, so we need to read an extra byte.
+	 */
+	if (cmd == MIPI_DCS_GET_DISPLAY_ID ||
+	    cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
+		if (!(len == 3 || len == 4))
+			return -EINVAL;
+
+		tr[1].len = len + 1;
+	}
+
+	buf = kmalloc(tr[1].len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	tr[1].rx_buf = buf;
+	gpiod_set_value_cansleep(mipi->dc, 0);
+
+	spi_message_init_with_transfers(&m, tr, ARRAY_SIZE(tr));
+	ret = spi_sync(spi, &m);
+	if (ret)
+		goto err_free;
+
+	tinydrm_dbg_spi_message(spi, &m);
+
+	if (tr[1].len == len) {
+		memcpy(data, buf, len);
+	} else {
+		unsigned int i;
+
+		for (i = 0; i < len; i++)
+			data[i] = (buf[i] << 1) | !!(buf[i + 1] & BIT(7));
+	}
+
+	MIPI_DBI_DEBUG_COMMAND(cmd, data, len);
+
+err_free:
+	kfree(buf);
+
+	return ret;
+}
+
+static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
+				   u8 *par, size_t num)
+{
+	struct spi_device *spi = mipi->spi;
+	unsigned int bpw = 8;
+	u32 speed_hz;
+	int ret;
+
+	if (mipi_dbi_command_is_read(mipi, cmd))
+		return mipi_dbi_typec3_command_read(mipi, cmd, par, num);
+
+	MIPI_DBI_DEBUG_COMMAND(cmd, par, num);
+
+	gpiod_set_value_cansleep(mipi->dc, 0);
+	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
+	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
+	if (ret || !num)
+		return ret;
+
+	if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
+		bpw = 16;
+
+	gpiod_set_value_cansleep(mipi->dc, 1);
+	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
+
+	return tinydrm_spi_transfer(spi, speed_hz, NULL, bpw, par, num);
+}
+
+/**
+ * mipi_dbi_spi_init - Initialize MIPI DBI SPI interfaced controller
+ * @spi: SPI device
+ * @dc: D/C gpio (optional)
+ * @mipi: &mipi_dbi structure to initialize
+ * @pipe_funcs: Display pipe functions
+ * @driver: DRM driver
+ * @mode: Display mode
+ * @rotation: Initial rotation in degrees Counter Clock Wise
+ *
+ * This function sets &mipi_dbi->command, enables &mipi->read_commands for the
+ * usual read commands and initializes @mipi using mipi_dbi_init().
+ *
+ * If @dc is set, a Type C Option 3 interface is assumed, if not
+ * Type C Option 1.
+ *
+ * If the SPI master driver doesn't support the necessary bits per word,
+ * the following transformation is used:
+ *
+ * - 9-bit: reorder buffer as 9x 8-bit words, padded with no-op command.
+ * - 16-bit: if big endian send as 8-bit, if little endian swap bytes
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
+		      struct gpio_desc *dc,
+		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
+		      struct drm_driver *driver,
+		      const struct drm_display_mode *mode,
+		      unsigned int rotation)
+{
+	size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0);
+	struct device *dev = &spi->dev;
+	int ret;
+
+	if (tx_size < 16) {
+		DRM_ERROR("SPI transmit buffer too small: %zu\n", tx_size);
+		return -EINVAL;
+	}
+
+	/*
+	 * Even though it's not the SPI device that does DMA (the master does),
+	 * the dma mask is necessary for the dma_alloc_wc() in
+	 * drm_gem_cma_create(). The dma_addr returned will be a physical
+	 * adddress which might be different from the bus address, but this is
+	 * not a problem since the address will not be used.
+	 * The virtual address is used in the transfer and the SPI core
+	 * re-maps it on the SPI master device using the DMA streaming API
+	 * (spi_map_buf()).
+	 */
+	if (!dev->coherent_dma_mask) {
+		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
+		if (ret) {
+			dev_warn(dev, "Failed to set dma mask %d\n", ret);
+			return ret;
+		}
+	}
+
+	mipi->spi = spi;
+	mipi->read_commands = mipi_dbi_dcs_read_commands;
+
+	if (dc) {
+		mipi->command = mipi_dbi_typec3_command;
+		mipi->dc = dc;
+		if (tinydrm_machine_little_endian() &&
+		    !tinydrm_spi_bpw_supported(spi, 16))
+			mipi->swap_bytes = true;
+	} else {
+		mipi->command = mipi_dbi_typec1_command;
+		mipi->tx_buf9_len = tx_size;
+		mipi->tx_buf9 = devm_kmalloc(dev, tx_size, GFP_KERNEL);
+		if (!mipi->tx_buf9)
+			return -ENOMEM;
+	}
+
+	return mipi_dbi_init(dev, mipi, pipe_funcs, driver, mode, rotation);
+}
+EXPORT_SYMBOL(mipi_dbi_spi_init);
+
+#endif /* CONFIG_SPI */
+
+#ifdef CONFIG_DEBUG_FS
+
+static ssize_t mipi_dbi_debugfs_command_write(struct file *file,
+					      const char __user *ubuf,
+					      size_t count, loff_t *ppos)
+{
+	struct seq_file *m = file->private_data;
+	struct mipi_dbi *mipi = m->private;
+	u8 val, cmd, parameters[64];
+	char *buf, *pos, *token;
+	unsigned int i;
+	int ret;
+
+	buf = memdup_user_nul(ubuf, count);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	/* strip trailing whitespace */
+	for (i = count - 1; i > 0; i--)
+		if (isspace(buf[i]))
+			buf[i] = '\0';
+		else
+			break;
+	i = 0;
+	pos = buf;
+	while (pos) {
+		token = strsep(&pos, " ");
+		if (!token) {
+			ret = -EINVAL;
+			goto err_free;
+		}
+
+		ret = kstrtou8(token, 16, &val);
+		if (ret < 0)
+			goto err_free;
+
+		if (token == buf)
+			cmd = val;
+		else
+			parameters[i++] = val;
+
+		if (i == 64) {
+			ret = -E2BIG;
+			goto err_free;
+		}
+	}
+
+	ret = mipi_dbi_command_buf(mipi, cmd, parameters, i);
+
+err_free:
+	kfree(buf);
+
+	return ret < 0 ? ret : count;
+}
+
+static int mipi_dbi_debugfs_command_show(struct seq_file *m, void *unused)
+{
+	struct mipi_dbi *mipi = m->private;
+	u8 cmd, val[4];
+	size_t len, i;
+	int ret;
+
+	for (cmd = 0; cmd < 255; cmd++) {
+		if (!mipi_dbi_command_is_read(mipi, cmd))
+			continue;
+
+		switch (cmd) {
+		case MIPI_DCS_READ_MEMORY_START:
+		case MIPI_DCS_READ_MEMORY_CONTINUE:
+			len = 2;
+			break;
+		case MIPI_DCS_GET_DISPLAY_ID:
+			len = 3;
+			break;
+		case MIPI_DCS_GET_DISPLAY_STATUS:
+			len = 4;
+			break;
+		default:
+			len = 1;
+			break;
+		}
+
+		seq_printf(m, "%02x: ", cmd);
+		ret = mipi_dbi_command_buf(mipi, cmd, val, len);
+		if (ret) {
+			seq_puts(m, "XX\n");
+			continue;
+		}
+
+		for (i = 0; i < len; i++)
+			seq_printf(m, "%02x", val[i]);
+		seq_puts(m, "\n");
+	}
+
+	return 0;
+}
+
+static int mipi_dbi_debugfs_command_open(struct inode *inode,
+					 struct file *file)
+{
+	return single_open(file, mipi_dbi_debugfs_command_show,
+			   inode->i_private);
+}
+
+static const struct file_operations mipi_dbi_debugfs_command_fops = {
+	.owner = THIS_MODULE,
+	.open = mipi_dbi_debugfs_command_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.write = mipi_dbi_debugfs_command_write,
+};
+
+static const struct drm_info_list mipi_dbi_debugfs_list[] = {
+	{ "fb",   drm_fb_cma_debugfs_show, 0 },
+};
+
+/**
+ * mipi_dbi_debugfs_init - Create debugfs entries
+ * @minor: DRM minor
+ *
+ * This function creates a 'command' debugfs file for sending commands to the
+ * controller or getting the read command values.
+ * Drivers can use this as their &drm_driver->debugfs_init callback.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_debugfs_init(struct drm_minor *minor)
+{
+	struct tinydrm_device *tdev = minor->dev->dev_private;
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	umode_t mode = S_IFREG | S_IWUSR;
+
+	if (mipi->read_commands)
+		mode |= S_IRUGO;
+	debugfs_create_file("command", mode, minor->debugfs_root, mipi,
+			    &mipi_dbi_debugfs_command_fops);
+
+	return drm_debugfs_create_files(mipi_dbi_debugfs_list,
+					ARRAY_SIZE(mipi_dbi_debugfs_list),
+					minor->debugfs_root, minor);
+}
+EXPORT_SYMBOL(mipi_dbi_debugfs_init);
+
+#endif
+
+MODULE_LICENSE("GPL");
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
new file mode 100644
index 0000000..d137b16
--- /dev/null
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -0,0 +1,107 @@
+/*
+ * MIPI Display Bus Interface (DBI) LCD controller support
+ *
+ * Copyright 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_MIPI_DBI_H
+#define __LINUX_MIPI_DBI_H
+
+#include <drm/tinydrm/tinydrm.h>
+
+struct spi_device;
+struct gpio_desc;
+struct regulator;
+
+/**
+ * struct mipi_dbi - MIPI DBI controller
+ * @tinydrm: tinydrm base
+ * @spi: SPI device
+ * @enabled: Pipeline is enabled
+ * @cmdlock: Command lock
+ * @command: Bus specific callback executing commands.
+ * @read_commands: Array of read commands terminated by a zero entry.
+ *                 Reading is disabled if this is NULL.
+ * @dc: Optional D/C gpio.
+ * @tx_buf: Buffer used for transfer (copy clip rect area)
+ * @tx_buf9: Buffer used for Option 1 9-bit conversion
+ * @tx_buf9_len: Size of tx_buf9.
+ * @swap_bytes: Swap bytes in buffer before transfer
+ * @reset: Optional reset gpio
+ * @rotation: initial rotation in degrees Counter Clock Wise
+ * @backlight: backlight device (optional)
+ * @regulator: power regulator (optional)
+ */
+struct mipi_dbi {
+	struct tinydrm_device tinydrm;
+	struct spi_device *spi;
+	bool enabled;
+	struct mutex cmdlock;
+	int (*command)(struct mipi_dbi *mipi, u8 cmd, u8 *param, size_t num);
+	const u8 *read_commands;
+	struct gpio_desc *dc;
+	u16 *tx_buf;
+	void *tx_buf9;
+	size_t tx_buf9_len;
+	bool swap_bytes;
+	struct gpio_desc *reset;
+	unsigned int rotation;
+	struct backlight_device *backlight;
+	struct regulator *regulator;
+};
+
+static inline struct mipi_dbi *
+mipi_dbi_from_tinydrm(struct tinydrm_device *tdev)
+{
+	return container_of(tdev, struct mipi_dbi, tinydrm);
+}
+
+int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
+		      struct gpio_desc *dc,
+		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
+		      struct drm_driver *driver,
+		      const struct drm_display_mode *mode,
+		      unsigned int rotation);
+int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
+		  const struct drm_simple_display_pipe_funcs *pipe_funcs,
+		  struct drm_driver *driver,
+		  const struct drm_display_mode *mode, unsigned int rotation);
+void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
+			  struct drm_crtc_state *crtc_state);
+void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
+void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
+bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
+
+int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
+int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
+
+/**
+ * mipi_dbi_command - MIPI DCS command with optional parameter(s)
+ * @mipi: MIPI structure
+ * @cmd: Command
+ * @seq...: Optional parameter(s)
+ *
+ * Send MIPI DCS command to the controller. Use mipi_dbi_command_read() for
+ * get/read.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+#define mipi_dbi_command(mipi, cmd, seq...) \
+({ \
+	u8 d[] = { seq }; \
+	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
+})
+
+#ifdef CONFIG_DEBUG_FS
+int mipi_dbi_debugfs_init(struct drm_minor *minor);
+#else
+#define mipi_dbi_debugfs_init		NULL
+#endif
+
+#endif /* __LINUX_MIPI_DBI_H */
-- 
2.10.2

[PATCH v3 4/7] of: Add vendor prefix for Multi-Inno

[Noralf Trønnes]

Multi-Inno Technology Co.,Ltd is a Hong Kong based company offering
LCD, LCD module products and complete panel solutions.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 11c7e5c..6879193 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -193,6 +193,7 @@ mpl	MPL AG
 mqmaker	mqmaker Inc.
 msi	Micro-Star International Co. Ltd.
 mti	Imagination Technologies Ltd. (formerly MIPS Technologies Inc.)
+multi-inno	Multi-Inno Technology Co.,Ltd
 mundoreader	Mundo Reader S.L.
 murata	Murata Manufacturing Co., Ltd.
 mxicy	Macronix International Co., Ltd.
-- 
2.10.2

[PATCH v3 5/7] dt-bindings: display: Add common rotation property

[Noralf Trønnes]

Display panels can be oriented many ways, especially in the embedded
world. The rotation property is a way to describe this orientation.
The counter clockwise direction is chosen because that's what fbdev
and drm use.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/devicetree/bindings/display/display.txt | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/display.txt

diff --git a/Documentation/devicetree/bindings/display/display.txt b/Documentation/devicetree/bindings/display/display.txt
new file mode 100644
index 0000000..e2e6867
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/display.txt
@@ -0,0 +1,4 @@
+Common display properties
+-------------------------
+
+- rotation:	Display rotation in degrees counter clockwise (0,90,180,270)
-- 
2.10.2

[PATCH v3 6/7] dt-bindings: Add Multi-Inno MI0283QT binding

[Noralf Trønnes]

Add device-tree binding documentation for the MI0283QT display panel.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---

Datasheet: https://cdn-shop.adafruit.com/datasheets/MI0283QT-11+V1.1.PDF

 .../bindings/display/multi-inno,mi0283qt.txt       | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt

diff --git a/Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt b/Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
new file mode 100644
index 0000000..eed48c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
@@ -0,0 +1,27 @@
+Multi-Inno MI0283QT display panel
+
+Required properties:
+- compatible:	"multi-inno,mi0283qt".
+
+The node for this driver must be a child node of a SPI controller, hence
+all mandatory properties described in ../spi/spi-bus.txt must be specified.
+
+Optional properties:
+- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
+		the panel interface mode (IM[3:0] pins):
+		- present: IM=x110 4-wire 8-bit data serial interface
+		- absent:  IM=x101 3-wire 9-bit data serial interface
+- reset-gpios:	Reset pin
+- power-supply:	A regulator node for the supply voltage.
+- backlight:	phandle of the backlight device attached to the panel
+- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
+
+Example:
+	mi0283qt@0{
+		compatible = "multi-inno,mi0283qt";
+		reg = <0>;
+		spi-max-frequency = <32000000>;
+		rotation = <90>;
+		dc-gpios = <&gpio 25 0>;
+		backlight = <&backlight>;
+	};
--
2.10.2

[PATCH v3 7/7] drm/tinydrm: Add support for Multi-Inno MI0283QT display

[Noralf Trønnes]

Add driver to support the Multi-Inno MI0283QT display panel.
It has an ILI9341 MIPI DBI compatible display controller.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 MAINTAINERS                        |   6 +
 drivers/gpu/drm/tinydrm/Kconfig    |   8 ++
 drivers/gpu/drm/tinydrm/Makefile   |   3 +
 drivers/gpu/drm/tinydrm/mi0283qt.c | 279 +++++++++++++++++++++++++++++++++++++
 include/drm/tinydrm/ili9341.h      |  54 +++++++
 5 files changed, 350 insertions(+)
 create mode 100644 drivers/gpu/drm/tinydrm/mi0283qt.c
 create mode 100644 include/drm/tinydrm/ili9341.h

diff --git a/MAINTAINERS b/MAINTAINERS
index d935135..5c4666b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4278,6 +4278,12 @@ S:	Supported
 F:	drivers/gpu/drm/mediatek/
 F:	Documentation/devicetree/bindings/display/mediatek/
 
+DRM DRIVER FOR MI0283QT
+M:	Noralf Trønnes <noralf@tronnes.org>
+S:	Maintained
+F:	drivers/gpu/drm/tinydrm/mi0283qt.c
+F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
+
 DRM DRIVER FOR MSM ADRENO GPU
 M:	Rob Clark <robdclark@gmail.com>
 L:	linux-arm-msm@vger.kernel.org
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index 128d2f3..d3161fd 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -9,3 +9,11 @@ menuconfig DRM_TINYDRM
 
 config TINYDRM_MIPI_DBI
 	tristate
+
+config TINYDRM_MI0283QT
+	tristate "DRM support for MI0283QT"
+	depends on DRM_TINYDRM && SPI
+	select TINYDRM_MIPI_DBI
+	help
+	  DRM driver for the Multi-Inno MI0283QT display panel
+	  If M is selected the module will be called mi0283qt.
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index fe5d4c6..7a3604c 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -2,3 +2,6 @@ obj-$(CONFIG_DRM_TINYDRM)		+= core/
 
 # Controllers
 obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
+
+# Displays
+obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
new file mode 100644
index 0000000..b29fe86
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -0,0 +1,279 @@
+/*
+ * DRM driver for Multi-Inno MI0283QT panels
+ *
+ * Copyright 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <drm/tinydrm/ili9341.h>
+#include <drm/tinydrm/mipi-dbi.h>
+#include <drm/tinydrm/tinydrm-helpers.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <video/mipi_display.h>
+
+static int mi0283qt_init(struct mipi_dbi *mipi)
+{
+	struct tinydrm_device *tdev = &mipi->tinydrm;
+	struct device *dev = tdev->drm->dev;
+	u8 addr_mode;
+	int ret;
+
+	DRM_DEBUG_KMS("\n");
+
+	ret = regulator_enable(mipi->regulator);
+	if (ret) {
+		dev_err(dev, "Failed to enable regulator %d\n", ret);
+		return ret;
+	}
+
+	/* Avoid flicker by skipping setup if the bootloader has done it */
+	if (mipi_dbi_display_is_on(mipi))
+		return 0;
+
+	mipi_dbi_hw_reset(mipi);
+	ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+	if (ret) {
+		dev_err(dev, "Error sending command %d\n", ret);
+		regulator_disable(mipi->regulator);
+		return ret;
+	}
+
+	msleep(20);
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
+
+	mipi_dbi_command(mipi, ILI9341_PWCTRLB, 0x00, 0x83, 0x30);
+	mipi_dbi_command(mipi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
+	mipi_dbi_command(mipi, ILI9341_DTCTRLA, 0x85, 0x01, 0x79);
+	mipi_dbi_command(mipi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
+	mipi_dbi_command(mipi, ILI9341_PUMPCTRL, 0x20);
+	mipi_dbi_command(mipi, ILI9341_DTCTRLB, 0x00, 0x00);
+
+	/* Power Control */
+	mipi_dbi_command(mipi, ILI9341_PWCTRL1, 0x26);
+	mipi_dbi_command(mipi, ILI9341_PWCTRL2, 0x11);
+	/* VCOM */
+	mipi_dbi_command(mipi, ILI9341_VMCTRL1, 0x35, 0x3e);
+	mipi_dbi_command(mipi, ILI9341_VMCTRL2, 0xbe);
+
+	/* Memory Access Control */
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT, 0x55);
+
+	switch (mipi->rotation) {
+	default:
+		addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
+			    ILI9341_MADCTL_MX;
+		break;
+	case 90:
+		addr_mode = ILI9341_MADCTL_MY;
+		break;
+	case 180:
+		addr_mode = ILI9341_MADCTL_MV;
+		break;
+	case 270:
+		addr_mode = ILI9341_MADCTL_MX;
+		break;
+	}
+	addr_mode |= ILI9341_MADCTL_BGR;
+	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+
+	/* Frame Rate */
+	mipi_dbi_command(mipi, ILI9341_FRMCTR1, 0x00, 0x1b);
+
+	/* Gamma */
+	mipi_dbi_command(mipi, ILI9341_EN3GAM, 0x08);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
+	mipi_dbi_command(mipi, ILI9341_PGAMCTRL,
+		       0x1f, 0x1a, 0x18, 0x0a, 0x0f, 0x06, 0x45, 0x87,
+		       0x32, 0x0a, 0x07, 0x02, 0x07, 0x05, 0x00);
+	mipi_dbi_command(mipi, ILI9341_NGAMCTRL,
+		       0x00, 0x25, 0x27, 0x05, 0x10, 0x09, 0x3a, 0x78,
+		       0x4d, 0x05, 0x18, 0x0d, 0x38, 0x3a, 0x1f);
+
+	/* DDRAM */
+	mipi_dbi_command(mipi, ILI9341_ETMOD, 0x07);
+
+	/* Display */
+	mipi_dbi_command(mipi, ILI9341_DISCTRL, 0x0a, 0x82, 0x27, 0x00);
+	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(100);
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+	msleep(100);
+
+	return 0;
+}
+
+static void mi0283qt_fini(void *data)
+{
+	struct mipi_dbi *mipi = data;
+
+	DRM_DEBUG_KMS("\n");
+	regulator_disable(mipi->regulator);
+}
+
+static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
+	.enable = mipi_dbi_pipe_enable,
+	.disable = mipi_dbi_pipe_disable,
+	.update = tinydrm_display_pipe_update,
+	.prepare_fb = tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode mi0283qt_mode = {
+	TINYDRM_MODE(320, 240, 58, 43),
+};
+
+static struct drm_driver mi0283qt_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
+				  DRIVER_ATOMIC,
+	TINYDRM_GEM_DRIVER_OPS,
+	.lastclose		= tinydrm_lastclose,
+	.debugfs_init		= mipi_dbi_debugfs_init,
+	.name			= "mi0283qt",
+	.desc			= "Multi-Inno MI0283QT",
+	.date			= "20160614",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static const struct of_device_id mi0283qt_of_match[] = {
+	{ .compatible = "multi-inno,mi0283qt" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mi0283qt_of_match);
+
+static const struct spi_device_id mi0283qt_id[] = {
+	{ "mi0283qt", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, mi0283qt_id);
+
+static int mi0283qt_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct tinydrm_device *tdev;
+	struct mipi_dbi *mipi;
+	struct gpio_desc *dc;
+	u32 rotation = 0;
+	int ret;
+
+	mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
+	if (!mipi)
+		return -ENOMEM;
+
+	mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(mipi->reset)) {
+		dev_err(dev, "Failed to get gpio 'reset'\n");
+		return PTR_ERR(mipi->reset);
+	}
+
+	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dc)) {
+		dev_err(dev, "Failed to get gpio 'dc'\n");
+		return PTR_ERR(dc);
+	}
+
+	mipi->regulator = devm_regulator_get(dev, "power");
+	if (IS_ERR(mipi->regulator))
+		return PTR_ERR(mipi->regulator);
+
+	mipi->backlight = tinydrm_of_find_backlight(dev);
+	if (IS_ERR(mipi->backlight))
+		return PTR_ERR(mipi->backlight);
+
+	device_property_read_u32(dev, "rotation", &rotation);
+
+	ret = mipi_dbi_spi_init(spi, mipi, dc, &mi0283qt_pipe_funcs,
+				&mi0283qt_driver, &mi0283qt_mode, rotation);
+	if (ret)
+		return ret;
+
+	ret = mi0283qt_init(mipi);
+	if (ret)
+		return ret;
+
+	/* use devres to fini after drm unregister (drv->remove is before) */
+	ret = devm_add_action(dev, mi0283qt_fini, mipi);
+	if (ret) {
+		mi0283qt_fini(mipi);
+		return ret;
+	}
+
+	tdev = &mipi->tinydrm;
+
+	ret = devm_tinydrm_register(tdev);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, mipi);
+
+	DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",
+			 tdev->drm->driver->name, dev_name(dev),
+			 spi->max_speed_hz / 1000000,
+			 tdev->drm->primary->index);
+
+	return 0;
+}
+
+static void mi0283qt_shutdown(struct spi_device *spi)
+{
+	struct mipi_dbi *mipi = spi_get_drvdata(spi);
+
+	tinydrm_shutdown(&mipi->tinydrm);
+}
+
+static int __maybe_unused mi0283qt_pm_suspend(struct device *dev)
+{
+	struct mipi_dbi *mipi = dev_get_drvdata(dev);
+	int ret;
+
+	ret = tinydrm_suspend(&mipi->tinydrm);
+	if (ret)
+		return ret;
+
+	mi0283qt_fini(mipi);
+
+	return 0;
+}
+
+static int __maybe_unused mi0283qt_pm_resume(struct device *dev)
+{
+	struct mipi_dbi *mipi = dev_get_drvdata(dev);
+	int ret;
+
+	ret = mi0283qt_init(mipi);
+	if (ret)
+		return ret;
+
+	return tinydrm_resume(&mipi->tinydrm);
+}
+
+static const struct dev_pm_ops mi0283qt_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(mi0283qt_pm_suspend, mi0283qt_pm_resume)
+};
+
+static struct spi_driver mi0283qt_spi_driver = {
+	.driver = {
+		.name = "mi0283qt",
+		.owner = THIS_MODULE,
+		.of_match_table = mi0283qt_of_match,
+		.pm = &mi0283qt_pm_ops,
+	},
+	.id_table = mi0283qt_id,
+	.probe = mi0283qt_probe,
+	.shutdown = mi0283qt_shutdown,
+};
+module_spi_driver(mi0283qt_spi_driver);
+
+MODULE_DESCRIPTION("Multi-Inno MI0283QT DRM driver");
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
diff --git a/include/drm/tinydrm/ili9341.h b/include/drm/tinydrm/ili9341.h
new file mode 100644
index 0000000..807a09f
--- /dev/null
+++ b/include/drm/tinydrm/ili9341.h
@@ -0,0 +1,54 @@
+/*
+ * ILI9341 LCD controller
+ *
+ * Copyright 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_ILI9341_H
+#define __LINUX_ILI9341_H
+
+#define ILI9341_FRMCTR1    0xb1
+#define ILI9341_FRMCTR2    0xb2
+#define ILI9341_FRMCTR3    0xb3
+#define ILI9341_INVTR      0xb4
+#define ILI9341_PRCTR      0xb5
+#define ILI9341_DISCTRL    0xb6
+#define ILI9341_ETMOD      0xb7
+
+#define ILI9341_PWCTRL1    0xc0
+#define ILI9341_PWCTRL2    0xc1
+#define ILI9341_VMCTRL1    0xc5
+#define ILI9341_VMCTRL2    0xc7
+#define ILI9341_PWCTRLA    0xcb
+#define ILI9341_PWCTRLB    0xcf
+
+#define ILI9341_RDID1      0xda
+#define ILI9341_RDID2      0xdb
+#define ILI9341_RDID3      0xdc
+#define ILI9341_RDID4      0xd3
+
+#define ILI9341_PGAMCTRL   0xe0
+#define ILI9341_NGAMCTRL   0xe1
+#define ILI9341_DGAMCTRL1  0xe2
+#define ILI9341_DGAMCTRL2  0xe3
+#define ILI9341_DTCTRLA    0xe8
+#define ILI9341_DTCTRLB    0xea
+#define ILI9341_PWRSEQ     0xed
+
+#define ILI9341_EN3GAM     0xf2
+#define ILI9341_IFCTRL     0xf6
+#define ILI9341_PUMPCTRL   0xf7
+
+#define ILI9341_MADCTL_MH  BIT(2)
+#define ILI9341_MADCTL_BGR BIT(3)
+#define ILI9341_MADCTL_ML  BIT(4)
+#define ILI9341_MADCTL_MV  BIT(5)
+#define ILI9341_MADCTL_MX  BIT(6)
+#define ILI9341_MADCTL_MY  BIT(7)
+
+#endif /* __LINUX_ILI9341_H */
-- 
2.10.2

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

[Daniel Vetter]

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>

I spotted another tiny one, but yeah looks are supremely reasonable, ack
on all the drm parts. And please send a pull request for all of it as soon
as you have the dt acks.

Cheers, Daniel

> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> new file mode 100644
> index 0000000..c0a2eb6
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> @@ -0,0 +1,377 @@
> +/*
> + * Copyright (C) 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/tinydrm/tinydrm.h>
> +#include <linux/device.h>
> +#include <linux/dma-buf.h>
> +
> +/**
> + * DOC: overview

This kerneldoc DOC here isn't pulled into the .rst. I'd just merge it with
the one below.
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Noralf Trønnes]

Den 31.01.2017 17.23, skrev Daniel Vetter:
> 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>
> I spotted another tiny one, but yeah looks are supremely reasonable, ack
> on all the drm parts. And please send a pull request for all of it as soon
> as you have the dt acks.

The first DOC section is just an overview and the second is how to use
the core functionality. They are referenced separately in tinydrm.rst.
At least they show up when I build htmldocs :-)

Thanks Daniel.

Noralf.


> Cheers, Daniel
>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> new file mode 100644
>> index 0000000..c0a2eb6
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> @@ -0,0 +1,377 @@
>> +/*
>> + * Copyright (C) 2016 Noralf Trønnes
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/tinydrm/tinydrm.h>
>> +#include <linux/device.h>
>> +#include <linux/dma-buf.h>
>> +
>> +/**
>> + * DOC: overview
> This kerneldoc DOC here isn't pulled into the .rst. I'd just merge it with
> the one below.

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

[Daniel Vetter]

On Tue, Jan 31, 2017 at 07:01:10PM +0100, Noralf Trønnes wrote:
> 
> Den 31.01.2017 17.23, skrev Daniel Vetter:
> > 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>
> > I spotted another tiny one, but yeah looks are supremely reasonable, ack
> > on all the drm parts. And please send a pull request for all of it as soon
> > as you have the dt acks.
> 
> The first DOC section is just an overview and the second is how to use
> the core functionality. They are referenced separately in tinydrm.rst.
> At least they show up when I build htmldocs :-)

I was blind, yes it's all there on 2nd look :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 5/7] dt-bindings: display: Add common rotation property

[Rob Herring]

On Tue, Jan 31, 2017 at 05:03:17PM +0100, Noralf Trønnes wrote:
> Display panels can be oriented many ways, especially in the embedded
> world. The rotation property is a way to describe this orientation.
> The counter clockwise direction is chosen because that's what fbdev
> and drm use.

The h/w mounting is rotated counter clockwise, so the framebuffers' 
contents are rotated clockwise, right? 

> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  Documentation/devicetree/bindings/display/display.txt | 4 ++++

This is panel property, so bindings/display/panel/panel.txt.

>  1 file changed, 4 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/display.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/display.txt b/Documentation/devicetree/bindings/display/display.txt
> new file mode 100644
> index 0000000..e2e6867
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/display.txt
> @@ -0,0 +1,4 @@
> +Common display properties
> +-------------------------
> +
> +- rotation:	Display rotation in degrees counter clockwise (0,90,180,270)
> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 6/7] dt-bindings: Add Multi-Inno MI0283QT binding

[Rob Herring]

On Tue, Jan 31, 2017 at 05:03:18PM +0100, Noralf Trønnes wrote:
> Add device-tree binding documentation for the MI0283QT display panel.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> 
> Datasheet: https://cdn-shop.adafruit.com/datasheets/MI0283QT-11+V1.1.PDF
> 
>  .../bindings/display/multi-inno,mi0283qt.txt       | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 5/7] dt-bindings: display: Add common rotation property

[Noralf Trønnes]

Thierry, please have a look at this.
In which direction should we rotate to match how drm rotation works?


Den 01.02.2017 18.41, skrev Rob Herring:
> On Tue, Jan 31, 2017 at 05:03:17PM +0100, Noralf Trønnes wrote:
>> Display panels can be oriented many ways, especially in the embedded
>> world. The rotation property is a way to describe this orientation.
>> The counter clockwise direction is chosen because that's what fbdev
>> and drm use.
> The h/w mounting is rotated counter clockwise, so the framebuffers'
> contents are rotated clockwise, right?
>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   Documentation/devicetree/bindings/display/display.txt | 4 ++++
> This is panel property, so bindings/display/panel/panel.txt.
>
>>   1 file changed, 4 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/display/display.txt
>>
>> diff --git a/Documentation/devicetree/bindings/display/display.txt b/Documentation/devicetree/bindings/display/display.txt
>> new file mode 100644
>> index 0000000..e2e6867
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/display.txt
>> @@ -0,0 +1,4 @@
>> +Common display properties
>> +-------------------------
>> +
>> +- rotation:	Display rotation in degrees counter clockwise (0,90,180,270)
>> -- 
>> 2.10.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/7] dt-bindings: display: Add common rotation property

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]

On Fri, Feb 03, 2017 at 01:16:45PM +0100, Noralf Trønnes wrote:
> Thierry, please have a look at this.
> In which direction should we rotate to match how drm rotation works?
> 
> 
> Den 01.02.2017 18.41, skrev Rob Herring:
> > On Tue, Jan 31, 2017 at 05:03:17PM +0100, Noralf Trønnes wrote:
> > > Display panels can be oriented many ways, especially in the embedded
> > > world. The rotation property is a way to describe this orientation.
> > > The counter clockwise direction is chosen because that's what fbdev
> > > and drm use.
> > The h/w mounting is rotated counter clockwise, so the framebuffers'
> > contents are rotated clockwise, right?

Given that this describes the rotation of the panel, I think it should
be described in terms of how the panel is rotated, not how the
framebuffer needs to be rotated. Using counter-clockwise, as described
in this binding seems fine to me.

It would have to be up to fbdev and DRM/KMS to transform that into a
clockwise rotation for the framebuffer, CRTC, plane, ...

Ideally with some sort of helper.

Thierry

> > 
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > >   Documentation/devicetree/bindings/display/display.txt | 4 ++++
> > This is panel property, so bindings/display/panel/panel.txt.
> > 
> > >   1 file changed, 4 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/display/display.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/display.txt b/Documentation/devicetree/bindings/display/display.txt
> > > new file mode 100644
> > > index 0000000..e2e6867
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/display.txt
> > > @@ -0,0 +1,4 @@
> > > +Common display properties
> > > +-------------------------
> > > +
> > > +- rotation:	Display rotation in degrees counter clockwise (0,90,180,270)
> > > -- 
> > > 2.10.2
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 7/7] drm/tinydrm: Add support for Multi-Inno MI0283QT display

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 15128 bytes --]

On Tue, Jan 31, 2017 at 05:03:19PM +0100, Noralf Trønnes wrote:
> Add driver to support the Multi-Inno MI0283QT display panel.
> It has an ILI9341 MIPI DBI compatible display controller.

So what exactly does this entail? What if we ever see a second panel
that has one of these controllers? Would it end up duplicating a lot
of the code from this driver?

Or would we be able to reuse the driver by just adding a different
compatible string and parameterizing some more?

> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  MAINTAINERS                        |   6 +
>  drivers/gpu/drm/tinydrm/Kconfig    |   8 ++
>  drivers/gpu/drm/tinydrm/Makefile   |   3 +
>  drivers/gpu/drm/tinydrm/mi0283qt.c | 279 +++++++++++++++++++++++++++++++++++++
>  include/drm/tinydrm/ili9341.h      |  54 +++++++
>  5 files changed, 350 insertions(+)
>  create mode 100644 drivers/gpu/drm/tinydrm/mi0283qt.c
>  create mode 100644 include/drm/tinydrm/ili9341.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d935135..5c4666b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4278,6 +4278,12 @@ S:	Supported
>  F:	drivers/gpu/drm/mediatek/
>  F:	Documentation/devicetree/bindings/display/mediatek/
>  
> +DRM DRIVER FOR MI0283QT
> +M:	Noralf Trønnes <noralf@tronnes.org>
> +S:	Maintained
> +F:	drivers/gpu/drm/tinydrm/mi0283qt.c
> +F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
> +
>  DRM DRIVER FOR MSM ADRENO GPU
>  M:	Rob Clark <robdclark@gmail.com>
>  L:	linux-arm-msm@vger.kernel.org
> diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
> index 128d2f3..d3161fd 100644
> --- a/drivers/gpu/drm/tinydrm/Kconfig
> +++ b/drivers/gpu/drm/tinydrm/Kconfig
> @@ -9,3 +9,11 @@ menuconfig DRM_TINYDRM
>  
>  config TINYDRM_MIPI_DBI
>  	tristate
> +
> +config TINYDRM_MI0283QT
> +	tristate "DRM support for MI0283QT"
> +	depends on DRM_TINYDRM && SPI
> +	select TINYDRM_MIPI_DBI
> +	help
> +	  DRM driver for the Multi-Inno MI0283QT display panel
> +	  If M is selected the module will be called mi0283qt.
> diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
> index fe5d4c6..7a3604c 100644
> --- a/drivers/gpu/drm/tinydrm/Makefile
> +++ b/drivers/gpu/drm/tinydrm/Makefile
> @@ -2,3 +2,6 @@ obj-$(CONFIG_DRM_TINYDRM)		+= core/
>  
>  # Controllers
>  obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
> +
> +# Displays
> +obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> new file mode 100644
> index 0000000..b29fe86
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -0,0 +1,279 @@
> +/*
> + * DRM driver for Multi-Inno MI0283QT panels
> + *
> + * Copyright 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <drm/tinydrm/ili9341.h>
> +#include <drm/tinydrm/mipi-dbi.h>
> +#include <drm/tinydrm/tinydrm-helpers.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <video/mipi_display.h>

The ordering of includes is somewhat weird here. Usually the grouping is
such that we have linux/*, drm/* and video/*

> +
> +static int mi0283qt_init(struct mipi_dbi *mipi)
> +{
> +	struct tinydrm_device *tdev = &mipi->tinydrm;
> +	struct device *dev = tdev->drm->dev;
> +	u8 addr_mode;
> +	int ret;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	ret = regulator_enable(mipi->regulator);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulator %d\n", ret);

Nit: I find error messages such as these confusing because it is
sometimes ambiguous what the number is supposed to mean. I think the
error message becomes much clearer when it reads:

	Failed to enable regulator: %d

> +		return ret;
> +	}
> +
> +	/* Avoid flicker by skipping setup if the bootloader has done it */
> +	if (mipi_dbi_display_is_on(mipi))
> +		return 0;
> +
> +	mipi_dbi_hw_reset(mipi);
> +	ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
> +	if (ret) {
> +		dev_err(dev, "Error sending command %d\n", ret);

This is a case where I think it can be confusing. The number could be
mistaken for the command that failed to be sent. Adding a separator can
help reduce that ambiguity.

> +		regulator_disable(mipi->regulator);
> +		return ret;
> +	}
> +
> +	msleep(20);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> +
> +	mipi_dbi_command(mipi, ILI9341_PWCTRLB, 0x00, 0x83, 0x30);
> +	mipi_dbi_command(mipi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
> +	mipi_dbi_command(mipi, ILI9341_DTCTRLA, 0x85, 0x01, 0x79);
> +	mipi_dbi_command(mipi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
> +	mipi_dbi_command(mipi, ILI9341_PUMPCTRL, 0x20);
> +	mipi_dbi_command(mipi, ILI9341_DTCTRLB, 0x00, 0x00);
> +
> +	/* Power Control */
> +	mipi_dbi_command(mipi, ILI9341_PWCTRL1, 0x26);
> +	mipi_dbi_command(mipi, ILI9341_PWCTRL2, 0x11);
> +	/* VCOM */
> +	mipi_dbi_command(mipi, ILI9341_VMCTRL1, 0x35, 0x3e);
> +	mipi_dbi_command(mipi, ILI9341_VMCTRL2, 0xbe);
> +
> +	/* Memory Access Control */
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT, 0x55);

MIPI_DCS_PIXEL_FMT_16BIT?

> +	switch (mipi->rotation) {
> +	default:
> +		addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
> +			    ILI9341_MADCTL_MX;
> +		break;
> +	case 90:
> +		addr_mode = ILI9341_MADCTL_MY;
> +		break;
> +	case 180:
> +		addr_mode = ILI9341_MADCTL_MV;
> +		break;
> +	case 270:
> +		addr_mode = ILI9341_MADCTL_MX;
> +		break;
> +	}
> +	addr_mode |= ILI9341_MADCTL_BGR;
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);

Would it be possible to add defines corresponding to the definition in
MIPI DCS? Looks like they are equivalent.

> +
> +	/* Frame Rate */
> +	mipi_dbi_command(mipi, ILI9341_FRMCTR1, 0x00, 0x1b);

Should this be parameterized refresh rate of the mode?

That said, is there a publicly available datasheet for this display?
There's a lot of magic values here that aren't trivial to decode. It
seems as if some documentation must exist, otherwise you wouldn't be
using such nice register names.

Don't get me wrong, this is already a lot better than most other
panel drivers that I've seen.

> +
> +	/* Gamma */
> +	mipi_dbi_command(mipi, ILI9341_EN3GAM, 0x08);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
> +	mipi_dbi_command(mipi, ILI9341_PGAMCTRL,
> +		       0x1f, 0x1a, 0x18, 0x0a, 0x0f, 0x06, 0x45, 0x87,
> +		       0x32, 0x0a, 0x07, 0x02, 0x07, 0x05, 0x00);
> +	mipi_dbi_command(mipi, ILI9341_NGAMCTRL,
> +		       0x00, 0x25, 0x27, 0x05, 0x10, 0x09, 0x3a, 0x78,
> +		       0x4d, 0x05, 0x18, 0x0d, 0x38, 0x3a, 0x1f);
> +
> +	/* DDRAM */
> +	mipi_dbi_command(mipi, ILI9341_ETMOD, 0x07);
> +
> +	/* Display */
> +	mipi_dbi_command(mipi, ILI9341_DISCTRL, 0x0a, 0x82, 0x27, 0x00);
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	msleep(100);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> +	msleep(100);
> +
> +	return 0;
> +}
> +
> +static void mi0283qt_fini(void *data)
> +{
> +	struct mipi_dbi *mipi = data;
> +
> +	DRM_DEBUG_KMS("\n");
> +	regulator_disable(mipi->regulator);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
> +	.enable = mipi_dbi_pipe_enable,
> +	.disable = mipi_dbi_pipe_disable,
> +	.update = tinydrm_display_pipe_update,
> +	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_display_mode mi0283qt_mode = {
> +	TINYDRM_MODE(320, 240, 58, 43),
> +};
> +
> +static struct drm_driver mi0283qt_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
> +				  DRIVER_ATOMIC,
> +	TINYDRM_GEM_DRIVER_OPS,
> +	.lastclose		= tinydrm_lastclose,
> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "mi0283qt",
> +	.desc			= "Multi-Inno MI0283QT",
> +	.date			= "20160614",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static const struct of_device_id mi0283qt_of_match[] = {
> +	{ .compatible = "multi-inno,mi0283qt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mi0283qt_of_match);
> +
> +static const struct spi_device_id mi0283qt_id[] = {
> +	{ "mi0283qt", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(spi, mi0283qt_id);

Do we actually still need this? Will these devices ever be instantiated
from non-OF code?

> +
> +static int mi0283qt_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct tinydrm_device *tdev;
> +	struct mipi_dbi *mipi;
> +	struct gpio_desc *dc;
> +	u32 rotation = 0;
> +	int ret;
> +
> +	mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
> +	if (!mipi)
> +		return -ENOMEM;
> +
> +	mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(mipi->reset)) {
> +		dev_err(dev, "Failed to get gpio 'reset'\n");
> +		return PTR_ERR(mipi->reset);
> +	}
> +
> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc)) {
> +		dev_err(dev, "Failed to get gpio 'dc'\n");
> +		return PTR_ERR(dc);
> +	}
> +
> +	mipi->regulator = devm_regulator_get(dev, "power");
> +	if (IS_ERR(mipi->regulator))
> +		return PTR_ERR(mipi->regulator);
> +
> +	mipi->backlight = tinydrm_of_find_backlight(dev);
> +	if (IS_ERR(mipi->backlight))
> +		return PTR_ERR(mipi->backlight);
> +
> +	device_property_read_u32(dev, "rotation", &rotation);
> +
> +	ret = mipi_dbi_spi_init(spi, mipi, dc, &mi0283qt_pipe_funcs,
> +				&mi0283qt_driver, &mi0283qt_mode, rotation);
> +	if (ret)
> +		return ret;
> +
> +	ret = mi0283qt_init(mipi);
> +	if (ret)
> +		return ret;
> +
> +	/* use devres to fini after drm unregister (drv->remove is before) */
> +	ret = devm_add_action(dev, mi0283qt_fini, mipi);
> +	if (ret) {
> +		mi0283qt_fini(mipi);
> +		return ret;
> +	}

This is somewhat asymmetric. I understand that you can't simply do this
in ->remove() here because of devm_tinydrm_register(), but wouldn't it
be somewhat simpler to add an explicit callback somewhere that is called
at exactly the right time? I think it'll be fairly confusing down the
road to have every driver use devm_add_action() explicitly to clean up.
TinyDRM should have that built-in, in my opinion.

> +
> +	tdev = &mipi->tinydrm;
> +
> +	ret = devm_tinydrm_register(tdev);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, mipi);
> +
> +	DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",
> +			 tdev->drm->driver->name, dev_name(dev),
> +			 spi->max_speed_hz / 1000000,
> +			 tdev->drm->primary->index);
> +
> +	return 0;
> +}
> +
> +static void mi0283qt_shutdown(struct spi_device *spi)
> +{
> +	struct mipi_dbi *mipi = spi_get_drvdata(spi);
> +
> +	tinydrm_shutdown(&mipi->tinydrm);
> +}
> +
> +static int __maybe_unused mi0283qt_pm_suspend(struct device *dev)
> +{
> +	struct mipi_dbi *mipi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = tinydrm_suspend(&mipi->tinydrm);
> +	if (ret)
> +		return ret;
> +
> +	mi0283qt_fini(mipi);

This looks slightly weird from a naming perspective. My first instinct
was that this was a bug because it's cleaning up a device on suspend
(_fini is usually a suffix for destructor-like functions). Then I
realized that mi0283qt_fini() was really only disabling a regulator,
so it's obviously fine to call it here. Might be worth choosing a less
confusing name here.

> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mi0283qt_pm_resume(struct device *dev)
> +{
> +	struct mipi_dbi *mipi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = mi0283qt_init(mipi);
> +	if (ret)
> +		return ret;

Similar here. I probably would've chose something like mi0283qt_enable()
and mi0283qt_disable() for these. But feel free to leave this as is if
you prefer your bikeshed colored this way.

> +
> +	return tinydrm_resume(&mipi->tinydrm);
> +}
> +
> +static const struct dev_pm_ops mi0283qt_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(mi0283qt_pm_suspend, mi0283qt_pm_resume)
> +};
> +
> +static struct spi_driver mi0283qt_spi_driver = {
> +	.driver = {
> +		.name = "mi0283qt",
> +		.owner = THIS_MODULE,
> +		.of_match_table = mi0283qt_of_match,
> +		.pm = &mi0283qt_pm_ops,
> +	},
> +	.id_table = mi0283qt_id,
> +	.probe = mi0283qt_probe,
> +	.shutdown = mi0283qt_shutdown,
> +};
> +module_spi_driver(mi0283qt_spi_driver);
> +
> +MODULE_DESCRIPTION("Multi-Inno MI0283QT DRM driver");
> +MODULE_AUTHOR("Noralf Trønnes");
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/tinydrm/ili9341.h b/include/drm/tinydrm/ili9341.h
> new file mode 100644
> index 0000000..807a09f
> --- /dev/null
> +++ b/include/drm/tinydrm/ili9341.h
> @@ -0,0 +1,54 @@
> +/*
> + * ILI9341 LCD controller
> + *
> + * Copyright 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __LINUX_ILI9341_H
> +#define __LINUX_ILI9341_H
> +
> +#define ILI9341_FRMCTR1    0xb1
> +#define ILI9341_FRMCTR2    0xb2
> +#define ILI9341_FRMCTR3    0xb3
> +#define ILI9341_INVTR      0xb4
> +#define ILI9341_PRCTR      0xb5
> +#define ILI9341_DISCTRL    0xb6
> +#define ILI9341_ETMOD      0xb7
> +
> +#define ILI9341_PWCTRL1    0xc0
> +#define ILI9341_PWCTRL2    0xc1
> +#define ILI9341_VMCTRL1    0xc5
> +#define ILI9341_VMCTRL2    0xc7
> +#define ILI9341_PWCTRLA    0xcb
> +#define ILI9341_PWCTRLB    0xcf
> +
> +#define ILI9341_RDID1      0xda
> +#define ILI9341_RDID2      0xdb
> +#define ILI9341_RDID3      0xdc
> +#define ILI9341_RDID4      0xd3
> +
> +#define ILI9341_PGAMCTRL   0xe0
> +#define ILI9341_NGAMCTRL   0xe1
> +#define ILI9341_DGAMCTRL1  0xe2
> +#define ILI9341_DGAMCTRL2  0xe3
> +#define ILI9341_DTCTRLA    0xe8
> +#define ILI9341_DTCTRLB    0xea
> +#define ILI9341_PWRSEQ     0xed
> +
> +#define ILI9341_EN3GAM     0xf2
> +#define ILI9341_IFCTRL     0xf6
> +#define ILI9341_PUMPCTRL   0xf7
> +
> +#define ILI9341_MADCTL_MH  BIT(2)
> +#define ILI9341_MADCTL_BGR BIT(3)
> +#define ILI9341_MADCTL_ML  BIT(4)
> +#define ILI9341_MADCTL_MV  BIT(5)
> +#define ILI9341_MADCTL_MX  BIT(6)
> +#define ILI9341_MADCTL_MY  BIT(7)
> +
> +#endif /* __LINUX_ILI9341_H */

Wouldn't it be possible to inline these into the source file? Or if you
think anyone will ever reuse these for a different panel driver, maybe
put it alongside the source file. Doesn't look like something that needs
to be in include/drm.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/7] drm/tinydrm: Add MIPI DBI support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]

On Tue, Jan 31, 2017 at 05:03:15PM +0100, Noralf Trønnes wrote:
> Add support for MIPI DBI compatible controllers.
> Interface type C option 1 and 3 are supported (SPI).
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  Documentation/gpu/tinydrm.rst      |   12 +
>  drivers/gpu/drm/tinydrm/Kconfig    |    3 +
>  drivers/gpu/drm/tinydrm/Makefile   |    3 +
>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 1005 ++++++++++++++++++++++++++++++++++++
>  include/drm/tinydrm/mipi-dbi.h     |  107 ++++
>  5 files changed, 1130 insertions(+)
>  create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
>  create mode 100644 include/drm/tinydrm/mipi-dbi.h

Any reason why this is in the tinydrm subdirectory? Looks like this
could be useful to drivers outside of it.

> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> new file mode 100644
> index 0000000..5ded299
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -0,0 +1,1005 @@
> +/*
> + * MIPI Display Bus Interface (DBI) LCD controller support
> + *
> + * Copyright 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <drm/tinydrm/mipi-dbi.h>
> +#include <drm/tinydrm/tinydrm-helpers.h>
> +#include <linux/debugfs.h>
> +#include <linux/dma-buf.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <video/mipi_display.h>
> +
> +#define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */
> +
> +#define DCS_POWER_MODE_DISPLAY			BIT(2)
> +#define DCS_POWER_MODE_DISPLAY_NORMAL_MODE	BIT(3)
> +#define DCS_POWER_MODE_SLEEP_MODE		BIT(4)
> +#define DCS_POWER_MODE_PARTIAL_MODE		BIT(5)
> +#define DCS_POWER_MODE_IDLE_MODE		BIT(6)
> +#define DCS_POWER_MODE_RESERVED_MASK		(BIT(0) | BIT(1) | BIT(7))

Should these perhaps be defined in include/video/mipi_display.h since
that already defines the MIPI_DCS_GET_POWER_MODE that this is used with?

> +/**
> + * mipi_dbi_command - MIPI DCS command with optional parameter(s)
> + * @mipi: MIPI structure
> + * @cmd: Command
> + * @seq...: Optional parameter(s)
> + *
> + * Send MIPI DCS command to the controller. Use mipi_dbi_command_read() for
> + * get/read.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +#define mipi_dbi_command(mipi, cmd, seq...) \
> +({ \
> +	u8 d[] = { seq }; \
> +	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
> +})

I feel obligated to object to this because I objected to the same macro
when Andrzej wanted to add the same macro for MIPI DSI. But since I'm
apparently the only one that doesn't like this, maybe it's time for me
to embrace this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 12575 bytes --]

On Tue, Jan 31, 2017 at 05:03:14PM +0100, Noralf Trønnes wrote:
> Add common functionality needed by many tinydrm drivers.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/gpu/tinydrm.rst                  |   9 +
>  drivers/gpu/drm/tinydrm/core/Makefile          |   2 +-
>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 462 +++++++++++++++++++++++++
>  include/drm/tinydrm/tinydrm-helpers.h          | 100 ++++++
>  4 files changed, 572 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>  create mode 100644 include/drm/tinydrm/tinydrm-helpers.h
> 
> diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst
> index ec4a20d..fb256d2 100644
> --- a/Documentation/gpu/tinydrm.rst
> +++ b/Documentation/gpu/tinydrm.rst
> @@ -19,3 +19,12 @@ Core functionality
>  
>  .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
>     :export:
> +
> +Additional helpers
> +==================
> +
> +.. kernel-doc:: include/drm/tinydrm/tinydrm-helpers.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +   :export:
> diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile
> index 4f14a0f..fb221e6 100644
> --- a/drivers/gpu/drm/tinydrm/core/Makefile
> +++ b/drivers/gpu/drm/tinydrm/core/Makefile
> @@ -1,3 +1,3 @@
> -tinydrm-y := tinydrm-core.o tinydrm-pipe.o
> +tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o
>  
>  obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> new file mode 100644
> index 0000000..75e4e54
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -0,0 +1,462 @@
> +/*
> + * Copyright (C) 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <drm/tinydrm/tinydrm.h>
> +#include <drm/tinydrm/tinydrm-helpers.h>
> +#include <linux/backlight.h>
> +#include <linux/pm.h>
> +#include <linux/spi/spi.h>
> +#include <linux/swab.h>
> +
> +static unsigned int spi_max;
> +module_param(spi_max, uint, 0400);
> +MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
> +
> +/**
> + * tinydrm_merge_clips - Merge clip rectangles
> + * @dst: Destination clip rectangle
> + * @src: Source clip rectangle(s)
> + * @num_clips: Number of @src clip rectangles
> + * @flags: Dirty fb ioctl flags
> + * @max_width: Maximum width of @dst
> + * @max_height: Maximum height of @dst
> + *
> + * This function merges @src clip rectangle(s) into @dst. If @src is NULL,
> + * @max_width and @min_width is used to set a full @dst clip rectangle.
> + *
> + * Returns:
> + * true if it's a full clip, false otherwise
> + */
> +bool tinydrm_merge_clips(struct drm_clip_rect *dst,
> +			 struct drm_clip_rect *src, unsigned int num_clips,
> +			 unsigned int flags, u32 max_width, u32 max_height)
> +{
> +	unsigned int i;
> +
> +	if (!src || !num_clips) {
> +		dst->x1 = 0;
> +		dst->x2 = max_width;
> +		dst->y1 = 0;
> +		dst->y2 = max_height;
> +		return true;
> +	}
> +
> +	dst->x1 = ~0;
> +	dst->y1 = ~0;
> +	dst->x2 = 0;
> +	dst->y2 = 0;
> +
> +	for (i = 0; i < num_clips; i++) {
> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> +			i++;
> +		dst->x1 = min(dst->x1, src[i].x1);
> +		dst->x2 = max(dst->x2, src[i].x2);
> +		dst->y1 = min(dst->y1, src[i].y1);
> +		dst->y2 = max(dst->y2, src[i].y2);
> +	}
> +
> +	if (dst->x2 > max_width || dst->y2 > max_height ||
> +	    dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
> +		DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
> +			      dst->x1, dst->x2, dst->y1, dst->y2);
> +		dst->x1 = 0;
> +		dst->y1 = 0;
> +		dst->x2 = max_width;
> +		dst->y2 = max_height;
> +	}
> +
> +	return (dst->x2 - dst->x1) == max_width &&
> +	       (dst->y2 - dst->y1) == max_height;
> +}
> +EXPORT_SYMBOL(tinydrm_merge_clips);

Should this perhaps be part of drm_rect.c?

> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> +/**
> + * tinydrm_of_find_backlight - Find backlight device in device-tree
> + * @dev: Device
> + *
> + * This function looks for a DT node pointed to by a property named 'backlight'
> + * and uses of_find_backlight_by_node() to get the backlight device.
> + * Additionally if the brightness property is zero, it is set to
> + * max_brightness.
> + *
> + * Returns:
> + * NULL if there's no backlight property.
> + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> + * is found.
> + * If the backlight device is found, a pointer to the structure is returned.
> + */
> +struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> +{
> +	struct backlight_device *backlight;
> +	struct device_node *np;
> +
> +	np = of_parse_phandle(dev->of_node, "backlight", 0);
> +	if (!np)
> +		return NULL;
> +
> +	backlight = of_find_backlight_by_node(np);
> +	of_node_put(np);
> +
> +	if (!backlight)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	if (!backlight->props.brightness) {
> +		backlight->props.brightness = backlight->props.max_brightness;
> +		DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> +			      backlight->props.brightness);
> +	}
> +
> +	return backlight;
> +}
> +EXPORT_SYMBOL(tinydrm_of_find_backlight);
> +
> +/**
> + * tinydrm_enable_backlight - Enable backlight helper
> + * @backlight: Backlight device
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int tinydrm_enable_backlight(struct backlight_device *backlight)
> +{
> +	unsigned int old_state;
> +	int ret;
> +
> +	if (!backlight)
> +		return 0;
> +
> +	old_state = backlight->props.state;
> +	backlight->props.state &= ~BL_CORE_FBBLANK;
> +	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> +		      backlight->props.state);
> +
> +	ret = backlight_update_status(backlight);
> +	if (ret)
> +		DRM_ERROR("Failed to enable backlight %d\n", ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(tinydrm_enable_backlight);
> +
> +/**
> + * tinydrm_disable_backlight - Disable backlight helper
> + * @backlight: Backlight device
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int tinydrm_disable_backlight(struct backlight_device *backlight)
> +{
> +	unsigned int old_state;
> +	int ret;
> +
> +	if (!backlight)
> +		return 0;
> +
> +	old_state = backlight->props.state;
> +	backlight->props.state |= BL_CORE_FBBLANK;
> +	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> +		      backlight->props.state);
> +	ret = backlight_update_status(backlight);
> +	if (ret)
> +		DRM_ERROR("Failed to disable backlight %d\n", ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(tinydrm_disable_backlight);
> +#endif

These look like they really should be part of the backlight subsystem. I
don't see anything DRM specific about them. Well, except for the error
messages.

> +#ifdef CONFIG_SPI
> +
> +/**
> + * tinydrm_spi_max_transfer_size - Determine max SPI transfer size
> + * @spi: SPI device
> + * @max_len: Maximum buffer size needed (optional)
> + *
> + * This function returns the maximum size to use for SPI transfers. It checks
> + * the SPI master, the optional @max_len and the module parameter spi_max and
> + * returns the smallest.
> + *
> + * Returns:
> + * Maximum size for SPI transfers
> + */
> +size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len)
> +{
> +	size_t ret;
> +
> +	ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len);
> +	if (max_len)
> +		ret = min(ret, max_len);
> +	if (spi_max)
> +		ret = min_t(size_t, ret, spi_max);
> +	ret &= ~0x3;
> +	if (ret < 4)
> +		ret = 4;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(tinydrm_spi_max_transfer_size);
> +
> +/**
> + * tinydrm_spi_bpw_supported - Check if bits per word is supported
> + * @spi: SPI device
> + * @bpw: Bits per word
> + *
> + * This function checks to see if the SPI master driver supports @bpw.
> + *
> + * Returns:
> + * True if @bpw is supported, false otherwise.
> + */
> +bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw)
> +{
> +	u32 bpw_mask = spi->master->bits_per_word_mask;
> +
> +	if (bpw == 8)
> +		return true;
> +
> +	if (!bpw_mask) {
> +		dev_warn_once(&spi->dev,
> +			      "bits_per_word_mask not set, assume 8-bit only\n");
> +		return false;
> +	}
> +
> +	if (bpw_mask & SPI_BPW_MASK(bpw))
> +		return true;
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(tinydrm_spi_bpw_supported);
> +
> +static void
> +tinydrm_dbg_spi_print(struct spi_device *spi, struct spi_transfer *tr,
> +		      const void *buf, int idx, bool tx)
> +{
> +	u32 speed_hz = tr->speed_hz ? tr->speed_hz : spi->max_speed_hz;
> +	char linebuf[3 * 32];
> +
> +	hex_dump_to_buffer(buf, tr->len, 16,
> +			   DIV_ROUND_UP(tr->bits_per_word, 8),
> +			   linebuf, sizeof(linebuf), false);
> +
> +	printk(KERN_DEBUG
> +	       "    tr(%i): speed=%u%s, bpw=%i, len=%u, %s_buf=[%s%s]\n", idx,
> +	       speed_hz > 1000000 ? speed_hz / 1000000 : speed_hz / 1000,
> +	       speed_hz > 1000000 ? "MHz" : "kHz", tr->bits_per_word, tr->len,
> +	       tx ? "tx" : "rx", linebuf, tr->len > 16 ? " ..." : "");
> +}
> +
> +/* called through tinydrm_dbg_spi_message() */
> +void _tinydrm_dbg_spi_message(struct spi_device *spi, struct spi_message *m)
> +{
> +	struct spi_transfer *tmp;
> +	struct list_head *pos;
> +	int i = 0;
> +
> +	list_for_each(pos, &m->transfers) {
> +		tmp = list_entry(pos, struct spi_transfer, transfer_list);
> +
> +		if (tmp->tx_buf)
> +			tinydrm_dbg_spi_print(spi, tmp, tmp->tx_buf, i, true);
> +		if (tmp->rx_buf)
> +			tinydrm_dbg_spi_print(spi, tmp, tmp->rx_buf, i, false);
> +		i++;
> +	}
> +}
> +EXPORT_SYMBOL(_tinydrm_dbg_spi_message);
> +
> +/**
> + * tinydrm_spi_transfer - SPI transfer helper
> + * @spi: SPI device
> + * @speed_hz: Override speed (optional)
> + * @header: Optional header transfer
> + * @bpw: Bits per word
> + * @buf: Buffer to transfer
> + * @len: Buffer length
> + *
> + * This SPI transfer helper breaks up the transfer of @buf into chunks which
> + * the SPI master driver can handle. If the machine is Little Endian and the
> + * SPI master driver doesn't support 16 bits per word, it swaps the bytes and
> + * does a 8-bit transfer.
> + * If @header is set, it is prepended to each SPI message.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
> +			 struct spi_transfer *header, u8 bpw, const void *buf,
> +			 size_t len)
> +{
> +	struct spi_transfer tr = {
> +		.bits_per_word = bpw,
> +		.speed_hz = speed_hz,
> +	};
> +	struct spi_message m;
> +	u16 *swap_buf = NULL;
> +	size_t max_chunk;
> +	size_t chunk;
> +	int ret = 0;
> +
> +	if (WARN_ON_ONCE(bpw != 8 && bpw != 16))
> +		return -EINVAL;
> +
> +	max_chunk = tinydrm_spi_max_transfer_size(spi, 0);
> +
> +	if (drm_debug & DRM_UT_DRIVER)
> +		pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n",
> +			 __func__, bpw, max_chunk);
> +
> +	if (bpw == 16 && !tinydrm_spi_bpw_supported(spi, 16)) {
> +		tr.bits_per_word = 8;
> +		if (tinydrm_machine_little_endian()) {
> +			swap_buf = kmalloc(min(len, max_chunk), GFP_KERNEL);
> +			if (!swap_buf)
> +				return -ENOMEM;
> +		}
> +	}
> +
> +	spi_message_init(&m);
> +	if (header)
> +		spi_message_add_tail(header, &m);
> +	spi_message_add_tail(&tr, &m);
> +
> +	while (len) {
> +		chunk = min(len, max_chunk);
> +
> +		tr.tx_buf = buf;
> +		tr.len = chunk;
> +
> +		if (swap_buf) {
> +			const u16 *buf16 = buf;
> +			unsigned int i;
> +
> +			for (i = 0; i < chunk / 2; i++)
> +				swap_buf[i] = swab16(buf16[i]);
> +
> +			tr.tx_buf = swap_buf;
> +		}
> +
> +		buf += chunk;
> +		len -= chunk;
> +
> +		tinydrm_dbg_spi_message(spi, &m);
> +		ret = spi_sync(spi, &m);
> +		if (ret)
> +			return ret;
> +	};
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tinydrm_spi_transfer);

Similarly for the above.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Daniel Vetter]

On Mon, Feb 06, 2017 at 09:56:29AM +0100, Thierry Reding wrote:
> On Tue, Jan 31, 2017 at 05:03:14PM +0100, Noralf Trønnes wrote:
> > Add common functionality needed by many tinydrm drivers.
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  Documentation/gpu/tinydrm.rst                  |   9 +
> >  drivers/gpu/drm/tinydrm/core/Makefile          |   2 +-
> >  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 462 +++++++++++++++++++++++++
> >  include/drm/tinydrm/tinydrm-helpers.h          | 100 ++++++
> >  4 files changed, 572 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> >  create mode 100644 include/drm/tinydrm/tinydrm-helpers.h
> > 
> > diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst
> > index ec4a20d..fb256d2 100644
> > --- a/Documentation/gpu/tinydrm.rst
> > +++ b/Documentation/gpu/tinydrm.rst
> > @@ -19,3 +19,12 @@ Core functionality
> >  
> >  .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> >     :export:
> > +
> > +Additional helpers
> > +==================
> > +
> > +.. kernel-doc:: include/drm/tinydrm/tinydrm-helpers.h
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > +   :export:
> > diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile
> > index 4f14a0f..fb221e6 100644
> > --- a/drivers/gpu/drm/tinydrm/core/Makefile
> > +++ b/drivers/gpu/drm/tinydrm/core/Makefile
> > @@ -1,3 +1,3 @@
> > -tinydrm-y := tinydrm-core.o tinydrm-pipe.o
> > +tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o
> >  
> >  obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o
> > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > new file mode 100644
> > index 0000000..75e4e54
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > @@ -0,0 +1,462 @@
> > +/*
> > + * Copyright (C) 2016 Noralf Trønnes
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <drm/tinydrm/tinydrm.h>
> > +#include <drm/tinydrm/tinydrm-helpers.h>
> > +#include <linux/backlight.h>
> > +#include <linux/pm.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/swab.h>
> > +
> > +static unsigned int spi_max;
> > +module_param(spi_max, uint, 0400);
> > +MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
> > +
> > +/**
> > + * tinydrm_merge_clips - Merge clip rectangles
> > + * @dst: Destination clip rectangle
> > + * @src: Source clip rectangle(s)
> > + * @num_clips: Number of @src clip rectangles
> > + * @flags: Dirty fb ioctl flags
> > + * @max_width: Maximum width of @dst
> > + * @max_height: Maximum height of @dst
> > + *
> > + * This function merges @src clip rectangle(s) into @dst. If @src is NULL,
> > + * @max_width and @min_width is used to set a full @dst clip rectangle.
> > + *
> > + * Returns:
> > + * true if it's a full clip, false otherwise
> > + */
> > +bool tinydrm_merge_clips(struct drm_clip_rect *dst,
> > +			 struct drm_clip_rect *src, unsigned int num_clips,
> > +			 unsigned int flags, u32 max_width, u32 max_height)
> > +{
> > +	unsigned int i;
> > +
> > +	if (!src || !num_clips) {
> > +		dst->x1 = 0;
> > +		dst->x2 = max_width;
> > +		dst->y1 = 0;
> > +		dst->y2 = max_height;
> > +		return true;
> > +	}
> > +
> > +	dst->x1 = ~0;
> > +	dst->y1 = ~0;
> > +	dst->x2 = 0;
> > +	dst->y2 = 0;
> > +
> > +	for (i = 0; i < num_clips; i++) {
> > +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> > +			i++;
> > +		dst->x1 = min(dst->x1, src[i].x1);
> > +		dst->x2 = max(dst->x2, src[i].x2);
> > +		dst->y1 = min(dst->y1, src[i].y1);
> > +		dst->y2 = max(dst->y2, src[i].y2);
> > +	}
> > +
> > +	if (dst->x2 > max_width || dst->y2 > max_height ||
> > +	    dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
> > +		DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
> > +			      dst->x1, dst->x2, dst->y1, dst->y2);
> > +		dst->x1 = 0;
> > +		dst->y1 = 0;
> > +		dst->x2 = max_width;
> > +		dst->y2 = max_height;
> > +	}
> > +
> > +	return (dst->x2 - dst->x1) == max_width &&
> > +	       (dst->y2 - dst->y1) == max_height;
> > +}
> > +EXPORT_SYMBOL(tinydrm_merge_clips);
> 
> Should this perhaps be part of drm_rect.c?

drm_rect is about struct drm_rect, this here is struct drm_clip_rect. I
think fixing this up is a lot bigger patch series, and we've postponed it
already with the dirtyfb trakcing patches for the fbdev helpers. I think
postponing once more is ok.

> > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> > +/**
> > + * tinydrm_of_find_backlight - Find backlight device in device-tree
> > + * @dev: Device
> > + *
> > + * This function looks for a DT node pointed to by a property named 'backlight'
> > + * and uses of_find_backlight_by_node() to get the backlight device.
> > + * Additionally if the brightness property is zero, it is set to
> > + * max_brightness.
> > + *
> > + * Returns:
> > + * NULL if there's no backlight property.
> > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > + * is found.
> > + * If the backlight device is found, a pointer to the structure is returned.
> > + */
> > +struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> > +{
> > +	struct backlight_device *backlight;
> > +	struct device_node *np;
> > +
> > +	np = of_parse_phandle(dev->of_node, "backlight", 0);
> > +	if (!np)
> > +		return NULL;
> > +
> > +	backlight = of_find_backlight_by_node(np);
> > +	of_node_put(np);
> > +
> > +	if (!backlight)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	if (!backlight->props.brightness) {
> > +		backlight->props.brightness = backlight->props.max_brightness;
> > +		DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > +			      backlight->props.brightness);
> > +	}
> > +
> > +	return backlight;
> > +}
> > +EXPORT_SYMBOL(tinydrm_of_find_backlight);
> > +
> > +/**
> > + * tinydrm_enable_backlight - Enable backlight helper
> > + * @backlight: Backlight device
> > + *
> > + * Returns:
> > + * Zero on success, negative error code on failure.
> > + */
> > +int tinydrm_enable_backlight(struct backlight_device *backlight)
> > +{
> > +	unsigned int old_state;
> > +	int ret;
> > +
> > +	if (!backlight)
> > +		return 0;
> > +
> > +	old_state = backlight->props.state;
> > +	backlight->props.state &= ~BL_CORE_FBBLANK;
> > +	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> > +		      backlight->props.state);
> > +
> > +	ret = backlight_update_status(backlight);
> > +	if (ret)
> > +		DRM_ERROR("Failed to enable backlight %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(tinydrm_enable_backlight);
> > +
> > +/**
> > + * tinydrm_disable_backlight - Disable backlight helper
> > + * @backlight: Backlight device
> > + *
> > + * Returns:
> > + * Zero on success, negative error code on failure.
> > + */
> > +int tinydrm_disable_backlight(struct backlight_device *backlight)
> > +{
> > +	unsigned int old_state;
> > +	int ret;
> > +
> > +	if (!backlight)
> > +		return 0;
> > +
> > +	old_state = backlight->props.state;
> > +	backlight->props.state |= BL_CORE_FBBLANK;
> > +	DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> > +		      backlight->props.state);
> > +	ret = backlight_update_status(backlight);
> > +	if (ret)
> > +		DRM_ERROR("Failed to disable backlight %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(tinydrm_disable_backlight);
> > +#endif
> 
> These look like they really should be part of the backlight subsystem. I
> don't see anything DRM specific about them. Well, except for the error
> messages.

So this is a bit an unpopular opinion with some folks, but I don't require
anyone to submit new code to subsystems outside of drm for new drivers.
Simply because it takes months to get stuff landed, and in general it's
not worth the trouble.

We have piles of stuff in drm and drm drivers that should be in core but
isn't.

Imo the only reasonable way is to merge as-is, then follow-up with a patch
series to move the helper into the right subsystem. Most often
unfortunately that follow-up patch series will just die.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3336 bytes --]

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.

[...]
> +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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5663 bytes --]

On Mon, Feb 06, 2017 at 10:09:18AM +0100, Daniel Vetter wrote:
> On Mon, Feb 06, 2017 at 09:56:29AM +0100, Thierry Reding wrote:
> > On Tue, Jan 31, 2017 at 05:03:14PM +0100, Noralf Trønnes wrote:
[...]
> > > +/**
> > > + * tinydrm_merge_clips - Merge clip rectangles
> > > + * @dst: Destination clip rectangle
> > > + * @src: Source clip rectangle(s)
> > > + * @num_clips: Number of @src clip rectangles
> > > + * @flags: Dirty fb ioctl flags
> > > + * @max_width: Maximum width of @dst
> > > + * @max_height: Maximum height of @dst
> > > + *
> > > + * This function merges @src clip rectangle(s) into @dst. If @src is NULL,
> > > + * @max_width and @min_width is used to set a full @dst clip rectangle.
> > > + *
> > > + * Returns:
> > > + * true if it's a full clip, false otherwise
> > > + */
> > > +bool tinydrm_merge_clips(struct drm_clip_rect *dst,
> > > +			 struct drm_clip_rect *src, unsigned int num_clips,
> > > +			 unsigned int flags, u32 max_width, u32 max_height)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	if (!src || !num_clips) {
> > > +		dst->x1 = 0;
> > > +		dst->x2 = max_width;
> > > +		dst->y1 = 0;
> > > +		dst->y2 = max_height;
> > > +		return true;
> > > +	}
> > > +
> > > +	dst->x1 = ~0;
> > > +	dst->y1 = ~0;
> > > +	dst->x2 = 0;
> > > +	dst->y2 = 0;
> > > +
> > > +	for (i = 0; i < num_clips; i++) {
> > > +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
> > > +			i++;
> > > +		dst->x1 = min(dst->x1, src[i].x1);
> > > +		dst->x2 = max(dst->x2, src[i].x2);
> > > +		dst->y1 = min(dst->y1, src[i].y1);
> > > +		dst->y2 = max(dst->y2, src[i].y2);
> > > +	}
> > > +
> > > +	if (dst->x2 > max_width || dst->y2 > max_height ||
> > > +	    dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
> > > +		DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
> > > +			      dst->x1, dst->x2, dst->y1, dst->y2);
> > > +		dst->x1 = 0;
> > > +		dst->y1 = 0;
> > > +		dst->x2 = max_width;
> > > +		dst->y2 = max_height;
> > > +	}
> > > +
> > > +	return (dst->x2 - dst->x1) == max_width &&
> > > +	       (dst->y2 - dst->y1) == max_height;
> > > +}
> > > +EXPORT_SYMBOL(tinydrm_merge_clips);
> > 
> > Should this perhaps be part of drm_rect.c?
> 
> drm_rect is about struct drm_rect, this here is struct drm_clip_rect. I
> think fixing this up is a lot bigger patch series, and we've postponed it
> already with the dirtyfb trakcing patches for the fbdev helpers. I think
> postponing once more is ok.

Okay, that's fine with me.

> > > +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> > > +/**
> > > + * tinydrm_of_find_backlight - Find backlight device in device-tree
> > > + * @dev: Device
> > > + *
> > > + * This function looks for a DT node pointed to by a property named 'backlight'
> > > + * and uses of_find_backlight_by_node() to get the backlight device.
> > > + * Additionally if the brightness property is zero, it is set to
> > > + * max_brightness.
> > > + *
> > > + * Returns:
> > > + * NULL if there's no backlight property.
> > > + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> > > + * is found.
> > > + * If the backlight device is found, a pointer to the structure is returned.
> > > + */
> > > +struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> > > +{
[...]
> > > +}
> > > +EXPORT_SYMBOL(tinydrm_of_find_backlight);
> > > +
> > > +/**
> > > + * tinydrm_enable_backlight - Enable backlight helper
> > > + * @backlight: Backlight device
> > > + *
> > > + * Returns:
> > > + * Zero on success, negative error code on failure.
> > > + */
> > > +int tinydrm_enable_backlight(struct backlight_device *backlight)
> > > +{
[...]
> > > +}
> > > +EXPORT_SYMBOL(tinydrm_enable_backlight);
> > > +
> > > +/**
> > > + * tinydrm_disable_backlight - Disable backlight helper
> > > + * @backlight: Backlight device
> > > + *
> > > + * Returns:
> > > + * Zero on success, negative error code on failure.
> > > + */
> > > +int tinydrm_disable_backlight(struct backlight_device *backlight)
> > > +{
[...]
> > > +}
> > > +EXPORT_SYMBOL(tinydrm_disable_backlight);
> > > +#endif
> > 
> > These look like they really should be part of the backlight subsystem. I
> > don't see anything DRM specific about them. Well, except for the error
> > messages.
> 
> So this is a bit an unpopular opinion with some folks, but I don't require
> anyone to submit new code to subsystems outside of drm for new drivers.
> Simply because it takes months to get stuff landed, and in general it's
> not worth the trouble.

"Not worth the trouble" is very subjective. If you look at the Linux
kernel in general, one of the reasons why it works so well is because
the changes we make apply to the kernel as a whole. Yes, sometimes that
makes things more difficult and time-consuming, but it also means that
the end result will be much more widely usable and therefore benefits
everyone else in return. In my opinion that's a large part of why the
kernel is so successful.

> We have piles of stuff in drm and drm drivers that should be in core but
> isn't.
> 
> Imo the only reasonable way is to merge as-is, then follow-up with a patch
> series to move the helper into the right subsystem. Most often
> unfortunately that follow-up patch series will just die.

Of course follow-up series die. That's because nobody cares to follow-up
once their code has been merged.

Collecting our own helpers or variants of subsystems is a great way of
isolating ourselves from the rest of the community. I don't think that's
a good solution in the long run at all.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Jani Nikula]

On Tue, 31 Jan 2017, Noralf Trønnes <noralf@tronnes.org> wrote:
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE

BACKLIGHT_CLASS_DEVICE is a tristate, you'll want

#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

and probably either

	depends on BACKLIGHT_CLASS_DEVICE

or

	depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n

in your Kconfig.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Jani Nikula]

On Tue, 31 Jan 2017, Noralf Trønnes <noralf@tronnes.org> wrote:
> +const struct file_operations tinydrm_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= drm_open,
> +	.release	= drm_release,
> +	.unlocked_ioctl	= drm_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl	= drm_compat_ioctl,
> +#endif

No need for the ifdefs, drm_compat_ioctl will be defined NULL for
CONFIG_COMPAT=n.

BR,
Jani.


> +	.poll		= drm_poll,
> +	.read		= drm_read,
> +	.llseek		= no_llseek,
> +	.mmap		= drm_gem_cma_mmap,
> +};
> +EXPORT_SYMBOL(tinydrm_fops);

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4016 bytes --]

On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote:
> On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com>
> wrote:
> 
> > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight);
> > > > > +#endif
> > > >
> > > > These look like they really should be part of the backlight subsystem.
> > I
> > > > don't see anything DRM specific about them. Well, except for the error
> > > > messages.
> > >
> > > So this is a bit an unpopular opinion with some folks, but I don't
> > require
> > > anyone to submit new code to subsystems outside of drm for new drivers.
> > > Simply because it takes months to get stuff landed, and in general it's
> > > not worth the trouble.
> >
> > "Not worth the trouble" is very subjective. If you look at the Linux
> > kernel in general, one of the reasons why it works so well is because
> > the changes we make apply to the kernel as a whole. Yes, sometimes that
> > makes things more difficult and time-consuming, but it also means that
> > the end result will be much more widely usable and therefore benefits
> > everyone else in return. In my opinion that's a large part of why the
> > kernel is so successful.
> >
> > > We have piles of stuff in drm and drm drivers that should be in core but
> > > isn't.
> > >
> > > Imo the only reasonable way is to merge as-is, then follow-up with a
> > patch
> > > series to move the helper into the right subsystem. Most often
> > > unfortunately that follow-up patch series will just die.
> >
> > Of course follow-up series die. That's because nobody cares to follow-up
> > once their code has been merged.
> >
> > Collecting our own helpers or variants of subsystems is a great way of
> > isolating ourselves from the rest of the community. I don't think that's
> > a good solution in the long run at all.
> >
> 
> We have a bunch of patch series that we resubmit for months and they go
> exactly nowhere. They don't die because we stop caring, they die because
> they die. Some of them we even need to constantly rebase and carry around
> in drm-tip since our CI would Oops or spew WARNIGs all over the place.
> There's simply some areas of the kernel which seem overloaded under patches
> and no one is willing or able to fix things, and I can't fix the entire
> kernel. Nor expect contributors (who have much less political weight to
> throw around than me) to do that and succeed. And we don't end up with
> worse code in the drm subsystem, since we can still do the refactoring
> within drm helpers and end up with clean drivers.
> 
> I fully agree that it's not great for the kernel's future, but when I'm
> stuck with the option to get shit done or burning out playing the
> upstreaming game, the choice is easy. And in the end I care about open
> source gfx much more than the kernel, and I think for open source gfx's
> success it's crucial that we're welcoming to new contributors and don't
> throw up massive roadblocks. Open source gfx is tiny and still far away
> from world domination, we need _lots_ more people. If that means routing
> around other subsystems for them, I'm all for it.

I can't say I fully agree with that sentiment. I do see how routing
around subsystems can be useful occasionally. If nobody will merge the
code, or if nobody cares, then by all means, let's make them DRM-
specific helpers.

But I think we need to at least try to do the right thing. If only to
teach people what the right way is. If we start accepting such things
by default, how can we expect contributors to even try?

I also think that contributors will often end up contributing not only
to DRM but to the kernel as a whole. As such it should be part of our
mentoring to teach them about how the process works as a rule, even if
the occasional exception is necessary to get things done.

In this particular case, I know for a fact that both backlight and SPI
maintainers are very responsive, so that's not a good excuse.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/7] drm/tinydrm: Add MIPI DBI support

[Jani Nikula]

On Tue, 31 Jan 2017, Noralf Trønnes <noralf@tronnes.org> wrote:
> +static bool mipi_dbi_command_is_read(struct mipi_dbi *mipi, u8 cmd)

Okay this is one giant bikeshedding nitpick, but here goes anyway. Why
do you call MIPI DBI just "mipi", here and all around? Why not
"mipi_dbi" or "dbi"? We never talk about "vesa" displays or use that as
a shorthand when we refer to Display Port either.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v3 3/7] drm/tinydrm: Add MIPI DBI support

[Jani Nikula]

On Mon, 06 Feb 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Jan 31, 2017 at 05:03:15PM +0100, Noralf Trønnes wrote:
>> Add support for MIPI DBI compatible controllers.
>> Interface type C option 1 and 3 are supported (SPI).
>> 
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>  Documentation/gpu/tinydrm.rst      |   12 +
>>  drivers/gpu/drm/tinydrm/Kconfig    |    3 +
>>  drivers/gpu/drm/tinydrm/Makefile   |    3 +
>>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 1005 ++++++++++++++++++++++++++++++++++++
>>  include/drm/tinydrm/mipi-dbi.h     |  107 ++++
>>  5 files changed, 1130 insertions(+)
>>  create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
>>  create mode 100644 include/drm/tinydrm/mipi-dbi.h
>
> Any reason why this is in the tinydrm subdirectory? Looks like this
> could be useful to drivers outside of it.
>
>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> new file mode 100644
>> index 0000000..5ded299
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> @@ -0,0 +1,1005 @@
>> +/*
>> + * MIPI Display Bus Interface (DBI) LCD controller support
>> + *
>> + * Copyright 2016 Noralf Trønnes
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <drm/tinydrm/mipi-dbi.h>
>> +#include <drm/tinydrm/tinydrm-helpers.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/dma-buf.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +#include <video/mipi_display.h>
>> +
>> +#define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */
>> +
>> +#define DCS_POWER_MODE_DISPLAY			BIT(2)
>> +#define DCS_POWER_MODE_DISPLAY_NORMAL_MODE	BIT(3)
>> +#define DCS_POWER_MODE_SLEEP_MODE		BIT(4)
>> +#define DCS_POWER_MODE_PARTIAL_MODE		BIT(5)
>> +#define DCS_POWER_MODE_IDLE_MODE		BIT(6)
>> +#define DCS_POWER_MODE_RESERVED_MASK		(BIT(0) | BIT(1) | BIT(7))
>
> Should these perhaps be defined in include/video/mipi_display.h since
> that already defines the MIPI_DCS_GET_POWER_MODE that this is used with?

Agreed.

>
>> +/**
>> + * mipi_dbi_command - MIPI DCS command with optional parameter(s)
>> + * @mipi: MIPI structure
>> + * @cmd: Command
>> + * @seq...: Optional parameter(s)
>> + *
>> + * Send MIPI DCS command to the controller. Use mipi_dbi_command_read() for
>> + * get/read.
>> + *
>> + * Returns:
>> + * Zero on success, negative error code on failure.
>> + */
>> +#define mipi_dbi_command(mipi, cmd, seq...) \
>> +({ \
>> +	u8 d[] = { seq }; \
>> +	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
>> +})
>
> I feel obligated to object to this because I objected to the same macro
> when Andrzej wanted to add the same macro for MIPI DSI. But since I'm
> apparently the only one that doesn't like this, maybe it's time for me
> to embrace this.

I think that's less interesting for MIPI DSI now that we have specific
functions for most standard DCS commands anyway. That also avoids the
need to have a list of read functions like here.

I guess the question is if it feels like too much duplication to have
the DCS commands as functions also for DBI. And whether it's worth
trying to deduplicate DSI and DBI in this case, or if it gets just too
confusing/complicated.

BR,
Jani.


>
> Thierry
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v3 3/7] drm/tinydrm: Add MIPI DBI support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4662 bytes --]

On Mon, Feb 06, 2017 at 01:30:09PM +0200, Jani Nikula wrote:
> On Mon, 06 Feb 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Jan 31, 2017 at 05:03:15PM +0100, Noralf Trønnes wrote:
> >> Add support for MIPI DBI compatible controllers.
> >> Interface type C option 1 and 3 are supported (SPI).
> >> 
> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> ---
> >>  Documentation/gpu/tinydrm.rst      |   12 +
> >>  drivers/gpu/drm/tinydrm/Kconfig    |    3 +
> >>  drivers/gpu/drm/tinydrm/Makefile   |    3 +
> >>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 1005 ++++++++++++++++++++++++++++++++++++
> >>  include/drm/tinydrm/mipi-dbi.h     |  107 ++++
> >>  5 files changed, 1130 insertions(+)
> >>  create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
> >>  create mode 100644 include/drm/tinydrm/mipi-dbi.h
> >
> > Any reason why this is in the tinydrm subdirectory? Looks like this
> > could be useful to drivers outside of it.
> >
> >> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> >> new file mode 100644
> >> index 0000000..5ded299
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> >> @@ -0,0 +1,1005 @@
> >> +/*
> >> + * MIPI Display Bus Interface (DBI) LCD controller support
> >> + *
> >> + * Copyright 2016 Noralf Trønnes
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + */
> >> +
> >> +#include <drm/tinydrm/mipi-dbi.h>
> >> +#include <drm/tinydrm/tinydrm-helpers.h>
> >> +#include <linux/debugfs.h>
> >> +#include <linux/dma-buf.h>
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/module.h>
> >> +#include <linux/regulator/consumer.h>
> >> +#include <linux/spi/spi.h>
> >> +#include <video/mipi_display.h>
> >> +
> >> +#define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */
> >> +
> >> +#define DCS_POWER_MODE_DISPLAY			BIT(2)
> >> +#define DCS_POWER_MODE_DISPLAY_NORMAL_MODE	BIT(3)
> >> +#define DCS_POWER_MODE_SLEEP_MODE		BIT(4)
> >> +#define DCS_POWER_MODE_PARTIAL_MODE		BIT(5)
> >> +#define DCS_POWER_MODE_IDLE_MODE		BIT(6)
> >> +#define DCS_POWER_MODE_RESERVED_MASK		(BIT(0) | BIT(1) | BIT(7))
> >
> > Should these perhaps be defined in include/video/mipi_display.h since
> > that already defines the MIPI_DCS_GET_POWER_MODE that this is used with?
> 
> Agreed.
> 
> >
> >> +/**
> >> + * mipi_dbi_command - MIPI DCS command with optional parameter(s)
> >> + * @mipi: MIPI structure
> >> + * @cmd: Command
> >> + * @seq...: Optional parameter(s)
> >> + *
> >> + * Send MIPI DCS command to the controller. Use mipi_dbi_command_read() for
> >> + * get/read.
> >> + *
> >> + * Returns:
> >> + * Zero on success, negative error code on failure.
> >> + */
> >> +#define mipi_dbi_command(mipi, cmd, seq...) \
> >> +({ \
> >> +	u8 d[] = { seq }; \
> >> +	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
> >> +})
> >
> > I feel obligated to object to this because I objected to the same macro
> > when Andrzej wanted to add the same macro for MIPI DSI. But since I'm
> > apparently the only one that doesn't like this, maybe it's time for me
> > to embrace this.
> 
> I think that's less interesting for MIPI DSI now that we have specific
> functions for most standard DCS commands anyway. That also avoids the
> need to have a list of read functions like here.
> 
> I guess the question is if it feels like too much duplication to have
> the DCS commands as functions also for DBI. And whether it's worth
> trying to deduplicate DSI and DBI in this case, or if it gets just too
> confusing/complicated.

The problem with this is that we can't possible have all accesses
covered by specific functions. Most of the calls using this function or
macro are for panel-specific commands that often take an arbitrary
number of arguments.

I personally think that Maxime's implementation for ST7789V is somewhat
nicer, though it would require some more work for DSI and DBI to have it
all stashed into a single buffer for the transfer.

Anyway, my main concern with these is that we need to allow for proper
error handling, and the above correctly propagates error codes, so the
matter that it's a macro with a variable argument list and might store
potentially huge arrays on the stack could be addressed when they
become really problematic, rather than just annoying me because they
aren't pretty.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/7] drm/tinydrm: Add MIPI DBI support

[Andrzej Hajda]

On 06.02.2017 12:53, Thierry Reding wrote:
> On Mon, Feb 06, 2017 at 01:30:09PM +0200, Jani Nikula wrote:
>> On Mon, 06 Feb 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Tue, Jan 31, 2017 at 05:03:15PM +0100, Noralf Trønnes wrote:
>>>> Add support for MIPI DBI compatible controllers.
>>>> Interface type C option 1 and 3 are supported (SPI).
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>  Documentation/gpu/tinydrm.rst      |   12 +
>>>>  drivers/gpu/drm/tinydrm/Kconfig    |    3 +
>>>>  drivers/gpu/drm/tinydrm/Makefile   |    3 +
>>>>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 1005 ++++++++++++++++++++++++++++++++++++
>>>>  include/drm/tinydrm/mipi-dbi.h     |  107 ++++
>>>>  5 files changed, 1130 insertions(+)
>>>>  create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>>  create mode 100644 include/drm/tinydrm/mipi-dbi.h
>>> Any reason why this is in the tinydrm subdirectory? Looks like this
>>> could be useful to drivers outside of it.
>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>> new file mode 100644
>>>> index 0000000..5ded299
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>> @@ -0,0 +1,1005 @@
>>>> +/*
>>>> + * MIPI Display Bus Interface (DBI) LCD controller support
>>>> + *
>>>> + * Copyright 2016 Noralf Trønnes
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + */
>>>> +
>>>> +#include <drm/tinydrm/mipi-dbi.h>
>>>> +#include <drm/tinydrm/tinydrm-helpers.h>
>>>> +#include <linux/debugfs.h>
>>>> +#include <linux/dma-buf.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/spi/spi.h>
>>>> +#include <video/mipi_display.h>
>>>> +
>>>> +#define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */
>>>> +
>>>> +#define DCS_POWER_MODE_DISPLAY			BIT(2)
>>>> +#define DCS_POWER_MODE_DISPLAY_NORMAL_MODE	BIT(3)
>>>> +#define DCS_POWER_MODE_SLEEP_MODE		BIT(4)
>>>> +#define DCS_POWER_MODE_PARTIAL_MODE		BIT(5)
>>>> +#define DCS_POWER_MODE_IDLE_MODE		BIT(6)
>>>> +#define DCS_POWER_MODE_RESERVED_MASK		(BIT(0) | BIT(1) | BIT(7))
>>> Should these perhaps be defined in include/video/mipi_display.h since
>>> that already defines the MIPI_DCS_GET_POWER_MODE that this is used with?
>> Agreed.
>>
>>>> +/**
>>>> + * mipi_dbi_command - MIPI DCS command with optional parameter(s)
>>>> + * @mipi: MIPI structure
>>>> + * @cmd: Command
>>>> + * @seq...: Optional parameter(s)
>>>> + *
>>>> + * Send MIPI DCS command to the controller. Use mipi_dbi_command_read() for
>>>> + * get/read.
>>>> + *
>>>> + * Returns:
>>>> + * Zero on success, negative error code on failure.
>>>> + */
>>>> +#define mipi_dbi_command(mipi, cmd, seq...) \
>>>> +({ \
>>>> +	u8 d[] = { seq }; \
>>>> +	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
>>>> +})
>>> I feel obligated to object to this because I objected to the same macro
>>> when Andrzej wanted to add the same macro for MIPI DSI. But since I'm
>>> apparently the only one that doesn't like this, maybe it's time for me
>>> to embrace this.
>> I think that's less interesting for MIPI DSI now that we have specific
>> functions for most standard DCS commands anyway. That also avoids the
>> need to have a list of read functions like here.
>>
>> I guess the question is if it feels like too much duplication to have
>> the DCS commands as functions also for DBI. And whether it's worth
>> trying to deduplicate DSI and DBI in this case, or if it gets just too
>> confusing/complicated.
> The problem with this is that we can't possible have all accesses
> covered by specific functions. Most of the calls using this function or
> macro are for panel-specific commands that often take an arbitrary
> number of arguments.
>
> I personally think that Maxime's implementation for ST7789V is somewhat
> nicer, though it would require some more work for DSI and DBI to have it
> all stashed into a single buffer for the transfer.
>
> Anyway, my main concern with these is that we need to allow for proper
> error handling, and the above correctly propagates error codes, so the
> matter that it's a macro with a variable argument list and might store
> potentially huge arrays on the stack could be addressed when they
> become really problematic, rather than just annoying me because they
> aren't pretty.

You can always add compile time check for sequence length to the macro
[1]. Other thing is that sequence is often constant, in such case, to
save few bits of space, it can be stored in 'static const' array, but
for this array passed to mipi_dbi_command_buf should be constant. And of
course it requires another macro.

[1]:
http://lxr.free-electrons.com/source/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c?v=4.4#L169

Regards
Andrzej

>
> Thierry
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/7] drm/tinydrm: Add MIPI DBI support

[Noralf Trønnes]

(Adding Maxime)

Den 06.02.2017 13.34, skrev Andrzej Hajda:
> On 06.02.2017 12:53, Thierry Reding wrote:
>> On Mon, Feb 06, 2017 at 01:30:09PM +0200, Jani Nikula wrote:
>>> On Mon, 06 Feb 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
>>>> On Tue, Jan 31, 2017 at 05:03:15PM +0100, Noralf Trønnes wrote:
>>>>> Add support for MIPI DBI compatible controllers.
>>>>> Interface type C option 1 and 3 are supported (SPI).
>>>>>
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>   Documentation/gpu/tinydrm.rst      |   12 +
>>>>>   drivers/gpu/drm/tinydrm/Kconfig    |    3 +
>>>>>   drivers/gpu/drm/tinydrm/Makefile   |    3 +
>>>>>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 1005 ++++++++++++++++++++++++++++++++++++
>>>>>   include/drm/tinydrm/mipi-dbi.h     |  107 ++++
>>>>>   5 files changed, 1130 insertions(+)
>>>>>   create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>>>   create mode 100644 include/drm/tinydrm/mipi-dbi.h
>>>> Any reason why this is in the tinydrm subdirectory? Looks like this
>>>> could be useful to drivers outside of it.

I did consider having it outside, but I couldn't find any users in drm
that could benefit from it (there is one backlight driver).
But now there's Maxime's panel driver.

How about something like this:
(I have not included the framebuffer dirty function since it will only
be used in tinydrm anyway)

include/drm/drm_mipi_dbi.h:

struct mipi_dbi_device;

/**
  * mipi_dbi_dcs_write - MIPI DCS command with optional parameter(s)
  * @dbi: MIPI DBI structure
  * @cmd: Command
  * @seq...: Optional parameter(s)
  *
  * Send MIPI DCS command to the controller. Use 
mipi_dbi_dcs_read_buffer() for
  * get/read commands.
  *
  * Returns:
  * Zero on success, negative error code on failure.
  */
#define mipi_dbi_dcs_write(dbi, cmd, seq...) \
({ \
     const u8 d[] = { seq }; \
     BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too big for 
stack");\
     mipi_dbi_dcs_write_buffer(dbi, cmd, d, ARRAY_SIZE(d)); \
})

...

drivers/gpu/drm/drm_mipi_dbi.c:

struct mipi_dbi_device {
     struct mutex lock;
     int (*write)(struct mipi_dbi_device *dbi, u8 cmd, const u8 *par,
              size_t num);
     int (*read)(struct mipi_dbi_device *dbi, u8 cmd, u8 *par, size_t num);
     bool swap16;
};

/* MIPI DBI Type C options 1 and 3 */
struct mipi_dbi_spi_device {
     struct mipi_dbi_device dbi;
     struct spi_device *spi;
     struct gpio_desc *dc;
};

/*
  * MIPI DBI Type B - Intel 8080 type parallel bus
  * I need this to fully convert staging/fbtft to drm
  */
struct mipi_dbi_i80_device {
     struct mipi_dbi_device dbi;
     struct device *dev;
     struct gpio_desc *cs;
     struct gpio_desc *dc;
     struct gpio_desc *wr;
     struct gpio_descs *db;
};

/**
  * mipi_dbi_get_swap16 - Is byteswapping 16-bit pixel data needed?
  * @dbi: MIPI DBI device
  *
  * Byte swapping 16-bit pixel data is necessary if the bus can't 
support 16-bit
  * big endian transfers (e.g. if SPI can only do 8-bit and the machine is
  * little endian). This applies to the MIPI_DCS_WRITE_MEMORY_START command.
  *
  * Returns:
  * True if it's neccesary to swap bytes, false otherwise.
  */
bool mipi_dbi_get_swap16(struct mipi_dbi_device *dbi)
{
     return dbi->swap16;
}

/**
  * mipi_dbi_spi_init - Initialize MIPI DBI SPI device
  * @spi: SPI device
  * @dc: D/C gpio (optional)
  * @writeonly: True if it's not possible to read from the controller.
  *
  * If @dc is set, a Type C Option 3 interface is assumed, if not
  * Type C Option 1 (9-bit).
  *
  * If the SPI master driver doesn't support the necessary bits per word,
  * the following transformation is used:
  *
  * - 9-bit: reorder buffer as 9x 8-bit words, padded with no-op command.
  * - 16-bit (pixel data): if machine is big endian send as 8-bit, if little
  *   endian the user is responsible for swapping the bytes.
  *   See mipi_dbi_get_swap_pixel_bytes().
  *
  * Returns:
  * Pointer to &mipi_dbi_device on success, ERR_PTR on failure.
  */
struct mipi_dbi_device *mipi_dbi_spi_init(struct spi_device *spi,
                       struct gpio_desc *dc, bool writeonly)
{
...
}

struct mipi_dbi_device *mipi_dbi_i80_init(...)


Noralf.

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Daniel Vetter]

On Mon, Feb 06, 2017 at 12:08:47PM +0100, Thierry Reding wrote:
> On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote:
> > On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com>
> > wrote:
> > 
> > > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight);
> > > > > > +#endif
> > > > >
> > > > > These look like they really should be part of the backlight subsystem.
> > > I
> > > > > don't see anything DRM specific about them. Well, except for the error
> > > > > messages.
> > > >
> > > > So this is a bit an unpopular opinion with some folks, but I don't
> > > require
> > > > anyone to submit new code to subsystems outside of drm for new drivers.
> > > > Simply because it takes months to get stuff landed, and in general it's
> > > > not worth the trouble.
> > >
> > > "Not worth the trouble" is very subjective. If you look at the Linux
> > > kernel in general, one of the reasons why it works so well is because
> > > the changes we make apply to the kernel as a whole. Yes, sometimes that
> > > makes things more difficult and time-consuming, but it also means that
> > > the end result will be much more widely usable and therefore benefits
> > > everyone else in return. In my opinion that's a large part of why the
> > > kernel is so successful.
> > >
> > > > We have piles of stuff in drm and drm drivers that should be in core but
> > > > isn't.
> > > >
> > > > Imo the only reasonable way is to merge as-is, then follow-up with a
> > > patch
> > > > series to move the helper into the right subsystem. Most often
> > > > unfortunately that follow-up patch series will just die.
> > >
> > > Of course follow-up series die. That's because nobody cares to follow-up
> > > once their code has been merged.
> > >
> > > Collecting our own helpers or variants of subsystems is a great way of
> > > isolating ourselves from the rest of the community. I don't think that's
> > > a good solution in the long run at all.
> > >
> > 
> > We have a bunch of patch series that we resubmit for months and they go
> > exactly nowhere. They don't die because we stop caring, they die because
> > they die. Some of them we even need to constantly rebase and carry around
> > in drm-tip since our CI would Oops or spew WARNIGs all over the place.
> > There's simply some areas of the kernel which seem overloaded under patches
> > and no one is willing or able to fix things, and I can't fix the entire
> > kernel. Nor expect contributors (who have much less political weight to
> > throw around than me) to do that and succeed. And we don't end up with
> > worse code in the drm subsystem, since we can still do the refactoring
> > within drm helpers and end up with clean drivers.
> > 
> > I fully agree that it's not great for the kernel's future, but when I'm
> > stuck with the option to get shit done or burning out playing the
> > upstreaming game, the choice is easy. And in the end I care about open
> > source gfx much more than the kernel, and I think for open source gfx's
> > success it's crucial that we're welcoming to new contributors and don't
> > throw up massive roadblocks. Open source gfx is tiny and still far away
> > from world domination, we need _lots_ more people. If that means routing
> > around other subsystems for them, I'm all for it.
> 
> I can't say I fully agree with that sentiment. I do see how routing
> around subsystems can be useful occasionally. If nobody will merge the
> code, or if nobody cares, then by all means, let's make them DRM-
> specific helpers.
> 
> But I think we need to at least try to do the right thing. If only to
> teach people what the right way is. If we start accepting such things
> by default, how can we expect contributors to even try?
> 
> I also think that contributors will often end up contributing not only
> to DRM but to the kernel as a whole. As such it should be part of our
> mentoring to teach them about how the process works as a rule, even if
> the occasional exception is necessary to get things done.
> 
> In this particular case, I know for a fact that both backlight and SPI
> maintainers are very responsive, so that's not a good excuse.

I definitely don't want that we don't attempt this. But brought from years
of experience, I recommend to merge first (with pre-refactoring already
applied, but helpers only extracted, not yet at the right spot), and then
follow up with. Because on average, there's way too many trees with
overloaded maintainers who maybe look at your patch once per kernel
release cycle.

If you know that backlight and spi isn't one of these areas (anything that
goes through takashi/sound is a similar good experience for us on the i915
side), then I guess we can try. But then Noralf has already written a few
months worth of really great refactoring, and I'm seriously starting to
feel guilty for volunteering him for all of this. Even though he seems to
be really good at it, and seems to not mind, it's getting a bit silly.
Given that I'd say up to Noralf.

In short, there's always a balance.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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


Noralf.

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Noralf Trønnes]

Den 06.02.2017 16.53, skrev Daniel Vetter:
> On Mon, Feb 06, 2017 at 12:08:47PM +0100, Thierry Reding wrote:
>> On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote:
>>> On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com>
>>> wrote:
>>>
>>>>>>> +EXPORT_SYMBOL(tinydrm_disable_backlight);
>>>>>>> +#endif
>>>>>> These look like they really should be part of the backlight subsystem.
>>>> I
>>>>>> don't see anything DRM specific about them. Well, except for the error
>>>>>> messages.
>>>>> So this is a bit an unpopular opinion with some folks, but I don't
>>>> require
>>>>> anyone to submit new code to subsystems outside of drm for new drivers.
>>>>> Simply because it takes months to get stuff landed, and in general it's
>>>>> not worth the trouble.
>>>> "Not worth the trouble" is very subjective. If you look at the Linux
>>>> kernel in general, one of the reasons why it works so well is because
>>>> the changes we make apply to the kernel as a whole. Yes, sometimes that
>>>> makes things more difficult and time-consuming, but it also means that
>>>> the end result will be much more widely usable and therefore benefits
>>>> everyone else in return. In my opinion that's a large part of why the
>>>> kernel is so successful.
>>>>
>>>>> We have piles of stuff in drm and drm drivers that should be in core but
>>>>> isn't.
>>>>>
>>>>> Imo the only reasonable way is to merge as-is, then follow-up with a
>>>> patch
>>>>> series to move the helper into the right subsystem. Most often
>>>>> unfortunately that follow-up patch series will just die.
>>>> Of course follow-up series die. That's because nobody cares to follow-up
>>>> once their code has been merged.
>>>>
>>>> Collecting our own helpers or variants of subsystems is a great way of
>>>> isolating ourselves from the rest of the community. I don't think that's
>>>> a good solution in the long run at all.
>>>>
>>> We have a bunch of patch series that we resubmit for months and they go
>>> exactly nowhere. They don't die because we stop caring, they die because
>>> they die. Some of them we even need to constantly rebase and carry around
>>> in drm-tip since our CI would Oops or spew WARNIGs all over the place.
>>> There's simply some areas of the kernel which seem overloaded under patches
>>> and no one is willing or able to fix things, and I can't fix the entire
>>> kernel. Nor expect contributors (who have much less political weight to
>>> throw around than me) to do that and succeed. And we don't end up with
>>> worse code in the drm subsystem, since we can still do the refactoring
>>> within drm helpers and end up with clean drivers.
>>>
>>> I fully agree that it's not great for the kernel's future, but when I'm
>>> stuck with the option to get shit done or burning out playing the
>>> upstreaming game, the choice is easy. And in the end I care about open
>>> source gfx much more than the kernel, and I think for open source gfx's
>>> success it's crucial that we're welcoming to new contributors and don't
>>> throw up massive roadblocks. Open source gfx is tiny and still far away
>>> from world domination, we need _lots_ more people. If that means routing
>>> around other subsystems for them, I'm all for it.
>> I can't say I fully agree with that sentiment. I do see how routing
>> around subsystems can be useful occasionally. If nobody will merge the
>> code, or if nobody cares, then by all means, let's make them DRM-
>> specific helpers.
>>
>> But I think we need to at least try to do the right thing. If only to
>> teach people what the right way is. If we start accepting such things
>> by default, how can we expect contributors to even try?
>>
>> I also think that contributors will often end up contributing not only
>> to DRM but to the kernel as a whole. As such it should be part of our
>> mentoring to teach them about how the process works as a rule, even if
>> the occasional exception is necessary to get things done.
>>
>> In this particular case, I know for a fact that both backlight and SPI
>> maintainers are very responsive, so that's not a good excuse.
> I definitely don't want that we don't attempt this. But brought from years
> of experience, I recommend to merge first (with pre-refactoring already
> applied, but helpers only extracted, not yet at the right spot), and then
> follow up with. Because on average, there's way too many trees with
> overloaded maintainers who maybe look at your patch once per kernel
> release cycle.
>
> If you know that backlight and spi isn't one of these areas (anything that
> goes through takashi/sound is a similar good experience for us on the i915
> side), then I guess we can try. But then Noralf has already written a few
> months worth of really great refactoring, and I'm seriously starting to
> feel guilty for volunteering him for all of this. Even though he seems to
> be really good at it, and seems to not mind, it's getting a bit silly.
> Given that I'd say up to Noralf.
>
> In short, there's always a balance.

Yes, it's a balance between the perfect and not's so perfect,
the professinal and the amateur, and the drm expert and the newbie.

If I knew how much time I would have to spend on tinydrm to get it
merged, then I would never have started it. I wondered, a couple of
months back, if I should just cut my losses and move to something else.
dri-devel is a friendly place and I do get help to keep moving, but I
get the feeling that drm is really a place for professionals that write
kernel code 50 hours a week. And there's nothing wrong in that, drm is
complex and maybe that kind of expertice and work-hours are needed to
do work here.

It's not that I plan on backing out now, I'm just putting down a few
words about the challenge it has been for me as a hobbyist to meet drm.


As for the specifics,

Backlight enable/disable helpers, I haven't thought about those as
backlight subsystem helpers. But I see some drm panel drivers that do
the same, so it makes sense.
But what I'm feeling is this:
If I'm expected to get everything right, then I will "never" get this 
merged.
This thing by itself isn't much, but adding up every little thing
amounts to a lot. And it's not just "make this patch", it's probably
clean up the drivers as well :-)

tinydrm_spi_transfer() can print the content of the buffer. This is
important at least until I have seen some real use of the drivers.
I have emulation code for handling bit widths not supported by the SPI
controller, so I want to see what goes over the wire. SPI core uses
trace events to track what's going on, and I don't think I can get
buffer dumping into that code.

Don't get me wrong Thierry, I do appreciate that you take the time to
look at the code. I'm just frustrated that it takes so long to get this
right. I thought that all I needed now was a DT maintainer ack :-)


Noralf.

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Dave Airlie]

>
> I definitely don't want that we don't attempt this. But brought from years
> of experience, I recommend to merge first (with pre-refactoring already
> applied, but helpers only extracted, not yet at the right spot), and then
> follow up with. Because on average, there's way too many trees with
> overloaded maintainers who maybe look at your patch once per kernel
> release cycle.
>
> If you know that backlight and spi isn't one of these areas (anything that
> goes through takashi/sound is a similar good experience for us on the i915
> side), then I guess we can try. But then Noralf has already written a few
> months worth of really great refactoring, and I'm seriously starting to
> feel guilty for volunteering him for all of this. Even though he seems to
> be really good at it, and seems to not mind, it's getting a bit silly.
> Given that I'd say up to Noralf.
>
> In short, there's always a balance.

I don't think we can make a rule for this, it will always depend on the
code. There is always going to be stuff we put in drm that should go
elsewhere, and stuff that is elsewhere that drm should use.

I think however if we do add stuff like this, someone should keep track
of them and try to make them get further into the kernel. In this case
I don't think the patches are too insane to keep in drm and refactor up
later, in other cases I'm sure it'll be lot more obvious (i.e. we could
make the same argument for chunks of DAL :-)

Dave.

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Rob Herring]

On Mon, Feb 6, 2017 at 5:08 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote:
>> On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com>
>> wrote:
>>
>> > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight);
>> > > > > +#endif
>> > > >
>> > > > These look like they really should be part of the backlight subsystem.
>> > I
>> > > > don't see anything DRM specific about them. Well, except for the error
>> > > > messages.
>> > >
>> > > So this is a bit an unpopular opinion with some folks, but I don't
>> > require
>> > > anyone to submit new code to subsystems outside of drm for new drivers.
>> > > Simply because it takes months to get stuff landed, and in general it's
>> > > not worth the trouble.
>> >
>> > "Not worth the trouble" is very subjective. If you look at the Linux
>> > kernel in general, one of the reasons why it works so well is because
>> > the changes we make apply to the kernel as a whole. Yes, sometimes that
>> > makes things more difficult and time-consuming, but it also means that
>> > the end result will be much more widely usable and therefore benefits
>> > everyone else in return. In my opinion that's a large part of why the
>> > kernel is so successful.
>> >
>> > > We have piles of stuff in drm and drm drivers that should be in core but
>> > > isn't.
>> > >
>> > > Imo the only reasonable way is to merge as-is, then follow-up with a
>> > patch
>> > > series to move the helper into the right subsystem. Most often
>> > > unfortunately that follow-up patch series will just die.
>> >
>> > Of course follow-up series die. That's because nobody cares to follow-up
>> > once their code has been merged.
>> >
>> > Collecting our own helpers or variants of subsystems is a great way of
>> > isolating ourselves from the rest of the community. I don't think that's
>> > a good solution in the long run at all.
>> >
>>
>> We have a bunch of patch series that we resubmit for months and they go
>> exactly nowhere. They don't die because we stop caring, they die because
>> they die. Some of them we even need to constantly rebase and carry around
>> in drm-tip since our CI would Oops or spew WARNIGs all over the place.
>> There's simply some areas of the kernel which seem overloaded under patches
>> and no one is willing or able to fix things, and I can't fix the entire
>> kernel. Nor expect contributors (who have much less political weight to
>> throw around than me) to do that and succeed. And we don't end up with
>> worse code in the drm subsystem, since we can still do the refactoring
>> within drm helpers and end up with clean drivers.
>>
>> I fully agree that it's not great for the kernel's future, but when I'm
>> stuck with the option to get shit done or burning out playing the
>> upstreaming game, the choice is easy. And in the end I care about open
>> source gfx much more than the kernel, and I think for open source gfx's
>> success it's crucial that we're welcoming to new contributors and don't
>> throw up massive roadblocks. Open source gfx is tiny and still far away
>> from world domination, we need _lots_ more people. If that means routing
>> around other subsystems for them, I'm all for it.
>
> I can't say I fully agree with that sentiment. I do see how routing
> around subsystems can be useful occasionally. If nobody will merge the
> code, or if nobody cares, then by all means, let's make them DRM-
> specific helpers.

In this case, these backlight helpers aren't even common across DRM.
They are tinydrm specific, but only in name and location. They look
like they'd be helpful to panel-simple.c and other panel drivers, too.
:)

Who's to blame for duplication within DRM then? If only I had 1
location of OF graph code to clean-up... I get new DT functions all
the time that other subsystems want, so I don't think the problem lies
there.

> But I think we need to at least try to do the right thing. If only to
> teach people what the right way is. If we start accepting such things
> by default, how can we expect contributors to even try?
>
> I also think that contributors will often end up contributing not only
> to DRM but to the kernel as a whole. As such it should be part of our
> mentoring to teach them about how the process works as a rule, even if
> the occasional exception is necessary to get things done.

Yes, it's important for contributors to learn to avoid "the platform
problem"[1].

Rob

[1] https://lwn.net/Articles/443531/

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

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Daniel Vetter]

On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote:
> >
> > I definitely don't want that we don't attempt this. But brought from years
> > of experience, I recommend to merge first (with pre-refactoring already
> > applied, but helpers only extracted, not yet at the right spot), and then
> > follow up with. Because on average, there's way too many trees with
> > overloaded maintainers who maybe look at your patch once per kernel
> > release cycle.
> >
> > If you know that backlight and spi isn't one of these areas (anything that
> > goes through takashi/sound is a similar good experience for us on the i915
> > side), then I guess we can try. But then Noralf has already written a few
> > months worth of really great refactoring, and I'm seriously starting to
> > feel guilty for volunteering him for all of this. Even though he seems to
> > be really good at it, and seems to not mind, it's getting a bit silly.
> > Given that I'd say up to Noralf.
> >
> > In short, there's always a balance.
> 
> I don't think we can make a rule for this, it will always depend on the
> code. There is always going to be stuff we put in drm that should go
> elsewhere, and stuff that is elsewhere that drm should use.
> 
> I think however if we do add stuff like this, someone should keep track
> of them and try to make them get further into the kernel. In this case
> I don't think the patches are too insane to keep in drm and refactor up
> later, in other cases I'm sure it'll be lot more obvious (i.e. we could
> make the same argument for chunks of DAL :-)

DAL's not under this exception, since DAL is all about using drm stuff
more (and maybe fixing a few things in drm helpers). My concern here is
only about going outside of drm, where at least sometimes it's just utter
pain to get even the most trivial bits merged. And holding up an entire
driver for that seems silly, hence merge first, try to further refactor
later.

For DAL amd has started to work on things properly, and since Alex is now
drm-misc committer that should all progress reasonably quickly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Daniel Vetter]

On Mon, Feb 06, 2017 at 04:55:55PM -0600, Rob Herring wrote:
> On Mon, Feb 6, 2017 at 5:08 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote:
> >> On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com>
> >> wrote:
> >>
> >> > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight);
> >> > > > > +#endif
> >> > > >
> >> > > > These look like they really should be part of the backlight subsystem.
> >> > I
> >> > > > don't see anything DRM specific about them. Well, except for the error
> >> > > > messages.
> >> > >
> >> > > So this is a bit an unpopular opinion with some folks, but I don't
> >> > require
> >> > > anyone to submit new code to subsystems outside of drm for new drivers.
> >> > > Simply because it takes months to get stuff landed, and in general it's
> >> > > not worth the trouble.
> >> >
> >> > "Not worth the trouble" is very subjective. If you look at the Linux
> >> > kernel in general, one of the reasons why it works so well is because
> >> > the changes we make apply to the kernel as a whole. Yes, sometimes that
> >> > makes things more difficult and time-consuming, but it also means that
> >> > the end result will be much more widely usable and therefore benefits
> >> > everyone else in return. In my opinion that's a large part of why the
> >> > kernel is so successful.
> >> >
> >> > > We have piles of stuff in drm and drm drivers that should be in core but
> >> > > isn't.
> >> > >
> >> > > Imo the only reasonable way is to merge as-is, then follow-up with a
> >> > patch
> >> > > series to move the helper into the right subsystem. Most often
> >> > > unfortunately that follow-up patch series will just die.
> >> >
> >> > Of course follow-up series die. That's because nobody cares to follow-up
> >> > once their code has been merged.
> >> >
> >> > Collecting our own helpers or variants of subsystems is a great way of
> >> > isolating ourselves from the rest of the community. I don't think that's
> >> > a good solution in the long run at all.
> >> >
> >>
> >> We have a bunch of patch series that we resubmit for months and they go
> >> exactly nowhere. They don't die because we stop caring, they die because
> >> they die. Some of them we even need to constantly rebase and carry around
> >> in drm-tip since our CI would Oops or spew WARNIGs all over the place.
> >> There's simply some areas of the kernel which seem overloaded under patches
> >> and no one is willing or able to fix things, and I can't fix the entire
> >> kernel. Nor expect contributors (who have much less political weight to
> >> throw around than me) to do that and succeed. And we don't end up with
> >> worse code in the drm subsystem, since we can still do the refactoring
> >> within drm helpers and end up with clean drivers.
> >>
> >> I fully agree that it's not great for the kernel's future, but when I'm
> >> stuck with the option to get shit done or burning out playing the
> >> upstreaming game, the choice is easy. And in the end I care about open
> >> source gfx much more than the kernel, and I think for open source gfx's
> >> success it's crucial that we're welcoming to new contributors and don't
> >> throw up massive roadblocks. Open source gfx is tiny and still far away
> >> from world domination, we need _lots_ more people. If that means routing
> >> around other subsystems for them, I'm all for it.
> >
> > I can't say I fully agree with that sentiment. I do see how routing
> > around subsystems can be useful occasionally. If nobody will merge the
> > code, or if nobody cares, then by all means, let's make them DRM-
> > specific helpers.
> 
> In this case, these backlight helpers aren't even common across DRM.
> They are tinydrm specific, but only in name and location. They look
> like they'd be helpful to panel-simple.c and other panel drivers, too.
> :)
> 
> Who's to blame for duplication within DRM then? If only I had 1
> location of OF graph code to clean-up... I get new DT functions all
> the time that other subsystems want, so I don't think the problem lies
> there.
> 
> > But I think we need to at least try to do the right thing. If only to
> > teach people what the right way is. If we start accepting such things
> > by default, how can we expect contributors to even try?
> >
> > I also think that contributors will often end up contributing not only
> > to DRM but to the kernel as a whole. As such it should be part of our
> > mentoring to teach them about how the process works as a rule, even if
> > the occasional exception is necessary to get things done.
> 
> Yes, it's important for contributors to learn to avoid "the platform
> problem"[1].

Stuff Noralf has done over the past few months to get tinydrm merged:
- proper deferred io support in fbdev helpers
- refactoring all drivers to use the same that rolled their own
- bunch of refactoring all around in drm core
- remove the debugfs cleanup code from drivers and move into core

I don't think you can make a "platform problem" case here at all. And as
I've said (and Noralf seems to go that way too) I think this is starting
to get a bit silly, and in my opinion better to merge now and polish
further once merged. At least if ever drm driver would need to do this
much refactoring compared to the code size before we allow it to land I
don't think we'd have any drivers really.

So can we get some acks for landing this please? Or do we need to bikeshed
this a few more months to make sure it doesn't ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote:
> >
> > I definitely don't want that we don't attempt this. But brought from years
> > of experience, I recommend to merge first (with pre-refactoring already
> > applied, but helpers only extracted, not yet at the right spot), and then
> > follow up with. Because on average, there's way too many trees with
> > overloaded maintainers who maybe look at your patch once per kernel
> > release cycle.
> >
> > If you know that backlight and spi isn't one of these areas (anything that
> > goes through takashi/sound is a similar good experience for us on the i915
> > side), then I guess we can try. But then Noralf has already written a few
> > months worth of really great refactoring, and I'm seriously starting to
> > feel guilty for volunteering him for all of this. Even though he seems to
> > be really good at it, and seems to not mind, it's getting a bit silly.
> > Given that I'd say up to Noralf.
> >
> > In short, there's always a balance.
> 
> I don't think we can make a rule for this, it will always depend on the
> code. There is always going to be stuff we put in drm that should go
> elsewhere, and stuff that is elsewhere that drm should use.
> 
> I think however if we do add stuff like this, someone should keep track
> of them and try to make them get further into the kernel.

Yes, I think having some sort of TODO in drivers/gpu/drm could help
track things that we know should eventually be moved out. It could serve
as a list of janitorial tasks for newcomers that want to get their hands
dirty and tackle relatively trivial tasks.

That's not meant to devalue such contributions. Code would be in a
mostly finished form, so it'd be mostly about moving things into the
correct subsystem, interacting with maintainers, making sure things are
kept working along the way, that kind of thing. No in-depth knowledge
of DRM/KMS would be required, but perhaps be picked up along the way.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Daniel Vetter]

On Tue, Feb 07, 2017 at 12:11:28PM +0100, Thierry Reding wrote:
> On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote:
> > >
> > > I definitely don't want that we don't attempt this. But brought from years
> > > of experience, I recommend to merge first (with pre-refactoring already
> > > applied, but helpers only extracted, not yet at the right spot), and then
> > > follow up with. Because on average, there's way too many trees with
> > > overloaded maintainers who maybe look at your patch once per kernel
> > > release cycle.
> > >
> > > If you know that backlight and spi isn't one of these areas (anything that
> > > goes through takashi/sound is a similar good experience for us on the i915
> > > side), then I guess we can try. But then Noralf has already written a few
> > > months worth of really great refactoring, and I'm seriously starting to
> > > feel guilty for volunteering him for all of this. Even though he seems to
> > > be really good at it, and seems to not mind, it's getting a bit silly.
> > > Given that I'd say up to Noralf.
> > >
> > > In short, there's always a balance.
> > 
> > I don't think we can make a rule for this, it will always depend on the
> > code. There is always going to be stuff we put in drm that should go
> > elsewhere, and stuff that is elsewhere that drm should use.
> > 
> > I think however if we do add stuff like this, someone should keep track
> > of them and try to make them get further into the kernel.
> 
> Yes, I think having some sort of TODO in drivers/gpu/drm could help
> track things that we know should eventually be moved out. It could serve
> as a list of janitorial tasks for newcomers that want to get their hands
> dirty and tackle relatively trivial tasks.

We have this list already, it's at: http://www.x.org/wiki/DRMJanitors/

I guess I should highlight it more, maybe even add it to the docs? Eric
just asked about it last week too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 10535 bytes --]

On Mon, Feb 06, 2017 at 11:11:27PM +0100, Noralf Trønnes wrote:
> 
> Den 06.02.2017 16.53, skrev Daniel Vetter:
> > On Mon, Feb 06, 2017 at 12:08:47PM +0100, Thierry Reding wrote:
> > > On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote:
> > > > On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@gmail.com>
> > > > wrote:
> > > > 
> > > > > > > > +EXPORT_SYMBOL(tinydrm_disable_backlight);
> > > > > > > > +#endif
> > > > > > > These look like they really should be part of the backlight subsystem.
> > > > > I
> > > > > > > don't see anything DRM specific about them. Well, except for the error
> > > > > > > messages.
> > > > > > So this is a bit an unpopular opinion with some folks, but I don't
> > > > > require
> > > > > > anyone to submit new code to subsystems outside of drm for new drivers.
> > > > > > Simply because it takes months to get stuff landed, and in general it's
> > > > > > not worth the trouble.
> > > > > "Not worth the trouble" is very subjective. If you look at the Linux
> > > > > kernel in general, one of the reasons why it works so well is because
> > > > > the changes we make apply to the kernel as a whole. Yes, sometimes that
> > > > > makes things more difficult and time-consuming, but it also means that
> > > > > the end result will be much more widely usable and therefore benefits
> > > > > everyone else in return. In my opinion that's a large part of why the
> > > > > kernel is so successful.
> > > > > 
> > > > > > We have piles of stuff in drm and drm drivers that should be in core but
> > > > > > isn't.
> > > > > > 
> > > > > > Imo the only reasonable way is to merge as-is, then follow-up with a
> > > > > patch
> > > > > > series to move the helper into the right subsystem. Most often
> > > > > > unfortunately that follow-up patch series will just die.
> > > > > Of course follow-up series die. That's because nobody cares to follow-up
> > > > > once their code has been merged.
> > > > > 
> > > > > Collecting our own helpers or variants of subsystems is a great way of
> > > > > isolating ourselves from the rest of the community. I don't think that's
> > > > > a good solution in the long run at all.
> > > > > 
> > > > We have a bunch of patch series that we resubmit for months and they go
> > > > exactly nowhere. They don't die because we stop caring, they die because
> > > > they die. Some of them we even need to constantly rebase and carry around
> > > > in drm-tip since our CI would Oops or spew WARNIGs all over the place.
> > > > There's simply some areas of the kernel which seem overloaded under patches
> > > > and no one is willing or able to fix things, and I can't fix the entire
> > > > kernel. Nor expect contributors (who have much less political weight to
> > > > throw around than me) to do that and succeed. And we don't end up with
> > > > worse code in the drm subsystem, since we can still do the refactoring
> > > > within drm helpers and end up with clean drivers.
> > > > 
> > > > I fully agree that it's not great for the kernel's future, but when I'm
> > > > stuck with the option to get shit done or burning out playing the
> > > > upstreaming game, the choice is easy. And in the end I care about open
> > > > source gfx much more than the kernel, and I think for open source gfx's
> > > > success it's crucial that we're welcoming to new contributors and don't
> > > > throw up massive roadblocks. Open source gfx is tiny and still far away
> > > > from world domination, we need _lots_ more people. If that means routing
> > > > around other subsystems for them, I'm all for it.
> > > I can't say I fully agree with that sentiment. I do see how routing
> > > around subsystems can be useful occasionally. If nobody will merge the
> > > code, or if nobody cares, then by all means, let's make them DRM-
> > > specific helpers.
> > > 
> > > But I think we need to at least try to do the right thing. If only to
> > > teach people what the right way is. If we start accepting such things
> > > by default, how can we expect contributors to even try?
> > > 
> > > I also think that contributors will often end up contributing not only
> > > to DRM but to the kernel as a whole. As such it should be part of our
> > > mentoring to teach them about how the process works as a rule, even if
> > > the occasional exception is necessary to get things done.
> > > 
> > > In this particular case, I know for a fact that both backlight and SPI
> > > maintainers are very responsive, so that's not a good excuse.
> > I definitely don't want that we don't attempt this. But brought from years
> > of experience, I recommend to merge first (with pre-refactoring already
> > applied, but helpers only extracted, not yet at the right spot), and then
> > follow up with. Because on average, there's way too many trees with
> > overloaded maintainers who maybe look at your patch once per kernel
> > release cycle.
> > 
> > If you know that backlight and spi isn't one of these areas (anything that
> > goes through takashi/sound is a similar good experience for us on the i915
> > side), then I guess we can try. But then Noralf has already written a few
> > months worth of really great refactoring, and I'm seriously starting to
> > feel guilty for volunteering him for all of this. Even though he seems to
> > be really good at it, and seems to not mind, it's getting a bit silly.
> > Given that I'd say up to Noralf.
> > 
> > In short, there's always a balance.
> 
> Yes, it's a balance between the perfect and not's so perfect,
> the professinal and the amateur, and the drm expert and the newbie.

I may have come across as a bit rough. You've been doing great work,
there's no doubt about that. I was merely raising this issue because I
think it's something that we need to keep an eye out for. We don't want
to end up in a situation where we duplicate code or keep code to DRM
that would be useful outside of DRM.

The link that Rob pointed to is very insightful, and I think it's very
easy to loose touch with other parts of the kernel when you work on one
subsystem (almost) exclusively. It's completely normal, but because of
that I think it's all the more important that we remind ourselves to
look beyond our noses and refactor beyond just DRM where possible.

> If I knew how much time I would have to spend on tinydrm to get it
> merged, then I would never have started it. I wondered, a couple of
> months back, if I should just cut my losses and move to something else.
> dri-devel is a friendly place and I do get help to keep moving, but I
> get the feeling that drm is really a place for professionals that write
> kernel code 50 hours a week. And there's nothing wrong in that, drm is
> complex and maybe that kind of expertice and work-hours are needed to
> do work here.

I don't think that's true in general. If graphics, and especially the
lowlevel parts of it, is something that you're interested in, then
dri-devel is the perfect place for you. It doesn't matter if you work
on it in your spare time or on the day job.

It just so happens that most people end up working on it on the day job
because there's so much work to do that spare time isn't usually enough
to do all you want.

This is probably true for most of the other areas of the kernel, too.
It's not unusual for regular contributors to be offered a job to keep
doing what they do. That's probably why we have a higher degree of
professionalization in the kernel community as a whole. I don't think
that's a bad thing, quite the contrary, it's what we had all been
wishing for for a very long time. I can understand how that might be
frustrating to hobbyists at times, but I hope we're at least not making
things worse by actively discouraging hobbyists.

> It's not that I plan on backing out now, I'm just putting down a few
> words about the challenge it has been for me as a hobbyist to meet drm.
> 
> 
> As for the specifics,
> 
> Backlight enable/disable helpers, I haven't thought about those as
> backlight subsystem helpers. But I see some drm panel drivers that do
> the same, so it makes sense.
> But what I'm feeling is this:
> If I'm expected to get everything right, then I will "never" get this
> merged.
> This thing by itself isn't much, but adding up every little thing
> amounts to a lot. And it's not just "make this patch", it's probably
> clean up the drivers as well :-)

Been there, done that. I think the problem that triggers the kind of
response that I gave is that it's often a handful of people that end up
doing the refactorings. Usually that handful of people is overloaded as
it is, so refactorings end up taking very long and are very tiring. The
easy way out, of course, is to require new patches to do the right thing
to begin with, because people are often motivated by the fact that they
will finally get their patch in. That's of course also a very good way
of discouraging contributions because it may be more than people set out
for.

Unfortunately I don't think there's a good solution for this. But, as
Daniel and Dave have already said, it shouldn't be a blocker for your
series. It'd be really nice if you somehow found the time to follow up,
though, after TinyDRM was merged.

> tinydrm_spi_transfer() can print the content of the buffer. This is
> important at least until I have seen some real use of the drivers.
> I have emulation code for handling bit widths not supported by the SPI
> controller, so I want to see what goes over the wire. SPI core uses
> trace events to track what's going on, and I don't think I can get
> buffer dumping into that code.

I know Mark is a very reasonable person, so I think it can't hurt to
try. Often people will end up rolling buffer dumping code of their own
and having something in the SPI core to do that, possibly hidden behind
a SPI_DEBUG_BUFFER Kconfig option or something like that, would save a
lot of people a lot of time.

Not sure if it would help, but if you end up writing such a patch and
you Cc me when you post it, I'm going to ACK it.

> Don't get me wrong Thierry, I do appreciate that you take the time to
> look at the code. I'm just frustrated that it takes so long to get this
> right. I thought that all I needed now was a DT maintainer ack :-)

I think that's fine. You've done a great job so far and we can always
fine-tune later on.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]

On Tue, Feb 07, 2017 at 12:21:28PM +0100, Daniel Vetter wrote:
> On Tue, Feb 07, 2017 at 12:11:28PM +0100, Thierry Reding wrote:
> > On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote:
> > > >
> > > > I definitely don't want that we don't attempt this. But brought from years
> > > > of experience, I recommend to merge first (with pre-refactoring already
> > > > applied, but helpers only extracted, not yet at the right spot), and then
> > > > follow up with. Because on average, there's way too many trees with
> > > > overloaded maintainers who maybe look at your patch once per kernel
> > > > release cycle.
> > > >
> > > > If you know that backlight and spi isn't one of these areas (anything that
> > > > goes through takashi/sound is a similar good experience for us on the i915
> > > > side), then I guess we can try. But then Noralf has already written a few
> > > > months worth of really great refactoring, and I'm seriously starting to
> > > > feel guilty for volunteering him for all of this. Even though he seems to
> > > > be really good at it, and seems to not mind, it's getting a bit silly.
> > > > Given that I'd say up to Noralf.
> > > >
> > > > In short, there's always a balance.
> > > 
> > > I don't think we can make a rule for this, it will always depend on the
> > > code. There is always going to be stuff we put in drm that should go
> > > elsewhere, and stuff that is elsewhere that drm should use.
> > > 
> > > I think however if we do add stuff like this, someone should keep track
> > > of them and try to make them get further into the kernel.
> > 
> > Yes, I think having some sort of TODO in drivers/gpu/drm could help
> > track things that we know should eventually be moved out. It could serve
> > as a list of janitorial tasks for newcomers that want to get their hands
> > dirty and tackle relatively trivial tasks.
> 
> We have this list already, it's at: http://www.x.org/wiki/DRMJanitors/
> 
> I guess I should highlight it more, maybe even add it to the docs? Eric
> just asked about it last week too.

Yeah, I'm aware of that list. I think it's a little problematic that
it's in a wiki and far removed from where the actual work is happening.
I think we should just take that list and add it as a TODO in
drivers/gpu/drm, or alternatively keep it as part of the GPU
documentation. That way we can more easily mark things as done or add
new stuff as work gets done.

For cases like this I think we could just add new items as they are
pointed out during review. For things that are already merged we can
add items separately. Once the refactoring is done, the patch series
can contain a final patch that simply removes the items again. I think
that has much less potential to become out-dated than a separate wiki
page.

FWIW, I'll volunteer to move the list to git if we decide to go ahead
with that.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4745 bytes --]

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@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 :-)

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4797 bytes --]

On Tue, Jan 31, 2017 at 05:03:12PM +0100, Noralf Trønnes wrote:
> drm: Add support for tiny LCD displays
> 
> This is an attempt at providing a DRM version of drivers/staging/fbtft.
> 
> The tinydrm library provides a very simplified view of DRM in particular
> for tiny displays that has onboard video memory and is connected through
> a slow bus like SPI/I2C.
> 
> The main change this time is adding 'rotation' as a common display
> Device Tree property.
> 
> 
> Noralf.
> 
> Changes since version 2:
> - Remove fbdev after drm unregister, not before.
> - Added Documentation/devicetree/bindings/display/display.txt
> 
> Changes since version 1:
> - Add tinydrm.rst
> - Set tdev->fbdev_cma=NULL on unregister (lastclose is called after that).
> - Remove some DRM_DEBUG*()
> - Write-combined memory has uncached reads, so speed up by copying/buffering
>   one pixel line before conversion.
> 
> Changes since RFC v2:
> - Rebased on new core helpers
> - Don't use drm_panel
> - Flush when the framebuffer is changed on the plane
> - Add devm_tinydrm_init()
> - Fix PRIME support, set vaddr
> - Use atomic helpers in suspend/resume
> - Add a tinydrm_connector with one display mode
> - Set mode_config.preferred_depth and use it for fbdev
> - Subclass tinydrm_device in drivers instead of bloating the structure
> - The PiTFT display uses a MI0283QT panel, write driver for that instead.
> - Drop homegrown lcdreg module, it ended up as a collection of special
>   cases.
> - Add more documentation
> 
> Changes since RFC v1:
> - Add fb_deferred_io support to drm_fb_helper and drm_fb_cma_helper,
>   and use drm_fb_cma_helper instead.
> - Move display pipeline code to drm_simple_kms_helper.
> - Don't use (struct drm_driver *)->load().
> - Make tinydrm more like a library, exporting the internals.
> - Move the struct drm_driver definition from the tinydrm module to the
>   driver using a helper macro: TINYDRM_DRM_DRIVER.
> - Remove dirtyfb() async code.
> - Added support for partial display updates.
> 
> 
> Noralf Trønnes (7):
>   drm: Add DRM support for tiny LCD displays
>   drm/tinydrm: Add helper functions
>   drm/tinydrm: Add MIPI DBI support
>   of: Add vendor prefix for Multi-Inno
>   dt-bindings: display: Add common rotation property
>   dt-bindings: Add Multi-Inno MI0283QT binding
>   drm/tinydrm: Add support for Multi-Inno MI0283QT display
> 
>  .../devicetree/bindings/display/display.txt        |    4 +
>  .../bindings/display/multi-inno,mi0283qt.txt       |   27 +
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  Documentation/gpu/index.rst                        |    1 +
>  Documentation/gpu/tinydrm.rst                      |   42 +
>  MAINTAINERS                                        |    6 +
>  drivers/gpu/drm/Kconfig                            |    2 +
>  drivers/gpu/drm/Makefile                           |    1 +
>  drivers/gpu/drm/tinydrm/Kconfig                    |   19 +
>  drivers/gpu/drm/tinydrm/Makefile                   |    7 +
>  drivers/gpu/drm/tinydrm/core/Makefile              |    3 +
>  drivers/gpu/drm/tinydrm/core/tinydrm-core.c        |  377 ++++++++
>  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c     |  462 +++++++++
>  drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c        |  234 +++++
>  drivers/gpu/drm/tinydrm/mi0283qt.c                 |  279 ++++++
>  drivers/gpu/drm/tinydrm/mipi-dbi.c                 | 1005 ++++++++++++++++++++
>  include/drm/tinydrm/ili9341.h                      |   54 ++
>  include/drm/tinydrm/mipi-dbi.h                     |  107 +++
>  include/drm/tinydrm/tinydrm-helpers.h              |  100 ++
>  include/drm/tinydrm/tinydrm.h                      |  115 +++
>  20 files changed, 2846 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/display.txt
>  create mode 100644 Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
>  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-helpers.c
>  create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
>  create mode 100644 drivers/gpu/drm/tinydrm/mi0283qt.c
>  create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
>  create mode 100644 include/drm/tinydrm/ili9341.h
>  create mode 100644 include/drm/tinydrm/mipi-dbi.h
>  create mode 100644 include/drm/tinydrm/tinydrm-helpers.h
>  create mode 100644 include/drm/tinydrm/tinydrm.h

The series:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Daniel Vetter]

On Tue, Feb 07, 2017 at 12:44:19PM +0100, Thierry Reding wrote:
> On Tue, Feb 07, 2017 at 12:21:28PM +0100, Daniel Vetter wrote:
> > On Tue, Feb 07, 2017 at 12:11:28PM +0100, Thierry Reding wrote:
> > > On Tue, Feb 07, 2017 at 08:28:16AM +1000, Dave Airlie wrote:
> > > > >
> > > > > I definitely don't want that we don't attempt this. But brought from years
> > > > > of experience, I recommend to merge first (with pre-refactoring already
> > > > > applied, but helpers only extracted, not yet at the right spot), and then
> > > > > follow up with. Because on average, there's way too many trees with
> > > > > overloaded maintainers who maybe look at your patch once per kernel
> > > > > release cycle.
> > > > >
> > > > > If you know that backlight and spi isn't one of these areas (anything that
> > > > > goes through takashi/sound is a similar good experience for us on the i915
> > > > > side), then I guess we can try. But then Noralf has already written a few
> > > > > months worth of really great refactoring, and I'm seriously starting to
> > > > > feel guilty for volunteering him for all of this. Even though he seems to
> > > > > be really good at it, and seems to not mind, it's getting a bit silly.
> > > > > Given that I'd say up to Noralf.
> > > > >
> > > > > In short, there's always a balance.
> > > > 
> > > > I don't think we can make a rule for this, it will always depend on the
> > > > code. There is always going to be stuff we put in drm that should go
> > > > elsewhere, and stuff that is elsewhere that drm should use.
> > > > 
> > > > I think however if we do add stuff like this, someone should keep track
> > > > of them and try to make them get further into the kernel.
> > > 
> > > Yes, I think having some sort of TODO in drivers/gpu/drm could help
> > > track things that we know should eventually be moved out. It could serve
> > > as a list of janitorial tasks for newcomers that want to get their hands
> > > dirty and tackle relatively trivial tasks.
> > 
> > We have this list already, it's at: http://www.x.org/wiki/DRMJanitors/
> > 
> > I guess I should highlight it more, maybe even add it to the docs? Eric
> > just asked about it last week too.
> 
> Yeah, I'm aware of that list. I think it's a little problematic that
> it's in a wiki and far removed from where the actual work is happening.
> I think we should just take that list and add it as a TODO in
> drivers/gpu/drm, or alternatively keep it as part of the GPU
> documentation. That way we can more easily mark things as done or add
> new stuff as work gets done.
> 
> For cases like this I think we could just add new items as they are
> pointed out during review. For things that are already merged we can
> add items separately. Once the refactoring is done, the patch series
> can contain a final patch that simply removes the items again. I think
> that has much less potential to become out-dated than a separate wiki
> page.
> 
> FWIW, I'll volunteer to move the list to git if we decide to go ahead
> with that.

One upside of a wiki is that it's quicker to edit, if someone spots a
drive-by refactoring they might not bother with the formal requirements of
a full patch. Otoh, having it in-source-tree definitely has benefits, too.

If you do the conversion I'd vote for Documentation/gpu/TODO.rst, and
linking it into our documentation (maybe even cross-link from
introduction.rst under a "Getting Started" heading, as reasonable ramp-up
tasks after some checkpatch patches). I think that would help highlight it
a bit. And of course the wiki page needs to be removed and replaced with a
link to the new canonical thing (probably best to point at the source-file
in drm-tip.git, that should be the most up-to-date).

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch