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
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
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.
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.
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
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
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.
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
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
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);
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 */
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
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
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
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. -Daniel
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
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
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.
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
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.
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
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
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
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
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
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
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
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.
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 */
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
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
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
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-s6e...
Regards Andrzej
Thierry
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
(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.
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.
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.
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)
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
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
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
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
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
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 */
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
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
dri-devel@lists.freedesktop.org