* Re: [PATCH 2/2] drm/tiny: add support for tft displays based on ilitek,ili9488
2022-10-18 16:45 ` [PATCH 2/2] drm/tiny: add support " Tommaso Merciai
@ 2022-10-18 18:31 ` Michael Nazzareno Trimarchi
2022-10-19 9:04 ` Tommaso Merciai
2022-10-19 9:11 ` [PATCH 2/2] drm/tiny: add support for tft displays based on ilitek, ili9488 Javier Martinez Canillas
2022-10-24 12:52 ` Thomas Zimmermann
2 siblings, 1 reply; 11+ messages in thread
From: Michael Nazzareno Trimarchi @ 2022-10-18 18:31 UTC (permalink / raw)
To: Tommaso Merciai
Cc: linuxfancy, linux-amarula, kamlesh.gurudasani, David Airlie,
Daniel Vetter, Maxime Ripard, Thomas Zimmermann,
Javier Martinez Canillas, Sam Ravnborg, Noralf Trønnes,
linux-kernel, dri-devel
Hi
On Tue, Oct 18, 2022 at 6:46 PM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> wrote:
>
> This adds support for ilitek,ili9488 based displays with shift register
> in front of controller. Waveshare,pico-restouch-lcd-3.5 are such displays
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
Because I start to make it working this driver, I think that my
signed-off is missing here
> drivers/gpu/drm/tiny/Kconfig | 13 +
> drivers/gpu/drm/tiny/Makefile | 1 +
> drivers/gpu/drm/tiny/ili9488.c | 440 +++++++++++++++++++++++++++++++++
> 3 files changed, 454 insertions(+)
> create mode 100644 drivers/gpu/drm/tiny/ili9488.c
>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index 027cd87c3d0d7..6e708e8414806 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -148,6 +148,19 @@ config TINYDRM_ILI9486
>
> If M is selected the module will be called ili9486.
>
> +config TINYDRM_ILI9488
> + tristate "DRM support for ILI9488 display panels"
> + depends on DRM && SPI
> + select DRM_KMS_HELPER
> + select DRM_GEM_CMA_HELPER
> + select DRM_MIPI_DBI
> + select BACKLIGHT_CLASS_DEVICE
> + help
> + DRM driver for the following Ilitek ILI9488 panels:
> + * LCD 3.5" 320x480 TFT (Waveshare Pico-ResTouch-LCD-3.5")
> +
> + If M is selected the module will be called ili9486.
> +
> config TINYDRM_MI0283QT
> tristate "DRM support for MI0283QT"
> depends on DRM && SPI
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index 1d9d6227e7ab7..aad6683b2ac40 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_TINYDRM_ILI9163) += ili9163.o
> obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o
> obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o
> obj-$(CONFIG_TINYDRM_ILI9486) += ili9486.o
> +obj-$(CONFIG_TINYDRM_ILI9488) += ili9488.o
> obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
> obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
> obj-$(CONFIG_TINYDRM_ST7586) += st7586.o
> diff --git a/drivers/gpu/drm/tiny/ili9488.c b/drivers/gpu/drm/tiny/ili9488.c
> new file mode 100644
> index 0000000000000..b94d9d4ff4544
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/ili9488.c
> @@ -0,0 +1,440 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * DRM driver for Ilitek ILI9488 panels
> + *
> + * Copyright 2020 Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>
> + */
Code was changed a bit so please add copyright of me and you
> +
> +#include <linux/delay.h>
> +#include <linux/dma-buf.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/spi/spi.h>
> +#include <video/mipi_display.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_modeset_helper.h>
> +
> +#define ILI9488_VCOM_CONTROL_1 0xC5
> +#define ILI9488_COLUMN_ADDRESS_SET 0x2A
> +#define ILI9488_PAGE_ADDRESS_SET 0x2B
> +#define ILI9488_MEMORY_WRITE 0x2C
> +#define ILI9488_POSITIVE_GAMMA_CORRECTION 0xE0
> +#define ILI9488_NEGATIVE_GAMMA_CORRECTION 0xE1
> +#define ILI9488_POWER_CONTROL_1 0xC0
> +#define ILI9488_POWER_CONTROL_2 0xC1
> +#define ILI9488_POWER_CONTROL_3 0xC2
> +#define ILI9488_MEM_ACCESS_CONTROL 0x36
> +#define ILI9488_COLMOD_PIXEL_FORMAT_SET 0x3A
> +#define ILI9488_INTERFACE_MODE_CONTROL 0xB0
> +#define ILI9488_FRAME_RATE_CONTROL_PARTIAL 0xB3
> +#define ILI9488_DISPLAY_INVERSION_ON 0x21
> +#define ILI9488_DISPLAY_INVERSION_CONTROL 0xB4
> +#define ILI9488_DISPLAY_FUNCTION_CONTROL 0xB6
> +#define ILI9488_ENTRY_MODE_SET 0xB7
> +#define ILI9488_HS_LANES_CONTROL 0xBE
> +#define ILI9488_SET_IMAGE_FUNCTION 0xE9
> +#define ILI9488_ADJUST_CONTROL_3 0xF7
> +#define ILI9488_DISPLAY_ON 0x29
> +#define ILI9488_DISPLAY_OFF 0x28
> +#define ILI9488_ENTER_SLEEP_MODE 0x10
> +#define ILI9488_DBI_BPP18 0x06
> +#define ILI9488_DBI_BPP16 0x05
> +#define ILI9488_DPI_BPP24 0x70
> +#define ILI9488_DPI_BPP18 0x60
> +#define ILI9488_DPI_BPP16 0x50
> +#define ILI9488_FRAME_RATE_CONTROL_NORMAL 0xB1
> +#define ILI9488_SLEEP_OUT 0x11
> +
> +#define ILI9488_MADCTL_RGB BIT(2)
> +#define ILI9488_MADCTL_BGR BIT(3)
> +#define ILI9488_MADCTL_MV BIT(5)
> +#define ILI9488_MADCTL_MX BIT(6)
> +#define ILI9488_MADCTL_MY BIT(7)
> +
> +static void ili9488_rgb565_to_rgb666_line(u8 *dst, u16 *sbuf,
> + unsigned int pixels)
> +{
> + unsigned int x;
> +
> + for (x = 0; x < pixels; x++) {
> + *dst++ = ((*sbuf & 0xF800) >> 8);
> + *dst++ = ((*sbuf & 0x07E0) >> 3);
> + *dst++ = ((*sbuf & 0x001F) << 3);
> + sbuf++;
> + }
> +}
> +
> +static void ili9488_rgb565_to_rgb666(u8 *dst, void *vaddr,
> + struct drm_framebuffer *fb,
> + struct drm_rect *rect)
> +{
> + unsigned long linepixels = drm_rect_width(rect);
> + unsigned long lines = drm_rect_height(rect);
> + size_t dst_len = linepixels * 3;
> + size_t src_len = linepixels * fb->format->cpp[0];
> + unsigned int y;
> + u16 *sbuf;
> +
> + /*
> + * The cma memory is write-combined so reads are uncached.
> + * Speed up by fetching one line at a time.
> + */
> + sbuf = kmalloc(src_len, GFP_KERNEL);
> + if (!sbuf)
> + return;
> +
> + memset(sbuf, 0, src_len);
> +
Is this really needed?. This will be write on the copy
> + vaddr += rect->y1 * fb->pitches[0] + rect->x1 * fb->format->cpp[0];
> + for (y = 0; y < lines; y++) {
> + memcpy(sbuf, vaddr, src_len);
> + ili9488_rgb565_to_rgb666_line(dst, sbuf, linepixels);
> + vaddr += fb->pitches[0];
> + dst += dst_len;
> + }
> + kfree(sbuf);
> +}
> +
> +static int ili9488_buf_copy(void *dst, struct drm_framebuffer *fb,
> + struct drm_rect *rect)
> +{
> + struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> + void *src = cma_obj->vaddr;
> + int ret = 0;
> +
> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> + if (ret)
> + return ret;
> +
> + switch (fb->format->format) {
> + case DRM_FORMAT_RGB565:
> + ili9488_rgb565_to_rgb666(dst, src, fb, rect);
> + break;
> + default:
> + dev_err_once(fb->dev->dev, "Format is not supported: %p4cc\n",
> + &fb->format->format);
> + return -EINVAL;
> + }
> +
> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +
> + return ret;
> +}
> +
> +static void ili9488_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
> + unsigned int xs, unsigned int xe,
> + unsigned int ys, unsigned int ye)
> +{
> + struct mipi_dbi *dbi = &dbidev->dbi;
> +
> + xs += dbidev->left_offset;
> + xe += dbidev->left_offset;
> + ys += dbidev->top_offset;
> + ye += dbidev->top_offset;
> +
> + mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, (xs >> 8) & 0xff,
> + xs & 0xff, (xe >> 8) & 0xff, xe & 0xff);
> + mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, (ys >> 8) & 0xff,
> + ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
> +}
> +
This is duplicated from the drm and maybe we can export from there
> +static void ili9488_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{
> + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
> + unsigned int height = rect->y2 - rect->y1;
> + unsigned int width = rect->x2 - rect->x1;
> + struct mipi_dbi *dbi = &dbidev->dbi;
> + int idx, ret = 0;
> + bool full;
> + void *tr;
> +
> + if (WARN_ON(!fb))
> + return;
> +
> + if (!drm_dev_enter(fb->dev, &idx))
> + return;
> +
> + ret = drm_gem_fb_vmap(fb, map, data);
> + if (ret)
> + goto err_drm_dev_exit;
> +
> + full = width == fb->width && height == fb->height;
> +
> + DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
> +
> + if (!dbi->dc || !full ||
> + fb->format->format == DRM_FORMAT_RGB565) {
> + tr = dbidev->tx_buf;
> + ret = ili9488_buf_copy(dbidev->tx_buf, fb, rect);
> + if (ret)
> + goto err_msg;
> + } else {
> + tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
> + }
> +
> + ili9488_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
> + rect->y2 - 1);
> +
> + ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
> + width * height * 3);
> +err_msg:
> + if (ret)
> + drm_err_once(fb->dev, "Failed to update display %d\n", ret);
> +
> + drm_gem_fb_vunmap(fb, map);
> +
> +err_drm_dev_exit:
> + drm_dev_exit(idx);
> +}
> +
> +static void ili9488_pipe_update(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *old_state)
> +{
> + struct drm_plane_state *state = pipe->plane.state;
> + struct drm_rect rect;
> +
> + if (!pipe->crtc.state->active)
> + return;
> +
> + if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> + ili9488_fb_dirty(state->fb, &rect);
> +}
> +
> +static void ili9488_pipe_enable(struct drm_simple_display_pipe *pipe,
> + struct drm_crtc_state *crtc_state,
> + struct drm_plane_state *plane_state)
> +{
> + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> + struct drm_framebuffer *fb = plane_state->fb;
> + struct mipi_dbi *dbi = &dbidev->dbi;
> + u8 addr_mode;
> + struct drm_rect rect = {
> + .x1 = 0,
> + .x2 = fb->width,
> + .y1 = 0,
> + .y2 = fb->height,
> + };
rect can be dropped
> + int ret, idx;
> +
> + if (!drm_dev_enter(pipe->crtc.dev, &idx))
> + return;
> +
> + DRM_DEBUG_KMS("\n");
> +
> + ret = mipi_dbi_poweron_conditional_reset(dbidev);
> + if (ret < 0)
> + goto out_exit;
> + if (ret == 1)
> + goto out_enable;
> +
> + mipi_dbi_command(dbi, ILI9488_DISPLAY_INVERSION_ON);
> + mipi_dbi_command(dbi, ILI9488_POWER_CONTROL_3, 0x33);
> + mipi_dbi_command(dbi, ILI9488_FRAME_RATE_CONTROL_NORMAL, 0xB0);
> + mipi_dbi_command(dbi, ILI9488_MEM_ACCESS_CONTROL, 0x28);
> +
> + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, 0x65);
> +
> + mipi_dbi_command(dbi, ILI9488_POSITIVE_GAMMA_CORRECTION,
> + 0x00, 0x13, 0x18, 0x04, 0x0F,
> + 0x06, 0x3A, 0x56, 0x4D, 0x03,
> + 0x0A, 0x06, 0x30, 0x3E, 0x0F);
> + mipi_dbi_command(dbi, ILI9488_NEGATIVE_GAMMA_CORRECTION,
> + 0x00, 0x13, 0x18, 0x01, 0x11,
> + 0x06, 0x38, 0x3A, 0x4D, 0x06,
> + 0x0D, 0x0B, 0x31, 0x37, 0x0F);
> +
> + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT);
> + mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
> + msleep(120);
> + mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
> + msleep(100);
> +
> +out_enable:
> + switch (dbidev->rotation) {
> + default:
> + addr_mode = ILI9488_MADCTL_MX;
> + break;
> + case 90:
> + addr_mode = ILI9488_MADCTL_MV;
> + break;
> + case 180:
> + addr_mode = ILI9488_MADCTL_MY;
> + break;
> + case 270:
> + addr_mode = ILI9488_MADCTL_MV | ILI9488_MADCTL_MY |
> + ILI9488_MADCTL_MX;
> + break;
> + }
> + addr_mode |= ILI9488_MADCTL_BGR;
> + mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> + ili9488_fb_dirty(fb, &rect);
This is not needed and and rect can be drop as I have on latest code
mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
The flush should call the pipeline_update and you don't need to have
then anything more than this
Michael
> +out_exit:
> + drm_dev_exit(idx);
> +}
> +
> +static void ili9488_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> + /*
> + * This callback is not protected by drm_dev_enter/exit since we want to
> + * turn off the display on regular driver unload. It's highly unlikely
> + * that the underlying SPI controller is gone should this be called
> + * after unplug.
> + */
> +
> + DRM_DEBUG_KMS("\n");
> +
> + mipi_dbi_command(&dbidev->dbi, MIPI_DCS_SET_DISPLAY_OFF);
> +}
> +
> +static const u32 ili9488_formats[] = {
> + DRM_FORMAT_RGB565,
> +};
> +
> +static const struct drm_simple_display_pipe_funcs ili9488_pipe_funcs = {
> + .enable = ili9488_pipe_enable,
> + .disable = ili9488_pipe_disable,
> + .update = ili9488_pipe_update,
> +};
> +
> +static const struct drm_display_mode ili9488_mode = {
> + DRM_SIMPLE_MODE(320, 480, 49, 73),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(ili9488_fops);
> +
> +static struct drm_driver ili9488_driver = {
> + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> + .fops = &ili9488_fops,
> + DRM_GEM_CMA_DRIVER_OPS_VMAP,
> + .debugfs_init = mipi_dbi_debugfs_init,
> + .name = "ili9488",
> + .desc = "Ilitek ILI9488",
> + .date = "20221017",
> + .major = 1,
> + .minor = 0,
> +};
> +
> +static const struct of_device_id ili9488_of_match[] = {
> + { .compatible = "waveshare,pico-rt-lcd-35" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ili9488_of_match);
> +
> +static const struct spi_device_id ili9488_id[] = {
> + { "ili9488", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ili9488_id);
> +
> +static int ili9488_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct mipi_dbi_dev *dbidev;
> + struct drm_device *drm;
> + struct mipi_dbi *dbi;
> + struct gpio_desc *dc;
> + u32 rotation = 0;
> + size_t bufsize;
> + int ret;
> +
> + dbidev = devm_drm_dev_alloc(dev, &ili9488_driver,
> + struct mipi_dbi_dev, drm);
> + if (IS_ERR(dbidev))
> + return PTR_ERR(dbidev);
> +
> + dbi = &dbidev->dbi;
> + drm = &dbidev->drm;
> +
> + bufsize = ili9488_mode.vdisplay * ili9488_mode.hdisplay * 3;
> +
> + dbi->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(dbi->reset)) {
> + DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
> + return PTR_ERR(dbi->reset);
> + }
> +
> + dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW);
> + if (IS_ERR(dc)) {
> + DRM_DEV_ERROR(dev, "Failed to get gpio 'dc'\n");
> + return PTR_ERR(dc);
> + }
> +
> + dbidev->backlight = devm_of_find_backlight(dev);
> + if (IS_ERR(dbidev->backlight))
> + return PTR_ERR(dbidev->backlight);
> +
> + device_property_read_u32(dev, "rotation", &rotation);
> +
> + ret = mipi_dbi_spi_init(spi, dbi, dc);
> + if (ret)
> + return ret;
> +
> + dbidev->drm.mode_config.preferred_depth = 16;
> +
> + ret = mipi_dbi_dev_init_with_formats(dbidev, &ili9488_pipe_funcs,
> + ili9488_formats,
> + ARRAY_SIZE(ili9488_formats),
> + &ili9488_mode, rotation, bufsize);
> + if (ret)
> + return ret;
> +
> + dbi->swap_bytes = true;
> + drm_mode_config_reset(drm);
> +
> + ret = drm_dev_register(drm, 0);
> + if (ret)
> + return ret;
> +
> + spi_set_drvdata(spi, drm);
> +
> + drm_fbdev_generic_setup(drm, 0);
> +
> + return 0;
> +}
> +
> +static void ili9488_remove(struct spi_device *spi)
> +{
> + struct drm_device *drm = spi_get_drvdata(spi);
> +
> + drm_dev_unplug(drm);
> + drm_atomic_helper_shutdown(drm);
> +}
> +
> +static void ili9488_shutdown(struct spi_device *spi)
> +{
> + drm_atomic_helper_shutdown(spi_get_drvdata(spi));
> +}
> +
> +static struct spi_driver ili9488_spi_driver = {
> + .driver = {
> + .name = "ili9488",
> + .owner = THIS_MODULE,
> + .of_match_table = ili9488_of_match,
> + },
> + .id_table = ili9488_id,
> + .probe = ili9488_probe,
> + .remove = ili9488_remove,
> + .shutdown = ili9488_shutdown,
> +};
> +module_spi_driver(ili9488_spi_driver);
> +
> +MODULE_DESCRIPTION("Ilitek ILI9488 DRM driver");
> +MODULE_AUTHOR("Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>");
> +MODULE_AUTHOR("Michael Trimarchi <michael@amarulasolutions.com>");
> +MODULE_AUTHOR("Tommaso Merciai <tommaso.merciai@amarulasolutions.com>");
> +MODULE_LICENSE("GPL");
> \ No newline at end of file
> --
> 2.25.1
>
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] drm/tiny: add support for tft displays based on ilitek,ili9488
2022-10-18 18:31 ` Michael Nazzareno Trimarchi
@ 2022-10-19 9:04 ` Tommaso Merciai
0 siblings, 0 replies; 11+ messages in thread
From: Tommaso Merciai @ 2022-10-19 9:04 UTC (permalink / raw)
To: Michael Nazzareno Trimarchi
Cc: linuxfancy, linux-amarula, kamlesh.gurudasani, David Airlie,
Daniel Vetter, Maxime Ripard, Thomas Zimmermann,
Javier Martinez Canillas, Sam Ravnborg, Noralf Trønnes,
linux-kernel, dri-devel
On Tue, Oct 18, 2022 at 08:31:22PM +0200, Michael Nazzareno Trimarchi wrote:
Hi Michael,
> Hi
>
> On Tue, Oct 18, 2022 at 6:46 PM Tommaso Merciai
> <tommaso.merciai@amarulasolutions.com> wrote:
> >
> > This adds support for ilitek,ili9488 based displays with shift register
> > in front of controller. Waveshare,pico-restouch-lcd-3.5 are such displays
> >
> > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > ---
>
> Because I start to make it working this driver, I think that my
> signed-off is missing here
Yes, right. :)
I upload in v2, my bad
>
> > drivers/gpu/drm/tiny/Kconfig | 13 +
> > drivers/gpu/drm/tiny/Makefile | 1 +
> > drivers/gpu/drm/tiny/ili9488.c | 440 +++++++++++++++++++++++++++++++++
> > 3 files changed, 454 insertions(+)
> > create mode 100644 drivers/gpu/drm/tiny/ili9488.c
> >
> > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> > index 027cd87c3d0d7..6e708e8414806 100644
> > --- a/drivers/gpu/drm/tiny/Kconfig
> > +++ b/drivers/gpu/drm/tiny/Kconfig
> > @@ -148,6 +148,19 @@ config TINYDRM_ILI9486
> >
> > If M is selected the module will be called ili9486.
> >
> > +config TINYDRM_ILI9488
> > + tristate "DRM support for ILI9488 display panels"
> > + depends on DRM && SPI
> > + select DRM_KMS_HELPER
> > + select DRM_GEM_CMA_HELPER
> > + select DRM_MIPI_DBI
> > + select BACKLIGHT_CLASS_DEVICE
> > + help
> > + DRM driver for the following Ilitek ILI9488 panels:
> > + * LCD 3.5" 320x480 TFT (Waveshare Pico-ResTouch-LCD-3.5")
> > +
> > + If M is selected the module will be called ili9486.
> > +
> > config TINYDRM_MI0283QT
> > tristate "DRM support for MI0283QT"
> > depends on DRM && SPI
> > diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> > index 1d9d6227e7ab7..aad6683b2ac40 100644
> > --- a/drivers/gpu/drm/tiny/Makefile
> > +++ b/drivers/gpu/drm/tiny/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_TINYDRM_ILI9163) += ili9163.o
> > obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o
> > obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o
> > obj-$(CONFIG_TINYDRM_ILI9486) += ili9486.o
> > +obj-$(CONFIG_TINYDRM_ILI9488) += ili9488.o
> > obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
> > obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
> > obj-$(CONFIG_TINYDRM_ST7586) += st7586.o
> > diff --git a/drivers/gpu/drm/tiny/ili9488.c b/drivers/gpu/drm/tiny/ili9488.c
> > new file mode 100644
> > index 0000000000000..b94d9d4ff4544
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tiny/ili9488.c
> > @@ -0,0 +1,440 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * DRM driver for Ilitek ILI9488 panels
> > + *
> > + * Copyright 2020 Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>
> > + */
>
> Code was changed a bit so please add copyright of me and you
Agree, thanks.
>
> > +
> > +#include <linux/delay.h>
> > +#include <linux/dma-buf.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/property.h>
> > +#include <linux/spi/spi.h>
> > +#include <video/mipi_display.h>
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_drv.h>
> > +#include <drm/drm_fb_cma_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_damage_helper.h>
> > +#include <drm/drm_framebuffer.h>
> > +#include <drm/drm_gem_atomic_helper.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_managed.h>
> > +#include <drm/drm_mipi_dbi.h>
> > +#include <drm/drm_modeset_helper.h>
> > +
> > +#define ILI9488_VCOM_CONTROL_1 0xC5
> > +#define ILI9488_COLUMN_ADDRESS_SET 0x2A
> > +#define ILI9488_PAGE_ADDRESS_SET 0x2B
> > +#define ILI9488_MEMORY_WRITE 0x2C
> > +#define ILI9488_POSITIVE_GAMMA_CORRECTION 0xE0
> > +#define ILI9488_NEGATIVE_GAMMA_CORRECTION 0xE1
> > +#define ILI9488_POWER_CONTROL_1 0xC0
> > +#define ILI9488_POWER_CONTROL_2 0xC1
> > +#define ILI9488_POWER_CONTROL_3 0xC2
> > +#define ILI9488_MEM_ACCESS_CONTROL 0x36
> > +#define ILI9488_COLMOD_PIXEL_FORMAT_SET 0x3A
> > +#define ILI9488_INTERFACE_MODE_CONTROL 0xB0
> > +#define ILI9488_FRAME_RATE_CONTROL_PARTIAL 0xB3
> > +#define ILI9488_DISPLAY_INVERSION_ON 0x21
> > +#define ILI9488_DISPLAY_INVERSION_CONTROL 0xB4
> > +#define ILI9488_DISPLAY_FUNCTION_CONTROL 0xB6
> > +#define ILI9488_ENTRY_MODE_SET 0xB7
> > +#define ILI9488_HS_LANES_CONTROL 0xBE
> > +#define ILI9488_SET_IMAGE_FUNCTION 0xE9
> > +#define ILI9488_ADJUST_CONTROL_3 0xF7
> > +#define ILI9488_DISPLAY_ON 0x29
> > +#define ILI9488_DISPLAY_OFF 0x28
> > +#define ILI9488_ENTER_SLEEP_MODE 0x10
> > +#define ILI9488_DBI_BPP18 0x06
> > +#define ILI9488_DBI_BPP16 0x05
> > +#define ILI9488_DPI_BPP24 0x70
> > +#define ILI9488_DPI_BPP18 0x60
> > +#define ILI9488_DPI_BPP16 0x50
> > +#define ILI9488_FRAME_RATE_CONTROL_NORMAL 0xB1
> > +#define ILI9488_SLEEP_OUT 0x11
> > +
> > +#define ILI9488_MADCTL_RGB BIT(2)
> > +#define ILI9488_MADCTL_BGR BIT(3)
> > +#define ILI9488_MADCTL_MV BIT(5)
> > +#define ILI9488_MADCTL_MX BIT(6)
> > +#define ILI9488_MADCTL_MY BIT(7)
> > +
> > +static void ili9488_rgb565_to_rgb666_line(u8 *dst, u16 *sbuf,
> > + unsigned int pixels)
> > +{
> > + unsigned int x;
> > +
> > + for (x = 0; x < pixels; x++) {
> > + *dst++ = ((*sbuf & 0xF800) >> 8);
> > + *dst++ = ((*sbuf & 0x07E0) >> 3);
> > + *dst++ = ((*sbuf & 0x001F) << 3);
> > + sbuf++;
> > + }
> > +}
> > +
> > +static void ili9488_rgb565_to_rgb666(u8 *dst, void *vaddr,
> > + struct drm_framebuffer *fb,
> > + struct drm_rect *rect)
> > +{
> > + unsigned long linepixels = drm_rect_width(rect);
> > + unsigned long lines = drm_rect_height(rect);
> > + size_t dst_len = linepixels * 3;
> > + size_t src_len = linepixels * fb->format->cpp[0];
> > + unsigned int y;
> > + u16 *sbuf;
> > +
> > + /*
> > + * The cma memory is write-combined so reads are uncached.
> > + * Speed up by fetching one line at a time.
> > + */
> > + sbuf = kmalloc(src_len, GFP_KERNEL);
> > + if (!sbuf)
> > + return;
> > +
> > + memset(sbuf, 0, src_len);
> > +
>
> Is this really needed?. This will be write on the copy
You are right, this comes from some test. I remove this in v2.
>
> > + vaddr += rect->y1 * fb->pitches[0] + rect->x1 * fb->format->cpp[0];
> > + for (y = 0; y < lines; y++) {
> > + memcpy(sbuf, vaddr, src_len);
> > + ili9488_rgb565_to_rgb666_line(dst, sbuf, linepixels);
> > + vaddr += fb->pitches[0];
> > + dst += dst_len;
> > + }
> > + kfree(sbuf);
> > +}
> > +
> > +static int ili9488_buf_copy(void *dst, struct drm_framebuffer *fb,
> > + struct drm_rect *rect)
> > +{
> > + struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> > + void *src = cma_obj->vaddr;
> > + int ret = 0;
> > +
> > + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> > + if (ret)
> > + return ret;
> > +
> > + switch (fb->format->format) {
> > + case DRM_FORMAT_RGB565:
> > + ili9488_rgb565_to_rgb666(dst, src, fb, rect);
> > + break;
> > + default:
> > + dev_err_once(fb->dev->dev, "Format is not supported: %p4cc\n",
> > + &fb->format->format);
> > + return -EINVAL;
> > + }
> > +
> > + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> > +
> > + return ret;
> > +}
> > +
> > +static void ili9488_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
> > + unsigned int xs, unsigned int xe,
> > + unsigned int ys, unsigned int ye)
> > +{
> > + struct mipi_dbi *dbi = &dbidev->dbi;
> > +
> > + xs += dbidev->left_offset;
> > + xe += dbidev->left_offset;
> > + ys += dbidev->top_offset;
> > + ye += dbidev->top_offset;
> > +
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, (xs >> 8) & 0xff,
> > + xs & 0xff, (xe >> 8) & 0xff, xe & 0xff);
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, (ys >> 8) & 0xff,
> > + ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
> > +}
> > +
>
> This is duplicated from the drm and maybe we can export from there
I'll check and let you know.
>
> > +static void ili9488_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> > +{
> > + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> > + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> > + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
> > + unsigned int height = rect->y2 - rect->y1;
> > + unsigned int width = rect->x2 - rect->x1;
> > + struct mipi_dbi *dbi = &dbidev->dbi;
> > + int idx, ret = 0;
> > + bool full;
> > + void *tr;
> > +
> > + if (WARN_ON(!fb))
> > + return;
> > +
> > + if (!drm_dev_enter(fb->dev, &idx))
> > + return;
> > +
> > + ret = drm_gem_fb_vmap(fb, map, data);
> > + if (ret)
> > + goto err_drm_dev_exit;
> > +
> > + full = width == fb->width && height == fb->height;
> > +
> > + DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
> > +
> > + if (!dbi->dc || !full ||
> > + fb->format->format == DRM_FORMAT_RGB565) {
> > + tr = dbidev->tx_buf;
> > + ret = ili9488_buf_copy(dbidev->tx_buf, fb, rect);
> > + if (ret)
> > + goto err_msg;
> > + } else {
> > + tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
> > + }
> > +
> > + ili9488_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
> > + rect->y2 - 1);
> > +
> > + ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
> > + width * height * 3);
> > +err_msg:
> > + if (ret)
> > + drm_err_once(fb->dev, "Failed to update display %d\n", ret);
> > +
> > + drm_gem_fb_vunmap(fb, map);
> > +
> > +err_drm_dev_exit:
> > + drm_dev_exit(idx);
> > +}
> > +
> > +static void ili9488_pipe_update(struct drm_simple_display_pipe *pipe,
> > + struct drm_plane_state *old_state)
> > +{
> > + struct drm_plane_state *state = pipe->plane.state;
> > + struct drm_rect rect;
> > +
> > + if (!pipe->crtc.state->active)
> > + return;
> > +
> > + if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> > + ili9488_fb_dirty(state->fb, &rect);
> > +}
> > +
> > +static void ili9488_pipe_enable(struct drm_simple_display_pipe *pipe,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_plane_state *plane_state)
> > +{
> > + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> > + struct drm_framebuffer *fb = plane_state->fb;
> > + struct mipi_dbi *dbi = &dbidev->dbi;
> > + u8 addr_mode;
> > + struct drm_rect rect = {
> > + .x1 = 0,
> > + .x2 = fb->width,
> > + .y1 = 0,
> > + .y2 = fb->height,
> > + };
>
> rect can be dropped
>
> > + int ret, idx;
> > +
> > + if (!drm_dev_enter(pipe->crtc.dev, &idx))
> > + return;
> > +
> > + DRM_DEBUG_KMS("\n");
> > +
> > + ret = mipi_dbi_poweron_conditional_reset(dbidev);
> > + if (ret < 0)
> > + goto out_exit;
> > + if (ret == 1)
> > + goto out_enable;
> > +
> > + mipi_dbi_command(dbi, ILI9488_DISPLAY_INVERSION_ON);
> > + mipi_dbi_command(dbi, ILI9488_POWER_CONTROL_3, 0x33);
> > + mipi_dbi_command(dbi, ILI9488_FRAME_RATE_CONTROL_NORMAL, 0xB0);
> > + mipi_dbi_command(dbi, ILI9488_MEM_ACCESS_CONTROL, 0x28);
> > +
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, 0x65);
> > +
> > + mipi_dbi_command(dbi, ILI9488_POSITIVE_GAMMA_CORRECTION,
> > + 0x00, 0x13, 0x18, 0x04, 0x0F,
> > + 0x06, 0x3A, 0x56, 0x4D, 0x03,
> > + 0x0A, 0x06, 0x30, 0x3E, 0x0F);
> > + mipi_dbi_command(dbi, ILI9488_NEGATIVE_GAMMA_CORRECTION,
> > + 0x00, 0x13, 0x18, 0x01, 0x11,
> > + 0x06, 0x38, 0x3A, 0x4D, 0x06,
> > + 0x0D, 0x0B, 0x31, 0x37, 0x0F);
> > +
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT);
> > + mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
> > + msleep(120);
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
> > + msleep(100);
> > +
> > +out_enable:
> > + switch (dbidev->rotation) {
> > + default:
> > + addr_mode = ILI9488_MADCTL_MX;
> > + break;
> > + case 90:
> > + addr_mode = ILI9488_MADCTL_MV;
> > + break;
> > + case 180:
> > + addr_mode = ILI9488_MADCTL_MY;
> > + break;
> > + case 270:
> > + addr_mode = ILI9488_MADCTL_MV | ILI9488_MADCTL_MY |
> > + ILI9488_MADCTL_MX;
> > + break;
> > + }
> > + addr_mode |= ILI9488_MADCTL_BGR;
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> > + ili9488_fb_dirty(fb, &rect);
>
> This is not needed and and rect can be drop as I have on latest code
>
> mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
>
> The flush should call the pipeline_update and you don't need to have
> then anything more than this
Don't agree on this I found the following description:
drivers/gpu/drm/drm_mipi_dbi.c
Note: Drivers which don't use mipi_dbi_pipe_update() because they have custom
framebuffer flushing, can't use this function since they both use the same
flushing code.
Thanks & Regards,
Tommaso
>
> Michael
>
> > +out_exit:
> > + drm_dev_exit(idx);
> > +}
> > +
> > +static void ili9488_pipe_disable(struct drm_simple_display_pipe *pipe)
> > +{
> > + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> > + /*
> > + * This callback is not protected by drm_dev_enter/exit since we want to
> > + * turn off the display on regular driver unload. It's highly unlikely
> > + * that the underlying SPI controller is gone should this be called
> > + * after unplug.
> > + */
> > +
> > + DRM_DEBUG_KMS("\n");
> > +
> > + mipi_dbi_command(&dbidev->dbi, MIPI_DCS_SET_DISPLAY_OFF);
> > +}
> > +
> > +static const u32 ili9488_formats[] = {
> > + DRM_FORMAT_RGB565,
> > +};
> > +
> > +static const struct drm_simple_display_pipe_funcs ili9488_pipe_funcs = {
> > + .enable = ili9488_pipe_enable,
> > + .disable = ili9488_pipe_disable,
> > + .update = ili9488_pipe_update,
> > +};
> > +
> > +static const struct drm_display_mode ili9488_mode = {
> > + DRM_SIMPLE_MODE(320, 480, 49, 73),
> > +};
> > +
> > +DEFINE_DRM_GEM_CMA_FOPS(ili9488_fops);
> > +
> > +static struct drm_driver ili9488_driver = {
> > + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> > + .fops = &ili9488_fops,
> > + DRM_GEM_CMA_DRIVER_OPS_VMAP,
> > + .debugfs_init = mipi_dbi_debugfs_init,
> > + .name = "ili9488",
> > + .desc = "Ilitek ILI9488",
> > + .date = "20221017",
> > + .major = 1,
> > + .minor = 0,
> > +};
> > +
> > +static const struct of_device_id ili9488_of_match[] = {
> > + { .compatible = "waveshare,pico-rt-lcd-35" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ili9488_of_match);
> > +
> > +static const struct spi_device_id ili9488_id[] = {
> > + { "ili9488", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(spi, ili9488_id);
> > +
> > +static int ili9488_probe(struct spi_device *spi)
> > +{
> > + struct device *dev = &spi->dev;
> > + struct mipi_dbi_dev *dbidev;
> > + struct drm_device *drm;
> > + struct mipi_dbi *dbi;
> > + struct gpio_desc *dc;
> > + u32 rotation = 0;
> > + size_t bufsize;
> > + int ret;
> > +
> > + dbidev = devm_drm_dev_alloc(dev, &ili9488_driver,
> > + struct mipi_dbi_dev, drm);
> > + if (IS_ERR(dbidev))
> > + return PTR_ERR(dbidev);
> > +
> > + dbi = &dbidev->dbi;
> > + drm = &dbidev->drm;
> > +
> > + bufsize = ili9488_mode.vdisplay * ili9488_mode.hdisplay * 3;
> > +
> > + dbi->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(dbi->reset)) {
> > + DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
> > + return PTR_ERR(dbi->reset);
> > + }
> > +
> > + dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW);
> > + if (IS_ERR(dc)) {
> > + DRM_DEV_ERROR(dev, "Failed to get gpio 'dc'\n");
> > + return PTR_ERR(dc);
> > + }
> > +
> > + dbidev->backlight = devm_of_find_backlight(dev);
> > + if (IS_ERR(dbidev->backlight))
> > + return PTR_ERR(dbidev->backlight);
> > +
> > + device_property_read_u32(dev, "rotation", &rotation);
> > +
> > + ret = mipi_dbi_spi_init(spi, dbi, dc);
> > + if (ret)
> > + return ret;
> > +
> > + dbidev->drm.mode_config.preferred_depth = 16;
> > +
> > + ret = mipi_dbi_dev_init_with_formats(dbidev, &ili9488_pipe_funcs,
> > + ili9488_formats,
> > + ARRAY_SIZE(ili9488_formats),
> > + &ili9488_mode, rotation, bufsize);
> > + if (ret)
> > + return ret;
> > +
> > + dbi->swap_bytes = true;
> > + drm_mode_config_reset(drm);
> > +
> > + ret = drm_dev_register(drm, 0);
> > + if (ret)
> > + return ret;
> > +
> > + spi_set_drvdata(spi, drm);
> > +
> > + drm_fbdev_generic_setup(drm, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static void ili9488_remove(struct spi_device *spi)
> > +{
> > + struct drm_device *drm = spi_get_drvdata(spi);
> > +
> > + drm_dev_unplug(drm);
> > + drm_atomic_helper_shutdown(drm);
> > +}
> > +
> > +static void ili9488_shutdown(struct spi_device *spi)
> > +{
> > + drm_atomic_helper_shutdown(spi_get_drvdata(spi));
> > +}
> > +
> > +static struct spi_driver ili9488_spi_driver = {
> > + .driver = {
> > + .name = "ili9488",
> > + .owner = THIS_MODULE,
> > + .of_match_table = ili9488_of_match,
> > + },
> > + .id_table = ili9488_id,
> > + .probe = ili9488_probe,
> > + .remove = ili9488_remove,
> > + .shutdown = ili9488_shutdown,
> > +};
> > +module_spi_driver(ili9488_spi_driver);
> > +
> > +MODULE_DESCRIPTION("Ilitek ILI9488 DRM driver");
> > +MODULE_AUTHOR("Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>");
> > +MODULE_AUTHOR("Michael Trimarchi <michael@amarulasolutions.com>");
> > +MODULE_AUTHOR("Tommaso Merciai <tommaso.merciai@amarulasolutions.com>");
> > +MODULE_LICENSE("GPL");
> > \ No newline at end of file
> > --
> > 2.25.1
> >
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com
--
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/tiny: add support for tft displays based on ilitek, ili9488
2022-10-18 16:45 ` [PATCH 2/2] drm/tiny: add support " Tommaso Merciai
2022-10-18 18:31 ` Michael Nazzareno Trimarchi
@ 2022-10-19 9:11 ` Javier Martinez Canillas
2022-10-24 12:52 ` Thomas Zimmermann
2 siblings, 0 replies; 11+ messages in thread
From: Javier Martinez Canillas @ 2022-10-19 9:11 UTC (permalink / raw)
To: Tommaso Merciai
Cc: michael, Thomas Zimmermann, Sam Ravnborg, linuxfancy, dri-devel,
linux-kernel, Noralf Trønnes, Maxime Ripard,
kamlesh.gurudasani, linux-amarula
Hello Tommaso,
On 10/18/22 18:45, Tommaso Merciai wrote:
[...]
> +config TINYDRM_ILI9488
> + tristate "DRM support for ILI9488 display panels"
> + depends on DRM && SPI
> + select DRM_KMS_HELPER
> + select DRM_GEM_CMA_HELPER
> + select DRM_MIPI_DBI
> + select BACKLIGHT_CLASS_DEVICE
> + help
> + DRM driver for the following Ilitek ILI9488 panels:
> + * LCD 3.5" 320x480 TFT (Waveshare Pico-ResTouch-LCD-3.5")
> +
> + If M is selected the module will be called ili9486.
> +
Typo here, should be ili9488.
[...]
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_modeset_helper.h>
> +
Please sort these alphabetically
> +
> +static void ili9488_rgb565_to_rgb666_line(u8 *dst, u16 *sbuf,
> + unsigned int pixels)
> +{
> + unsigned int x;
> +
> + for (x = 0; x < pixels; x++) {
> + *dst++ = ((*sbuf & 0xF800) >> 8);
> + *dst++ = ((*sbuf & 0x07E0) >> 3);
> + *dst++ = ((*sbuf & 0x001F) << 3);
> + sbuf++;
> + }
> +}
> +
If these format conversions helpers are really needed, they need to be
added as helpers to drivers/gpu/drm/drm_format_helper.c instead.
> +static void ili9488_rgb565_to_rgb666(u8 *dst, void *vaddr,
> + struct drm_framebuffer *fb,
> + struct drm_rect *rect)
> +{
> + unsigned long linepixels = drm_rect_width(rect);
> + unsigned long lines = drm_rect_height(rect);
> + size_t dst_len = linepixels * 3;
> + size_t src_len = linepixels * fb->format->cpp[0];
> + unsigned int y;
> + u16 *sbuf;
> +
> + /*
> + * The cma memory is write-combined so reads are uncached.
> + * Speed up by fetching one line at a time.
> + */
> + sbuf = kmalloc(src_len, GFP_KERNEL);
> + if (!sbuf)
> + return;
This will cause an extra copy even when CMA isn't used. Please take a look
how the format helpers do this. You should pass a struct iosys_map param
and internally use the drm_fb_xfrm() helper that would handle both the I/O
mem and system memory cases.
> +static int ili9488_buf_copy(void *dst, struct drm_framebuffer *fb,
> + struct drm_rect *rect)
> +{
> + struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> + void *src = cma_obj->vaddr;
> + int ret = 0;
> +
> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> + if (ret)
> + return ret;
> +
If you rebase on top of the "[PATCH 0/5] drm: Add new plane helpers to
begin/end FB access" series then an explicit CPU access to GEM BOs sync
isn't needed anymore:
https://lore.kernel.org/dri-devel/20221017111510.20818-1-tzimmermann@suse.de/
> + switch (fb->format->format) {
> + case DRM_FORMAT_RGB565:
> + ili9488_rgb565_to_rgb666(dst, src, fb, rect);
This is the biggest issue I see with this driver. The exported format is
RGB565 but RGB666 is used. I believe the policy is for the driver to expose
the native format to user-space.
I know that there isn't a DRM_FORMAT_RGB666 in neither DRM nor mesa, but
still I think that adding it should be part of this series. If you also
want to expose DRM_FORMAT_RGB565 for compatibility reasons, then I guess
that's OK but as mentioned the format helpers need to be in the DRM core.
[...]
> +static void ili9488_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{
This looks very similar, if not the same than the mipi_dbi_fb_dirty() helper.
[...]
> +static void ili9488_pipe_update(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *old_state)
> +{
And same for this, it's basically mipi_dbi_pipe_update() if you end using the
mipi_dbi_fb_dirty() helper instead of a custom ili9488_fb_dirty() handler.
> + struct drm_plane_state *state = pipe->plane.state;
> + struct drm_rect rect;
> +
> + if (!pipe->crtc.state->active)
> + return;
> +
> + if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> + ili9488_fb_dirty(state->fb, &rect);
I see that most MIPI DBI drivers use this function to merge all the damage clips
into a big rectangle. Is there a reason why this is done in this way rather than
just iterating over all the damage areas and update them one by one?
Since for example there are multiple damage clips that aren't close to each other,
the resulting rectangle could be quite big.
[...]
> +DEFINE_DRM_GEM_CMA_FOPS(ili9488_fops);
> +
Do you really need CMA for this? Can't you just use DEFINE_DRM_GEM_FOPS() instead?
> +static struct drm_driver ili9488_driver = {
> + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> + .fops = &ili9488_fops,
> + DRM_GEM_CMA_DRIVER_OPS_VMAP,
DRM_GEM_SHMEM_DRIVER_OPS, ?
> + .debugfs_init = mipi_dbi_debugfs_init,
> + .name = "ili9488",
> + .desc = "Ilitek ILI9488",
> + .date = "20221017",
> + .major = 1,
> + .minor = 0,
> +};
> +
> +static const struct of_device_id ili9488_of_match[] = {
> + { .compatible = "waveshare,pico-rt-lcd-35" },
> + { }
> +};
Do you really need to make the compatible that specific? I would just have an entry
for "ilitek,ili9488".
> +MODULE_DEVICE_TABLE(of, ili9488_of_match);
> +
> +static const struct spi_device_id ili9488_id[] = {
> + { "ili9488", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ili9488_id);
> +
The fact that this doesn't match the OF compatible string would break module auto
loading. Because the SPI core doesn't report an OF module alias, but always a SPI
alias regardless whether the device was registered using Device Trees or not.
[...]
> + dbi->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(dbi->reset)) {
> + DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
> + return PTR_ERR(dbi->reset);
> + }
You could use dev_err_probe() instead. And same in other places.
> +static void ili9488_remove(struct spi_device *spi)
> +{
> + struct drm_device *drm = spi_get_drvdata(spi);
> +
> + drm_dev_unplug(drm);
> + drm_atomic_helper_shutdown(drm);
I believe drm_atomic_helper_shutdown() isn't needed here since is done already
in ili9488_shutdown().
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] drm/tiny: add support for tft displays based on ilitek, ili9488
2022-10-18 16:45 ` [PATCH 2/2] drm/tiny: add support " Tommaso Merciai
2022-10-18 18:31 ` Michael Nazzareno Trimarchi
2022-10-19 9:11 ` [PATCH 2/2] drm/tiny: add support for tft displays based on ilitek, ili9488 Javier Martinez Canillas
@ 2022-10-24 12:52 ` Thomas Zimmermann
2 siblings, 0 replies; 11+ messages in thread
From: Thomas Zimmermann @ 2022-10-24 12:52 UTC (permalink / raw)
To: Tommaso Merciai
Cc: michael, Sam Ravnborg, linuxfancy, Javier Martinez Canillas,
dri-devel, linux-kernel, Noralf Trønnes, Maxime Ripard,
kamlesh.gurudasani, linux-amarula
[-- Attachment #1.1: Type: text/plain, Size: 18326 bytes --]
Hi,
thanks for your driver submission.
Am 18.10.22 um 18:45 schrieb Tommaso Merciai:
> This adds support for ilitek,ili9488 based displays with shift register
> in front of controller. Waveshare,pico-restouch-lcd-3.5 are such displays
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
> drivers/gpu/drm/tiny/Kconfig | 13 +
> drivers/gpu/drm/tiny/Makefile | 1 +
> drivers/gpu/drm/tiny/ili9488.c | 440 +++++++++++++++++++++++++++++++++
> 3 files changed, 454 insertions(+)
> create mode 100644 drivers/gpu/drm/tiny/ili9488.c
>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index 027cd87c3d0d7..6e708e8414806 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -148,6 +148,19 @@ config TINYDRM_ILI9486
>
> If M is selected the module will be called ili9486.
>
> +config TINYDRM_ILI9488
> + tristate "DRM support for ILI9488 display panels"
> + depends on DRM && SPI
> + select DRM_KMS_HELPER
> + select DRM_GEM_CMA_HELPER
This is now called DRM_GEM_DMA_HELPER and all _cma_ infixes have been
renamed to _dma_. Please rebase onto drm-tip or drm-misc-next and you'll
notice.
BUT I think this driver should use GEM SHMEM helpers instead (see
DRM_GEM_SHMEM_HELPER). Nothing here needs DMA memory AFAICT.
> + select DRM_MIPI_DBI
> + select BACKLIGHT_CLASS_DEVICE
> + help
> + DRM driver for the following Ilitek ILI9488 panels:
> + * LCD 3.5" 320x480 TFT (Waveshare Pico-ResTouch-LCD-3.5")
> +
> + If M is selected the module will be called ili9486.
> +
> config TINYDRM_MI0283QT
> tristate "DRM support for MI0283QT"
> depends on DRM && SPI
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index 1d9d6227e7ab7..aad6683b2ac40 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_TINYDRM_ILI9163) += ili9163.o
> obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o
> obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o
> obj-$(CONFIG_TINYDRM_ILI9486) += ili9486.o
> +obj-$(CONFIG_TINYDRM_ILI9488) += ili9488.o
> obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
> obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
> obj-$(CONFIG_TINYDRM_ST7586) += st7586.o
> diff --git a/drivers/gpu/drm/tiny/ili9488.c b/drivers/gpu/drm/tiny/ili9488.c
> new file mode 100644
> index 0000000000000..b94d9d4ff4544
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/ili9488.c
> @@ -0,0 +1,440 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * DRM driver for Ilitek ILI9488 panels
> + *
> + * Copyright 2020 Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>
He's also in CC. Shouldn't he have signed off the patch?
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/dma-buf.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/spi/spi.h>
empty line here
> +#include <video/mipi_display.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_modeset_helper.h>
This block has to be order alphabetically.
> +
> +#define ILI9488_VCOM_CONTROL_1 0xC5
> +#define ILI9488_COLUMN_ADDRESS_SET 0x2A
> +#define ILI9488_PAGE_ADDRESS_SET 0x2B
> +#define ILI9488_MEMORY_WRITE 0x2C
> +#define ILI9488_POSITIVE_GAMMA_CORRECTION 0xE0
> +#define ILI9488_NEGATIVE_GAMMA_CORRECTION 0xE1
> +#define ILI9488_POWER_CONTROL_1 0xC0
> +#define ILI9488_POWER_CONTROL_2 0xC1
> +#define ILI9488_POWER_CONTROL_3 0xC2
> +#define ILI9488_MEM_ACCESS_CONTROL 0x36
> +#define ILI9488_COLMOD_PIXEL_FORMAT_SET 0x3A
> +#define ILI9488_INTERFACE_MODE_CONTROL 0xB0
> +#define ILI9488_FRAME_RATE_CONTROL_PARTIAL 0xB3
> +#define ILI9488_DISPLAY_INVERSION_ON 0x21
> +#define ILI9488_DISPLAY_INVERSION_CONTROL 0xB4
> +#define ILI9488_DISPLAY_FUNCTION_CONTROL 0xB6
> +#define ILI9488_ENTRY_MODE_SET 0xB7
> +#define ILI9488_HS_LANES_CONTROL 0xBE
> +#define ILI9488_SET_IMAGE_FUNCTION 0xE9
> +#define ILI9488_ADJUST_CONTROL_3 0xF7
> +#define ILI9488_DISPLAY_ON 0x29
> +#define ILI9488_DISPLAY_OFF 0x28
> +#define ILI9488_ENTER_SLEEP_MODE 0x10
> +#define ILI9488_DBI_BPP18 0x06
> +#define ILI9488_DBI_BPP16 0x05
> +#define ILI9488_DPI_BPP24 0x70
> +#define ILI9488_DPI_BPP18 0x60
> +#define ILI9488_DPI_BPP16 0x50
> +#define ILI9488_FRAME_RATE_CONTROL_NORMAL 0xB1
> +#define ILI9488_SLEEP_OUT 0x11
> +
> +#define ILI9488_MADCTL_RGB BIT(2)
> +#define ILI9488_MADCTL_BGR BIT(3)
> +#define ILI9488_MADCTL_MV BIT(5)
> +#define ILI9488_MADCTL_MX BIT(6)
> +#define ILI9488_MADCTL_MY BIT(7)
> +
> +static void ili9488_rgb565_to_rgb666_line(u8 *dst, u16 *sbuf,
> + unsigned int pixels)
> +{
> + unsigned int x;
> +
> + for (x = 0; x < pixels; x++) {
> + *dst++ = ((*sbuf & 0xF800) >> 8);
> + *dst++ = ((*sbuf & 0x07E0) >> 3);
> + *dst++ = ((*sbuf & 0x001F) << 3);
> + sbuf++;
> + }
> +}
> +
> +static void ili9488_rgb565_to_rgb666(u8 *dst, void *vaddr,
> + struct drm_framebuffer *fb,
> + struct drm_rect *rect)
> +{
> + unsigned long linepixels = drm_rect_width(rect);
> + unsigned long lines = drm_rect_height(rect);
> + size_t dst_len = linepixels * 3;
> + size_t src_len = linepixels * fb->format->cpp[0];
> + unsigned int y;
> + u16 *sbuf;
> +
> + /*
> + * The cma memory is write-combined so reads are uncached.
> + * Speed up by fetching one line at a time.
> + */
> + sbuf = kmalloc(src_len, GFP_KERNEL);
> + if (!sbuf)
> + return;
> +
> + memset(sbuf, 0, src_len);
> +
> + vaddr += rect->y1 * fb->pitches[0] + rect->x1 * fb->format->cpp[0];
> + for (y = 0; y < lines; y++) {
> + memcpy(sbuf, vaddr, src_len);
> + ili9488_rgb565_to_rgb666_line(dst, sbuf, linepixels);
> + vaddr += fb->pitches[0];
> + dst += dst_len;
> + }
> + kfree(sbuf);
> +}
We now have much better support for format conversion. From reading the
conversion code, the native pixel is 3-byte wide with 6 filler bits at
the MSB. So the canonical name should be DRM_FORMAT_XRGB6666.
1) Add DRM_FORMAT_XRGB6666 to include/uapi/drm/drm_fourcc.h
2) Add the RGB666 format into to drm_fourcc.c
3) Move the helper function ili9488_rgb565_to_rgb666_line() into
drm_format_helper.c with an adapted name; and wire up
DRM_FORMAT_XRGB6666 with the conversion helpers. The conversion at [1]
is a good example to duplicate.
[1]
https://elixir.bootlin.com/linux/v6.1-rc2/source/drivers/gpu/drm/drm_format_helper.c#L673
> +
> +static int ili9488_buf_copy(void *dst, struct drm_framebuffer *fb,
> + struct drm_rect *rect)
> +{
> + struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> + void *src = cma_obj->vaddr;
> + int ret = 0;
> +
> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> + if (ret)
> + return ret;
> +
> + switch (fb->format->format) {
> + case DRM_FORMAT_RGB565:
> + ili9488_rgb565_to_rgb666(dst, src, fb, rect);
> + break;
> + default:
> + dev_err_once(fb->dev->dev, "Format is not supported: %p4cc\n",
> + &fb->format->format);
> + return -EINVAL;
> + }
> +
> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +
> + return ret;
> +}
> +
> +static void ili9488_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
> + unsigned int xs, unsigned int xe,
> + unsigned int ys, unsigned int ye)
> +{
> + struct mipi_dbi *dbi = &dbidev->dbi;
> +
> + xs += dbidev->left_offset;
> + xe += dbidev->left_offset;
> + ys += dbidev->top_offset;
> + ye += dbidev->top_offset;
> +
> + mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, (xs >> 8) & 0xff,
> + xs & 0xff, (xe >> 8) & 0xff, xe & 0xff);
> + mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, (ys >> 8) & 0xff,
> + ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
> +}
> +
> +static void ili9488_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{
> + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
> + unsigned int height = rect->y2 - rect->y1;
> + unsigned int width = rect->x2 - rect->x1;
> + struct mipi_dbi *dbi = &dbidev->dbi;
> + int idx, ret = 0;
> + bool full;
> + void *tr;
> +
> + if (WARN_ON(!fb))
> + return;
> +
> + if (!drm_dev_enter(fb->dev, &idx))
> + return;
> +
> + ret = drm_gem_fb_vmap(fb, map, data);
> + if (ret)
> + goto err_drm_dev_exit;
Calling vmap here is too late. I know that other MIPI drivers to this.
The overall MIPI code needs an update.
The correct way is to use shadow-plane helpers. They will give you a
plane state with the vmap-ing already established and they will remove
the mapping after the commit. Errors will be handled correctly. Please
see DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS within the DRM
drivers. I think tiny/cirrus.c should be a good example.
Because the rest of MIPI isn't yet there, it's ok to keep your driver
as-is. But shadow-plane helpers should be used at some point.
> +
> + full = width == fb->width && height == fb->height;
> +
> + DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
> +
> + if (!dbi->dc || !full ||
> + fb->format->format == DRM_FORMAT_RGB565) {
> + tr = dbidev->tx_buf;
> + ret = ili9488_buf_copy(dbidev->tx_buf, fb, rect);
> + if (ret)
> + goto err_msg;
> + } else {
> + tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
> + }
> +
> + ili9488_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
> + rect->y2 - 1);
> +
> + ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
> + width * height * 3);
> +err_msg:
> + if (ret)
> + drm_err_once(fb->dev, "Failed to update display %d\n", ret);
> +
> + drm_gem_fb_vunmap(fb, map);
> +
> +err_drm_dev_exit:
> + drm_dev_exit(idx);
> +}
> +
> +static void ili9488_pipe_update(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *old_state)
> +{
> + struct drm_plane_state *state = pipe->plane.state;
> + struct drm_rect rect;
> +
> + if (!pipe->crtc.state->active)
> + return;
> +
> + if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> + ili9488_fb_dirty(state->fb, &rect);
> +}
> +
> +static void ili9488_pipe_enable(struct drm_simple_display_pipe *pipe,
> + struct drm_crtc_state *crtc_state,
> + struct drm_plane_state *plane_state)
> +{
> + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> + struct drm_framebuffer *fb = plane_state->fb;
> + struct mipi_dbi *dbi = &dbidev->dbi;
> + u8 addr_mode;
> + struct drm_rect rect = {
> + .x1 = 0,
> + .x2 = fb->width,
> + .y1 = 0,
> + .y2 = fb->height,
> + };
> + int ret, idx;
> +
> + if (!drm_dev_enter(pipe->crtc.dev, &idx))
> + return;
> +
> + DRM_DEBUG_KMS("\n");
> +
> + ret = mipi_dbi_poweron_conditional_reset(dbidev);
> + if (ret < 0)
> + goto out_exit;
> + if (ret == 1)
> + goto out_enable;
> +
> + mipi_dbi_command(dbi, ILI9488_DISPLAY_INVERSION_ON);
> + mipi_dbi_command(dbi, ILI9488_POWER_CONTROL_3, 0x33);
> + mipi_dbi_command(dbi, ILI9488_FRAME_RATE_CONTROL_NORMAL, 0xB0);
> + mipi_dbi_command(dbi, ILI9488_MEM_ACCESS_CONTROL, 0x28);
> +
> + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, 0x65);
> +
> + mipi_dbi_command(dbi, ILI9488_POSITIVE_GAMMA_CORRECTION,
> + 0x00, 0x13, 0x18, 0x04, 0x0F,
> + 0x06, 0x3A, 0x56, 0x4D, 0x03,
> + 0x0A, 0x06, 0x30, 0x3E, 0x0F);
> + mipi_dbi_command(dbi, ILI9488_NEGATIVE_GAMMA_CORRECTION,
> + 0x00, 0x13, 0x18, 0x01, 0x11,
> + 0x06, 0x38, 0x3A, 0x4D, 0x06,
> + 0x0D, 0x0B, 0x31, 0x37, 0x0F);
> +
> + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT);
> + mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
> + msleep(120);
> + mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
> + msleep(100);
> +
> +out_enable:
> + switch (dbidev->rotation) {
> + default:
> + addr_mode = ILI9488_MADCTL_MX;
> + break;
> + case 90:
> + addr_mode = ILI9488_MADCTL_MV;
> + break;
> + case 180:
> + addr_mode = ILI9488_MADCTL_MY;
> + break;
> + case 270:
> + addr_mode = ILI9488_MADCTL_MV | ILI9488_MADCTL_MY |
> + ILI9488_MADCTL_MX;
> + break;
> + }
> + addr_mode |= ILI9488_MADCTL_BGR;
> + mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> + ili9488_fb_dirty(fb, &rect);
> +out_exit:
> + drm_dev_exit(idx);
> +}
> +
> +static void ili9488_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> + /*
> + * This callback is not protected by drm_dev_enter/exit since we want to
> + * turn off the display on regular driver unload. It's highly unlikely
> + * that the underlying SPI controller is gone should this be called
> + * after unplug.
> + */
> +
> + DRM_DEBUG_KMS("\n");
> +
> + mipi_dbi_command(&dbidev->dbi, MIPI_DCS_SET_DISPLAY_OFF);
> +}
> +
> +static const u32 ili9488_formats[] = {
> + DRM_FORMAT_RGB565,
About formats: the diplay's native formats should go first. These are
(implicitly) preferred by userspace. Ideally your driver should be able
to copy RGB666 buffers directly to the device. But XRGB6666 has no
userspace support anywhere AFAIK. So it's probably ok to not export that
format.
What you should definately export is XRGB8888. That's what most
userspace expects from the driver. You'll need conversion code similarly
to the RGB565 helpers.
> +};
> +
> +static const struct drm_simple_display_pipe_funcs ili9488_pipe_funcs = {
> + .enable = ili9488_pipe_enable,
> + .disable = ili9488_pipe_disable,
> + .update = ili9488_pipe_update,
> +};
> +
> +static const struct drm_display_mode ili9488_mode = {
> + DRM_SIMPLE_MODE(320, 480, 49, 73),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(ili9488_fops);
> +
> +static struct drm_driver ili9488_driver = {
> + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> + .fops = &ili9488_fops,
> + DRM_GEM_CMA_DRIVER_OPS_VMAP,
> + .debugfs_init = mipi_dbi_debugfs_init,
> + .name = "ili9488",
> + .desc = "Ilitek ILI9488",
> + .date = "20221017",
> + .major = 1,
> + .minor = 0,
> +};
> +
> +static const struct of_device_id ili9488_of_match[] = {
> + { .compatible = "waveshare,pico-rt-lcd-35" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ili9488_of_match);
> +
> +static const struct spi_device_id ili9488_id[] = {
> + { "ili9488", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ili9488_id);
> +
> +static int ili9488_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct mipi_dbi_dev *dbidev;
> + struct drm_device *drm;
> + struct mipi_dbi *dbi;
> + struct gpio_desc *dc;
> + u32 rotation = 0;
> + size_t bufsize;
> + int ret;
> +
> + dbidev = devm_drm_dev_alloc(dev, &ili9488_driver,
> + struct mipi_dbi_dev, drm);
> + if (IS_ERR(dbidev))
> + return PTR_ERR(dbidev);
> +
> + dbi = &dbidev->dbi;
> + drm = &dbidev->drm;
> +
> + bufsize = ili9488_mode.vdisplay * ili9488_mode.hdisplay * 3;
> +
> + dbi->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(dbi->reset)) {
> + DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
> + return PTR_ERR(dbi->reset);
> + }
> +
> + dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW);
> + if (IS_ERR(dc)) {
> + DRM_DEV_ERROR(dev, "Failed to get gpio 'dc'\n");
> + return PTR_ERR(dc);
> + }
> +
> + dbidev->backlight = devm_of_find_backlight(dev);
> + if (IS_ERR(dbidev->backlight))
> + return PTR_ERR(dbidev->backlight);
> +
> + device_property_read_u32(dev, "rotation", &rotation);
> +
> + ret = mipi_dbi_spi_init(spi, dbi, dc);
> + if (ret)
> + return ret;
> +
> + dbidev->drm.mode_config.preferred_depth = 16;
> +
> + ret = mipi_dbi_dev_init_with_formats(dbidev, &ili9488_pipe_funcs,
> + ili9488_formats,
> + ARRAY_SIZE(ili9488_formats),
> + &ili9488_mode, rotation, bufsize);
> + if (ret)
> + return ret;
> +
> + dbi->swap_bytes = true;
> + drm_mode_config_reset(drm);
> +
> + ret = drm_dev_register(drm, 0);
> + if (ret)
> + return ret;
> +
> + spi_set_drvdata(spi, drm);
This could already be too late. After drm_dev_register() the device file
will be visible to userspace and maybe called without this drvdata set.
And maybe you also need the drvdata for error cleanups if probe fails;
don't know.
It's best to set this as soon as you have the drm pointer.
Best regards
Thomas
> +
> + drm_fbdev_generic_setup(drm, 0);
> +
> + return 0;
> +}
> +
> +static void ili9488_remove(struct spi_device *spi)
> +{
> + struct drm_device *drm = spi_get_drvdata(spi);
> +
> + drm_dev_unplug(drm);
> + drm_atomic_helper_shutdown(drm);
> +}
> +
> +static void ili9488_shutdown(struct spi_device *spi)
> +{
> + drm_atomic_helper_shutdown(spi_get_drvdata(spi));
> +}
> +
> +static struct spi_driver ili9488_spi_driver = {
> + .driver = {
> + .name = "ili9488",
> + .owner = THIS_MODULE,
> + .of_match_table = ili9488_of_match,
> + },
> + .id_table = ili9488_id,
> + .probe = ili9488_probe,
> + .remove = ili9488_remove,
> + .shutdown = ili9488_shutdown,
> +};
> +module_spi_driver(ili9488_spi_driver);
> +
> +MODULE_DESCRIPTION("Ilitek ILI9488 DRM driver");
> +MODULE_AUTHOR("Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>");
> +MODULE_AUTHOR("Michael Trimarchi <michael@amarulasolutions.com>");
> +MODULE_AUTHOR("Tommaso Merciai <tommaso.merciai@amarulasolutions.com>");
> +MODULE_LICENSE("GPL");
> \ No newline at end of file
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread