[RFC PATCH 0/2] Add DRM panel driver for Ilitek ILI9341 based panels in parallel RGB mode

[RFC PATCH 0/2] Add DRM panel driver for Ilitek ILI9341 based panels in parallel RGB mode

[Josef Lusticky]

These patches add panel driver for ili9341-based panels in parallel RGB mode.
The driver was developed for DispleyTech DT024CTFT LCD panel [1] which features ILI9341 chip [2].
The driver was tested on the Allwinner A13 (sun5i) platform.

The driver supports 240x320 pixel resolution with 18-bit RGB (6 wires per color)
and SPI control bus with Data/Command GPIO pin:
DisplayTech DT024CTFT panel is configured with the IM[0:3] pins
set to "1110" - see page 10 in datasheet [2].

The Data/Command GPIO is optional, however at the moment the driver requires it:
The display itself is capable of 9-bit SPI without the Data/Command GPIO.
Support for such configuration can be added later to the driver.

Optional HW reset gpio can be specified, otherwise SW reset command is used
during the initialization.

The ILI9341 displays have two command sets:
Level 1 conforms to MIPI specs
Level 2 is outside the MIPI specs - custom defines are used in the driver

The ILI9341 supports various RGB modes (e.g. NVSYNC, DE_LOW, clock freq, etc.),
however values that are presented in the ILI9341 datasheet [2] are used
by the driver in struct drm_display_mode.


General note on ILI9341-based displays:
The ILI9341 chip can be used in parallel RGB with SPI control
or in SPI only mode where the image data itself is also transferred via SPI.
This driver supports parallel RGB displays - it works with displays wired with 18-bit RGB input,
it does not support SPI data mode (i.e. Multi-inno mi0283qt or Adafruit yx240qv29 are not supported by this driver).
The SPI data mode is naturally much slower than the parallel RGB mode.

General note on DisplayTech DT024CTFT panel:
The panel supports different configuation options (18/16/6-bit RGB or 9/8-bit SPI) depending on the IM[0:3] wiring.
To keep this patch small for reveiw, at the moment only 18-bit RGB mode and 8-bit SPI with Data/Command GPIO
is supported by this driver.


I kindly ask you for a patch review.

Links to datasheet:
[1] https://www.displaytech-us.com/sites/default/files/display-data-sheet/DT024CTFT-v10_0.pdf
[2] https://cdn-shop.adafruit.com/datasheets/ILI9341.pdf

Best regards,
Josef Lusticky

Josef Lusticky (2):
  drm/panel: Add Ilitek ILI9341 parallel RGB panel driver
  dt-bindings: panel: Add Ilitek ILI9341 panel documentation

 .../bindings/display/panel/ilitek,ili9341.txt |  33 ++
 MAINTAINERS                                   |   6 +
 drivers/gpu/drm/panel/Kconfig                 |   7 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c  | 320 ++++++++++++++++++
 5 files changed, 367 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt
 create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c

-- 
2.20.1

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

[RFC PATCH 1/2] drm/panel: Add Ilitek ILI9341 parallel RGB panel driver

[Josef Lusticky]

---
 MAINTAINERS                                  |   6 +
 drivers/gpu/drm/panel/Kconfig                |   7 +
 drivers/gpu/drm/panel/Makefile               |   1 +
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 320 +++++++++++++++++++
 4 files changed, 334 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a4a4bf563f94..d2e03c5ad04d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4821,6 +4821,12 @@ S:	Maintained
 F:	drivers/gpu/drm/tinydrm/ili9225.c
 F:	Documentation/devicetree/bindings/display/ilitek,ili9225.txt
 
+DRM DRIVER FOR ILITEK ILI9341 PANELS
+M:	Josef Lusticky <josef@lusticky.cz>
+S:	Maintained
+F:	drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+F:	Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt
+
 DRM DRIVER FOR HX8357D PANELS
 M:	Eric Anholt <eric@anholt.net>
 T:	git git://anongit.freedesktop.org/drm/drm-misc
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index f53f817356db..a59cfff614c0 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -46,6 +46,13 @@ config DRM_PANEL_ILITEK_IL9322
 	  Say Y here if you want to enable support for Ilitek IL9322
 	  QVGA (320x240) RGB, YUV and ITU-T BT.656 panels.
 
+config DRM_PANEL_ILITEK_IL9341
+	tristate "Ilitek ILI9341 240x320 panels"
+	depends on OF && SPI
+	help
+	  Say Y here if you want to enable support for Ilitek IL9341
+	  QVGA (240x320) RGB panel.
+
 config DRM_PANEL_ILITEK_ILI9881C
 	tristate "Ilitek ILI9881C-based panels"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 7834947a53b0..1ce3ff8d6204 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_DRM_PANEL_ARM_VERSATILE) += panel-arm-versatile.o
 obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o
+obj-$(CONFIG_DRM_PANEL_ILITEK_IL9341) += panel-ilitek-ili9341.o
 obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o
 obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
new file mode 100644
index 000000000000..51ed03140f8d
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Ilitek ILI9341 drm_panel driver
+ * 240RGBx320 dots resolution TFT LCD display
+ *
+ * This driver support the following panel configurations:
+ * - 18-bit parallel RGB interface
+ * - 8-bit SPI with Data/Command GPIO
+ *
+ * Copyright (C) 2019 Josef Lusticky <josef@lusticky.cz>
+ *
+ */
+
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+
+#include <video/mipi_display.h>
+
+/* ILI9341 extended register set */
+#define ILI9341_IFMODE         0xB0 // clock polarity
+#define ILI9341_IFCTL          0xF6 // interface conrol
+#define ILI9341_PGAMCTRL       0xE0 // positive gamma control
+#define ILI9341_NGAMCTRL       0xE1 // negative gamma control
+
+#define ILI9341_MADCTL_MV      BIT(5)
+#define ILI9341_MADCTL_MX      BIT(6)
+#define ILI9341_MADCTL_MY      BIT(7)
+
+/**
+ * struct ili9341_config - the display specific configuration
+ * @width_mm: physical panel width [mm]
+ * @height_mm: physical panel height [mm]
+ */
+struct ili9341_config {
+	u32 width_mm;
+	u32 height_mm;
+};
+
+struct ili9341 {
+	const struct ili9341_config *conf;
+	struct drm_panel panel;
+	struct spi_device *spi;
+	struct backlight_device *backlight;
+	struct gpio_desc *dc_gpio;
+	struct gpio_desc *reset_gpio;
+};
+
+static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
+{
+	return container_of(panel, struct ili9341, panel);
+}
+
+static int ili9341_spi_write_command(struct ili9341 *ili, u8 cmd)
+{
+	struct spi_transfer xfer = {
+		.tx_buf = &cmd,
+		.bits_per_word = 8,
+		.len = 1
+	};
+	struct spi_message msg;
+	spi_message_init(&msg);
+	spi_message_add_tail(&xfer, &msg);
+
+	gpiod_set_value(ili->dc_gpio, 0);
+
+	return spi_sync(ili->spi, &msg);
+}
+
+static int ili9341_spi_write_data(struct ili9341 *ili, u8 *data, size_t size)
+{
+	struct spi_transfer xfer = {
+		.tx_buf = data,
+		.bits_per_word = 8,
+		.len = size
+	};
+
+	struct spi_message msg;
+	spi_message_init(&msg);
+	spi_message_add_tail(&xfer, &msg);
+
+	gpiod_set_value(ili->dc_gpio, 1);
+
+	return spi_sync(ili->spi, &msg);
+}
+
+#define ili9341_spi_write(ili, cmd, data...) \
+({ \
+	u8 d[] = { data }; \
+	ili9341_spi_write_command(ili, cmd); \
+	if (ARRAY_SIZE(d) > 0) \
+		ili9341_spi_write_data(ili, d, ARRAY_SIZE(d)); \
+})
+
+static int ili9341_deinit(struct drm_panel *panel, struct ili9341 *ili)
+{
+	ili9341_spi_write(ili, MIPI_DCS_SET_DISPLAY_OFF);
+	ili9341_spi_write(ili, MIPI_DCS_ENTER_SLEEP_MODE);
+	msleep(5);
+	return 0;
+}
+
+static int ili9341_init(struct drm_panel *panel, struct ili9341 *ili)
+{
+	/* HW / SW Reset display and wait */
+	if (ili->reset_gpio) {
+		gpiod_set_value(ili->reset_gpio, 0);
+		usleep_range(20, 1000);
+		gpiod_set_value(ili->reset_gpio, 1);
+	} else {
+		ili9341_spi_write(ili, MIPI_DCS_SOFT_RESET);
+	}
+	msleep(120);
+
+	/* Polarity */
+	ili9341_spi_write(ili, ILI9341_IFMODE, 0xC0);
+
+	/* Interface control */
+	ili9341_spi_write(ili, ILI9341_IFCTL, 0x09, 0x01, 0x26);
+
+	/* Pixel format */
+	ili9341_spi_write(ili, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT << 4);
+
+	/* Gamma */
+	ili9341_spi_write(ili, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
+	ili9341_spi_write(ili, ILI9341_PGAMCTRL,
+		0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
+		0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
+	ili9341_spi_write(ili, ILI9341_NGAMCTRL,
+		0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
+		0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
+
+	/* Rotation */
+	ili9341_spi_write(ili, MIPI_DCS_SET_ADDRESS_MODE, ILI9341_MADCTL_MX);
+
+	/* Exit sleep mode */
+	ili9341_spi_write(ili, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(120);
+
+	ili9341_spi_write(ili, MIPI_DCS_SET_DISPLAY_ON);
+
+	return 0;
+}
+
+static int ili9341_unprepare(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	return ili9341_deinit(panel, ili);
+}
+
+static int ili9341_prepare(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	int ret;
+
+	ret = ili9341_init(panel, ili);
+	if (ret < 0)
+		ili9341_unprepare(panel);
+	return ret;
+}
+
+static int ili9341_enable(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	return backlight_enable(ili->backlight);
+}
+
+static int ili9341_disable(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	return backlight_disable(ili->backlight);
+}
+
+static const struct drm_display_mode prgb_240x320_mode = {
+	.clock = 6350,
+
+	.hdisplay = 240,
+	.hsync_start = 240 + 10,
+	.hsync_end = 240 + 10 + 10,
+	.htotal = 240 + 10 + 10 + 20,
+
+	.vdisplay = 320,
+	.vsync_start = 320 + 4,
+	.vsync_end = 320 + 4 + 2,
+	.vtotal = 320 + 4 + 2 + 2,
+
+	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED
+};
+
+static int ili9341_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(panel->drm, &prgb_240x320_mode);
+	drm_mode_set_name(mode);
+
+	mode->width_mm = ili->conf->width_mm;
+	mode->height_mm = ili->conf->height_mm;
+
+	strncpy(connector->display_info.name, "ILI9341 TFT LCD driver\0",
+		DRM_DISPLAY_INFO_LEN);
+	connector->display_info.width_mm = mode->width_mm;
+	connector->display_info.height_mm = mode->height_mm;
+	connector->display_info.bus_flags |= DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE |
+		DRM_BUS_FLAG_SYNC_NEGEDGE;
+
+	drm_mode_probed_add(connector, mode);
+
+	return 1; /* Number of modes */
+}
+
+static const struct drm_panel_funcs ili9341_drm_funcs = {
+	.disable = ili9341_disable,
+	.unprepare = ili9341_unprepare,
+	.prepare = ili9341_prepare,
+	.enable = ili9341_enable,
+	.get_modes = ili9341_get_modes,
+};
+
+static int ili9341_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ili9341 *ili;
+	int ret;
+
+	ili = devm_kzalloc(dev, sizeof(struct ili9341), GFP_KERNEL);
+	if (!ili)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, ili);
+
+	ili->spi = spi;
+
+	/*
+	 * Every new incarnation of this display must have a unique
+	 * data entry for the system in this driver.
+	 */
+	ili->conf = of_device_get_match_data(dev);
+	if (!ili->conf) {
+		DRM_DEV_ERROR(dev, "missing device configuration\n");
+		return -ENODEV;
+	}
+
+	ili->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ili->reset_gpio)) {
+		DRM_DEV_ERROR(dev, "failed to get reset gpio\n");
+		return PTR_ERR(ili->reset_gpio);
+	}
+
+	ili->dc_gpio = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(ili->dc_gpio)) {
+		DRM_DEV_ERROR(dev, "failed to get dc gpio\n");
+		return PTR_ERR(ili->dc_gpio);
+	}
+
+	ili->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(ili->backlight)) {
+		DRM_DEV_ERROR(dev, "failed to get backlight\n");
+		return PTR_ERR(ili->backlight);
+	}
+
+	ret = spi_setup(spi);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "spi setup failed\n");
+		return ret;
+	}
+
+	drm_panel_init(&ili->panel);
+	ili->panel.dev = dev;
+	ili->panel.funcs = &ili9341_drm_funcs;
+
+	return drm_panel_add(&ili->panel);
+}
+
+static int ili9341_remove(struct spi_device *spi)
+{
+	struct ili9341 *ili = spi_get_drvdata(spi);
+
+	drm_panel_remove(&ili->panel);
+
+	ili9341_disable(&ili->panel);
+	ili9341_unprepare(&ili->panel);
+
+	return 0;
+}
+
+static const struct ili9341_config dt024ctft_data = {
+	.width_mm = 37,
+	.height_mm = 49,
+};
+
+static const struct of_device_id ili9341_of_match[] = {
+	{ .compatible = "displaytech,dt024ctft", .data = &dt024ctft_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ili9341_of_match);
+
+static struct spi_driver ili9341_driver = {
+	.probe = ili9341_probe,
+	.remove = ili9341_remove,
+	.driver = {
+		.name = "panel-ilitek-ili9341",
+		.of_match_table = ili9341_of_match,
+	},
+};
+module_spi_driver(ili9341_driver);
+
+MODULE_AUTHOR("Josef Lusticky <josef@lusticky.cz>");
+MODULE_DESCRIPTION("ILI9341 LCD panel driver");
+MODULE_LICENSE("GPL");
-- 
2.20.1

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

Re: [RFC PATCH 1/2] drm/panel: Add Ilitek ILI9341 parallel RGB panel driver

[Rob Herring]

On Mon, Mar 4, 2019 at 6:51 AM Josef Lusticky <josef@lusticky.cz> wrote:
>

S-o-b and commit msg?

> ---
>  MAINTAINERS                                  |   6 +
>  drivers/gpu/drm/panel/Kconfig                |   7 +
>  drivers/gpu/drm/panel/Makefile               |   1 +
>  drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 320 +++++++++++++++++++

Would be nice to share some code with the existing driver.

>  4 files changed, 334 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a4a4bf563f94..d2e03c5ad04d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4821,6 +4821,12 @@ S:       Maintained
>  F:     drivers/gpu/drm/tinydrm/ili9225.c
>  F:     Documentation/devicetree/bindings/display/ilitek,ili9225.txt
>
> +DRM DRIVER FOR ILITEK ILI9341 PANELS
> +M:     Josef Lusticky <josef@lusticky.cz>
> +S:     Maintained
> +F:     drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> +F:     Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt
> +
>  DRM DRIVER FOR HX8357D PANELS
>  M:     Eric Anholt <eric@anholt.net>
>  T:     git git://anongit.freedesktop.org/drm/drm-misc
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index f53f817356db..a59cfff614c0 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -46,6 +46,13 @@ config DRM_PANEL_ILITEK_IL9322
>           Say Y here if you want to enable support for Ilitek IL9322
>           QVGA (320x240) RGB, YUV and ITU-T BT.656 panels.
>
> +config DRM_PANEL_ILITEK_IL9341
> +       tristate "Ilitek ILI9341 240x320 panels"
> +       depends on OF && SPI
> +       help
> +         Say Y here if you want to enable support for Ilitek IL9341
> +         QVGA (240x320) RGB panel.
> +
>  config DRM_PANEL_ILITEK_ILI9881C
>         tristate "Ilitek ILI9881C-based panels"
>         depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 7834947a53b0..1ce3ff8d6204 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_DRM_PANEL_ARM_VERSATILE) += panel-arm-versatile.o
>  obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
>  obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>  obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o
> +obj-$(CONFIG_DRM_PANEL_ILITEK_IL9341) += panel-ilitek-ili9341.o
>  obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o
>  obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
>  obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> new file mode 100644
> index 000000000000..51ed03140f8d
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> @@ -0,0 +1,320 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Ilitek ILI9341 drm_panel driver
> + * 240RGBx320 dots resolution TFT LCD display
> + *
> + * This driver support the following panel configurations:
> + * - 18-bit parallel RGB interface
> + * - 8-bit SPI with Data/Command GPIO
> + *
> + * Copyright (C) 2019 Josef Lusticky <josef@lusticky.cz>
> + *
> + */
> +
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +
> +#include <video/mipi_display.h>
> +
> +/* ILI9341 extended register set */
> +#define ILI9341_IFMODE         0xB0 // clock polarity
> +#define ILI9341_IFCTL          0xF6 // interface conrol
> +#define ILI9341_PGAMCTRL       0xE0 // positive gamma control
> +#define ILI9341_NGAMCTRL       0xE1 // negative gamma control
> +
> +#define ILI9341_MADCTL_MV      BIT(5)
> +#define ILI9341_MADCTL_MX      BIT(6)
> +#define ILI9341_MADCTL_MY      BIT(7)
> +
> +/**
> + * struct ili9341_config - the display specific configuration
> + * @width_mm: physical panel width [mm]
> + * @height_mm: physical panel height [mm]
> + */
> +struct ili9341_config {
> +       u32 width_mm;
> +       u32 height_mm;
> +};
> +
> +struct ili9341 {
> +       const struct ili9341_config *conf;
> +       struct drm_panel panel;
> +       struct spi_device *spi;
> +       struct backlight_device *backlight;
> +       struct gpio_desc *dc_gpio;
> +       struct gpio_desc *reset_gpio;
> +};
> +
> +static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
> +{
> +       return container_of(panel, struct ili9341, panel);
> +}
> +
> +static int ili9341_spi_write_command(struct ili9341 *ili, u8 cmd)
> +{
> +       struct spi_transfer xfer = {
> +               .tx_buf = &cmd,
> +               .bits_per_word = 8,
> +               .len = 1
> +       };
> +       struct spi_message msg;
> +       spi_message_init(&msg);
> +       spi_message_add_tail(&xfer, &msg);
> +
> +       gpiod_set_value(ili->dc_gpio, 0);
> +
> +       return spi_sync(ili->spi, &msg);

Can't you use existing mipi_dbi_* functions instead of rolling your own?

> +}
> +
> +static int ili9341_spi_write_data(struct ili9341 *ili, u8 *data, size_t size)
> +{
> +       struct spi_transfer xfer = {
> +               .tx_buf = data,
> +               .bits_per_word = 8,
> +               .len = size
> +       };
> +
> +       struct spi_message msg;
> +       spi_message_init(&msg);
> +       spi_message_add_tail(&xfer, &msg);
> +
> +       gpiod_set_value(ili->dc_gpio, 1);
> +
> +       return spi_sync(ili->spi, &msg);
> +}
> +
> +#define ili9341_spi_write(ili, cmd, data...) \
> +({ \
> +       u8 d[] = { data }; \
> +       ili9341_spi_write_command(ili, cmd); \
> +       if (ARRAY_SIZE(d) > 0) \
> +               ili9341_spi_write_data(ili, d, ARRAY_SIZE(d)); \
> +})
> +
> +static int ili9341_deinit(struct drm_panel *panel, struct ili9341 *ili)
> +{
> +       ili9341_spi_write(ili, MIPI_DCS_SET_DISPLAY_OFF);
> +       ili9341_spi_write(ili, MIPI_DCS_ENTER_SLEEP_MODE);
> +       msleep(5);
> +       return 0;
> +}
> +
> +static int ili9341_init(struct drm_panel *panel, struct ili9341 *ili)
> +{
> +       /* HW / SW Reset display and wait */
> +       if (ili->reset_gpio) {
> +               gpiod_set_value(ili->reset_gpio, 0);
> +               usleep_range(20, 1000);
> +               gpiod_set_value(ili->reset_gpio, 1);
> +       } else {
> +               ili9341_spi_write(ili, MIPI_DCS_SOFT_RESET);
> +       }
> +       msleep(120);
> +
> +       /* Polarity */
> +       ili9341_spi_write(ili, ILI9341_IFMODE, 0xC0);
> +
> +       /* Interface control */
> +       ili9341_spi_write(ili, ILI9341_IFCTL, 0x09, 0x01, 0x26);
> +
> +       /* Pixel format */
> +       ili9341_spi_write(ili, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT << 4);
> +
> +       /* Gamma */
> +       ili9341_spi_write(ili, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
> +       ili9341_spi_write(ili, ILI9341_PGAMCTRL,
> +               0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
> +               0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
> +       ili9341_spi_write(ili, ILI9341_NGAMCTRL,
> +               0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
> +               0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
> +
> +       /* Rotation */
> +       ili9341_spi_write(ili, MIPI_DCS_SET_ADDRESS_MODE, ILI9341_MADCTL_MX);
> +
> +       /* Exit sleep mode */
> +       ili9341_spi_write(ili, MIPI_DCS_EXIT_SLEEP_MODE);
> +       msleep(120);
> +
> +       ili9341_spi_write(ili, MIPI_DCS_SET_DISPLAY_ON);
> +
> +       return 0;
> +}
> +
> +static int ili9341_unprepare(struct drm_panel *panel)
> +{
> +       struct ili9341 *ili = panel_to_ili9341(panel);
> +       return ili9341_deinit(panel, ili);
> +}
> +
> +static int ili9341_prepare(struct drm_panel *panel)
> +{
> +       struct ili9341 *ili = panel_to_ili9341(panel);
> +       int ret;
> +
> +       ret = ili9341_init(panel, ili);
> +       if (ret < 0)
> +               ili9341_unprepare(panel);
> +       return ret;
> +}
> +
> +static int ili9341_enable(struct drm_panel *panel)
> +{
> +       struct ili9341 *ili = panel_to_ili9341(panel);
> +       return backlight_enable(ili->backlight);
> +}
> +
> +static int ili9341_disable(struct drm_panel *panel)
> +{
> +       struct ili9341 *ili = panel_to_ili9341(panel);
> +       return backlight_disable(ili->backlight);
> +}
> +
> +static const struct drm_display_mode prgb_240x320_mode = {
> +       .clock = 6350,
> +
> +       .hdisplay = 240,
> +       .hsync_start = 240 + 10,
> +       .hsync_end = 240 + 10 + 10,
> +       .htotal = 240 + 10 + 10 + 20,
> +
> +       .vdisplay = 320,
> +       .vsync_start = 320 + 4,
> +       .vsync_end = 320 + 4 + 2,
> +       .vtotal = 320 + 4 + 2 + 2,
> +
> +       .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +       .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED
> +};
> +
> +static int ili9341_get_modes(struct drm_panel *panel)
> +{
> +       struct drm_connector *connector = panel->connector;
> +       struct ili9341 *ili = panel_to_ili9341(panel);
> +       struct drm_display_mode *mode;
> +
> +       mode = drm_mode_duplicate(panel->drm, &prgb_240x320_mode);
> +       drm_mode_set_name(mode);
> +
> +       mode->width_mm = ili->conf->width_mm;
> +       mode->height_mm = ili->conf->height_mm;
> +
> +       strncpy(connector->display_info.name, "ILI9341 TFT LCD driver\0",
> +               DRM_DISPLAY_INFO_LEN);
> +       connector->display_info.width_mm = mode->width_mm;
> +       connector->display_info.height_mm = mode->height_mm;
> +       connector->display_info.bus_flags |= DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE |
> +               DRM_BUS_FLAG_SYNC_NEGEDGE;
> +
> +       drm_mode_probed_add(connector, mode);
> +
> +       return 1; /* Number of modes */
> +}
> +
> +static const struct drm_panel_funcs ili9341_drm_funcs = {
> +       .disable = ili9341_disable,
> +       .unprepare = ili9341_unprepare,
> +       .prepare = ili9341_prepare,
> +       .enable = ili9341_enable,
> +       .get_modes = ili9341_get_modes,
> +};
> +
> +static int ili9341_probe(struct spi_device *spi)
> +{
> +       struct device *dev = &spi->dev;
> +       struct ili9341 *ili;
> +       int ret;
> +
> +       ili = devm_kzalloc(dev, sizeof(struct ili9341), GFP_KERNEL);
> +       if (!ili)
> +               return -ENOMEM;
> +
> +       spi_set_drvdata(spi, ili);
> +
> +       ili->spi = spi;
> +
> +       /*
> +        * Every new incarnation of this display must have a unique
> +        * data entry for the system in this driver.
> +        */
> +       ili->conf = of_device_get_match_data(dev);
> +       if (!ili->conf) {
> +               DRM_DEV_ERROR(dev, "missing device configuration\n");
> +               return -ENODEV;
> +       }
> +
> +       ili->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(ili->reset_gpio)) {
> +               DRM_DEV_ERROR(dev, "failed to get reset gpio\n");
> +               return PTR_ERR(ili->reset_gpio);
> +       }
> +
> +       ili->dc_gpio = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +       if (IS_ERR(ili->dc_gpio)) {
> +               DRM_DEV_ERROR(dev, "failed to get dc gpio\n");
> +               return PTR_ERR(ili->dc_gpio);
> +       }
> +
> +       ili->backlight = devm_of_find_backlight(dev);
> +       if (IS_ERR(ili->backlight)) {
> +               DRM_DEV_ERROR(dev, "failed to get backlight\n");
> +               return PTR_ERR(ili->backlight);
> +       }
> +
> +       ret = spi_setup(spi);
> +       if (ret < 0) {
> +               DRM_DEV_ERROR(dev, "spi setup failed\n");
> +               return ret;
> +       }
> +
> +       drm_panel_init(&ili->panel);
> +       ili->panel.dev = dev;
> +       ili->panel.funcs = &ili9341_drm_funcs;
> +
> +       return drm_panel_add(&ili->panel);
> +}
> +
> +static int ili9341_remove(struct spi_device *spi)
> +{
> +       struct ili9341 *ili = spi_get_drvdata(spi);
> +
> +       drm_panel_remove(&ili->panel);
> +
> +       ili9341_disable(&ili->panel);
> +       ili9341_unprepare(&ili->panel);
> +
> +       return 0;
> +}
> +
> +static const struct ili9341_config dt024ctft_data = {
> +       .width_mm = 37,
> +       .height_mm = 49,
> +};
> +
> +static const struct of_device_id ili9341_of_match[] = {
> +       { .compatible = "displaytech,dt024ctft", .data = &dt024ctft_data },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, ili9341_of_match);
> +
> +static struct spi_driver ili9341_driver = {
> +       .probe = ili9341_probe,
> +       .remove = ili9341_remove,
> +       .driver = {
> +               .name = "panel-ilitek-ili9341",
> +               .of_match_table = ili9341_of_match,
> +       },
> +};
> +module_spi_driver(ili9341_driver);
> +
> +MODULE_AUTHOR("Josef Lusticky <josef@lusticky.cz>");
> +MODULE_DESCRIPTION("ILI9341 LCD panel driver");
> +MODULE_LICENSE("GPL");
> --
> 2.20.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 2/2] dt-bindings: panel: Add Ilitek ILI9341 panel documentation

[Josef Lusticky]

---
 .../bindings/display/panel/ilitek,ili9341.txt | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt

diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt
new file mode 100644
index 000000000000..4e0e483bc12e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt
@@ -0,0 +1,33 @@
+Ilitek ILI9341 TFT panel driver with SPI control bus
+
+This is a driver for 240x320 TFT panels with parallel RGB color input.
+
+Required properties:
+  - compatible: "displaytech,dt024ctft", "ilitek,ili9341"
+  - backlight: phandle of the backlight device attached to the panel
+
+Optional properties:
+  - dc-gpios: a GPIO spec for the Data/Command pin, see gpio/gpio.txt
+  - reset-gpios: a GPIO spec for the reset pin, see gpio/gpio.txt
+
+The panel must obey the rules for a SPI slave device as specified in
+spi/spi-bus.txt
+
+The device node can contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in
+media/video-interfaces.txt. This node should describe panel's video bus.
+
+Example:
+
+panel@0 {
+	compatible = "displaytech,dt024ctft", "ilitek,ili9341";
+	reg = <0>;
+	backlight = <&backlight>;
+	dc-gpios = <&pio 4 9 GPIO_ACTIVE_HIGH>;
+
+	port {
+		panel_in: endpoint {
+			remote-endpoint = <&display_out>;
+		};
+	};
+};
-- 
2.20.1

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

Re: [RFC PATCH 2/2] dt-bindings: panel: Add Ilitek ILI9341 panel documentation

[Rob Herring]

On Mon, Mar 04, 2019 at 01:50:33PM +0100, Josef Lusticky wrote:
> ---

You need a commit message and a Signed-off-by. Run checkpatch.pl, it 
will tell you these things.

>  .../bindings/display/panel/ilitek,ili9341.txt | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt
> new file mode 100644
> index 000000000000..4e0e483bc12e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt

I don't like the same h/w being documented in 2 places as we already 
have bindings/display/ilitek,ili9341.txt. Expand that to cover both 
modes. 

> @@ -0,0 +1,33 @@
> +Ilitek ILI9341 TFT panel driver with SPI control bus
> +
> +This is a driver for 240x320 TFT panels with parallel RGB color input.
> +
> +Required properties:
> +  - compatible: "displaytech,dt024ctft", "ilitek,ili9341"
> +  - backlight: phandle of the backlight device attached to the panel
> +
> +Optional properties:
> +  - dc-gpios: a GPIO spec for the Data/Command pin, see gpio/gpio.txt

How is this optional? Looks pretty important from reading the driver.

> +  - reset-gpios: a GPIO spec for the reset pin, see gpio/gpio.txt
> +
> +The panel must obey the rules for a SPI slave device as specified in
> +spi/spi-bus.txt
> +
> +The device node can contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in
> +media/video-interfaces.txt. This node should describe panel's video bus.
> +
> +Example:
> +
> +panel@0 {
> +	compatible = "displaytech,dt024ctft", "ilitek,ili9341";
> +	reg = <0>;
> +	backlight = <&backlight>;
> +	dc-gpios = <&pio 4 9 GPIO_ACTIVE_HIGH>;
> +
> +	port {
> +		panel_in: endpoint {
> +			remote-endpoint = <&display_out>;
> +		};
> +	};
> +};
> -- 
> 2.20.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] Add DRM ILI9341 parallel RGB panel driver

[Josef Lusticky]

Hi,
This is the v2 of the patch-set which adds support for
Ilitek ILI9341 parallel RGB panels.

The ILI9341 chip supports both parallel RGB input mode and SPI input mode.
This driver adds support for the parallel RGB input mode.

Changes since v1:
* Device-tree bindings in one file
* D/C GPIO pin is optional
* mipi_dbi_* functions used to initialize the display
* entry in MAINTAINERS sorted alphabetically
* Makefile, Kconfig: DRM_PANEL_ILITEK_IL9341 renamed to DRM_PANEL_ILITEK_ILI9341
* Kconfig: depend on BACKLIGHT_CLASS_DEVICE
* Kconfig: select TINYDRM_MIPI_DBI
* order of include files changed
* drm_mode_duplicate checked for failure

Thank you Rob Herring and Sam Ravnborg for comments.

Josef Lusticky (2):
  dt-bindings: panel: Add parallel RGB mode for Ilitek ILI9341 panels
  drm/panel: Add Ilitek ILI9341 parallel RGB panel driver

 .../bindings/display/ilitek,ili9341.txt       |  67 +++-
 MAINTAINERS                                   |   6 +
 drivers/gpu/drm/panel/Kconfig                 |   9 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c  | 291 ++++++++++++++++++
 5 files changed, 363 insertions(+), 11 deletions(-)
 create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c

-- 
2.20.1

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

[PATCH v2 1/2] dt-bindings: panel: Add parallel RGB mode for Ilitek ILI9341 panels

[Josef Lusticky]

ILI9341 supports both SPI input mode and parallel RGB input mode.
This commit adds parallel RGB input mode bindings.

Signed-off-by: Josef Lusticky <josef@lusticky.cz>
---
 .../bindings/display/ilitek,ili9341.txt       | 67 ++++++++++++++++---
 1 file changed, 56 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
index 169b32e4ee4e..629f38a1d0cd 100644
--- a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
+++ b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
@@ -1,27 +1,72 @@
 Ilitek ILI9341 display panels
 
-This binding is for display panels using an Ilitek ILI9341 controller in SPI
-mode.
+This binding is for display panels using an Ilitek ILI9341 controller.
+The display panels are supported in the following graphical input modes:
+- SPI input mode
+	MIPI-DBI Type 3 Option 1 or Option 3 is used to transfer
+	commands and graphical data
+- parallel RGB input mode
+	MIPI-DBI Type 3 Option 1 or Option 3 is used for commands
+	MIPI-DPI 18-bit parallel RGB connection is used to transfer
+	graphical data
 
-Required properties:
-- compatible:	"adafruit,yx240qv29", "ilitek,ili9341"
-- dc-gpios:	D/C pin
-- reset-gpios:	Reset pin
+
+SPI input mode:
 
 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.
+all mandatory properties described in spi/spi-bus.txt must be specified.
+
+Required properties in SPI input mode:
+- compatible:   "adafruit,yx240qv29", "ilitek,ili9341"
+- backlight:    phandle of the backlight device attached to the panel
+
+Optional properties in SPI input mode:
+- rotation:     panel rotation in degrees counter clockwise (0,90,180,270)
+- dc-gpios:     GPIO spec for the D/C pin, see gpio/gpio.txt
+- reset-gpios:  GPIO spec for the reset pin, see gpio/gpio.txt
+
+
+Parallel RGB input mode:
+
+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.
+
+Required properties in parallel RGB input mode:
+- compatible:   "displaytech,dt024ctft", "ilitek,ili9341"
+- backlight:    phandle of the backlight device attached to the panel
+
+Optional properties in parallel RGB input mode:
+- dc-gpios:     GPIO spec for the D/C pin, see gpio/gpio.txt
+- reset-gpios:  GPIO spec for the reset pin, see gpio/gpio.txt
 
-Optional properties:
-- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
-- backlight:	phandle of the backlight device attached to the panel
+In parallel RGB input mode,
+the device node can contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in
+media/video-interfaces.txt. This node should describe panel's video bus.
 
-Example:
+
+Example in SPI input mode:
 	display@0{
 		compatible = "adafruit,yx240qv29", "ilitek,ili9341";
 		reg = <0>;
 		spi-max-frequency = <32000000>;
 		dc-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
 		reset-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
+		backlight = <&backlight>;
 		rotation = <270>;
+	};
+
+Example in parallel RGB input mode:
+	panel@{
+		compatible = "displaytech,dt024ctft", "ilitek,ili9341";
+		reg = <0>;
+		spi-max-frequency = <32000000>;
+		dc-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
 		backlight = <&backlight>;
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&display_out>;
+			};
+		};
 	};
-- 
2.20.1

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

Re: [PATCH v2 1/2] dt-bindings: panel: Add parallel RGB mode for Ilitek ILI9341 panels

[Sam Ravnborg]

Hi Josef.

On Mon, Jul 08, 2019 at 04:56:17PM +0200, Josef Lusticky wrote:
> ILI9341 supports both SPI input mode and parallel RGB input mode.
> This commit adds parallel RGB input mode bindings.
> 
> Signed-off-by: Josef Lusticky <josef@lusticky.cz>
> ---
>  .../bindings/display/ilitek,ili9341.txt       | 67 ++++++++++++++++---
>  1 file changed, 56 insertions(+), 11 deletions(-)
With Robs patches landed in drm-misc-next yaml format is from now on
preferred for panel bindings, or at least this is my take on it.

So a yaml conversion would be appreciated, but not mandatory.

> 
> diff --git a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> index 169b32e4ee4e..629f38a1d0cd 100644
> --- a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> +++ b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> @@ -1,27 +1,72 @@
>  Ilitek ILI9341 display panels
>  
> -This binding is for display panels using an Ilitek ILI9341 controller in SPI
> -mode.
> +This binding is for display panels using an Ilitek ILI9341 controller.
> +The display panels are supported in the following graphical input modes:
> +- SPI input mode
> +	MIPI-DBI Type 3 Option 1 or Option 3 is used to transfer
> +	commands and graphical data
> +- parallel RGB input mode
> +	MIPI-DBI Type 3 Option 1 or Option 3 is used for commands
> +	MIPI-DPI 18-bit parallel RGB connection is used to transfer
> +	graphical data
>  
> -Required properties:
> -- compatible:	"adafruit,yx240qv29", "ilitek,ili9341"
> -- dc-gpios:	D/C pin
> -- reset-gpios:	Reset pin
> +
> +SPI input mode:
>  
>  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.
> +all mandatory properties described in spi/spi-bus.txt must be specified.
> +
> +Required properties in SPI input mode:
> +- compatible:   "adafruit,yx240qv29", "ilitek,ili9341"
> +- backlight:    phandle of the backlight device attached to the panel
> +
> +Optional properties in SPI input mode:
> +- rotation:     panel rotation in degrees counter clockwise (0,90,180,270)
> +- dc-gpios:     GPIO spec for the D/C pin, see gpio/gpio.txt
> +- reset-gpios:  GPIO spec for the reset pin, see gpio/gpio.txt
> +
> +
> +Parallel RGB input mode:
> +
> +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.
> +
> +Required properties in parallel RGB input mode:
> +- compatible:   "displaytech,dt024ctft", "ilitek,ili9341"
> +- backlight:    phandle of the backlight device attached to the panel
> +
> +Optional properties in parallel RGB input mode:
> +- dc-gpios:     GPIO spec for the D/C pin, see gpio/gpio.txt
> +- reset-gpios:  GPIO spec for the reset pin, see gpio/gpio.txt
>  
> -Optional properties:
> -- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> -- backlight:	phandle of the backlight device attached to the panel
> +In parallel RGB input mode,
> +the device node can contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in
> +media/video-interfaces.txt. This node should describe panel's video bus.
>  
> -Example:
> +
> +Example in SPI input mode:
>  	display@0{
>  		compatible = "adafruit,yx240qv29", "ilitek,ili9341";
>  		reg = <0>;
>  		spi-max-frequency = <32000000>;
>  		dc-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
>  		reset-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
> +		backlight = <&backlight>;
>  		rotation = <270>;
> +	};
> +
> +Example in parallel RGB input mode:
> +	panel@{
I think you need a number after "@" here.

With this fixed:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] dt-bindings: panel: Add parallel RGB mode for Ilitek ILI9341 panels

[Rob Herring]

On Mon, Jul 08, 2019 at 04:56:17PM +0200, Josef Lusticky wrote:
> ILI9341 supports both SPI input mode and parallel RGB input mode.
> This commit adds parallel RGB input mode bindings.
> 
> Signed-off-by: Josef Lusticky <josef@lusticky.cz>
> ---
>  .../bindings/display/ilitek,ili9341.txt       | 67 ++++++++++++++++---
>  1 file changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> index 169b32e4ee4e..629f38a1d0cd 100644
> --- a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> +++ b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> @@ -1,27 +1,72 @@
>  Ilitek ILI9341 display panels
>  
> -This binding is for display panels using an Ilitek ILI9341 controller in SPI
> -mode.
> +This binding is for display panels using an Ilitek ILI9341 controller.
> +The display panels are supported in the following graphical input modes:
> +- SPI input mode
> +	MIPI-DBI Type 3 Option 1 or Option 3 is used to transfer
> +	commands and graphical data
> +- parallel RGB input mode
> +	MIPI-DBI Type 3 Option 1 or Option 3 is used for commands
> +	MIPI-DPI 18-bit parallel RGB connection is used to transfer
> +	graphical data
>  
> -Required properties:
> -- compatible:	"adafruit,yx240qv29", "ilitek,ili9341"
> -- dc-gpios:	D/C pin
> -- reset-gpios:	Reset pin
> +
> +SPI input mode:
>  
>  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.
> +all mandatory properties described in spi/spi-bus.txt must be specified.
> +
> +Required properties in SPI input mode:
> +- compatible:   "adafruit,yx240qv29", "ilitek,ili9341"
> +- backlight:    phandle of the backlight device attached to the panel

Why is backlight now required?

> +
> +Optional properties in SPI input mode:
> +- rotation:     panel rotation in degrees counter clockwise (0,90,180,270)
> +- dc-gpios:     GPIO spec for the D/C pin, see gpio/gpio.txt
> +- reset-gpios:  GPIO spec for the reset pin, see gpio/gpio.txt
> +
> +
> +Parallel RGB input mode:
> +
> +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.
> +
> +Required properties in parallel RGB input mode:
> +- compatible:   "displaytech,dt024ctft", "ilitek,ili9341"
> +- backlight:    phandle of the backlight device attached to the panel
> +
> +Optional properties in parallel RGB input mode:
> +- dc-gpios:     GPIO spec for the D/C pin, see gpio/gpio.txt
> +- reset-gpios:  GPIO spec for the reset pin, see gpio/gpio.txt
>  
> -Optional properties:
> -- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> -- backlight:	phandle of the backlight device attached to the panel
> +In parallel RGB input mode,
> +the device node can contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in
> +media/video-interfaces.txt. This node should describe panel's video bus.
>  
> -Example:
> +
> +Example in SPI input mode:
>  	display@0{
>  		compatible = "adafruit,yx240qv29", "ilitek,ili9341";
>  		reg = <0>;
>  		spi-max-frequency = <32000000>;
>  		dc-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
>  		reset-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
> +		backlight = <&backlight>;
>  		rotation = <270>;
> +	};
> +
> +Example in parallel RGB input mode:
> +	panel@{
> +		compatible = "displaytech,dt024ctft", "ilitek,ili9341";
> +		reg = <0>;
> +		spi-max-frequency = <32000000>;
> +		dc-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
>  		backlight = <&backlight>;
> +		port {
> +			panel_in: endpoint {
> +				remote-endpoint = <&display_out>;
> +			};
> +		};
>  	};
> -- 
> 2.20.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm/panel: Add Ilitek ILI9341 parallel RGB panel driver

[Josef Lusticky]

Add driver for Ilitek ILI9341 panels in parallel RGB mode

Signed-off-by: Josef Lusticky <josef@lusticky.cz>
---
 MAINTAINERS                                  |   6 +
 drivers/gpu/drm/panel/Kconfig                |   9 +
 drivers/gpu/drm/panel/Makefile               |   1 +
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 291 +++++++++++++++++++
 4 files changed, 307 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0a76716874bd..a35bf56cc018 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5028,6 +5028,12 @@ S:	Maintained
 F:	drivers/gpu/drm/tinydrm/hx8357d.c
 F:	Documentation/devicetree/bindings/display/himax,hx8357d.txt
 
+DRM DRIVER FOR ILITEK ILI9341 PANELS
+M:	Josef Lusticky <josef@lusticky.cz>
+S:	Maintained
+F:	drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+F:	Documentation/devicetree/bindings/display/panel/ilitek,ili9341.txt
+
 DRM DRIVER FOR INTEL I810 VIDEO CARDS
 S:	Orphan / Obsolete
 F:	drivers/gpu/drm/i810/
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index eaecd40cc32e..34a5b262f924 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -56,6 +56,15 @@ config DRM_PANEL_ILITEK_IL9322
 	  Say Y here if you want to enable support for Ilitek IL9322
 	  QVGA (320x240) RGB, YUV and ITU-T BT.656 panels.
 
+config DRM_PANEL_ILITEK_ILI9341
+	tristate "Ilitek ILI9341 240x320 panels"
+	depends on OF && SPI
+	depends on BACKLIGHT_CLASS_DEVICE
+	select TINYDRM_MIPI_DBI
+	help
+	  Say Y here if you want to enable support for Ilitek ILI9341
+	  QVGA (240x320) RGB panel.
+
 config DRM_PANEL_ILITEK_ILI9881C
 	tristate "Ilitek ILI9881C-based panels"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 62dae45f8f74..ba4a303c1a66 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o
 obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o
+obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9341) += panel-ilitek-ili9341.o
 obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o
 obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
new file mode 100644
index 000000000000..0c700b171025
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Ilitek ILI9341 drm_panel driver
+ * 240RGBx320 dots resolution TFT LCD display
+ *
+ * This driver supports the following panel configurations:
+ * - Command interface:
+ *     - MIPI-DBI Type 3 Option 1
+ *       (9-bit SPI with Data/Command bit  - IM[3:0] = 1101)
+ *     - MIPI-DBI Type 3 Option 3
+ *       (8-bit SPI with Data/Command GPIO - IM[3:0] = 1110)
+ * - Graphical data interface:
+ *     - MIPI-DPI 18-bit parallel RGB interface
+ *
+ * Copyright (C) 2019 Josef Lusticky <josef@lusticky.cz>
+ *
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+#include <drm/tinydrm/mipi-dbi.h>
+
+/* ILI9341 extended register set (Vendor Command Set) */
+#define ILI9341_IFMODE         0xB0 // clock polarity
+#define ILI9341_IFCTL          0xF6 // interface conrol
+#define ILI9341_PGAMCTRL       0xE0 // positive gamma control
+#define ILI9341_NGAMCTRL       0xE1 // negative gamma control
+
+#define ILI9341_MADCTL_MV      BIT(5)
+#define ILI9341_MADCTL_MX      BIT(6)
+#define ILI9341_MADCTL_MY      BIT(7)
+
+/**
+ * struct ili9341_config - the display specific configuration
+ * @width_mm: physical panel width [mm]
+ * @height_mm: physical panel height [mm]
+ */
+struct ili9341_config {
+	u32 width_mm;
+	u32 height_mm;
+};
+
+struct ili9341 {
+	struct drm_panel panel;
+	struct mipi_dbi *mipi;
+	const struct ili9341_config *conf;
+};
+
+static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
+{
+	return container_of(panel, struct ili9341, panel);
+}
+
+static int ili9341_deinit(struct drm_panel *panel, struct ili9341 *ili)
+{
+	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_DISPLAY_OFF);
+	mipi_dbi_command(ili->mipi, MIPI_DCS_ENTER_SLEEP_MODE);
+	msleep(5);
+	return 0;
+}
+
+static int ili9341_init(struct drm_panel *panel, struct ili9341 *ili)
+{
+	/* HW / SW Reset display and wait */
+	if (ili->mipi->reset)
+		mipi_dbi_hw_reset(ili->mipi);
+
+	mipi_dbi_command(ili->mipi, MIPI_DCS_SOFT_RESET);
+	msleep(120);
+
+	/* Polarity */
+	mipi_dbi_command(ili->mipi, ILI9341_IFMODE, 0xC0);
+
+	/* Interface control */
+	mipi_dbi_command(ili->mipi, ILI9341_IFCTL, 0x09, 0x01, 0x26);
+
+	/* Pixel format */
+	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT << 4);
+
+	/* Gamma */
+	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
+	mipi_dbi_command(ili->mipi, ILI9341_PGAMCTRL,
+		0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
+		0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
+	mipi_dbi_command(ili->mipi, ILI9341_NGAMCTRL,
+		0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
+		0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
+
+	/* Rotation */
+	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_ADDRESS_MODE, ILI9341_MADCTL_MX);
+
+	/* Exit sleep mode */
+	mipi_dbi_command(ili->mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(120);
+
+	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+	return 0;
+}
+
+static int ili9341_unprepare(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+
+	return ili9341_deinit(panel, ili);
+}
+
+static int ili9341_prepare(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	int ret;
+
+	ret = ili9341_init(panel, ili);
+	if (ret < 0)
+		ili9341_unprepare(panel);
+	return ret;
+}
+
+static int ili9341_enable(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+
+	return backlight_enable(ili->mipi->backlight);
+}
+
+static int ili9341_disable(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+
+	return backlight_disable(ili->mipi->backlight);
+}
+
+static const struct drm_display_mode prgb_240x320_mode = {
+	.clock = 6350,
+
+	.hdisplay = 240,
+	.hsync_start = 240 + 10,
+	.hsync_end = 240 + 10 + 10,
+	.htotal = 240 + 10 + 10 + 20,
+
+	.vdisplay = 320,
+	.vsync_start = 320 + 4,
+	.vsync_end = 320 + 4 + 2,
+	.vtotal = 320 + 4 + 2 + 2,
+
+	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED
+};
+
+static int ili9341_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(panel->drm, &prgb_240x320_mode);
+	if (!mode) {
+		DRM_DEV_ERROR(panel->drm->dev, "bad mode or failed to add mode\n");
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+
+	mode->width_mm = ili->conf->width_mm;
+	mode->height_mm = ili->conf->height_mm;
+
+	connector->display_info.width_mm = mode->width_mm;
+	connector->display_info.height_mm = mode->height_mm;
+	connector->display_info.bus_flags |= DRM_BUS_FLAG_DE_HIGH |
+		DRM_BUS_FLAG_PIXDATA_POSEDGE | DRM_BUS_FLAG_SYNC_NEGEDGE;
+
+	drm_mode_probed_add(connector, mode);
+
+	return 1; /* Number of modes */
+}
+
+static const struct drm_panel_funcs ili9341_drm_funcs = {
+	.disable = ili9341_disable,
+	.unprepare = ili9341_unprepare,
+	.prepare = ili9341_prepare,
+	.enable = ili9341_enable,
+	.get_modes = ili9341_get_modes,
+};
+
+static int ili9341_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ili9341 *ili;
+	struct mipi_dbi *mipi;
+	struct gpio_desc *dc_gpio;
+	int ret;
+
+	mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
+	if (!mipi)
+		return -ENOMEM;
+
+	ili = devm_kzalloc(dev, sizeof(*ili), GFP_KERNEL);
+	if (!ili)
+		return -ENOMEM;
+
+	ili->mipi = mipi;
+
+	spi_set_drvdata(spi, ili);
+
+	/*
+	 * Every new incarnation of this display must have a unique
+	 * data entry for the system in this driver.
+	 */
+	ili->conf = of_device_get_match_data(dev);
+	if (!ili->conf) {
+		DRM_DEV_ERROR(dev, "missing device configuration\n");
+		return -ENODEV;
+	}
+
+	ili->mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ili->mipi->reset)) {
+		DRM_DEV_ERROR(dev, "failed to get gpio 'reset'\n");
+		return PTR_ERR(ili->mipi->reset);
+	}
+
+	ili->mipi->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(ili->mipi->backlight)) {
+		DRM_DEV_ERROR(dev, "failed to get backlight\n");
+		return PTR_ERR(ili->mipi->backlight);
+	}
+
+	dc_gpio = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dc_gpio)) {
+		DRM_DEV_ERROR(dev, "failed to get gpio 'dc'\n");
+		return PTR_ERR(dc_gpio);
+	}
+
+	ret = mipi_dbi_spi_init(spi, ili->mipi, dc_gpio);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "MIPI-DBI SPI setup failed\n");
+		return ret;
+	}
+
+	drm_panel_init(&ili->panel);
+	ili->panel.dev = dev;
+	ili->panel.funcs = &ili9341_drm_funcs;
+
+	return drm_panel_add(&ili->panel);
+}
+
+static int ili9341_remove(struct spi_device *spi)
+{
+	struct ili9341 *ili = spi_get_drvdata(spi);
+
+	drm_panel_remove(&ili->panel);
+
+	ili9341_disable(&ili->panel);
+	ili9341_unprepare(&ili->panel);
+
+	return 0;
+}
+
+static const struct ili9341_config dt024ctft_data = {
+	.width_mm = 37,
+	.height_mm = 49,
+};
+
+static const struct of_device_id ili9341_of_match[] = {
+	{ .compatible = "displaytech,dt024ctft", .data = &dt024ctft_data },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ili9341_of_match);
+
+static struct spi_driver ili9341_driver = {
+	.probe = ili9341_probe,
+	.remove = ili9341_remove,
+	.driver = {
+		.name = "panel-ilitek-ili9341",
+		.of_match_table = ili9341_of_match,
+	},
+};
+module_spi_driver(ili9341_driver);
+
+MODULE_AUTHOR("Josef Lusticky <josef@lusticky.cz>");
+MODULE_DESCRIPTION("ILI9341 LCD panel driver");
+MODULE_LICENSE("GPL");
-- 
2.20.1

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

Re: [PATCH v2 2/2] drm/panel: Add Ilitek ILI9341 parallel RGB panel driver

[Sam Ravnborg]

Hi Josef.

Thanks for updating the driver.

On Mon, Jul 08, 2019 at 04:56:18PM +0200, Josef Lusticky wrote:
> Add driver for Ilitek ILI9341 panels in parallel RGB mode
> 
> Signed-off-by: Josef Lusticky <josef@lusticky.cz>
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +
> +#include <video/mipi_display.h>
> +
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/tinydrm/mipi-dbi.h>
Good to see drivers that no longer uses drmP.h :-)

> +
> +/* ILI9341 extended register set (Vendor Command Set) */
> +#define ILI9341_IFMODE         0xB0 // clock polarity
> +#define ILI9341_IFCTL          0xF6 // interface conrol
> +#define ILI9341_PGAMCTRL       0xE0 // positive gamma control
> +#define ILI9341_NGAMCTRL       0xE1 // negative gamma control
> +
> +#define ILI9341_MADCTL_MV      BIT(5)
> +#define ILI9341_MADCTL_MX      BIT(6)
> +#define ILI9341_MADCTL_MY      BIT(7)
> +
> +/**
> + * struct ili9341_config - the display specific configuration
> + * @width_mm: physical panel width [mm]
> + * @height_mm: physical panel height [mm]
> + */
> +struct ili9341_config {
> +	u32 width_mm;
> +	u32 height_mm;
> +};
> +
> +struct ili9341 {
> +	struct drm_panel panel;
> +	struct mipi_dbi *mipi;
> +	const struct ili9341_config *conf;
> +};
> +
> +static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct ili9341, panel);
> +}
> +
> +static int ili9341_deinit(struct drm_panel *panel, struct ili9341 *ili)
> +{
> +	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_DISPLAY_OFF);
> +	mipi_dbi_command(ili->mipi, MIPI_DCS_ENTER_SLEEP_MODE);
> +	msleep(5);
> +	return 0;
> +}
> +
> +static int ili9341_init(struct drm_panel *panel, struct ili9341 *ili)
> +{
> +	/* HW / SW Reset display and wait */
> +	if (ili->mipi->reset)
> +		mipi_dbi_hw_reset(ili->mipi);
> +
> +	mipi_dbi_command(ili->mipi, MIPI_DCS_SOFT_RESET);
> +	msleep(120);
Consider a usleep_range here - to have the waiting a little more relaxed
in the system.

> +
> +	/* Polarity */
> +	mipi_dbi_command(ili->mipi, ILI9341_IFMODE, 0xC0);
> +
> +	/* Interface control */
> +	mipi_dbi_command(ili->mipi, ILI9341_IFCTL, 0x09, 0x01, 0x26);
> +
> +	/* Pixel format */
> +	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT << 4);
> +
> +	/* Gamma */
> +	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
> +	mipi_dbi_command(ili->mipi, ILI9341_PGAMCTRL,
> +		0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
> +		0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
> +	mipi_dbi_command(ili->mipi, ILI9341_NGAMCTRL,
> +		0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
> +		0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
> +
> +	/* Rotation */
> +	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_ADDRESS_MODE, ILI9341_MADCTL_MX);
> +
> +	/* Exit sleep mode */
> +	mipi_dbi_command(ili->mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	msleep(120);
> +
> +	mipi_dbi_command(ili->mipi, MIPI_DCS_SET_DISPLAY_ON);
> +
> +	return 0;
> +}
> +
> +static int ili9341_unprepare(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	return ili9341_deinit(panel, ili);
> +}
> +
> +static int ili9341_prepare(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +	int ret;
> +
> +	ret = ili9341_init(panel, ili);
> +	if (ret < 0)
> +		ili9341_unprepare(panel);
> +	return ret;
> +}
> +
> +static int ili9341_enable(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	return backlight_enable(ili->mipi->backlight);
> +}
> +
> +static int ili9341_disable(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	return backlight_disable(ili->mipi->backlight);
> +}
> +
> +static const struct drm_display_mode prgb_240x320_mode = {
> +	.clock = 6350,
> +
> +	.hdisplay = 240,
> +	.hsync_start = 240 + 10,
> +	.hsync_end = 240 + 10 + 10,
> +	.htotal = 240 + 10 + 10 + 20,
> +
> +	.vdisplay = 320,
> +	.vsync_start = 320 + 4,
> +	.vsync_end = 320 + 4 + 2,
> +	.vtotal = 320 + 4 + 2 + 2,
> +
> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED
> +};
> +
> +static int ili9341_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, &prgb_240x320_mode);
> +	if (!mode) {
> +		DRM_DEV_ERROR(panel->drm->dev, "bad mode or failed to add mode\n");
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->width_mm = ili->conf->width_mm;
> +	mode->height_mm = ili->conf->height_mm;
> +
> +	connector->display_info.width_mm = mode->width_mm;
> +	connector->display_info.height_mm = mode->height_mm;
> +	connector->display_info.bus_flags |= DRM_BUS_FLAG_DE_HIGH |
> +		DRM_BUS_FLAG_PIXDATA_POSEDGE | DRM_BUS_FLAG_SYNC_NEGEDGE;
> +
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1; /* Number of modes */
> +}
> +
> +static const struct drm_panel_funcs ili9341_drm_funcs = {
> +	.disable = ili9341_disable,
> +	.unprepare = ili9341_unprepare,
> +	.prepare = ili9341_prepare,
> +	.enable = ili9341_enable,
> +	.get_modes = ili9341_get_modes,
> +};
> +
> +static int ili9341_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct ili9341 *ili;
> +	struct mipi_dbi *mipi;
> +	struct gpio_desc *dc_gpio;
> +	int ret;
> +
> +	mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
> +	if (!mipi)
> +		return -ENOMEM;
> +
> +	ili = devm_kzalloc(dev, sizeof(*ili), GFP_KERNEL);
> +	if (!ili)
> +		return -ENOMEM;
> +
> +	ili->mipi = mipi;
> +
> +	spi_set_drvdata(spi, ili);
> +
> +	/*
> +	 * Every new incarnation of this display must have a unique
> +	 * data entry for the system in this driver.
> +	 */
> +	ili->conf = of_device_get_match_data(dev);
> +	if (!ili->conf) {
> +		DRM_DEV_ERROR(dev, "missing device configuration\n");
> +		return -ENODEV;
> +	}
> +
> +	ili->mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ili->mipi->reset)) {
> +		DRM_DEV_ERROR(dev, "failed to get gpio 'reset'\n");
> +		return PTR_ERR(ili->mipi->reset);
> +	}
> +
> +	ili->mipi->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(ili->mipi->backlight)) {
> +		DRM_DEV_ERROR(dev, "failed to get backlight\n");
> +		return PTR_ERR(ili->mipi->backlight);
> +	}
> +
> +	dc_gpio = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc_gpio)) {
> +		DRM_DEV_ERROR(dev, "failed to get gpio 'dc'\n");
> +		return PTR_ERR(dc_gpio);
> +	}
> +
> +	ret = mipi_dbi_spi_init(spi, ili->mipi, dc_gpio);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "MIPI-DBI SPI setup failed\n");
> +		return ret;
> +	}
> +
> +	drm_panel_init(&ili->panel);
> +	ili->panel.dev = dev;
> +	ili->panel.funcs = &ili9341_drm_funcs;
> +
> +	return drm_panel_add(&ili->panel);
> +}
> +
> +static int ili9341_remove(struct spi_device *spi)
> +{
> +	struct ili9341 *ili = spi_get_drvdata(spi);
> +
> +	drm_panel_remove(&ili->panel);
> +
> +	ili9341_disable(&ili->panel);
> +	ili9341_unprepare(&ili->panel);
> +
> +	return 0;
> +}
> +
> +static const struct ili9341_config dt024ctft_data = {
> +	.width_mm = 37,
> +	.height_mm = 49,
> +};
> +
> +static const struct of_device_id ili9341_of_match[] = {
> +	{ .compatible = "displaytech,dt024ctft", .data = &dt024ctft_data },
> +	{ /* sentinel */ }
> +};
If another display is supported then the drm_display_mode may not match.
So the above may not prove enough in the future.
for now it should be fine.


> +MODULE_DEVICE_TABLE(of, ili9341_of_match);
> +
> +static struct spi_driver ili9341_driver = {
> +	.probe = ili9341_probe,
> +	.remove = ili9341_remove,
> +	.driver = {
> +		.name = "panel-ilitek-ili9341",
> +		.of_match_table = ili9341_of_match,
> +	},
> +};
> +module_spi_driver(ili9341_driver);
> +
> +MODULE_AUTHOR("Josef Lusticky <josef@lusticky.cz>");
> +MODULE_DESCRIPTION("ILI9341 LCD panel driver");
> +MODULE_LICENSE("GPL");

Looks good.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

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

Re: [PATCH v2 2/2] drm/panel: Add Ilitek ILI9341 parallel RGB panel driver

[Josef Luštický]

Hi Sam,
thank you for comments.

st 10. 7. 2019 v 15:47 odesílatel Sam Ravnborg <sam@ravnborg.org> napsal:
>
> Hi Josef.
>
> Thanks for updating the driver.
>
> On Mon, Jul 08, 2019 at 04:56:18PM +0200, Josef Lusticky wrote:
> > Add driver for Ilitek ILI9341 panels in parallel RGB mode
> >
> > Signed-off-by: Josef Lusticky <josef@lusticky.cz>
> > + */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <video/mipi_display.h>
> > +
> > +#include <drm/drm_modes.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/tinydrm/mipi-dbi.h>
> Good to see drivers that no longer uses drmP.h :-)
>
> > +
> > +/* ILI9341 extended register set (Vendor Command Set) */
> > +#define ILI9341_IFMODE         0xB0 // clock polarity
> > +#define ILI9341_IFCTL          0xF6 // interface conrol
> > +#define ILI9341_PGAMCTRL       0xE0 // positive gamma control
> > +#define ILI9341_NGAMCTRL       0xE1 // negative gamma control
> > +
> > +#define ILI9341_MADCTL_MV      BIT(5)
> > +#define ILI9341_MADCTL_MX      BIT(6)
> > +#define ILI9341_MADCTL_MY      BIT(7)
> > +
> > +/**
> > + * struct ili9341_config - the display specific configuration
> > + * @width_mm: physical panel width [mm]
> > + * @height_mm: physical panel height [mm]
> > + */
> > +struct ili9341_config {
> > +     u32 width_mm;
> > +     u32 height_mm;
> > +};
> > +
> > +struct ili9341 {
> > +     struct drm_panel panel;
> > +     struct mipi_dbi *mipi;
> > +     const struct ili9341_config *conf;
> > +};
> > +
> > +static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
> > +{
> > +     return container_of(panel, struct ili9341, panel);
> > +}
> > +
> > +static int ili9341_deinit(struct drm_panel *panel, struct ili9341 *ili)
> > +{
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_DISPLAY_OFF);
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_ENTER_SLEEP_MODE);
> > +     msleep(5);
> > +     return 0;
> > +}
> > +
> > +static int ili9341_init(struct drm_panel *panel, struct ili9341 *ili)
> > +{
> > +     /* HW / SW Reset display and wait */
> > +     if (ili->mipi->reset)
> > +             mipi_dbi_hw_reset(ili->mipi);
> > +
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SOFT_RESET);
> > +     msleep(120);
> Consider a usleep_range here - to have the waiting a little more relaxed
> in the system.
>

I am using msleep here as it is recommended for sleeping for 10ms+
by Documentation/timers/timers-howto.txt.
Anyway, I'll change this to call the
mipi_dbi_poweron_reset():mipi_dbi.c function,
which does the same as above.
Plus, I should change the msleep(5) in ili9341_deinit() to
usleep_range(5000, 20000).

> > +
> > +     /* Polarity */
> > +     mipi_dbi_command(ili->mipi, ILI9341_IFMODE, 0xC0);
> > +
> > +     /* Interface control */
> > +     mipi_dbi_command(ili->mipi, ILI9341_IFCTL, 0x09, 0x01, 0x26);
> > +
> > +     /* Pixel format */
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT << 4);
> > +
> > +     /* Gamma */
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
> > +     mipi_dbi_command(ili->mipi, ILI9341_PGAMCTRL,
> > +             0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
> > +             0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
> > +     mipi_dbi_command(ili->mipi, ILI9341_NGAMCTRL,
> > +             0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
> > +             0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
> > +
> > +     /* Rotation */
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_ADDRESS_MODE, ILI9341_MADCTL_MX);
> > +
> > +     /* Exit sleep mode */
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> > +     msleep(120);
> > +
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_DISPLAY_ON);
> > +
> > +     return 0;
> > +}
> > +
> > +static int ili9341_unprepare(struct drm_panel *panel)
> > +{
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +
> > +     return ili9341_deinit(panel, ili);
> > +}
> > +
> > +static int ili9341_prepare(struct drm_panel *panel)
> > +{
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +     int ret;
> > +
> > +     ret = ili9341_init(panel, ili);
> > +     if (ret < 0)
> > +             ili9341_unprepare(panel);
> > +     return ret;
> > +}
> > +
> > +static int ili9341_enable(struct drm_panel *panel)
> > +{
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +
> > +     return backlight_enable(ili->mipi->backlight);
> > +}
> > +
> > +static int ili9341_disable(struct drm_panel *panel)
> > +{
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +
> > +     return backlight_disable(ili->mipi->backlight);
> > +}
> > +
> > +static const struct drm_display_mode prgb_240x320_mode = {
> > +     .clock = 6350,
> > +
> > +     .hdisplay = 240,
> > +     .hsync_start = 240 + 10,
> > +     .hsync_end = 240 + 10 + 10,
> > +     .htotal = 240 + 10 + 10 + 20,
> > +
> > +     .vdisplay = 320,
> > +     .vsync_start = 320 + 4,
> > +     .vsync_end = 320 + 4 + 2,
> > +     .vtotal = 320 + 4 + 2 + 2,
> > +
> > +     .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> > +     .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED
> > +};
> > +
> > +static int ili9341_get_modes(struct drm_panel *panel)
> > +{
> > +     struct drm_connector *connector = panel->connector;
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +     struct drm_display_mode *mode;
> > +
> > +     mode = drm_mode_duplicate(panel->drm, &prgb_240x320_mode);
> > +     if (!mode) {
> > +             DRM_DEV_ERROR(panel->drm->dev, "bad mode or failed to add mode\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     drm_mode_set_name(mode);
> > +
> > +     mode->width_mm = ili->conf->width_mm;
> > +     mode->height_mm = ili->conf->height_mm;
> > +
> > +     connector->display_info.width_mm = mode->width_mm;
> > +     connector->display_info.height_mm = mode->height_mm;
> > +     connector->display_info.bus_flags |= DRM_BUS_FLAG_DE_HIGH |
> > +             DRM_BUS_FLAG_PIXDATA_POSEDGE | DRM_BUS_FLAG_SYNC_NEGEDGE;
> > +
> > +     drm_mode_probed_add(connector, mode);
> > +
> > +     return 1; /* Number of modes */
> > +}
> > +
> > +static const struct drm_panel_funcs ili9341_drm_funcs = {
> > +     .disable = ili9341_disable,
> > +     .unprepare = ili9341_unprepare,
> > +     .prepare = ili9341_prepare,
> > +     .enable = ili9341_enable,
> > +     .get_modes = ili9341_get_modes,
> > +};
> > +
> > +static int ili9341_probe(struct spi_device *spi)
> > +{
> > +     struct device *dev = &spi->dev;
> > +     struct ili9341 *ili;
> > +     struct mipi_dbi *mipi;
> > +     struct gpio_desc *dc_gpio;
> > +     int ret;
> > +
> > +     mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
> > +     if (!mipi)
> > +             return -ENOMEM;
> > +
> > +     ili = devm_kzalloc(dev, sizeof(*ili), GFP_KERNEL);
> > +     if (!ili)
> > +             return -ENOMEM;
> > +
> > +     ili->mipi = mipi;
> > +
> > +     spi_set_drvdata(spi, ili);
> > +
> > +     /*
> > +      * Every new incarnation of this display must have a unique
> > +      * data entry for the system in this driver.
> > +      */
> > +     ili->conf = of_device_get_match_data(dev);
> > +     if (!ili->conf) {
> > +             DRM_DEV_ERROR(dev, "missing device configuration\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     ili->mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(ili->mipi->reset)) {
> > +             DRM_DEV_ERROR(dev, "failed to get gpio 'reset'\n");
> > +             return PTR_ERR(ili->mipi->reset);
> > +     }
> > +
> > +     ili->mipi->backlight = devm_of_find_backlight(dev);
> > +     if (IS_ERR(ili->mipi->backlight)) {
> > +             DRM_DEV_ERROR(dev, "failed to get backlight\n");
> > +             return PTR_ERR(ili->mipi->backlight);
> > +     }
> > +
> > +     dc_gpio = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> > +     if (IS_ERR(dc_gpio)) {
> > +             DRM_DEV_ERROR(dev, "failed to get gpio 'dc'\n");
> > +             return PTR_ERR(dc_gpio);
> > +     }
> > +
> > +     ret = mipi_dbi_spi_init(spi, ili->mipi, dc_gpio);
> > +     if (ret) {
> > +             DRM_DEV_ERROR(dev, "MIPI-DBI SPI setup failed\n");
> > +             return ret;
> > +     }
> > +
> > +     drm_panel_init(&ili->panel);
> > +     ili->panel.dev = dev;
> > +     ili->panel.funcs = &ili9341_drm_funcs;
> > +
> > +     return drm_panel_add(&ili->panel);
> > +}
> > +
> > +static int ili9341_remove(struct spi_device *spi)
> > +{
> > +     struct ili9341 *ili = spi_get_drvdata(spi);
> > +
> > +     drm_panel_remove(&ili->panel);
> > +
> > +     ili9341_disable(&ili->panel);
> > +     ili9341_unprepare(&ili->panel);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct ili9341_config dt024ctft_data = {
> > +     .width_mm = 37,
> > +     .height_mm = 49,
> > +};
> > +
> > +static const struct of_device_id ili9341_of_match[] = {
> > +     { .compatible = "displaytech,dt024ctft", .data = &dt024ctft_data },
> > +     { /* sentinel */ }
> > +};
> If another display is supported then the drm_display_mode may not match.
> So the above may not prove enough in the future.
> for now it should be fine.
>
>
> > +MODULE_DEVICE_TABLE(of, ili9341_of_match);
> > +
> > +static struct spi_driver ili9341_driver = {
> > +     .probe = ili9341_probe,
> > +     .remove = ili9341_remove,
> > +     .driver = {
> > +             .name = "panel-ilitek-ili9341",
> > +             .of_match_table = ili9341_of_match,
> > +     },
> > +};
> > +module_spi_driver(ili9341_driver);
> > +
> > +MODULE_AUTHOR("Josef Lusticky <josef@lusticky.cz>");
> > +MODULE_DESCRIPTION("ILI9341 LCD panel driver");
> > +MODULE_LICENSE("GPL");
>
> Looks good.
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>
>         Sam

Thank you!
Josef
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/2] Add DRM ILI9341 parallel RGB panel driver

[Sam Ravnborg]

Hi Josef.

On Mon, Jul 08, 2019 at 04:56:16PM +0200, Josef Lusticky wrote:
> Hi,
> This is the v2 of the patch-set which adds support for
> Ilitek ILI9341 parallel RGB panels.
> 
> The ILI9341 chip supports both parallel RGB input mode and SPI input mode.
> This driver adds support for the parallel RGB input mode.
> 
> Changes since v1:
> * Device-tree bindings in one file
> * D/C GPIO pin is optional
> * mipi_dbi_* functions used to initialize the display
> * entry in MAINTAINERS sorted alphabetically
> * Makefile, Kconfig: DRM_PANEL_ILITEK_IL9341 renamed to DRM_PANEL_ILITEK_ILI9341
> * Kconfig: depend on BACKLIGHT_CLASS_DEVICE
> * Kconfig: select TINYDRM_MIPI_DBI
> * order of include files changed
> * drm_mode_duplicate checked for failure

Thanks for doing all this.

Some minor details. Please fix and resend so we can these patches
applied.
Please also re-check if you forgot an ack from Rob on the bindings file.

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

Controllers with several interface options - one or more drivers?

[Sam Ravnborg]

Hi Josef, Daniel et al.

The driver that triggered this reply is a driver that adds parallel
support to ili9341 in a dedicated panel driver.
The issue here is that we already have a tiny driver that supports the
ili9341 controller - but with a slightly different configuration.

The ili9341 supports several interfaces - from the datasheet:
    "ILI9341 supports parallel 8-/9-/16-/18-bit data bus
     MCU interface, 6-/16-/18-bit data bus RGB interface and
     3-/4-line serial peripheral interface (SPI)"

Noralf - in another mail explained:
"
The MIPI Alliance has lots of standards some wrt. display controller
interfaces:
- MIPI DBI - Display Bus Interface (used for commands and optionally pixels)
- MIPI DPI - Display Pixel Interface (also called RGB interface or
DOTCLK interface)
- MIPI DSI - Display Serial Interface (commands and pixels)

The ili9341 supports both MIPI DBI and DPI.
"

MIPI DPI - is a good fit for a drm_panel driver.
MIPI DBI - requires a full display controller driver.

There are many other examples of driver SoC that in the same way
can be seen only as a panel or as a full display controller driver.

The open question here is if we should try to support both cases in the
same driver / file. Or shall we implment two different drivers.
One for the panel use-case. And one for the display controller usecase?

Not sure - so asking for feedback.

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

Re: Controllers with several interface options - one or more drivers?

[Daniel Vetter]

On Fri, Jul 26, 2019 at 02:25:10PM +0200, Sam Ravnborg wrote:
> Hi Josef, Daniel et al.
> 
> The driver that triggered this reply is a driver that adds parallel
> support to ili9341 in a dedicated panel driver.
> The issue here is that we already have a tiny driver that supports the
> ili9341 controller - but with a slightly different configuration.
> 
> The ili9341 supports several interfaces - from the datasheet:
>     "ILI9341 supports parallel 8-/9-/16-/18-bit data bus
>      MCU interface, 6-/16-/18-bit data bus RGB interface and
>      3-/4-line serial peripheral interface (SPI)"
> 
> Noralf - in another mail explained:
> "
> The MIPI Alliance has lots of standards some wrt. display controller
> interfaces:
> - MIPI DBI - Display Bus Interface (used for commands and optionally pixels)
> - MIPI DPI - Display Pixel Interface (also called RGB interface or
> DOTCLK interface)
> - MIPI DSI - Display Serial Interface (commands and pixels)
> 
> The ili9341 supports both MIPI DBI and DPI.
> "
> 
> MIPI DPI - is a good fit for a drm_panel driver.
> MIPI DBI - requires a full display controller driver.
> 
> There are many other examples of driver SoC that in the same way
> can be seen only as a panel or as a full display controller driver.
> 
> The open question here is if we should try to support both cases in the
> same driver / file. Or shall we implment two different drivers.
> One for the panel use-case. And one for the display controller usecase?
> 
> Not sure - so asking for feedback.

I'm not sure. Currently we do have DSI and dumb RGB panels all in
drm/panel. I don't think we have DBI panels in there yet, but then
drm/tiny is the only one supporting these.

I guess we could look into move some of the DBI panel drivers into panel
drivers, but that needs a bit more glue all around. I'm honestly not sure
how the current DSI drivers in drm_panel work exactly, especially for
command mode.

Or maybe we need a new interface for command mode.

Wrt sharing code between drivers for the same chip, but different
interfaces: I wouldn't worry too much about that. Maybe try to have a
shared header file at least for registers. Long term we could end up with
one driver module which exposes different flavours of the same chip, so
multiple drm_panel drivers, or maybe we'll get something more specific for
dsi/dbi.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Controllers with several interface options - one or more drivers?

[Daniel Vetter]

Also probably should add a few more (drm_bridge) people, I think
that's also somewhat relevant here.
-Daniel

On Fri, Jul 26, 2019 at 4:55 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Jul 26, 2019 at 02:25:10PM +0200, Sam Ravnborg wrote:
> > Hi Josef, Daniel et al.
> >
> > The driver that triggered this reply is a driver that adds parallel
> > support to ili9341 in a dedicated panel driver.
> > The issue here is that we already have a tiny driver that supports the
> > ili9341 controller - but with a slightly different configuration.
> >
> > The ili9341 supports several interfaces - from the datasheet:
> >     "ILI9341 supports parallel 8-/9-/16-/18-bit data bus
> >      MCU interface, 6-/16-/18-bit data bus RGB interface and
> >      3-/4-line serial peripheral interface (SPI)"
> >
> > Noralf - in another mail explained:
> > "
> > The MIPI Alliance has lots of standards some wrt. display controller
> > interfaces:
> > - MIPI DBI - Display Bus Interface (used for commands and optionally pixels)
> > - MIPI DPI - Display Pixel Interface (also called RGB interface or
> > DOTCLK interface)
> > - MIPI DSI - Display Serial Interface (commands and pixels)
> >
> > The ili9341 supports both MIPI DBI and DPI.
> > "
> >
> > MIPI DPI - is a good fit for a drm_panel driver.
> > MIPI DBI - requires a full display controller driver.
> >
> > There are many other examples of driver SoC that in the same way
> > can be seen only as a panel or as a full display controller driver.
> >
> > The open question here is if we should try to support both cases in the
> > same driver / file. Or shall we implment two different drivers.
> > One for the panel use-case. And one for the display controller usecase?
> >
> > Not sure - so asking for feedback.
>
> I'm not sure. Currently we do have DSI and dumb RGB panels all in
> drm/panel. I don't think we have DBI panels in there yet, but then
> drm/tiny is the only one supporting these.
>
> I guess we could look into move some of the DBI panel drivers into panel
> drivers, but that needs a bit more glue all around. I'm honestly not sure
> how the current DSI drivers in drm_panel work exactly, especially for
> command mode.
>
> Or maybe we need a new interface for command mode.
>
> Wrt sharing code between drivers for the same chip, but different
> interfaces: I wouldn't worry too much about that. Maybe try to have a
> shared header file at least for registers. Long term we could end up with
> one driver module which exposes different flavours of the same chip, so
> multiple drm_panel drivers, or maybe we'll get something more specific for
> dsi/dbi.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Controllers with several interface options - one or more drivers?

[Sam Ravnborg]

Hi Daniel.

Added Noralf - somehow I missed him on the original mail.

On Fri, Jul 26, 2019 at 05:06:03PM +0200, Daniel Vetter wrote:
> Also probably should add a few more (drm_bridge) people, I think
> that's also somewhat relevant here.
> -Daniel
> 
> On Fri, Jul 26, 2019 at 4:55 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Fri, Jul 26, 2019 at 02:25:10PM +0200, Sam Ravnborg wrote:
> > > Hi Josef, Daniel et al.
> > >
> > > The driver that triggered this reply is a driver that adds parallel
> > > support to ili9341 in a dedicated panel driver.
> > > The issue here is that we already have a tiny driver that supports the
> > > ili9341 controller - but with a slightly different configuration.
> > >
> > > The ili9341 supports several interfaces - from the datasheet:
> > >     "ILI9341 supports parallel 8-/9-/16-/18-bit data bus
> > >      MCU interface, 6-/16-/18-bit data bus RGB interface and
> > >      3-/4-line serial peripheral interface (SPI)"
> > >
> > > Noralf - in another mail explained:
> > > "
> > > The MIPI Alliance has lots of standards some wrt. display controller
> > > interfaces:
> > > - MIPI DBI - Display Bus Interface (used for commands and optionally pixels)
> > > - MIPI DPI - Display Pixel Interface (also called RGB interface or
> > > DOTCLK interface)
> > > - MIPI DSI - Display Serial Interface (commands and pixels)
> > >
> > > The ili9341 supports both MIPI DBI and DPI.
> > > "
> > >
> > > MIPI DPI - is a good fit for a drm_panel driver.
> > > MIPI DBI - requires a full display controller driver.
> > >
> > > There are many other examples of driver SoC that in the same way
> > > can be seen only as a panel or as a full display controller driver.
> > >
> > > The open question here is if we should try to support both cases in the
> > > same driver / file. Or shall we implment two different drivers.
> > > One for the panel use-case. And one for the display controller usecase?
> > >
> > > Not sure - so asking for feedback.
> >
> > I'm not sure. Currently we do have DSI and dumb RGB panels all in
> > drm/panel. I don't think we have DBI panels in there yet, but then
> > drm/tiny is the only one supporting these.
> >
> > I guess we could look into move some of the DBI panel drivers into panel
> > drivers, but that needs a bit more glue all around. I'm honestly not sure
> > how the current DSI drivers in drm_panel work exactly, especially for
> > command mode.
> >
> > Or maybe we need a new interface for command mode.
If I get around to do a driver for the ssd1306 then I will try to sewhat
makes sense then. For now we shall not stall the ili9341 driver.
> >
> > Wrt sharing code between drivers for the same chip, but different
> > interfaces: I wouldn't worry too much about that. Maybe try to have a
> > shared header file at least for registers.
This part should be the minimum. Somthing like include/drm/mipi/?

	Sam

> > Long term we could end up with
> > one driver module which exposes different flavours of the same chip, so
> > multiple drm_panel drivers, or maybe we'll get something more specific for
> > dsi/dbi.
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Controllers with several interface options - one or more drivers?

[Josef Luštický]

Hello,
I am happy to see this discussion.

I like Noralf's late work to move mipi_dbi to drm/ and remove tinydrm.
This helps to simplify implementation and maintenance of drivers for
displays that conform to MIPI_DBI set of commands,
no matter if they use MIPI_DBI to transfer the image data or not.

There are already MIPI_DBI compliant panels in drm/panel, but until
Noralf's refactor they needed to implement custom
functions (on top SPI) to initialize the display. See the following
drivers as examples:
- drm/panel/panel-sitronix-st7789v.c
- drivers/gpu/drm/panel/panel-tpo-tpg110.c
- drivers/gpu/drm/panel/panel-lg-lg4573.c
- drivers/gpu/drm/panel/panel-samsung-ld9040.c
- drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
there may be more...

There are also at least two drivers for the same chip ili9341. See:
- drm/tinydrm/mi0283qt.c
- drm/tinydrm/ili9341.c
+ another driver for the same chip, but with parallel RGB initial
configuration, was submitted

I think there should be a single driver supporting all of the chip's
capabilities / configurations
to avoid maintenance burden and user confusion:

As a user, I can see the following use cases if a display conforms to MIPI_DBI.
a) it has its internal GRAM and MIPI_DBI can be used to transfer image data
b) internal GRAM is not present, parallel RGB must be used to transfer
image data
c) the display supports both - ili9341 case (but also others available)

As a user I want to specify displays that are going to use option a)
and/or b) - i.e.
I can have multiple ili9341 displays connected to the same parallel
RGB MCU interface,
but on a different SPI bus (or just different chip_select).
The same image data can be transferred to the displays over the parallel RGB
or I can switch some of the displays to use the MIPI_DBI (SPI) for
different image data transfer.

Kind regards
Josef Lusticky

pá 26. 7. 2019 v 18:14 odesílatel Sam Ravnborg <sam@ravnborg.org> napsal:
>
> Hi Daniel.
>
> Added Noralf - somehow I missed him on the original mail.
>
> On Fri, Jul 26, 2019 at 05:06:03PM +0200, Daniel Vetter wrote:
> > Also probably should add a few more (drm_bridge) people, I think
> > that's also somewhat relevant here.
> > -Daniel
> >
> > On Fri, Jul 26, 2019 at 4:55 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Fri, Jul 26, 2019 at 02:25:10PM +0200, Sam Ravnborg wrote:
> > > > Hi Josef, Daniel et al.
> > > >
> > > > The driver that triggered this reply is a driver that adds parallel
> > > > support to ili9341 in a dedicated panel driver.
> > > > The issue here is that we already have a tiny driver that supports the
> > > > ili9341 controller - but with a slightly different configuration.
> > > >
> > > > The ili9341 supports several interfaces - from the datasheet:
> > > >     "ILI9341 supports parallel 8-/9-/16-/18-bit data bus
> > > >      MCU interface, 6-/16-/18-bit data bus RGB interface and
> > > >      3-/4-line serial peripheral interface (SPI)"
> > > >
> > > > Noralf - in another mail explained:
> > > > "
> > > > The MIPI Alliance has lots of standards some wrt. display controller
> > > > interfaces:
> > > > - MIPI DBI - Display Bus Interface (used for commands and optionally pixels)
> > > > - MIPI DPI - Display Pixel Interface (also called RGB interface or
> > > > DOTCLK interface)
> > > > - MIPI DSI - Display Serial Interface (commands and pixels)
> > > >
> > > > The ili9341 supports both MIPI DBI and DPI.
> > > > "
> > > >
> > > > MIPI DPI - is a good fit for a drm_panel driver.
> > > > MIPI DBI - requires a full display controller driver.
> > > >
> > > > There are many other examples of driver SoC that in the same way
> > > > can be seen only as a panel or as a full display controller driver.
> > > >
> > > > The open question here is if we should try to support both cases in the
> > > > same driver / file. Or shall we implment two different drivers.
> > > > One for the panel use-case. And one for the display controller usecase?
> > > >
> > > > Not sure - so asking for feedback.
> > >
> > > I'm not sure. Currently we do have DSI and dumb RGB panels all in
> > > drm/panel. I don't think we have DBI panels in there yet, but then
> > > drm/tiny is the only one supporting these.
> > >
> > > I guess we could look into move some of the DBI panel drivers into panel
> > > drivers, but that needs a bit more glue all around. I'm honestly not sure
> > > how the current DSI drivers in drm_panel work exactly, especially for
> > > command mode.
> > >
> > > Or maybe we need a new interface for command mode.
> If I get around to do a driver for the ssd1306 then I will try to sewhat
> makes sense then. For now we shall not stall the ili9341 driver.
> > >
> > > Wrt sharing code between drivers for the same chip, but different
> > > interfaces: I wouldn't worry too much about that. Maybe try to have a
> > > shared header file at least for registers.
> This part should be the minimum. Somthing like include/drm/mipi/?
>
>         Sam
>
> > > Long term we could end up with
> > > one driver module which exposes different flavours of the same chip, so
> > > multiple drm_panel drivers, or maybe we'll get something more specific for
> > > dsi/dbi.
> > > -Daniel
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel