First of all, the driver was introduced when it was already
two drivers available for Ilitek 9341 panels.
Second, the most recent (fourth!) driver has incorporated this one
and hence, when enabled, it covers the provided functionality.
Taking into account the above, remove duplicative driver and make
maintenance and support eaiser for everybody.
Also see discussion [1] for details about Ilitek 9341 duplication
code.
Link: https://lore.kernel.org/r/[email protected] [1]
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/tiny/Kconfig | 13 --
drivers/gpu/drm/tiny/Makefile | 1 -
drivers/gpu/drm/tiny/ili9341.c | 253 ---------------------------------
3 files changed, 267 deletions(-)
delete mode 100644 drivers/gpu/drm/tiny/ili9341.c
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index f6889f649bc1..2ab07bd0bb44 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -134,19 +134,6 @@ config TINYDRM_ILI9225
If M is selected the module will be called ili9225.
-config TINYDRM_ILI9341
- tristate "DRM support for ILI9341 display panels"
- depends on DRM && SPI
- select DRM_KMS_HELPER
- select DRM_GEM_DMA_HELPER
- select DRM_MIPI_DBI
- select BACKLIGHT_CLASS_DEVICE
- help
- DRM driver for the following Ilitek ILI9341 panels:
- * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4")
-
- If M is selected the module will be called ili9341.
-
config TINYDRM_ILI9486
tristate "DRM support for ILI9486 display panels"
depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 76dde89a044b..37cc9b27e79d 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -10,7 +10,6 @@ obj-$(CONFIG_DRM_SIMPLEDRM) += simpledrm.o
obj-$(CONFIG_TINYDRM_HX8357D) += hx8357d.o
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_MI0283QT) += mi0283qt.o
obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
deleted file mode 100644
index 47b61c3bf145..000000000000
--- a/drivers/gpu/drm/tiny/ili9341.c
+++ /dev/null
@@ -1,253 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * DRM driver for Ilitek ILI9341 panels
- *
- * Copyright 2018 David Lechner <[email protected]>
- *
- * Based on mi0283qt.c:
- * Copyright 2016 Noralf Trønnes
- */
-
-#include <linux/backlight.h>
-#include <linux/delay.h>
-#include <linux/gpio/consumer.h>
-#include <linux/module.h>
-#include <linux/property.h>
-#include <linux/spi/spi.h>
-
-#include <drm/drm_atomic_helper.h>
-#include <drm/drm_drv.h>
-#include <drm/drm_fbdev_generic.h>
-#include <drm/drm_gem_atomic_helper.h>
-#include <drm/drm_gem_dma_helper.h>
-#include <drm/drm_managed.h>
-#include <drm/drm_mipi_dbi.h>
-#include <drm/drm_modeset_helper.h>
-#include <video/mipi_display.h>
-
-#define ILI9341_FRMCTR1 0xb1
-#define ILI9341_DISCTRL 0xb6
-#define ILI9341_ETMOD 0xb7
-
-#define ILI9341_PWCTRL1 0xc0
-#define ILI9341_PWCTRL2 0xc1
-#define ILI9341_VMCTRL1 0xc5
-#define ILI9341_VMCTRL2 0xc7
-#define ILI9341_PWCTRLA 0xcb
-#define ILI9341_PWCTRLB 0xcf
-
-#define ILI9341_PGAMCTRL 0xe0
-#define ILI9341_NGAMCTRL 0xe1
-#define ILI9341_DTCTRLA 0xe8
-#define ILI9341_DTCTRLB 0xea
-#define ILI9341_PWRSEQ 0xed
-
-#define ILI9341_EN3GAM 0xf2
-#define ILI9341_PUMPCTRL 0xf7
-
-#define ILI9341_MADCTL_BGR BIT(3)
-#define ILI9341_MADCTL_MV BIT(5)
-#define ILI9341_MADCTL_MX BIT(6)
-#define ILI9341_MADCTL_MY BIT(7)
-
-static void yx240qv29_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 mipi_dbi *dbi = &dbidev->dbi;
- u8 addr_mode;
- 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, MIPI_DCS_SET_DISPLAY_OFF);
-
- mipi_dbi_command(dbi, ILI9341_PWCTRLB, 0x00, 0xc1, 0x30);
- mipi_dbi_command(dbi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
- mipi_dbi_command(dbi, ILI9341_DTCTRLA, 0x85, 0x00, 0x78);
- mipi_dbi_command(dbi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
- mipi_dbi_command(dbi, ILI9341_PUMPCTRL, 0x20);
- mipi_dbi_command(dbi, ILI9341_DTCTRLB, 0x00, 0x00);
-
- /* Power Control */
- mipi_dbi_command(dbi, ILI9341_PWCTRL1, 0x23);
- mipi_dbi_command(dbi, ILI9341_PWCTRL2, 0x10);
- /* VCOM */
- mipi_dbi_command(dbi, ILI9341_VMCTRL1, 0x3e, 0x28);
- mipi_dbi_command(dbi, ILI9341_VMCTRL2, 0x86);
-
- /* Memory Access Control */
- mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT);
-
- /* Frame Rate */
- mipi_dbi_command(dbi, ILI9341_FRMCTR1, 0x00, 0x1b);
-
- /* Gamma */
- mipi_dbi_command(dbi, ILI9341_EN3GAM, 0x00);
- mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
- mipi_dbi_command(dbi, ILI9341_PGAMCTRL,
- 0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
- 0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
- mipi_dbi_command(dbi, ILI9341_NGAMCTRL,
- 0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
- 0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
-
- /* DDRAM */
- mipi_dbi_command(dbi, ILI9341_ETMOD, 0x07);
-
- /* Display */
- mipi_dbi_command(dbi, ILI9341_DISCTRL, 0x08, 0x82, 0x27, 0x00);
- mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
- msleep(100);
-
- mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
- msleep(100);
-
-out_enable:
- switch (dbidev->rotation) {
- default:
- addr_mode = ILI9341_MADCTL_MX;
- break;
- case 90:
- addr_mode = ILI9341_MADCTL_MV;
- break;
- case 180:
- addr_mode = ILI9341_MADCTL_MY;
- break;
- case 270:
- addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
- ILI9341_MADCTL_MX;
- break;
- }
- addr_mode |= ILI9341_MADCTL_BGR;
- mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
- mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
-out_exit:
- drm_dev_exit(idx);
-}
-
-static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
- DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
-};
-
-static const struct drm_display_mode yx240qv29_mode = {
- DRM_SIMPLE_MODE(240, 320, 37, 49),
-};
-
-DEFINE_DRM_GEM_DMA_FOPS(ili9341_fops);
-
-static const struct drm_driver ili9341_driver = {
- .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
- .fops = &ili9341_fops,
- DRM_GEM_DMA_DRIVER_OPS_VMAP,
- .debugfs_init = mipi_dbi_debugfs_init,
- .name = "ili9341",
- .desc = "Ilitek ILI9341",
- .date = "20180514",
- .major = 1,
- .minor = 0,
-};
-
-static const struct of_device_id ili9341_of_match[] = {
- { .compatible = "adafruit,yx240qv29" },
- { }
-};
-MODULE_DEVICE_TABLE(of, ili9341_of_match);
-
-static const struct spi_device_id ili9341_id[] = {
- { "yx240qv29", 0 },
- { }
-};
-MODULE_DEVICE_TABLE(spi, ili9341_id);
-
-static int ili9341_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;
- int ret;
-
- dbidev = devm_drm_dev_alloc(dev, &ili9341_driver,
- struct mipi_dbi_dev, drm);
- if (IS_ERR(dbidev))
- return PTR_ERR(dbidev);
-
- dbi = &dbidev->dbi;
- drm = &dbidev->drm;
-
- dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(dbi->reset))
- return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO 'reset'\n");
-
- dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
- if (IS_ERR(dc))
- return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO 'dc'\n");
-
- 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;
-
- ret = mipi_dbi_dev_init(dbidev, &ili9341_pipe_funcs, &yx240qv29_mode, rotation);
- if (ret)
- return ret;
-
- 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 ili9341_remove(struct spi_device *spi)
-{
- struct drm_device *drm = spi_get_drvdata(spi);
-
- drm_dev_unplug(drm);
- drm_atomic_helper_shutdown(drm);
-}
-
-static void ili9341_shutdown(struct spi_device *spi)
-{
- drm_atomic_helper_shutdown(spi_get_drvdata(spi));
-}
-
-static struct spi_driver ili9341_spi_driver = {
- .driver = {
- .name = "ili9341",
- .of_match_table = ili9341_of_match,
- },
- .id_table = ili9341_id,
- .probe = ili9341_probe,
- .remove = ili9341_remove,
- .shutdown = ili9341_shutdown,
-};
-module_spi_driver(ili9341_spi_driver);
-
-MODULE_DESCRIPTION("Ilitek ILI9341 DRM driver");
-MODULE_AUTHOR("David Lechner <[email protected]>");
-MODULE_LICENSE("GPL");
--
2.43.0.rc1.1336.g36b5255a03ac
Hi,
On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote:
> First of all, the driver was introduced when it was already
> two drivers available for Ilitek 9341 panels.
>
> Second, the most recent (fourth!) driver has incorporated this one
> and hence, when enabled, it covers the provided functionality.
>
> Taking into account the above, remove duplicative driver and make
> maintenance and support eaiser for everybody.
>
> Also see discussion [1] for details about Ilitek 9341 duplication
> code.
>
> Link: https://lore.kernel.org/r/[email protected] [1]
> Signed-off-by: Andy Shevchenko <[email protected]>
I think it should be the other way around and we should remove the
mipi-dbi handling from panel/panel-ilitek-ili9341.c
It's basically two drivers glued together for no particular reason and
handling two very different use cases which just adds more complexity
than it needs to.
And it's the only driver doing so afaik, so it's definitely not "least
surprise" compliant.
Maxime
On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote:
> Hi,
>
> On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote:
> > First of all, the driver was introduced when it was already
> > two drivers available for Ilitek 9341 panels.
> >
> > Second, the most recent (fourth!) driver has incorporated this one
> > and hence, when enabled, it covers the provided functionality.
> >
> > Taking into account the above, remove duplicative driver and make
> > maintenance and support eaiser for everybody.
> >
> > Also see discussion [1] for details about Ilitek 9341 duplication
> > code.
> >
> > Link: https://lore.kernel.org/r/[email protected] [1]
> > Signed-off-by: Andy Shevchenko <[email protected]>
>
> I think it should be the other way around and we should remove the
> mipi-dbi handling from panel/panel-ilitek-ili9341.c
Then please do it! I whining already for a few years about this.
> It's basically two drivers glued together for no particular reason and
> handling two very different use cases which just adds more complexity
> than it needs to.
>
> And it's the only driver doing so afaik, so it's definitely not "least
> surprise" compliant.
--
With Best Regards,
Andy Shevchenko
On Thu, Apr 25, 2024 at 06:04:50PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote:
> > Hi,
> >
> > On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote:
> > > First of all, the driver was introduced when it was already
> > > two drivers available for Ilitek 9341 panels.
> > >
> > > Second, the most recent (fourth!) driver has incorporated this one
> > > and hence, when enabled, it covers the provided functionality.
> > >
> > > Taking into account the above, remove duplicative driver and make
> > > maintenance and support eaiser for everybody.
> > >
> > > Also see discussion [1] for details about Ilitek 9341 duplication
> > > code.
> > >
> > > Link: https://lore.kernel.org/r/[email protected] [1]
> > > Signed-off-by: Andy Shevchenko <[email protected]>
> >
> > I think it should be the other way around and we should remove the
> > mipi-dbi handling from panel/panel-ilitek-ili9341.c
>
> Then please do it! I whining already for a few years about this.
I have neither the hardware nor the interest to do so. Seems it looks
like you have plenty of the latter at least, I'm sure you'll find some
time to tackle this.
Maxime
On Mon, Apr 29, 2024 at 01:39:06PM +0200, Maxime Ripard wrote:
> On Thu, Apr 25, 2024 at 06:04:50PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote:
> > > On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote:
> > > > First of all, the driver was introduced when it was already
> > > > two drivers available for Ilitek 9341 panels.
> > > >
> > > > Second, the most recent (fourth!) driver has incorporated this one
> > > > and hence, when enabled, it covers the provided functionality.
> > > >
> > > > Taking into account the above, remove duplicative driver and make
> > > > maintenance and support eaiser for everybody.
> > > >
> > > > Also see discussion [1] for details about Ilitek 9341 duplication
> > > > code.
> > > >
> > > > Link: https://lore.kernel.org/r/[email protected] [1]
> > > > Signed-off-by: Andy Shevchenko <[email protected]>
> > >
> > > I think it should be the other way around and we should remove the
> > > mipi-dbi handling from panel/panel-ilitek-ili9341.c
> >
> > Then please do it! I whining already for a few years about this.
>
> I have neither the hardware nor the interest to do so. Seems it looks
> like you have plenty of the latter at least, I'm sure you'll find some
> time to tackle this.
Hmm... Since the use of Arduino part in panel IliTek 9341 is clarified
in this thread, I won't use that, but I have no time to clean up the mess
in DRM in the nearest future, sorry. And TBH it seems you, guys, know much
better what you want.
FYI:
The drivers/gpu/drm/tiny/mi0283qt.c works for me (the plenty of the HW
you referred to).
TL;DR: consider this as a (bug/feature/cleanup) report.
--
With Best Regards,
Andy Shevchenko