* Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD
2017-08-01 22:11 ` [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD David Lechner
@ 2017-08-02 13:03 ` Noralf Trønnes
2017-08-02 16:24 ` David Lechner
2017-08-02 14:49 ` Noralf Trønnes
2017-08-10 15:40 ` Rob Herring
2 siblings, 1 reply; 11+ messages in thread
From: Noralf Trønnes @ 2017-08-02 13:03 UTC (permalink / raw)
To: David Lechner, dri-devel, devicetree
Cc: Daniel Vetter, David Airlie, Rob Herring, Mark Rutland,
Sekhar Nori, Kevin Hilman, linux-arm-kernel, linux-kernel
Den 02.08.2017 00.11, skrev David Lechner:
> LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
> module for the ST7586 controller with parameters for the EV3 LCD display.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
> .../devicetree/bindings/display/st7586.txt | 26 +
> drivers/gpu/drm/tinydrm/Kconfig | 10 +
> drivers/gpu/drm/tinydrm/Makefile | 1 +
> drivers/gpu/drm/tinydrm/st7586.c | 579 +++++++++++++++++++++
> include/drm/tinydrm/st7586.h | 34 ++
Please add a MAINTAINERS entry.
That way you'll be notified on changes.
> 5 files changed, 650 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/st7586.txt
> create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
> create mode 100644 include/drm/tinydrm/st7586.h
>
> diff --git a/Documentation/devicetree/bindings/display/st7586.txt b/Documentation/devicetree/bindings/display/st7586.txt
> new file mode 100644
> index 0000000..acf784aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/st7586.txt
> @@ -0,0 +1,26 @@
> +Sitronix ST7586 display panel
> +
> +Required properties:
> +- compatible: "lego,ev3-lcd".
> +
> +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
This doesn't match the ST7586 datasheet I have, the pins are called
IF[3:1] and the values are different.
> +- 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:
> + display@0{
> + compatible = "lego,ev3-lcd";
> + reg = <0>;
> + spi-max-frequency = <10000000>;
> + dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> + };
> diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
> index f17c3ca..2e790e7 100644
> --- a/drivers/gpu/drm/tinydrm/Kconfig
> +++ b/drivers/gpu/drm/tinydrm/Kconfig
> @@ -32,3 +32,13 @@ config TINYDRM_REPAPER
> 2.71" TFT EPD Panel (E2271CS021)
>
> If M is selected the module will be called repaper.
> +
> +config TINYDRM_ST7586
> + tristate "DRM support for Sitronix ST7586 display panels"
> + depends on DRM_TINYDRM && SPI
> + select TINYDRM_MIPI_DBI
> + help
> + DRM driver for the following Sitronix ST7586 panels:
> + * LEGO MINDSTORMS EV3
> +
> + If M is selected the module will be called st7586.
> diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
> index 95bb4d4..0c184bd 100644
> --- a/drivers/gpu/drm/tinydrm/Makefile
> +++ b/drivers/gpu/drm/tinydrm/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI) += mipi-dbi.o
> # Displays
> obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
> obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
> +obj-$(CONFIG_TINYDRM_ST7586) += st7586.o
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> new file mode 100644
> index 0000000..6093021
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -0,0 +1,579 @@
> +/*
> + * DRM driver for Sitronix ST7586 panels
> + *
> + * Copyright 2017 David Lechner <david@lechnology.com>
> + *
> + * 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 <linux/delay.h>
> +#include <linux/dma-buf.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>
> +
> +#include <drm/tinydrm/mipi-dbi.h>
> +#include <drm/tinydrm/st7586.h>
> +#include <drm/tinydrm/tinydrm-helpers.h>
> +
> +static inline u8 st7586_rgb565_to_gray2(u16 rgb)
> +{
> + u8 r = (rgb & 0xf800) >> 11;
> + u8 g = (rgb & 0x07e0) >> 5;
> + u8 b = rgb & 0x001f;
> + /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> + /* g is already * 2 because it is 6-bit */
> + u8 gray5 = (3 * r + 3 * g + b) / 10;
> +
> + return gray5 >> 3;
> +}
> +
> +static void st7586_from_rgb565(u8 *dst, void *vaddr,
> + struct drm_framebuffer *fb,
> + struct drm_clip_rect *clip)
> +{
> + size_t len;
> + unsigned int x, y;
> + u16 *src, *buf;
> + u8 val;
> +
> + /* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> + clip->x1 = clip->x1 / 3 * 3;
> + clip->x2 = (clip->x2 + 2) / 3 * 3;
> + len = (clip->x2 - clip->x1) * sizeof(u16);
> +
> + 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 += 3) {
> + val = st7586_rgb565_to_gray2(*src++) << 6;
> + if (val & 0xC0)
> + val |= 0x20;
> + val |= st7586_rgb565_to_gray2(*src++) << 3;
> + if (val & 0x18)
> + val |= 0x04;
> + val |= st7586_rgb565_to_gray2(*src++);
> + *dst++ = ~val;
> + }
> + }
> +
> + /* now adjust the clip so it applies to dst */
> + clip->x1 /= 3;
> + clip->x2 /= 3;
> +
> + kfree(buf);
> +}
> +
> +static inline u8 st7586_xrgb8888_to_gray2(u32 rgb)
> +{
> + u8 r = (rgb & 0x00ff0000) >> 16;
> + u8 g = (rgb & 0x0000ff00) >> 8;
> + u8 b = rgb & 0x000000ff;
> + /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> + u8 gray8 = (3 * r + 6 * g + b) / 10;
> +
> + return gray8 >> 6;
> +}
> +
> +static void st7586_from_xrgb8888(u8 *dst, void *vaddr,
> + struct drm_framebuffer *fb,
> + struct drm_clip_rect *clip)
> +{
> + size_t len;
> + unsigned int x, y;
> + u32 *src, *buf;
> + u8 val;
> +
> + /* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> + clip->x1 = clip->x1 / 3 * 3;
> + clip->x2 = (clip->x2 + 2) / 3 * 3;
> + len = (clip->x2 - clip->x1) * sizeof(u32);
> +
> + 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 += 3) {
> + val = st7586_xrgb8888_to_gray2(*src++) << 6;
> + if (val & 0xC0)
> + val |= 0x20;
> + val |= st7586_xrgb8888_to_gray2(*src++) << 3;
> + if (val & 0x18)
> + val |= 0x04;
> + val |= st7586_xrgb8888_to_gray2(*src++);
> + *dst++ = ~val;
> + }
> + }
> +
> + /* now adjust the clip so it applies to dst */
> + clip->x1 /= 3;
> + clip->x2 /= 3;
> +
> + kfree(buf);
> +}
Please use tinydrm_xrgb8888_to_gray8().
> +
> +static int st7586_mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
You can shorten your st7586_mipi_dbi_* to just st7586_*
> + struct drm_clip_rect *clip)
> +{
> + 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:
> + st7586_from_rgb565(dst, src, fb, clip);
> + break;
> + case DRM_FORMAT_XRGB8888:
> + st7586_from_xrgb8888(dst, src, fb, clip);
> + break;
> + default:
> + dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
> + drm_get_format_name(fb->format->format,
> + &format_name));
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (import_attach)
> + ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> + DMA_FROM_DEVICE);
> + return ret;
> +}
> +
> +static int st7586_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 tinydrm_device *tdev = fb->dev->dev_private;
> + struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> + struct drm_clip_rect clip;
> + int ret = 0;
> +
> + 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;
> +
> + 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);
> +
> + ret = st7586_mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip);
> + if (ret)
> + goto out_unlock;
> +
> + /* NB: st7586_mipi_dbi_buf_copy() modifies clip */
> +
> + 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,
> + (u8 *)mipi->tx_buf,
> + (clip.x2 - clip.x1) * (clip.y2 - clip.y1));
> +
> +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 st7586_mipi_dbi_fb_funcs = {
> + .destroy = drm_fb_cma_destroy,
> + .create_handle = drm_fb_cma_create_handle,
> + .dirty = st7586_mipi_dbi_fb_dirty,
> +};
> +
> +void st7586_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");
> +
You should use this function to enable the regulator and init the
controller. Don't look at mi0283qt it should have been fixed.
> + mipi->enabled = true;
> + if (fb)
I don't think it's possible to enable the pipe without a framebuffer,
but I wasn't able find out. drm is complex...
> + fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
> +
> + tinydrm_enable_backlight(mipi->backlight);
> +}
> +
> +static void st7586_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 + 2) / 3;
> + size_t len = width * height;
> +
> + 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);
> +}
> +
> +static void st7586_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
> + st7586_mipi_dbi_blank(mipi);
And disable the regulator here.
> +}
> +
> +static const u32 st7586_mipi_dbi_formats[] = {
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_XRGB8888,
Why 2 emulation formats?
I chose DRM_FORMAT_XRGB8888 since I expected everything to support it.
Please use only that.
> +};
> +
> +static int st7586_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 + 2) / 3 * mode->hdisplay;
> + struct tinydrm_device *tdev = &mipi->tinydrm;
> + int ret;
> +
> + if (!mipi->command)
> + return -EINVAL;
You know this is set after calling mipi_dbi_spi_init(), so you don't
have to check.
> +
> + 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, &st7586_mipi_dbi_fb_funcs, driver);
> + if (ret)
> + return ret;
> +
> + ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
> + DRM_MODE_CONNECTOR_VIRTUAL,
> + st7586_mipi_dbi_formats,
> + ARRAY_SIZE(st7586_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;
> +}
> +
> +static int st7586_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))
Looking at the datasheet it seems that it's not possible to read from
the controller via SPI?
> + return 0;
> +
> + mipi_dbi_hw_reset(mipi);
> + ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
> + if (ret) {
> + dev_err(dev, "Error sending command %d\n", ret);
> + regulator_disable(mipi->regulator);
> + return ret;
> + }
> +
> + mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
> +
> + msleep(10);
> +
> + mipi_dbi_command(mipi, ST7586_OTP_READ);
> +
> + msleep(20);
> +
> + mipi_dbi_command(mipi, ST7586_OTP_CTRL_OUT);
> + mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> +
> + msleep(50);
> +
> + mipi_dbi_command(mipi, ST7586_SET_VOP_OFFSET, 0x00);
> + mipi_dbi_command(mipi, ST7586_SET_VOP, 0xe3, 0x00);
> + mipi_dbi_command(mipi, ST7586_SET_BIAS_SYSTEM, 0x02);
> + mipi_dbi_command(mipi, ST7586_SET_BOOST_LEVEL, 0x04);
> + mipi_dbi_command(mipi, ST7586_ENABLE_ANALOG, 0x1d);
> + mipi_dbi_command(mipi, ST7586_SET_NLINE_INV, 0x00);
> + mipi_dbi_command(mipi, ST7586_DISP_MODE_GRAY);
> + mipi_dbi_command(mipi, ST7586_ENABLE_DDRAM, 0x02);
> +
> + switch (mipi->rotation) {
> + default:
> + addr_mode = 0x00;
> + break;
> + case 90:
> + addr_mode = ST7586_DISP_CTRL_MY;
> + break;
> + case 180:
> + addr_mode = ST7586_DISP_CTRL_MX | ST7586_DISP_CTRL_MY;
> + break;
> + case 270:
> + addr_mode = ST7586_DISP_CTRL_MX;
> + break;
> + }
> + mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> +
> + mipi_dbi_command(mipi, ST7586_SET_DISP_DUTY, 0x7f);
> + mipi_dbi_command(mipi, ST7586_SET_PART_DISP, 0xa0);
> + mipi_dbi_command(mipi, MIPI_DCS_SET_PARTIAL_AREA, 0x00, 0x00, 0x00, 0x77);
> + mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
> +
> + msleep(100);
> +
> + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> +
> + return 0;
> +}
> +
> +static void st7586_fini(void *data)
> +{
> + struct mipi_dbi *mipi = data;
> +
> + DRM_DEBUG_KMS("\n");
> + regulator_disable(mipi->regulator);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
> + .enable = st7586_mipi_dbi_pipe_enable,
> + .disable = st7586_mipi_dbi_pipe_disable,
> + .update = tinydrm_display_pipe_update,
> + .prepare_fb = tinydrm_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_display_mode st7586_mode = {
> + TINYDRM_MODE(178, 128, 37, 27),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(st7586_fops);
> +
> +static struct drm_driver st7586_driver = {
> + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
> + DRIVER_ATOMIC,
> + .fops = &st7586_fops,
> + TINYDRM_GEM_DRIVER_OPS,
> + .lastclose = tinydrm_lastclose,
> + .debugfs_init = mipi_dbi_debugfs_init,
> + .name = "st7586",
> + .desc = "Sitronix ST7586",
> + .date = "20170801",
> + .major = 1,
> + .minor = 0,
> +};
> +
> +static const struct of_device_id st7586_of_match[] = {
> + { .compatible = "lego,ev3-lcd" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, st7586_of_match);
> +
> +static const struct spi_device_id st7586_id[] = {
> + { "ev3-lcd", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(spi, st7586_id);
> +
> +static int st7586_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);
> + if (ret)
> + return ret;
Since you are using mipi_dbi_debugfs_init and the controller can't be
read, disable reading:
mipi->read_commands = NULL;
> +
> + /*
> + * we are using 8-bit data, so we are not actually swapping anything,
> + * but setting mipi->swap_bytes makes mipi_dbi_typec3_command() do the
> + * right thing and not use 16-bit transfers (which results in swapped
> + * bytes on little-endian systems and causes out of order data to be
> + * sent to the display).
> + */
> + mipi->swap_bytes = true;
> +
> + ret = st7586_mipi_dbi_init(&spi->dev, mipi, &st7586_pipe_funcs,
> + &st7586_driver, &st7586_mode, rotation);
> + if (ret)
> + return ret;
> +
> + ret = st7586_init(mipi);
This isn't necessary when you use .enable/.disable to control power/init,
and you can drop the devm_add_action.
> + if (ret)
> + return ret;
> +
> + /* use devres to fini after drm unregister (drv->remove is before) */
> + ret = devm_add_action(dev, st7586_fini, mipi);
> + if (ret) {
> + st7586_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 st7586_shutdown(struct spi_device *spi)
> +{
> + struct mipi_dbi *mipi = spi_get_drvdata(spi);
> +
> + tinydrm_shutdown(&mipi->tinydrm);
> +}
> +
> +static int __maybe_unused st7586_pm_suspend(struct device *dev)
> +{
> + struct mipi_dbi *mipi = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = tinydrm_suspend(&mipi->tinydrm);
> + if (ret)
> + return ret;
> +
> + st7586_fini(mipi);
> +
With .enable/.disable controlling power, tinydrm_suspend() is enough.
> + return 0;
> +}
> +
> +static int __maybe_unused st7586_pm_resume(struct device *dev)
> +{
> + struct mipi_dbi *mipi = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = st7586_init(mipi);
> + if (ret)
> + return ret;
> +
Same goes here.
> + return tinydrm_resume(&mipi->tinydrm);
> +}
> +
> +static const struct dev_pm_ops st7586_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(st7586_pm_suspend, st7586_pm_resume)
> +};
> +
> +static struct spi_driver st7586_spi_driver = {
> + .driver = {
> + .name = "st7586",
> + .owner = THIS_MODULE,
> + .of_match_table = st7586_of_match,
> + .pm = &st7586_pm_ops,
> + },
> + .id_table = st7586_id,
> + .probe = st7586_probe,
> + .shutdown = st7586_shutdown,
> +};
> +module_spi_driver(st7586_spi_driver);
> +
> +MODULE_DESCRIPTION("Sitronix ST7586 DRM driver");
> +MODULE_AUTHOR("David Lechner <david@lechnology.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/tinydrm/st7586.h b/include/drm/tinydrm/st7586.h
> new file mode 100644
> index 0000000..18fb56b
> --- /dev/null
> +++ b/include/drm/tinydrm/st7586.h
Please put this in drivers/drm/tinydrm, or you can even put the defines
in the driver if you want, it's quite short.
ili9341.h shouldn't have ended up in include/drm/tinydrm.
Noralf.
> @@ -0,0 +1,34 @@
> +/*
> + * ST7586 LCD controller
> + *
> + * Copyright (C) 2017 David Lechner <david@lechnology.com>
> + *
> + * 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_ST7856_H
> +#define __LINUX_ST7856_H
> +
> +#define ST7586_DISP_MODE_GRAY 0x38
> +#define ST7586_DISP_MODE_MONO 0x39
> +#define ST7586_ENABLE_DDRAM 0x3a
> +#define ST7586_SET_DISP_DUTY 0xb0
> +#define ST7586_SET_PART_DISP 0xb4
> +#define ST7586_SET_NLINE_INV 0xb5
> +#define ST7586_SET_VOP 0xc0
> +#define ST7586_SET_BIAS_SYSTEM 0xc3
> +#define ST7586_SET_BOOST_LEVEL 0xc4
> +#define ST7586_SET_VOP_OFFSET 0xc7
> +#define ST7586_ENABLE_ANALOG 0xd0
> +#define ST7586_AUTO_READ_CTRL 0xd7
> +#define ST7586_OTP_RW_CTRL 0xe0
> +#define ST7586_OTP_CTRL_OUT 0xe1
> +#define ST7586_OTP_READ 0xe3
> +
> +#define ST7586_DISP_CTRL_MX BIT(6)
> +#define ST7586_DISP_CTRL_MY BIT(7)
> +
> +#endif /* __LINUX_ST7856_H */
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD
2017-08-02 13:03 ` Noralf Trønnes
@ 2017-08-02 16:24 ` David Lechner
2017-08-02 18:49 ` Noralf Trønnes
0 siblings, 1 reply; 11+ messages in thread
From: David Lechner @ 2017-08-02 16:24 UTC (permalink / raw)
To: Noralf Trønnes, dri-devel, devicetree
Cc: Daniel Vetter, David Airlie, Rob Herring, Mark Rutland,
Sekhar Nori, Kevin Hilman, linux-arm-kernel, linux-kernel
On 08/02/2017 08:03 AM, Noralf Trønnes wrote:
>
>
> Please use tinydrm_xrgb8888_to_gray8().
I considered this, but is seems excessive to loop through the entire fb
twice just to make a 4x6 cursor blink.
> You should use this function to enable the regulator and init the
> controller. Don't look at mi0283qt it should have been fixed.
ACK. But this is why I was not keen on writing a standalone driver. I
know some things about kernel development, but not everything. How am I
supposed to know what is OK to copy from other drivers and what is not?
And if I am going to be put down as the maintainer of this driver, it
bothers me that I don't know so much about how it all works.
>
> Why 2 emulation formats?
> I chose DRM_FORMAT_XRGB8888 since I expected everything to support it.
> Please use only that.
Because my graphics library currently does not work with XRGB8888. ;-)
I can fix the graphics library and drop the RGB565 format in the kernel
driver.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD
2017-08-02 16:24 ` David Lechner
@ 2017-08-02 18:49 ` Noralf Trønnes
0 siblings, 0 replies; 11+ messages in thread
From: Noralf Trønnes @ 2017-08-02 18:49 UTC (permalink / raw)
To: David Lechner, dri-devel, devicetree
Cc: Daniel Vetter, David Airlie, Rob Herring, Mark Rutland,
Sekhar Nori, Kevin Hilman, linux-arm-kernel, linux-kernel
Den 02.08.2017 18.24, skrev David Lechner:
> On 08/02/2017 08:03 AM, Noralf Trønnes wrote:
>>
>>
>> Please use tinydrm_xrgb8888_to_gray8().
>
> I considered this, but is seems excessive to loop through the entire
> fb twice just to make a 4x6 cursor blink.
>
Yes, you're right about that.
Can you change tinydrm_xrgb8888_to_gray8 to match the other
copy/conversion functions so we can pass it a clip rectangle:
int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr,
struct drm_framebuffer *fb,
struct drm_clip_rect *clip)
And move the import_attach part to the driver (repaper).
repaper doesn't do partial updates, so pass a full clip.
>> You should use this function to enable the regulator and init the
>> controller. Don't look at mi0283qt it should have been fixed.
>
> ACK. But this is why I was not keen on writing a standalone driver. I
> know some things about kernel development, but not everything. How am
> I supposed to know what is OK to copy from other drivers and what is not?
You can't know, and it's part of the review process to sort that out.
That being said, I really should have fixed mi0283qt, because it will
be copied...
> And if I am going to be put down as the maintainer of this driver, it
> bothers me that I don't know so much about how it all works.
>
You don't need to know all the details. If I make a change, you can
assume that I know what I'm doing and ack it, but I need someone to
look at my patches, I can't apply my own patches without anyone looking
at it, we all make mistakes. And if someone else sends a patch and you
don't know if it is good or not, just ping me and I'll look at it.
Either way I have to look at all tinydrm patches before applying them.
I'm not keen on being default maintainer on drivers that I can't test.
The upside of being a maintainer is that you control the future of the
driver, what changes goes in.
And I'm kind of in the same boat as you, I know tinydrm, but drm is
very complex and I don't know half of what's going on. If I'm stuck or
out of my league, I have to ask for help. Like with adding drm formats.
Noralf.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD
2017-08-01 22:11 ` [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD David Lechner
2017-08-02 13:03 ` Noralf Trønnes
@ 2017-08-02 14:49 ` Noralf Trønnes
2017-08-10 15:40 ` Rob Herring
2 siblings, 0 replies; 11+ messages in thread
From: Noralf Trønnes @ 2017-08-02 14:49 UTC (permalink / raw)
To: David Lechner, dri-devel, devicetree
Cc: Daniel Vetter, David Airlie, Rob Herring, Mark Rutland,
Sekhar Nori, Kevin Hilman, linux-arm-kernel, linux-kernel
Den 02.08.2017 00.11, skrev David Lechner:
> LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
> module for the ST7586 controller with parameters for the EV3 LCD display.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
> .../devicetree/bindings/display/st7586.txt | 26 +
I forgot, the binding doc should be a separate patch.
Noralf.
> drivers/gpu/drm/tinydrm/Kconfig | 10 +
> drivers/gpu/drm/tinydrm/Makefile | 1 +
> drivers/gpu/drm/tinydrm/st7586.c | 579 +++++++++++++++++++++
> include/drm/tinydrm/st7586.h | 34 ++
> 5 files changed, 650 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/st7586.txt
> create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
> create mode 100644 include/drm/tinydrm/st7586.h
>
> diff --git a/Documentation/devicetree/bindings/display/st7586.txt b/Documentation/devicetree/bindings/display/st7586.txt
> new file mode 100644
> index 0000000..acf784aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/st7586.txt
> @@ -0,0 +1,26 @@
> +Sitronix ST7586 display panel
> +
> +Required properties:
> +- compatible: "lego,ev3-lcd".
> +
> +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:
> + display@0{
> + compatible = "lego,ev3-lcd";
> + reg = <0>;
> + spi-max-frequency = <10000000>;
> + dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> + };
> diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
> index f17c3ca..2e790e7 100644
> --- a/drivers/gpu/drm/tinydrm/Kconfig
> +++ b/drivers/gpu/drm/tinydrm/Kconfig
> @@ -32,3 +32,13 @@ config TINYDRM_REPAPER
> 2.71" TFT EPD Panel (E2271CS021)
>
> If M is selected the module will be called repaper.
> +
> +config TINYDRM_ST7586
> + tristate "DRM support for Sitronix ST7586 display panels"
> + depends on DRM_TINYDRM && SPI
> + select TINYDRM_MIPI_DBI
> + help
> + DRM driver for the following Sitronix ST7586 panels:
> + * LEGO MINDSTORMS EV3
> +
> + If M is selected the module will be called st7586.
> diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
> index 95bb4d4..0c184bd 100644
> --- a/drivers/gpu/drm/tinydrm/Makefile
> +++ b/drivers/gpu/drm/tinydrm/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI) += mipi-dbi.o
> # Displays
> obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
> obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
> +obj-$(CONFIG_TINYDRM_ST7586) += st7586.o
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> new file mode 100644
> index 0000000..6093021
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -0,0 +1,579 @@
> +/*
> + * DRM driver for Sitronix ST7586 panels
> + *
> + * Copyright 2017 David Lechner <david@lechnology.com>
> + *
> + * 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 <linux/delay.h>
> +#include <linux/dma-buf.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>
> +
> +#include <drm/tinydrm/mipi-dbi.h>
> +#include <drm/tinydrm/st7586.h>
> +#include <drm/tinydrm/tinydrm-helpers.h>
> +
> +static inline u8 st7586_rgb565_to_gray2(u16 rgb)
> +{
> + u8 r = (rgb & 0xf800) >> 11;
> + u8 g = (rgb & 0x07e0) >> 5;
> + u8 b = rgb & 0x001f;
> + /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> + /* g is already * 2 because it is 6-bit */
> + u8 gray5 = (3 * r + 3 * g + b) / 10;
> +
> + return gray5 >> 3;
> +}
> +
> +static void st7586_from_rgb565(u8 *dst, void *vaddr,
> + struct drm_framebuffer *fb,
> + struct drm_clip_rect *clip)
> +{
> + size_t len;
> + unsigned int x, y;
> + u16 *src, *buf;
> + u8 val;
> +
> + /* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> + clip->x1 = clip->x1 / 3 * 3;
> + clip->x2 = (clip->x2 + 2) / 3 * 3;
> + len = (clip->x2 - clip->x1) * sizeof(u16);
> +
> + 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 += 3) {
> + val = st7586_rgb565_to_gray2(*src++) << 6;
> + if (val & 0xC0)
> + val |= 0x20;
> + val |= st7586_rgb565_to_gray2(*src++) << 3;
> + if (val & 0x18)
> + val |= 0x04;
> + val |= st7586_rgb565_to_gray2(*src++);
> + *dst++ = ~val;
> + }
> + }
> +
> + /* now adjust the clip so it applies to dst */
> + clip->x1 /= 3;
> + clip->x2 /= 3;
> +
> + kfree(buf);
> +}
> +
> +static inline u8 st7586_xrgb8888_to_gray2(u32 rgb)
> +{
> + u8 r = (rgb & 0x00ff0000) >> 16;
> + u8 g = (rgb & 0x0000ff00) >> 8;
> + u8 b = rgb & 0x000000ff;
> + /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> + u8 gray8 = (3 * r + 6 * g + b) / 10;
> +
> + return gray8 >> 6;
> +}
> +
> +static void st7586_from_xrgb8888(u8 *dst, void *vaddr,
> + struct drm_framebuffer *fb,
> + struct drm_clip_rect *clip)
> +{
> + size_t len;
> + unsigned int x, y;
> + u32 *src, *buf;
> + u8 val;
> +
> + /* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> + clip->x1 = clip->x1 / 3 * 3;
> + clip->x2 = (clip->x2 + 2) / 3 * 3;
> + len = (clip->x2 - clip->x1) * sizeof(u32);
> +
> + 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 += 3) {
> + val = st7586_xrgb8888_to_gray2(*src++) << 6;
> + if (val & 0xC0)
> + val |= 0x20;
> + val |= st7586_xrgb8888_to_gray2(*src++) << 3;
> + if (val & 0x18)
> + val |= 0x04;
> + val |= st7586_xrgb8888_to_gray2(*src++);
> + *dst++ = ~val;
> + }
> + }
> +
> + /* now adjust the clip so it applies to dst */
> + clip->x1 /= 3;
> + clip->x2 /= 3;
> +
> + kfree(buf);
> +}
> +
> +static int st7586_mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
> + struct drm_clip_rect *clip)
> +{
> + 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:
> + st7586_from_rgb565(dst, src, fb, clip);
> + break;
> + case DRM_FORMAT_XRGB8888:
> + st7586_from_xrgb8888(dst, src, fb, clip);
> + break;
> + default:
> + dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
> + drm_get_format_name(fb->format->format,
> + &format_name));
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (import_attach)
> + ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> + DMA_FROM_DEVICE);
> + return ret;
> +}
> +
> +static int st7586_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 tinydrm_device *tdev = fb->dev->dev_private;
> + struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> + struct drm_clip_rect clip;
> + int ret = 0;
> +
> + 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;
> +
> + 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);
> +
> + ret = st7586_mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip);
> + if (ret)
> + goto out_unlock;
> +
> + /* NB: st7586_mipi_dbi_buf_copy() modifies clip */
> +
> + 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,
> + (u8 *)mipi->tx_buf,
> + (clip.x2 - clip.x1) * (clip.y2 - clip.y1));
> +
> +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 st7586_mipi_dbi_fb_funcs = {
> + .destroy = drm_fb_cma_destroy,
> + .create_handle = drm_fb_cma_create_handle,
> + .dirty = st7586_mipi_dbi_fb_dirty,
> +};
> +
> +void st7586_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);
> +}
> +
> +static void st7586_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 + 2) / 3;
> + size_t len = width * height;
> +
> + 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);
> +}
> +
> +static void st7586_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
> + st7586_mipi_dbi_blank(mipi);
> +}
> +
> +static const u32 st7586_mipi_dbi_formats[] = {
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_XRGB8888,
> +};
> +
> +static int st7586_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 + 2) / 3 * mode->hdisplay;
> + 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, &st7586_mipi_dbi_fb_funcs, driver);
> + if (ret)
> + return ret;
> +
> + ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
> + DRM_MODE_CONNECTOR_VIRTUAL,
> + st7586_mipi_dbi_formats,
> + ARRAY_SIZE(st7586_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;
> +}
> +
> +static int st7586_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, ST7586_AUTO_READ_CTRL, 0x9f);
> + if (ret) {
> + dev_err(dev, "Error sending command %d\n", ret);
> + regulator_disable(mipi->regulator);
> + return ret;
> + }
> +
> + mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
> +
> + msleep(10);
> +
> + mipi_dbi_command(mipi, ST7586_OTP_READ);
> +
> + msleep(20);
> +
> + mipi_dbi_command(mipi, ST7586_OTP_CTRL_OUT);
> + mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> +
> + msleep(50);
> +
> + mipi_dbi_command(mipi, ST7586_SET_VOP_OFFSET, 0x00);
> + mipi_dbi_command(mipi, ST7586_SET_VOP, 0xe3, 0x00);
> + mipi_dbi_command(mipi, ST7586_SET_BIAS_SYSTEM, 0x02);
> + mipi_dbi_command(mipi, ST7586_SET_BOOST_LEVEL, 0x04);
> + mipi_dbi_command(mipi, ST7586_ENABLE_ANALOG, 0x1d);
> + mipi_dbi_command(mipi, ST7586_SET_NLINE_INV, 0x00);
> + mipi_dbi_command(mipi, ST7586_DISP_MODE_GRAY);
> + mipi_dbi_command(mipi, ST7586_ENABLE_DDRAM, 0x02);
> +
> + switch (mipi->rotation) {
> + default:
> + addr_mode = 0x00;
> + break;
> + case 90:
> + addr_mode = ST7586_DISP_CTRL_MY;
> + break;
> + case 180:
> + addr_mode = ST7586_DISP_CTRL_MX | ST7586_DISP_CTRL_MY;
> + break;
> + case 270:
> + addr_mode = ST7586_DISP_CTRL_MX;
> + break;
> + }
> + mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> +
> + mipi_dbi_command(mipi, ST7586_SET_DISP_DUTY, 0x7f);
> + mipi_dbi_command(mipi, ST7586_SET_PART_DISP, 0xa0);
> + mipi_dbi_command(mipi, MIPI_DCS_SET_PARTIAL_AREA, 0x00, 0x00, 0x00, 0x77);
> + mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
> +
> + msleep(100);
> +
> + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> +
> + return 0;
> +}
> +
> +static void st7586_fini(void *data)
> +{
> + struct mipi_dbi *mipi = data;
> +
> + DRM_DEBUG_KMS("\n");
> + regulator_disable(mipi->regulator);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
> + .enable = st7586_mipi_dbi_pipe_enable,
> + .disable = st7586_mipi_dbi_pipe_disable,
> + .update = tinydrm_display_pipe_update,
> + .prepare_fb = tinydrm_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_display_mode st7586_mode = {
> + TINYDRM_MODE(178, 128, 37, 27),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(st7586_fops);
> +
> +static struct drm_driver st7586_driver = {
> + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
> + DRIVER_ATOMIC,
> + .fops = &st7586_fops,
> + TINYDRM_GEM_DRIVER_OPS,
> + .lastclose = tinydrm_lastclose,
> + .debugfs_init = mipi_dbi_debugfs_init,
> + .name = "st7586",
> + .desc = "Sitronix ST7586",
> + .date = "20170801",
> + .major = 1,
> + .minor = 0,
> +};
> +
> +static const struct of_device_id st7586_of_match[] = {
> + { .compatible = "lego,ev3-lcd" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, st7586_of_match);
> +
> +static const struct spi_device_id st7586_id[] = {
> + { "ev3-lcd", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(spi, st7586_id);
> +
> +static int st7586_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);
> + if (ret)
> + return ret;
> +
> + /*
> + * we are using 8-bit data, so we are not actually swapping anything,
> + * but setting mipi->swap_bytes makes mipi_dbi_typec3_command() do the
> + * right thing and not use 16-bit transfers (which results in swapped
> + * bytes on little-endian systems and causes out of order data to be
> + * sent to the display).
> + */
> + mipi->swap_bytes = true;
> +
> + ret = st7586_mipi_dbi_init(&spi->dev, mipi, &st7586_pipe_funcs,
> + &st7586_driver, &st7586_mode, rotation);
> + if (ret)
> + return ret;
> +
> + ret = st7586_init(mipi);
> + if (ret)
> + return ret;
> +
> + /* use devres to fini after drm unregister (drv->remove is before) */
> + ret = devm_add_action(dev, st7586_fini, mipi);
> + if (ret) {
> + st7586_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 st7586_shutdown(struct spi_device *spi)
> +{
> + struct mipi_dbi *mipi = spi_get_drvdata(spi);
> +
> + tinydrm_shutdown(&mipi->tinydrm);
> +}
> +
> +static int __maybe_unused st7586_pm_suspend(struct device *dev)
> +{
> + struct mipi_dbi *mipi = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = tinydrm_suspend(&mipi->tinydrm);
> + if (ret)
> + return ret;
> +
> + st7586_fini(mipi);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused st7586_pm_resume(struct device *dev)
> +{
> + struct mipi_dbi *mipi = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = st7586_init(mipi);
> + if (ret)
> + return ret;
> +
> + return tinydrm_resume(&mipi->tinydrm);
> +}
> +
> +static const struct dev_pm_ops st7586_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(st7586_pm_suspend, st7586_pm_resume)
> +};
> +
> +static struct spi_driver st7586_spi_driver = {
> + .driver = {
> + .name = "st7586",
> + .owner = THIS_MODULE,
> + .of_match_table = st7586_of_match,
> + .pm = &st7586_pm_ops,
> + },
> + .id_table = st7586_id,
> + .probe = st7586_probe,
> + .shutdown = st7586_shutdown,
> +};
> +module_spi_driver(st7586_spi_driver);
> +
> +MODULE_DESCRIPTION("Sitronix ST7586 DRM driver");
> +MODULE_AUTHOR("David Lechner <david@lechnology.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/tinydrm/st7586.h b/include/drm/tinydrm/st7586.h
> new file mode 100644
> index 0000000..18fb56b
> --- /dev/null
> +++ b/include/drm/tinydrm/st7586.h
> @@ -0,0 +1,34 @@
> +/*
> + * ST7586 LCD controller
> + *
> + * Copyright (C) 2017 David Lechner <david@lechnology.com>
> + *
> + * 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_ST7856_H
> +#define __LINUX_ST7856_H
> +
> +#define ST7586_DISP_MODE_GRAY 0x38
> +#define ST7586_DISP_MODE_MONO 0x39
> +#define ST7586_ENABLE_DDRAM 0x3a
> +#define ST7586_SET_DISP_DUTY 0xb0
> +#define ST7586_SET_PART_DISP 0xb4
> +#define ST7586_SET_NLINE_INV 0xb5
> +#define ST7586_SET_VOP 0xc0
> +#define ST7586_SET_BIAS_SYSTEM 0xc3
> +#define ST7586_SET_BOOST_LEVEL 0xc4
> +#define ST7586_SET_VOP_OFFSET 0xc7
> +#define ST7586_ENABLE_ANALOG 0xd0
> +#define ST7586_AUTO_READ_CTRL 0xd7
> +#define ST7586_OTP_RW_CTRL 0xe0
> +#define ST7586_OTP_CTRL_OUT 0xe1
> +#define ST7586_OTP_READ 0xe3
> +
> +#define ST7586_DISP_CTRL_MX BIT(6)
> +#define ST7586_DISP_CTRL_MY BIT(7)
> +
> +#endif /* __LINUX_ST7856_H */
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD
2017-08-01 22:11 ` [PATCH v2 2/4] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD David Lechner
2017-08-02 13:03 ` Noralf Trønnes
2017-08-02 14:49 ` Noralf Trønnes
@ 2017-08-10 15:40 ` Rob Herring
2 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2017-08-10 15:40 UTC (permalink / raw)
To: David Lechner
Cc: dri-devel, devicetree, Noralf Trønnes, Daniel Vetter,
David Airlie, Mark Rutland, Sekhar Nori, Kevin Hilman,
linux-arm-kernel, linux-kernel
On Tue, Aug 01, 2017 at 05:11:48PM -0500, David Lechner wrote:
> LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
> module for the ST7586 controller with parameters for the EV3 LCD display.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
> .../devicetree/bindings/display/st7586.txt | 26 +
Please split bindings to a separate patch.
> drivers/gpu/drm/tinydrm/Kconfig | 10 +
> drivers/gpu/drm/tinydrm/Makefile | 1 +
> drivers/gpu/drm/tinydrm/st7586.c | 579 +++++++++++++++++++++
> include/drm/tinydrm/st7586.h | 34 ++
> 5 files changed, 650 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/st7586.txt
> create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
> create mode 100644 include/drm/tinydrm/st7586.h
>
> diff --git a/Documentation/devicetree/bindings/display/st7586.txt b/Documentation/devicetree/bindings/display/st7586.txt
> new file mode 100644
> index 0000000..acf784aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/st7586.txt
> @@ -0,0 +1,26 @@
> +Sitronix ST7586 display panel
> +
> +Required properties:
> +- compatible: "lego,ev3-lcd".
> +
> +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
This is a Lego specific pin which in turn drives IM (or IF) signals?
This should have a vendor prefix with either lego or Sitronix depending
on the answer.
Presence/absence doesn't sense. Doesn't the state of the GPIO line
matter? You should just say if not present, the interface defaults to
3-wire mode.
> +- 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:
> + display@0{
> + compatible = "lego,ev3-lcd";
> + reg = <0>;
> + spi-max-frequency = <10000000>;
> + dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> + };
^ permalink raw reply [flat|nested] 11+ messages in thread