[PATCH 0/6] DBI/DSI, panel drivers, and tinyDRM compat

[PATCH 0/6] DBI/DSI, panel drivers, and tinyDRM compat

[Paul Cercueil]

Hi,

Here's a follow-up on the previous discussion about the current state of
DSI/DBI panel drivers, TinyDRM, and the need of a cleanup.

For the record, here is a small sum-up of the current situation:

- the current MIPI DBI code (drivers/gpu/drm/drm_mipi_dbi.c) is lagging
  way behind the MIPI DSI code (drivers/gpu/drm/drm_mipi_dsi.c). While
  the DSI code adds a proper bus support, with support for host drivers
  and client devices, there is no such thing with the DBI code. As such,
  it is currently impossible to write a standard DRM panel driver for a
  DBI panel.

- Even if the MIPI DBI code was updated with a proper bus, many panels
  and MIPI controllers support both DSI and DBI, so it would be a pain
  to support them without resolving to duplicating each driver.

- The panel drivers written against the DBI code are all "tinyDRM"
  drivers, which means that they will register a full yet simple DRM
  driver, and cannot be used as regular DRM panels for a different DRM
  driver.

- These "tinyDRM" drivers all use SPI directly, even though the panels
  they're driving can work on other interfaces (e.g. i8080 bus). Which
  means that one driver written for e.g. a ILI9341 would not work if
  the control interface is not SPI.

- The "tinyDRM" common code is entangled with DBI and there is no clear
  separation between the two. It could very well be moved to a single
  "tinyDRM" driver that works with a DRM panel obtained from devicetree,
  because the only requirement is that the panel supports a few given
  DCS commands.

This patchset introduces the following:

* Patch [2/6] slightly tweaks the MIPI DSI code so that it now also
  supports MIPI DBI over various hardware buses. Because if you think
  about it, DSI is just the same as DBI just on a different hardware
  bus. The DSI host drivers, now DSI/DBI host drivers, set compatibility
  bits for the hardware buses they support (DSI, DBI/i8080, DBI/SPI9,
  DBI/SPI+GPIO), which is matched against the hardware bus that panel
  drivers request.

* For the DBI panels connected over SPI, a new DSI/DBI host driver is
  added in patch [3/6]. It allows MIPI DBI panel drivers to be written
  with the DSI/DBI framework, even if they are connected over SPI,
  instead of registering as SPI device drivers. Since most of these
  panels can be connected over various buses, it permits to reuse the
  same driver independently of the bus used.

* Patch [4/6] adds a panel driver for NewVision NV3052C based panels.
  This has been successfully tested on the Anbernic RG350M handheld
  console, along with the dbi-spi bridge and the ingenic-drm driver.

* A TinyDRM driver for DSI/DBI panels, once again independent of the bus
  used; the only dependency (currently) being that the panel must
  understand DCS commands.

* A DRM panel driver to test the stack. This driver controls Ilitek
  ILI9341 based DBI panels, like the Adafruit YX240QV29-T 320x240 2.4"
  TFT LCD panel. This panel was converted from
  drivers/gpu/drm/tiny/ili9341.c.

Patches [1-4] were successfully tested on hardware.
Patches [5-6] were compile-tested and runtime-tested but without
hardware connected, so I'd want a Tested-by on these two.

Another thing to note, is that it does not break Device Tree ABI. The
display node stays the same:

spi {
	...

	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>;
		rotation = <270>;
		backlight = <&backlight>;
	};
};

The reason it works, is that the "adafruit,yx240qv29" device is probed
on the SPI bus, so it will match with the SPI/DBI host driver. This will
in turn register the very same node with the DSI bus, and the ILI9341
DRM panel driver will probe. The driver will detect that no controller
is linked to the panel, and eventually register the DBI/DSI TinyDRM
driver.

One drawback of this approach, is that the "adafruit,yx240qv29" must be
added to the dbi-spi bridge driver (unless a custom rule is added for a
"dbi-spi" fallback compatible string). I still think that it's a small
price to pay to avoid breaking the Device Tree bindings.

Feedback welcome.

Cheers,
-Paul

Paul Cercueil (6):
  dt-bindings: display: Document NewVision NV3052C DT node
  drm: dsi: Let host and device specify supported bus
  drm/bridge: Add SPI DBI host driver
  drm/panel: Add panel driver for NewVision NV3052C based LCDs
  drm/tiny: Add TinyDRM for DSI/DBI panels
  drm/panel: Add Ilitek ILI9341 DBI panel driver

 .../display/panel/newvision,nv3052c.yaml      |  69 +++
 drivers/gpu/drm/bridge/Kconfig                |   8 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/dbi-spi.c              | 262 +++++++++
 drivers/gpu/drm/drm_mipi_dsi.c                |   9 +
 drivers/gpu/drm/panel/Kconfig                 |  18 +
 drivers/gpu/drm/panel/Makefile                |   2 +
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c  | 345 ++++++++++++
 .../gpu/drm/panel/panel-newvision-nv3052c.c   | 523 ++++++++++++++++++
 drivers/gpu/drm/tiny/Kconfig                  |   8 +
 drivers/gpu/drm/tiny/Makefile                 |   1 +
 drivers/gpu/drm/tiny/tiny-dsi.c               | 266 +++++++++
 include/drm/drm_mipi_dsi.h                    |  31 ++
 13 files changed, 1543 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
 create mode 100644 drivers/gpu/drm/bridge/dbi-spi.c
 create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c
 create mode 100644 drivers/gpu/drm/panel/panel-newvision-nv3052c.c
 create mode 100644 drivers/gpu/drm/tiny/tiny-dsi.c

-- 
2.27.0

[PATCH 1/6] dt-bindings: display: Document NewVision NV3052C DT node

[Paul Cercueil]

Add documentation for the Device Tree node for LCD panels based on the
NewVision NV3052C controller.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 .../display/panel/newvision,nv3052c.yaml      | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml b/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
new file mode 100644
index 000000000000..751a28800fc2
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/newvision,nv3052c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NewVision NV3052C TFT LCD panel driver with SPI control bus
+
+maintainers:
+  - Paul Cercueil <paul@crapouillou.net>
+
+description: |
+  This is a driver for 320x240 TFT panels, accepting a variety of input
+  streams that get adapted and scaled to the panel. The panel output has
+  960 TFT source driver pins and 240 TFT gate driver pins, VCOM, VCOML and
+  VCOMH outputs.
+
+  The panel must obey the rules for a SPI slave device as specified in
+  spi/spi-controller.yaml
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - leadtek,ltk035c5444t-spi
+
+      - const: newvision,nv3052c
+
+  reg:
+    maxItems: 1
+
+  reset-gpios: true
+  port: true
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display@0 {
+        compatible = "leadtek,ltk035c5444t-spi", "newvision,nv3052c";
+        reg = <0>;
+
+        spi-max-frequency = <15000000>;
+        spi-3wire;
+        reset-gpios = <&gpe 2 GPIO_ACTIVE_LOW>;
+        backlight = <&backlight>;
+        power-supply = <&vcc>;
+
+        port {
+          panel_input: endpoint {
+              remote-endpoint = <&panel_output>;
+          };
+        };
+      };
+    };
+
+...
-- 
2.27.0

[PATCH 2/6] drm: dsi: Let host and device specify supported bus

[Paul Cercueil]

The current MIPI DSI framework can very well be used to support MIPI DBI
panels. In order to add support for the various bus types supported by
DBI, the DRM panel drivers should specify the bus type they will use,
and the DSI host drivers should specify the bus types they are
compatible with.

The DSI host driver can then use the information provided by the DBI/DSI
device driver, such as the bus type and the number of lanes, to
configure its hardware properly.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/drm_mipi_dsi.c |  9 +++++++++
 include/drm/drm_mipi_dsi.h     | 12 ++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 5dd475e82995..11ef885de765 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -281,6 +281,9 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
 {
 	struct device_node *node;
 
+	if (WARN_ON_ONCE(!host->bus_types))
+		host->bus_types = MIPI_DEVICE_TYPE_DSI;
+
 	for_each_available_child_of_node(host->dev->of_node, node) {
 		/* skip nodes without reg property */
 		if (!of_find_property(node, "reg", NULL))
@@ -323,6 +326,12 @@ int mipi_dsi_attach(struct mipi_dsi_device *dsi)
 {
 	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
 
+	if (WARN_ON_ONCE(!dsi->bus_type))
+		dsi->bus_type = MIPI_DEVICE_TYPE_DSI;
+
+	if (!(dsi->bus_type & dsi->host->bus_types))
+		return -EINVAL;
+
 	if (!ops || !ops->attach)
 		return -ENOSYS;
 
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 360e6377e84b..65d2961fc054 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -63,6 +63,14 @@ struct mipi_dsi_packet {
 int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
 			   const struct mipi_dsi_msg *msg);
 
+/* MIPI bus types */
+#define MIPI_DEVICE_TYPE_DSI		BIT(0)
+#define MIPI_DEVICE_TYPE_DBI_SPI_MODE1	BIT(1)
+#define MIPI_DEVICE_TYPE_DBI_SPI_MODE2	BIT(2)
+#define MIPI_DEVICE_TYPE_DBI_SPI_MODE3	BIT(3)
+#define MIPI_DEVICE_TYPE_DBI_M6800	BIT(4)
+#define MIPI_DEVICE_TYPE_DBI_I8080	BIT(5)
+
 /**
  * struct mipi_dsi_host_ops - DSI bus operations
  * @attach: attach DSI device to DSI host
@@ -94,11 +102,13 @@ struct mipi_dsi_host_ops {
  * struct mipi_dsi_host - DSI host device
  * @dev: driver model device node for this DSI host
  * @ops: DSI host operations
+ * @bus_types: Bitmask of supported MIPI bus types
  * @list: list management
  */
 struct mipi_dsi_host {
 	struct device *dev;
 	const struct mipi_dsi_host_ops *ops;
+	unsigned int bus_types;
 	struct list_head list;
 };
 
@@ -162,6 +172,7 @@ struct mipi_dsi_device_info {
  * @host: DSI host for this peripheral
  * @dev: driver model device node for this peripheral
  * @name: DSI peripheral chip type
+ * @bus_type: MIPI bus type (MIPI_DEVICE_TYPE_DSI/...)
  * @channel: virtual channel assigned to the peripheral
  * @format: pixel format for video mode
  * @lanes: number of active data lanes
@@ -178,6 +189,7 @@ struct mipi_dsi_device {
 	struct device dev;
 
 	char name[DSI_DEV_NAME_SIZE];
+	unsigned int bus_type;
 	unsigned int channel;
 	unsigned int lanes;
 	enum mipi_dsi_pixel_format format;
-- 
2.27.0

[PATCH 3/6] drm/bridge: Add SPI DBI host driver

[Paul Cercueil]

This driver will register a DBI host driver for panels connected over
SPI.

DBI types c1 and c3 are supported. C1 is a SPI protocol with 9 bits per
word, with the data/command information in the 9th (MSB) bit. C3 is a
SPI protocol with 8 bits per word, with the data/command information
carried by a separate GPIO.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/Kconfig   |   8 +
 drivers/gpu/drm/bridge/Makefile  |   1 +
 drivers/gpu/drm/bridge/dbi-spi.c | 261 +++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/dbi-spi.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index c7f0dacfb57a..ed38366847c1 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -219,6 +219,14 @@ config DRM_TI_TPD12S015
 	  Texas Instruments TPD12S015 HDMI level shifter and ESD protection
 	  driver.
 
+config DRM_MIPI_DBI_SPI
+	tristate "SPI host support for MIPI DBI"
+	depends on OF && SPI
+	select DRM_MIPI_DSI
+	select DRM_MIPI_DBI
+	help
+	  Driver to support DBI over SPI.
+
 source "drivers/gpu/drm/bridge/analogix/Kconfig"
 
 source "drivers/gpu/drm/bridge/adv7511/Kconfig"
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 7d7c123a95e4..c2c522c2774f 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
 obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
 obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
 obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
+obj-$(CONFIG_DRM_MIPI_DBI_SPI) += dbi-spi.o
 obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
 
 obj-y += analogix/
diff --git a/drivers/gpu/drm/bridge/dbi-spi.c b/drivers/gpu/drm/bridge/dbi-spi.c
new file mode 100644
index 000000000000..1060b8f95fba
--- /dev/null
+++ b/drivers/gpu/drm/bridge/dbi-spi.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MIPI Display Bus Interface (DBI) SPI support
+ *
+ * Copyright 2016 Noralf Trønnes
+ * Copyright 2020 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+
+#include <drm/drm_mipi_dbi.h>
+#include <drm/drm_mipi_dsi.h>
+
+#include <video/mipi_display.h>
+
+struct dbi_spi {
+	struct mipi_dsi_host host;
+	struct mipi_dsi_host_ops host_ops;
+
+	struct spi_device *spi;
+	struct gpio_desc *dc;
+	struct mutex cmdlock;
+
+	unsigned int current_bus_type;
+
+	/**
+	 * @tx_buf9: Buffer used for Option 1 9-bit conversion
+	 */
+	void *tx_buf9;
+
+	/**
+	 * @tx_buf9_len: Size of tx_buf9.
+	 */
+	size_t tx_buf9_len;
+};
+
+static inline struct dbi_spi *host_to_dbi_spi(struct mipi_dsi_host *host)
+{
+	return container_of(host, struct dbi_spi, host);
+}
+
+static ssize_t dbi_spi1_transfer(struct mipi_dsi_host *host,
+				 const struct mipi_dsi_msg *msg)
+{
+	struct dbi_spi *dbi = host_to_dbi_spi(host);
+	struct spi_device *spi = dbi->spi;
+	struct spi_transfer tr = {
+		.bits_per_word = 9,
+	};
+	const u8 *src8 = msg->tx_buf;
+	struct spi_message m;
+	size_t max_chunk, chunk;
+	size_t len = msg->tx_len;
+	bool cmd_byte = true;
+	unsigned int i;
+	u16 *dst16;
+	int ret;
+
+	tr.speed_hz = mipi_dbi_spi_cmd_max_speed(spi, len);
+	dst16 = dbi->tx_buf9;
+
+	max_chunk = min(dbi->tx_buf9_len / 2, len);
+
+	spi_message_init_with_transfers(&m, &tr, 1);
+	tr.tx_buf = dst16;
+
+	while (len) {
+		chunk = min(len, max_chunk);
+
+		for (i = 0; i < chunk; i++) {
+			dst16[i] = *src8++;
+
+			/* Bit 9: 0 means command, 1 means data */
+			if (!cmd_byte)
+				dst16[i] |= BIT(9);
+
+			cmd_byte = false;
+		}
+
+		tr.len = chunk * 2;
+		len -= chunk;
+
+		ret = spi_sync(spi, &m);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t dbi_spi3_transfer(struct mipi_dsi_host *host,
+				 const struct mipi_dsi_msg *msg)
+{
+	struct dbi_spi *dbi = host_to_dbi_spi(host);
+	struct spi_device *spi = dbi->spi;
+	const u8 *buf = msg->tx_buf;
+	unsigned int bpw = 8;
+	u32 speed_hz;
+	ssize_t ret;
+
+	/* for now we only support sending messages, not receiving */
+	if (msg->rx_len)
+		return -EINVAL;
+
+	gpiod_set_value_cansleep(dbi->dc, 0);
+
+	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
+	ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, buf, 1);
+	if (ret || msg->tx_len == 1)
+		return ret;
+
+	if (buf[0] == MIPI_DCS_WRITE_MEMORY_START)
+		bpw = 16;
+
+	gpiod_set_value_cansleep(dbi->dc, 1);
+	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, msg->tx_len - 1);
+
+	ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw,
+				    &buf[1], msg->tx_len - 1);
+	if (ret)
+		return ret;
+
+	return msg->tx_len;
+}
+
+static ssize_t dbi_spi_transfer(struct mipi_dsi_host *host,
+				const struct mipi_dsi_msg *msg)
+{
+	struct dbi_spi *dbi = host_to_dbi_spi(host);
+
+	switch (dbi->current_bus_type) {
+	case MIPI_DEVICE_TYPE_DBI_SPI_MODE1:
+		return dbi_spi1_transfer(host, msg);
+	case MIPI_DEVICE_TYPE_DBI_SPI_MODE3:
+		return dbi_spi3_transfer(host, msg);
+	default:
+		dev_err(&dbi->spi->dev, "Unknown transfer type\n");
+		return -EINVAL;
+	}
+}
+
+static int dbi_spi_attach(struct mipi_dsi_host *host,
+			  struct mipi_dsi_device *dsi)
+{
+	struct dbi_spi *dbi = host_to_dbi_spi(host);
+
+	dbi->current_bus_type = dsi->bus_type;
+
+	if (dbi->current_bus_type == MIPI_DEVICE_TYPE_DBI_SPI_MODE1) {
+		dbi->tx_buf9_len = SZ_16K;
+
+		dbi->tx_buf9 = kmalloc(dbi->tx_buf9_len, GFP_KERNEL);
+		if (!dbi->tx_buf9)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int dbi_spi_detach(struct mipi_dsi_host *host,
+			  struct mipi_dsi_device *dsi)
+{
+	struct dbi_spi *dbi = host_to_dbi_spi(host);
+
+	kfree(dbi->tx_buf9);
+	dbi->tx_buf9_len = 0;
+
+	return 0; /* Nothing to do */
+}
+
+static void dbi_spi_host_unregister(void *d)
+{
+	mipi_dsi_host_unregister(d);
+}
+
+static int dbi_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct mipi_dsi_device_info info = { };
+	struct mipi_dsi_device *dsi;
+	struct dbi_spi *dbi;
+	int ret;
+
+	dbi = devm_kzalloc(dev, sizeof(*dbi), GFP_KERNEL);
+	if (!dbi)
+		return -ENOMEM;
+
+	dbi->host.dev = dev;
+	dbi->host.ops = &dbi->host_ops;
+	dbi->spi = spi;
+	spi_set_drvdata(spi, dbi);
+
+	dbi->dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dbi->dc)) {
+		dev_err(dev, "Failed to get gpio 'dc'\n");
+		return PTR_ERR(dbi->dc);
+	}
+
+	if (spi_is_bpw_supported(spi, 9))
+		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE1;
+	if (dbi->dc)
+		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE3;
+
+	if (!dbi->host.bus_types) {
+		dev_err(dev, "Neither Type1 nor Type3 are supported\n");
+		return -EINVAL;
+	}
+
+	dbi->host_ops.transfer = dbi_spi_transfer;
+	dbi->host_ops.attach = dbi_spi_attach;
+	dbi->host_ops.detach = dbi_spi_detach;
+
+	mutex_init(&dbi->cmdlock);
+
+	ret = mipi_dsi_host_register(&dbi->host);
+	if (ret) {
+		dev_err(dev, "Unable to register DSI host\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, dbi_spi_host_unregister, &dbi->host);
+	if (ret)
+		return ret;
+
+	/*
+	 * Register our own node as a MIPI DSI device.
+	 * This ensures that the panel driver will be probed.
+	 */
+	info.channel = 0;
+	info.node = of_node_get(dev->of_node);
+
+	dsi = mipi_dsi_device_register_full(&dbi->host, &info);
+	if (IS_ERR(dsi)) {
+		dev_err(dev, "Failed to add DSI device\n");
+		return PTR_ERR(dsi);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id dbi_spi_of_match[] = {
+	{ .compatible = "adafruit,yx240qv29" },
+	{ .compatible = "leadtek,ltk035c5444t-spi" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, dbi_spi_of_match);
+
+static struct spi_driver dbi_spi_driver = {
+	.driver = {
+		.name = "dbi-spi",
+		.of_match_table = dbi_spi_of_match,
+	},
+	.probe = dbi_spi_probe,
+};
+module_spi_driver(dbi_spi_driver);
+
+MODULE_DESCRIPTION("DBI SPI bus driver");
+MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
+MODULE_LICENSE("GPL");
-- 
2.27.0

[PATCH 4/6] drm/panel: Add panel driver for NewVision NV3052C based LCDs

[Paul Cercueil]

This driver supports the NewVision NV3052C based LCDs. Right now, it
only supports the LeadTek LTK035C5444T 2.4" 640x480 TFT LCD panel, which
can be found in the Anbernic RG-350M handheld console.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/panel/Kconfig                 |   9 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-newvision-nv3052c.c   | 523 ++++++++++++++++++
 3 files changed, 533 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-newvision-nv3052c.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index de2f2a452be5..6a8a51a702d8 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -198,6 +198,15 @@ config DRM_PANEL_NEC_NL8048HL11
 	  panel (found on the Zoom2/3/3630 SDP boards). To compile this driver
 	  as a module, choose M here.
 
+config DRM_PANEL_NEWVISION_NV3052C
+	tristate "NewVision NV3052C DSI/SPI RGB panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for the panels built
+	  around the NewVision NV3052C display controller.
+
 config DRM_PANEL_NOVATEK_NT35510
 	tristate "Novatek NT35510 RGB panel driver"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index e45ceac6286f..a0516ced87db 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
 obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
+obj-$(CONFIG_DRM_PANEL_NEWVISION_NV3052C) += panel-newvision-nv3052c.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT39016) += panel-novatek-nt39016.o
 obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o
diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
new file mode 100644
index 000000000000..2feabef6dc3c
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
@@ -0,0 +1,523 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NevVision NV3052C IPS LCD panel driver
+ *
+ * Copyright (C) 2020, Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/media-bus-format.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+#include <video/mipi_display.h>
+
+struct nv3052c_panel_info {
+	const struct drm_display_mode *display_modes;
+	unsigned int num_modes;
+	unsigned int bus_type;
+	u16 width_mm, height_mm;
+	u32 bus_format, bus_flags;
+};
+
+struct nv3052c_reg {
+	u8 cmd;
+	u8 value;
+};
+
+struct nv3052c {
+	struct drm_panel drm_panel;
+	struct mipi_dsi_device *dsi;
+	struct device *dev;
+
+	struct regulator *supply;
+	const struct nv3052c_panel_info *panel_info;
+
+	struct gpio_desc *reset_gpio;
+
+	struct backlight_device *backlight;
+};
+
+static const struct nv3052c_reg nv3052c_regs[] = {
+	{ 0xff, 0x30 },
+	{ 0xff, 0x52 },
+	{ 0xff, 0x01 },
+	{ 0xe3, 0x00 },
+	{ 0x40, 0x00 },
+	{ 0x03, 0x40 },
+	{ 0x04, 0x00 },
+	{ 0x05, 0x03 },
+	{ 0x08, 0x00 },
+	{ 0x09, 0x07 },
+	{ 0x0a, 0x01 },
+	{ 0x0b, 0x32 },
+	{ 0x0c, 0x32 },
+	{ 0x0d, 0x0b },
+	{ 0x0e, 0x00 },
+	{ 0x23, 0xa0 },
+
+	{ 0x24, 0x0c },
+	{ 0x25, 0x06 },
+	{ 0x26, 0x14 },
+	{ 0x27, 0x14 },
+
+	{ 0x38, 0xcc },
+	{ 0x39, 0xd7 },
+	{ 0x3a, 0x4a },
+
+	{ 0x28, 0x40 },
+	{ 0x29, 0x01 },
+	{ 0x2a, 0xdf },
+	{ 0x49, 0x3c },
+	{ 0x91, 0x77 },
+	{ 0x92, 0x77 },
+	{ 0xa0, 0x55 },
+	{ 0xa1, 0x50 },
+	{ 0xa4, 0x9c },
+	{ 0xa7, 0x02 },
+	{ 0xa8, 0x01 },
+	{ 0xa9, 0x01 },
+	{ 0xaa, 0xfc },
+	{ 0xab, 0x28 },
+	{ 0xac, 0x06 },
+	{ 0xad, 0x06 },
+	{ 0xae, 0x06 },
+	{ 0xaf, 0x03 },
+	{ 0xb0, 0x08 },
+	{ 0xb1, 0x26 },
+	{ 0xb2, 0x28 },
+	{ 0xb3, 0x28 },
+	{ 0xb4, 0x33 },
+	{ 0xb5, 0x08 },
+	{ 0xb6, 0x26 },
+	{ 0xb7, 0x08 },
+	{ 0xb8, 0x26 },
+	{ 0xf0, 0x00 },
+	{ 0xf6, 0xc0 },
+
+	{ 0xff, 0x30 },
+	{ 0xff, 0x52 },
+	{ 0xff, 0x02 },
+	{ 0xb0, 0x0b },
+	{ 0xb1, 0x16 },
+	{ 0xb2, 0x17 },
+	{ 0xb3, 0x2c },
+	{ 0xb4, 0x32 },
+	{ 0xb5, 0x3b },
+	{ 0xb6, 0x29 },
+	{ 0xb7, 0x40 },
+	{ 0xb8, 0x0d },
+	{ 0xb9, 0x05 },
+	{ 0xba, 0x12 },
+	{ 0xbb, 0x10 },
+	{ 0xbc, 0x12 },
+	{ 0xbd, 0x15 },
+	{ 0xbe, 0x19 },
+	{ 0xbf, 0x0e },
+	{ 0xc0, 0x16 },
+	{ 0xc1, 0x0a },
+	{ 0xd0, 0x0c },
+	{ 0xd1, 0x17 },
+	{ 0xd2, 0x14 },
+	{ 0xd3, 0x2e },
+	{ 0xd4, 0x32 },
+	{ 0xd5, 0x3c },
+	{ 0xd6, 0x22 },
+	{ 0xd7, 0x3d },
+	{ 0xd8, 0x0d },
+	{ 0xd9, 0x07 },
+	{ 0xda, 0x13 },
+	{ 0xdb, 0x13 },
+	{ 0xdc, 0x11 },
+	{ 0xdd, 0x15 },
+	{ 0xde, 0x19 },
+	{ 0xdf, 0x10 },
+	{ 0xe0, 0x17 },
+	{ 0xe1, 0x0a },
+
+	{ 0xff, 0x30 },
+	{ 0xff, 0x52 },
+	{ 0xff, 0x03 },
+	{ 0x00, 0x2a },
+	{ 0x01, 0x2a },
+	{ 0x02, 0x2a },
+	{ 0x03, 0x2a },
+	{ 0x04, 0x61 },
+	{ 0x05, 0x80 },
+	{ 0x06, 0xc7 },
+	{ 0x07, 0x01 },
+	{ 0x08, 0x03 },
+	{ 0x09, 0x04 },
+	{ 0x70, 0x22 },
+	{ 0x71, 0x80 },
+	{ 0x30, 0x2a },
+	{ 0x31, 0x2a },
+	{ 0x32, 0x2a },
+	{ 0x33, 0x2a },
+	{ 0x34, 0x61 },
+	{ 0x35, 0xc5 },
+	{ 0x36, 0x80 },
+	{ 0x37, 0x23 },
+	{ 0x40, 0x03 },
+	{ 0x41, 0x04 },
+	{ 0x42, 0x05 },
+	{ 0x43, 0x06 },
+	{ 0x44, 0x11 },
+	{ 0x45, 0xe8 },
+	{ 0x46, 0xe9 },
+	{ 0x47, 0x11 },
+	{ 0x48, 0xea },
+	{ 0x49, 0xeb },
+	{ 0x50, 0x07 },
+	{ 0x51, 0x08 },
+	{ 0x52, 0x09 },
+	{ 0x53, 0x0a },
+	{ 0x54, 0x11 },
+	{ 0x55, 0xec },
+	{ 0x56, 0xed },
+	{ 0x57, 0x11 },
+	{ 0x58, 0xef },
+	{ 0x59, 0xf0 },
+	{ 0xb1, 0x01 },
+	{ 0xb4, 0x15 },
+	{ 0xb5, 0x16 },
+	{ 0xb6, 0x09 },
+	{ 0xb7, 0x0f },
+	{ 0xb8, 0x0d },
+	{ 0xb9, 0x0b },
+	{ 0xba, 0x00 },
+	{ 0xc7, 0x02 },
+	{ 0xca, 0x17 },
+	{ 0xcb, 0x18 },
+	{ 0xcc, 0x0a },
+	{ 0xcd, 0x10 },
+	{ 0xce, 0x0e },
+	{ 0xcf, 0x0c },
+	{ 0xd0, 0x00 },
+	{ 0x81, 0x00 },
+	{ 0x84, 0x15 },
+	{ 0x85, 0x16 },
+	{ 0x86, 0x10 },
+	{ 0x87, 0x0a },
+	{ 0x88, 0x0c },
+	{ 0x89, 0x0e },
+	{ 0x8a, 0x02 },
+	{ 0x97, 0x00 },
+	{ 0x9a, 0x17 },
+	{ 0x9b, 0x18 },
+	{ 0x9c, 0x0f },
+	{ 0x9d, 0x09 },
+	{ 0x9e, 0x0b },
+	{ 0x9f, 0x0d },
+	{ 0xa0, 0x01 },
+
+	{ 0xff, 0x30 },
+	{ 0xff, 0x52 },
+	{ 0xff, 0x02 },
+	{ 0x01, 0x01 },
+	{ 0x02, 0xda },
+	{ 0x03, 0xba },
+	{ 0x04, 0xa8 },
+	{ 0x05, 0x9a },
+	{ 0x06, 0x70 },
+	{ 0x07, 0xff },
+	{ 0x08, 0x91 },
+	{ 0x09, 0x90 },
+	{ 0x0a, 0xff },
+	{ 0x0b, 0x8f },
+	{ 0x0c, 0x60 },
+	{ 0x0d, 0x58 },
+	{ 0x0e, 0x48 },
+	{ 0x0f, 0x38 },
+	{ 0x10, 0x2b },
+
+	{ 0xff, 0x30 },
+	{ 0xff, 0x52 },
+	{ 0xff, 0x00 },
+	{ 0x36, 0x0a },
+};
+
+static inline struct nv3052c *to_nv3052c(struct drm_panel *panel)
+{
+	return container_of(panel, struct nv3052c, drm_panel);
+}
+
+static int nv3052c_prepare(struct drm_panel *drm_panel)
+{
+	struct nv3052c *priv = to_nv3052c(drm_panel);
+	unsigned int i;
+	int err;
+
+	err = regulator_enable(priv->supply);
+	if (err) {
+		dev_err(priv->dev, "Failed to enable power supply: %d\n", err);
+		return err;
+	}
+
+	/* Reset the chip */
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+	usleep_range(10, 1000);
+	gpiod_set_value_cansleep(priv->reset_gpio, 0);
+	usleep_range(5000, 20000);
+
+	for (i = 0; i < ARRAY_SIZE(nv3052c_regs); i++) {
+		err = mipi_dsi_dcs_write(priv->dsi, nv3052c_regs[i].cmd,
+					 &nv3052c_regs[i].value, 1);
+		if (err) {
+			dev_err(priv->dev, "Unable to set register: %d\n", err);
+			goto err_disable_regulator;
+		}
+	}
+
+	err = mipi_dsi_dcs_exit_sleep_mode(priv->dsi);
+	if (err) {
+		dev_err(priv->dev, "Unable to exit sleep mode: %d\n", err);
+		return err;
+	}
+
+	msleep(100);
+
+	return 0;
+
+err_disable_regulator:
+	regulator_disable(priv->supply);
+	return err;
+}
+
+static int nv3052c_unprepare(struct drm_panel *drm_panel)
+{
+	struct nv3052c *priv = to_nv3052c(drm_panel);
+	int err;
+
+	err = mipi_dsi_dcs_enter_sleep_mode(priv->dsi);
+	if (err) {
+		dev_err(priv->dev, "Unable to enter sleep mode: %d\n", err);
+		return err;
+	}
+
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+
+	regulator_disable(priv->supply);
+
+	return 0;
+}
+
+static int nv3052c_enable(struct drm_panel *drm_panel)
+{
+	struct nv3052c *priv = to_nv3052c(drm_panel);
+	int err;
+
+	err = mipi_dsi_dcs_set_display_on(priv->dsi);
+	if (err) {
+		dev_err(priv->dev, "Unable to enable display: %d\n", err);
+		return err;
+	}
+
+	if (priv->backlight) {
+		/* Wait for the picture to be ready before enabling backlight */
+		usleep_range(10000, 20000);
+
+		err = backlight_enable(priv->backlight);
+	}
+
+	return err;
+}
+
+static int nv3052c_disable(struct drm_panel *drm_panel)
+{
+	struct nv3052c *priv = to_nv3052c(drm_panel);
+	int err;
+
+	backlight_disable(priv->backlight);
+
+	err = mipi_dsi_dcs_set_display_off(priv->dsi);
+	if (err) {
+		dev_err(priv->dev, "Unable to disable display: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int nv3052c_get_modes(struct drm_panel *drm_panel,
+			     struct drm_connector *connector)
+{
+	struct nv3052c *priv = to_nv3052c(drm_panel);
+	const struct nv3052c_panel_info *panel_info = priv->panel_info;
+	struct drm_display_mode *mode;
+	unsigned int i;
+
+	for (i = 0; i < panel_info->num_modes; i++) {
+		mode = drm_mode_duplicate(connector->dev,
+					  &panel_info->display_modes[i]);
+		if (!mode)
+			return -ENOMEM;
+
+		drm_mode_set_name(mode);
+
+		mode->type = DRM_MODE_TYPE_DRIVER;
+		if (panel_info->num_modes == 1)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_probed_add(connector, mode);
+	}
+
+	connector->display_info.bpc = 8;
+	connector->display_info.width_mm = panel_info->width_mm;
+	connector->display_info.height_mm = panel_info->height_mm;
+
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &panel_info->bus_format, 1);
+	connector->display_info.bus_flags = panel_info->bus_flags;
+
+	return panel_info->num_modes;
+}
+
+static const struct drm_panel_funcs nv3052c_funcs = {
+	.prepare	= nv3052c_prepare,
+	.unprepare	= nv3052c_unprepare,
+	.enable		= nv3052c_enable,
+	.disable	= nv3052c_disable,
+	.get_modes	= nv3052c_get_modes,
+};
+
+static int nv3052c_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct nv3052c *priv;
+	int err;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->dsi = dsi;
+	mipi_dsi_set_drvdata(dsi, priv);
+
+	priv->panel_info = of_device_get_match_data(dev);
+	if (!priv->panel_info)
+		return -EINVAL;
+
+	dsi->bus_type = priv->panel_info->bus_type;
+
+	priv->supply = devm_regulator_get(dev, "power");
+	if (IS_ERR(priv->supply)) {
+		dev_err(dev, "Failed to get power supply\n");
+		return PTR_ERR(priv->supply);
+	}
+
+	priv->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->reset_gpio)) {
+		dev_err(dev, "Failed to get reset GPIO\n");
+		return PTR_ERR(priv->reset_gpio);
+	}
+
+	priv->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(priv->backlight)) {
+		err = PTR_ERR(priv->backlight);
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get backlight handle\n");
+		return err;
+	}
+
+	drm_panel_init(&priv->drm_panel, dev, &nv3052c_funcs,
+		       DRM_MODE_CONNECTOR_DPI);
+
+	err = drm_panel_add(&priv->drm_panel);
+	if (err < 0) {
+		dev_err(dev, "Failed to register priv\n");
+		return err;
+	}
+
+	err = mipi_dsi_attach(dsi);
+	if (err) {
+		dev_err(dev, "Failed to attach panel\n");
+		drm_panel_remove(&priv->drm_panel);
+		return err;
+	}
+
+	/*
+	 * We can't call mipi_dsi_maybe_register_tiny_driver(), since the
+	 * NV3052C does not support MIPI_DCS_WRITE_MEMORY_START.
+	 */
+
+	return 0;
+}
+
+static int nv3052c_remove(struct mipi_dsi_device *dsi)
+{
+	struct nv3052c *priv = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_panel_remove(&priv->drm_panel);
+
+	nv3052c_disable(&priv->drm_panel);
+	nv3052c_unprepare(&priv->drm_panel);
+
+	return 0;
+}
+
+static const struct drm_display_mode ltk035c5444t_modes[] = {
+	{ /* 60 Hz */
+		.clock = 24000,
+		.hdisplay = 640,
+		.hsync_start = 640 + 96,
+		.hsync_end = 640 + 96 + 16,
+		.htotal = 640 + 96 + 16 + 48,
+		.vdisplay = 480,
+		.vsync_start = 480 + 5,
+		.vsync_end = 480 + 5 + 2,
+		.vtotal = 480 + 5 + 2 + 13,
+		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	},
+	{ /* 50 Hz */
+		.clock = 18000,
+		.hdisplay = 640,
+		.hsync_start = 640 + 39,
+		.hsync_end = 640 + 39 + 2,
+		.htotal = 640 + 39 + 2 + 39,
+		.vdisplay = 480,
+		.vsync_start = 480 + 5,
+		.vsync_end = 480 + 5 + 2,
+		.vtotal = 480 + 5 + 2 + 13,
+		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	},
+};
+
+static const struct nv3052c_panel_info ltk035c5444t_spi_panel_info = {
+	.display_modes = ltk035c5444t_modes,
+	.num_modes = ARRAY_SIZE(ltk035c5444t_modes),
+	.bus_type = MIPI_DEVICE_TYPE_DBI_SPI_MODE1,
+	.width_mm = 77,
+	.height_mm = 64,
+	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
+};
+
+static const struct of_device_id nv3052c_of_match[] = {
+	{ .compatible = "leadtek,ltk035c5444t-spi", .data = &ltk035c5444t_spi_panel_info },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, nv3052c_of_match);
+
+static struct mipi_dsi_driver nv3052c_driver = {
+	.driver = {
+		.name = "nv3052c",
+		.of_match_table = nv3052c_of_match,
+	},
+	.probe = nv3052c_probe,
+	.remove = nv3052c_remove,
+};
+module_mipi_dsi_driver(nv3052c_driver);
+
+MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
+MODULE_LICENSE("GPL v2");
-- 
2.27.0

[PATCH 5/6] drm/tiny: Add TinyDRM for DSI/DBI panels

[Paul Cercueil]

The new API function mipi_dsi_maybe_register_tiny_driver() is supposed
to be called by DSI/DBI panel drivers at the end of their probe.

If it is detected that the panel is not connected to any controller,
because it has no port #0 node in Device Tree that points back to it,
then a TinyDRM driver is registered with it.

This TinyDRM driver expects that a DCS-compliant protocol is used by the
DSI/DBI panel and can only be used with these.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/tiny/Kconfig    |   8 +
 drivers/gpu/drm/tiny/Makefile   |   1 +
 drivers/gpu/drm/tiny/tiny-dsi.c | 266 ++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h      |  19 +++
 4 files changed, 294 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/tiny-dsi.c

diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 2b6414f0fa75..b62427b88dfe 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -28,6 +28,14 @@ config DRM_GM12U320
 	 This is a KMS driver for projectors which use the GM12U320 chipset
 	 for video transfer over USB2/3, such as the Acer C120 mini projector.
 
+config TINYDRM_DSI
+	tristate "DRM support for generic DBI/DSI display panels"
+	depends on DRM && DRM_MIPI_DSI
+	select DRM_MIPI_DBI
+	select DRM_KMS_CMA_HELPER
+	help
+	  DRM driver for generic DBI/DSI display panels
+
 config TINYDRM_HX8357D
 	tristate "DRM support for HX8357D display panels"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 6ae4e9e5a35f..2bfee13347a5 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -2,6 +2,7 @@
 
 obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
 obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
+obj-$(CONFIG_TINYDRM_DSI)		+= tiny-dsi.o
 obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
 obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
 obj-$(CONFIG_TINYDRM_ILI9341)		+= ili9341.o
diff --git a/drivers/gpu/drm/tiny/tiny-dsi.c b/drivers/gpu/drm/tiny/tiny-dsi.c
new file mode 100644
index 000000000000..b24d49836125
--- /dev/null
+++ b/drivers/gpu/drm/tiny/tiny-dsi.c
@@ -0,0 +1,266 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * TinyDRM driver for standard DSI/DBI panels
+ *
+ * Copyright 2020 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_mipi_dbi.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modeset_helper.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_probe_helper.h>
+
+#include <video/mipi_display.h>
+
+struct tiny_dsi {
+	struct drm_device drm;
+	struct drm_connector connector;
+	struct drm_simple_display_pipe pipe;
+
+	struct mipi_dsi_device *dsi;
+	struct drm_panel *panel;
+};
+
+#define mipi_dcs_command(dsi, cmd, seq...) \
+({ \
+	u8 d[] = { seq }; \
+	mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d)); \
+})
+
+static inline struct tiny_dsi *drm_to_tiny_dsi(struct drm_device *drm)
+{
+	return container_of(drm, struct tiny_dsi, drm);
+}
+
+static void tiny_dsi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
+{
+	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
+	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(gem);
+	struct tiny_dsi *priv = drm_to_tiny_dsi(fb->dev);
+	unsigned int height = rect->y2 - rect->y1;
+	unsigned int width = rect->x2 - rect->x1;
+	bool fb_convert;
+	int idx, ret;
+	void *tr;
+
+	if (!drm_dev_enter(fb->dev, &idx))
+		return;
+
+	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
+
+	fb_convert = width != fb->width || height != fb->height
+		|| fb->format->format == DRM_FORMAT_XRGB8888;
+	if (fb_convert) {
+		tr = kzalloc(width * height * 2, GFP_KERNEL);
+
+		/* TODO: swap pixels if needed */
+		ret = mipi_dbi_buf_copy(tr, fb, rect, false);
+		if (ret)
+			goto err_msg;
+	} else {
+		tr = cma_obj->vaddr;
+	}
+
+	mipi_dcs_command(priv->dsi, MIPI_DCS_SET_COLUMN_ADDRESS,
+			 (rect->x1 >> 8) & 0xff, rect->x1 & 0xff,
+			 (rect->x2 >> 8) & 0xff, rect->x2 & 0xff);
+	mipi_dcs_command(priv->dsi, MIPI_DCS_SET_PAGE_ADDRESS,
+			 (rect->y1 >> 8) & 0xff, rect->y1 & 0xff,
+			 (rect->y2 >> 8) & 0xff, rect->y2 & 0xff);
+
+	ret = mipi_dsi_dcs_write(priv->dsi, MIPI_DCS_WRITE_MEMORY_START,
+				 tr, width * height * 2);
+err_msg:
+	if (ret)
+		dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret);
+
+	if (fb_convert)
+		kfree(tr);
+	drm_dev_exit(idx);
+}
+
+static void tiny_dsi_enable(struct drm_simple_display_pipe *pipe,
+			    struct drm_crtc_state *crtc_state,
+			    struct drm_plane_state *plane_state)
+{
+	struct tiny_dsi *priv = drm_to_tiny_dsi(pipe->crtc.dev);
+
+	drm_panel_enable(priv->panel);
+}
+
+static void tiny_dsi_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct tiny_dsi *priv = drm_to_tiny_dsi(pipe->crtc.dev);
+
+	drm_panel_disable(priv->panel);
+}
+
+static void tiny_dsi_update(struct drm_simple_display_pipe *pipe,
+			    struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_rect rect;
+
+	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
+		tiny_dsi_fb_dirty(state->fb, &rect);
+}
+
+static const struct drm_simple_display_pipe_funcs tiny_dsi_pipe_funcs = {
+	.enable = tiny_dsi_enable,
+	.disable = tiny_dsi_disable,
+	.update = tiny_dsi_update,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
+};
+
+static int tiny_dsi_connector_get_modes(struct drm_connector *connector)
+{
+	struct tiny_dsi *priv = drm_to_tiny_dsi(connector->dev);
+
+	return drm_panel_get_modes(priv->panel, connector);
+}
+
+static const struct drm_connector_helper_funcs tiny_dsi_connector_hfuncs = {
+	.get_modes = tiny_dsi_connector_get_modes,
+};
+
+static const struct drm_connector_funcs tiny_dsi_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static const uint32_t tiny_dsi_formats[] = {
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+};
+
+static const struct drm_mode_config_funcs tiny_dsi_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+DEFINE_DRM_GEM_CMA_FOPS(tiny_dsi_fops);
+
+static struct drm_driver tiny_dsi_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.name			= "tiny-dsi",
+	.desc			= "Tiny DSI",
+	.date			= "20200605",
+	.major			= 1,
+	.minor			= 0,
+
+	.fops			= &tiny_dsi_fops,
+	DRM_GEM_CMA_DRIVER_OPS,
+};
+
+static void tiny_dsi_remove(void *drm)
+{
+	drm_dev_unplug(drm);
+	drm_atomic_helper_shutdown(drm);
+}
+
+int mipi_dsi_register_tiny_driver(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct drm_device *drm;
+	struct tiny_dsi *priv;
+	static const uint64_t modifiers[] = {
+		DRM_FORMAT_MOD_LINEAR,
+		DRM_FORMAT_MOD_INVALID
+	};
+	int ret;
+
+	/*
+	 * Even though it's not the SPI device that does DMA (the master does),
+	 * the dma mask is necessary for the dma_alloc_wc() in
+	 * drm_gem_cma_create(). The dma_addr returned will be a physical
+	 * address which might be different from the bus address, but this is
+	 * not a problem since the address will not be used.
+	 * The virtual address is used in the transfer and the SPI core
+	 * re-maps it on the SPI master device using the DMA streaming API
+	 * (spi_map_buf()).
+	 */
+	if (!dev->coherent_dma_mask) {
+		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
+		if (ret) {
+			dev_warn(dev, "Failed to set dma mask %d\n", ret);
+			return ret;
+		}
+	}
+
+	priv = devm_drm_dev_alloc(dev, &tiny_dsi_driver, struct tiny_dsi, drm);
+	if (!priv)
+		return ret;
+
+	priv->dsi = dsi;
+	drm = &priv->drm;
+
+	priv->panel = of_drm_find_panel(dev->of_node);
+	if (IS_ERR(priv->panel)) {
+		dev_err(dev, "Unable to find panel\n");
+		return PTR_ERR(priv->panel);
+	}
+
+	drm_mode_config_init(drm);
+
+	drm->mode_config.preferred_depth = 16;
+
+	drm->mode_config.funcs = &tiny_dsi_mode_config_funcs;
+	drm->mode_config.min_width = 0;
+	drm->mode_config.min_height = 0;
+	drm->mode_config.max_width = 4096;
+	drm->mode_config.max_height = 4096;
+
+	drm_connector_helper_add(&priv->connector, &tiny_dsi_connector_hfuncs);
+	ret = drm_connector_init(drm, &priv->connector, &tiny_dsi_connector_funcs,
+				 DRM_MODE_CONNECTOR_DSI);
+	if (ret) {
+		dev_err(dev, "Unable to init connector\n");
+		return ret;
+	}
+
+	ret = drm_simple_display_pipe_init(drm, &priv->pipe, &tiny_dsi_pipe_funcs,
+					   tiny_dsi_formats, ARRAY_SIZE(tiny_dsi_formats),
+					   modifiers, &priv->connector);
+	if (ret) {
+		dev_err(dev, "Unable to init display pipe\n");
+		return ret;
+	}
+
+	drm_plane_enable_fb_damage_clips(&priv->pipe.plane);
+
+	drm_mode_config_reset(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret) {
+		dev_err(dev, "Failed to register DRM driver\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, tiny_dsi_remove, drm);
+	if (ret)
+		return ret;
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_register_tiny_driver);
+
+MODULE_DESCRIPTION("DSI/DBI TinyDRM driver");
+MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
+MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 65d2961fc054..0c2589a55df6 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -10,6 +10,7 @@
 #define __DRM_MIPI_DSI_H__
 
 #include <linux/device.h>
+#include <linux/of_graph.h>
 
 struct mipi_dsi_host;
 struct mipi_dsi_device;
@@ -337,4 +338,22 @@ void mipi_dsi_driver_unregister(struct mipi_dsi_driver *driver);
 	module_driver(__mipi_dsi_driver, mipi_dsi_driver_register, \
 			mipi_dsi_driver_unregister)
 
+#if IS_ENABLED(CONFIG_TINYDRM_DSI)
+int mipi_dsi_register_tiny_driver(struct mipi_dsi_device *dsi);
+#else
+static inline int mipi_dsi_register_tiny_driver(struct mipi_dsi_device *dsi)
+{
+	return 0;
+}
+#endif
+
+static inline int mipi_dsi_maybe_register_tiny_driver(struct mipi_dsi_device *dsi)
+{
+	/* Register the TinyDRM DSI/DBI driver if the panel has no controller */
+	if (!of_graph_get_port_by_id(dsi->dev.of_node, 0))
+		return mipi_dsi_register_tiny_driver(dsi);
+
+	return 0;
+}
+
 #endif /* __DRM_MIPI_DSI__ */
-- 
2.27.0

[PATCH 6/6] drm/panel: Add Ilitek ILI9341 DBI panel driver

[Paul Cercueil]

This driver is for the Ilitek ILI9341 based YX240QV29-T 2.4" 240x320 TFT
LCD panel from Adafruit.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/panel/Kconfig                |   9 +
 drivers/gpu/drm/panel/Makefile               |   1 +
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 345 +++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 6a8a51a702d8..132a42a81895 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -105,6 +105,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 320x240 QVGA panels"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for Ilitek IL9341
+	  QVGA (320x240) RGB, YUV and ITU-T BT.656 panels.
+
 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 a0516ced87db..ad5e6089c3ef 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_DRM_PANEL_ELIDA_KD35T133) += panel-elida-kd35t133.o
 obj-$(CONFIG_DRM_PANEL_FEIXIN_K101_IM2BA02) += panel-feixin-k101-im2ba02.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..3af6628639d6
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -0,0 +1,345 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * DRM driver for Ilitek ILI9341 panels
+ *
+ * Copyright 2018 David Lechner <david@lechnology.com>
+ * Copyright 2020 Paul Cercueil <paul@crapouillou.net>
+ *
+ * Based on mi0283qt.c:
+ * Copyright 2016 Noralf Trønnes
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/dma-buf.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_graph.h>
+#include <linux/property.h>
+#include <drm/drm_atomic_helper.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.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)
+
+struct ili9341_pdata {
+	struct drm_display_mode mode;
+	unsigned int width_mm;
+	unsigned int height_mm;
+	unsigned int bus_type;
+	unsigned int lanes;
+};
+
+struct ili9341 {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+	const struct ili9341_pdata *pdata;
+
+	struct gpio_desc	*reset_gpiod;
+	struct backlight_device *backlight;
+	u32 rotation;
+};
+
+#define mipi_dcs_command(dsi, cmd, seq...) \
+({ \
+	u8 d[] = { seq }; \
+	mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d)); \
+})
+
+static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
+{
+	return container_of(panel, struct ili9341, panel);
+}
+
+static int ili9341_prepare(struct drm_panel *panel)
+{
+	struct ili9341 *priv = panel_to_ili9341(panel);
+	struct mipi_dsi_device *dsi = priv->dsi;
+	u8 addr_mode;
+	int ret;
+
+	gpiod_set_value_cansleep(priv->reset_gpiod, 0);
+	usleep_range(20, 1000);
+	gpiod_set_value_cansleep(priv->reset_gpiod, 1);
+	msleep(120);
+
+	ret = mipi_dcs_command(dsi, MIPI_DCS_SOFT_RESET);
+	if (ret) {
+		dev_err(panel->dev, "Failed to send reset command: %d\n", ret);
+		return ret;
+	}
+
+	/* Wait 5ms after soft reset per MIPI DCS spec */
+	usleep_range(5000, 20000);
+
+	mipi_dcs_command(dsi, MIPI_DCS_SET_DISPLAY_OFF);
+
+	mipi_dcs_command(dsi, ILI9341_PWCTRLB, 0x00, 0xc1, 0x30);
+	mipi_dcs_command(dsi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
+	mipi_dcs_command(dsi, ILI9341_DTCTRLA, 0x85, 0x00, 0x78);
+	mipi_dcs_command(dsi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
+	mipi_dcs_command(dsi, ILI9341_PUMPCTRL, 0x20);
+	mipi_dcs_command(dsi, ILI9341_DTCTRLB, 0x00, 0x00);
+
+	/* Power Control */
+	mipi_dcs_command(dsi, ILI9341_PWCTRL1, 0x23);
+	mipi_dcs_command(dsi, ILI9341_PWCTRL2, 0x10);
+	/* VCOM */
+	mipi_dcs_command(dsi, ILI9341_VMCTRL1, 0x3e, 0x28);
+	mipi_dcs_command(dsi, ILI9341_VMCTRL2, 0x86);
+
+	/* Memory Access Control */
+	mipi_dcs_command(dsi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT);
+
+	/* Frame Rate */
+	mipi_dcs_command(dsi, ILI9341_FRMCTR1, 0x00, 0x1b);
+
+	/* Gamma */
+	mipi_dcs_command(dsi, ILI9341_EN3GAM, 0x00);
+	mipi_dcs_command(dsi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
+	mipi_dcs_command(dsi, ILI9341_PGAMCTRL,
+			 0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
+			 0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
+	mipi_dcs_command(dsi, ILI9341_NGAMCTRL,
+			 0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
+			 0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
+
+	/* DDRAM */
+	mipi_dcs_command(dsi, ILI9341_ETMOD, 0x07);
+
+	/* Display */
+	mipi_dcs_command(dsi, ILI9341_DISCTRL, 0x08, 0x82, 0x27, 0x00);
+	mipi_dcs_command(dsi, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(100);
+
+	mipi_dcs_command(dsi, MIPI_DCS_SET_DISPLAY_ON);
+	msleep(100);
+
+	switch (priv->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_dcs_command(dsi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+
+	return 0;
+}
+
+static int ili9341_enable(struct drm_panel *panel)
+{
+	struct ili9341 *priv = panel_to_ili9341(panel);
+
+	backlight_enable(priv->backlight);
+
+	return 0;
+}
+
+static int ili9341_disable(struct drm_panel *panel)
+{
+	struct ili9341 *priv = panel_to_ili9341(panel);
+
+	backlight_disable(priv->backlight);
+
+	return 0;
+}
+
+static int ili9341_unprepare(struct drm_panel *panel)
+{
+	struct ili9341 *priv = panel_to_ili9341(panel);
+
+	mipi_dcs_command(priv->dsi, MIPI_DCS_SET_DISPLAY_OFF);
+
+	return 0;
+}
+
+static int ili9341_get_modes(struct drm_panel *panel,
+			     struct drm_connector *connector)
+{
+	struct ili9341 *priv = panel_to_ili9341(panel);
+	struct drm_display_mode *mode;
+	u32 format = MEDIA_BUS_FMT_RGB565_1X16;
+
+	mode = drm_mode_duplicate(connector->dev, &priv->pdata->mode);
+	if (!mode) {
+		dev_err(panel->dev, "failed to add mode %ux%u\n",
+			priv->pdata->mode.hdisplay, priv->pdata->mode.vdisplay);
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	connector->display_info.bpc = 8;
+	connector->display_info.width_mm = priv->pdata->width_mm;
+	connector->display_info.height_mm = priv->pdata->height_mm;
+
+	drm_display_info_set_bus_formats(&connector->display_info, &format, 1);
+	connector->display_info.bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE;
+
+	return 1;
+}
+
+static const struct drm_panel_funcs ili9341_funcs = {
+	.prepare	= ili9341_prepare,
+	.unprepare	= ili9341_unprepare,
+	.enable		= ili9341_enable,
+	.disable	= ili9341_disable,
+	.get_modes	= ili9341_get_modes,
+};
+
+static int ili9341_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct ili9341 *priv;
+	int ret;
+
+	/* See comment for mipi_dbi_spi_init() */
+	if (!dev->coherent_dma_mask) {
+		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
+		if (ret) {
+			dev_warn(dev, "Failed to set dma mask %d\n", ret);
+			return ret;
+		}
+	}
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mipi_dsi_set_drvdata(dsi, priv);
+	priv->dsi = dsi;
+
+	device_property_read_u32(dev, "rotation", &priv->rotation);
+
+	priv->pdata = device_get_match_data(dev);
+	if (!priv->pdata)
+		return -EINVAL;
+
+	drm_panel_init(&priv->panel, dev, &ili9341_funcs,
+		       DRM_MODE_CONNECTOR_DPI);
+
+	priv->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->reset_gpiod)) {
+		dev_err(dev, "Couldn't get our reset GPIO\n");
+		return PTR_ERR(priv->reset_gpiod);
+	}
+
+	priv->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(priv->backlight)) {
+		ret = PTR_ERR(priv->backlight);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get backlight handle\n");
+		return ret;
+	}
+
+	ret = drm_panel_add(&priv->panel);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register panel\n");
+		return ret;
+	}
+
+	dsi->bus_type = priv->pdata->bus_type;
+	dsi->lanes = priv->pdata->lanes;
+	dsi->format = MIPI_DSI_FMT_RGB565;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret) {
+		dev_err(dev, "Failed to attach DSI panel\n");
+		goto err_panel_remove;
+	}
+
+	ret = mipi_dsi_maybe_register_tiny_driver(dsi);
+	if (ret) {
+		dev_err(dev, "Failed to init TinyDRM driver\n");
+		goto err_mipi_dsi_detach;
+	}
+
+	return 0;
+
+err_mipi_dsi_detach:
+	mipi_dsi_detach(dsi);
+err_panel_remove:
+	drm_panel_remove(&priv->panel);
+	return ret;
+}
+
+static int ili9341_remove(struct mipi_dsi_device *dsi)
+{
+	struct ili9341 *priv = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_panel_remove(&priv->panel);
+
+	if (priv->backlight)
+		put_device(&priv->backlight->dev);
+
+	return 0;
+}
+
+static const struct ili9341_pdata yx240qv29_pdata = {
+	.mode = { DRM_SIMPLE_MODE(240, 320, 37, 49) },
+	.width_mm = 0, // TODO
+	.height_mm = 0, // TODO
+	.bus_type = MIPI_DEVICE_TYPE_DBI_SPI_MODE3,
+	.lanes = 1,
+};
+
+static const struct of_device_id ili9341_of_match[] = {
+	{ .compatible = "adafruit,yx240qv29", .data = &yx240qv29_pdata },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ili9341_of_match);
+
+static struct mipi_dsi_driver ili9341_dsi_driver = {
+	.probe		= ili9341_probe,
+	.remove		= ili9341_remove,
+	.driver = {
+		.name		= "ili9341-dsi",
+		.of_match_table	= ili9341_of_match,
+	},
+};
+module_mipi_dsi_driver(ili9341_dsi_driver);
+
+MODULE_DESCRIPTION("Ilitek ILI9341 DRM panel driver");
+MODULE_AUTHOR("David Lechner <david@lechnology.com>");
+MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
+MODULE_LICENSE("GPL");
-- 
2.27.0

Re: [PATCH 2/6] drm: dsi: Let host and device specify supported bus

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Jul 27, 2020 at 06:46:09PM +0200, Paul Cercueil wrote:
> The current MIPI DSI framework can very well be used to support MIPI DBI
> panels. In order to add support for the various bus types supported by
> DBI, the DRM panel drivers should specify the bus type they will use,
> and the DSI host drivers should specify the bus types they are
> compatible with.
> 
> The DSI host driver can then use the information provided by the DBI/DSI
> device driver, such as the bus type and the number of lanes, to
> configure its hardware properly.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c |  9 +++++++++
>  include/drm/drm_mipi_dsi.h     | 12 ++++++++++++

Use the mipi_dsi_* API for DBI panels will be very confusing to say the
least. Can we consider a global name refactoring to clarify all this ?

>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 5dd475e82995..11ef885de765 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -281,6 +281,9 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>  {
>  	struct device_node *node;
>  
> +	if (WARN_ON_ONCE(!host->bus_types))
> +		host->bus_types = MIPI_DEVICE_TYPE_DSI;
> +
>  	for_each_available_child_of_node(host->dev->of_node, node) {
>  		/* skip nodes without reg property */
>  		if (!of_find_property(node, "reg", NULL))
> @@ -323,6 +326,12 @@ int mipi_dsi_attach(struct mipi_dsi_device *dsi)
>  {
>  	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>  
> +	if (WARN_ON_ONCE(!dsi->bus_type))
> +		dsi->bus_type = MIPI_DEVICE_TYPE_DSI;
> +
> +	if (!(dsi->bus_type & dsi->host->bus_types))
> +		return -EINVAL;
> +
>  	if (!ops || !ops->attach)
>  		return -ENOSYS;
>  
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 360e6377e84b..65d2961fc054 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -63,6 +63,14 @@ struct mipi_dsi_packet {
>  int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
>  			   const struct mipi_dsi_msg *msg);
>  
> +/* MIPI bus types */
> +#define MIPI_DEVICE_TYPE_DSI		BIT(0)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE1	BIT(1)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE2	BIT(2)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE3	BIT(3)
> +#define MIPI_DEVICE_TYPE_DBI_M6800	BIT(4)
> +#define MIPI_DEVICE_TYPE_DBI_I8080	BIT(5)
> +
>  /**
>   * struct mipi_dsi_host_ops - DSI bus operations
>   * @attach: attach DSI device to DSI host
> @@ -94,11 +102,13 @@ struct mipi_dsi_host_ops {
>   * struct mipi_dsi_host - DSI host device
>   * @dev: driver model device node for this DSI host
>   * @ops: DSI host operations
> + * @bus_types: Bitmask of supported MIPI bus types
>   * @list: list management
>   */
>  struct mipi_dsi_host {
>  	struct device *dev;
>  	const struct mipi_dsi_host_ops *ops;
> +	unsigned int bus_types;
>  	struct list_head list;
>  };
>  
> @@ -162,6 +172,7 @@ struct mipi_dsi_device_info {
>   * @host: DSI host for this peripheral
>   * @dev: driver model device node for this peripheral
>   * @name: DSI peripheral chip type
> + * @bus_type: MIPI bus type (MIPI_DEVICE_TYPE_DSI/...)
>   * @channel: virtual channel assigned to the peripheral
>   * @format: pixel format for video mode
>   * @lanes: number of active data lanes
> @@ -178,6 +189,7 @@ struct mipi_dsi_device {
>  	struct device dev;
>  
>  	char name[DSI_DEV_NAME_SIZE];
> +	unsigned int bus_type;
>  	unsigned int channel;
>  	unsigned int lanes;
>  	enum mipi_dsi_pixel_format format;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] drm/bridge: Add SPI DBI host driver

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Jul 27, 2020 at 06:46:10PM +0200, Paul Cercueil wrote:
> This driver will register a DBI host driver for panels connected over
> SPI.
> 
> DBI types c1 and c3 are supported. C1 is a SPI protocol with 9 bits per
> word, with the data/command information in the 9th (MSB) bit. C3 is a
> SPI protocol with 8 bits per word, with the data/command information
> carried by a separate GPIO.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/Kconfig   |   8 +
>  drivers/gpu/drm/bridge/Makefile  |   1 +
>  drivers/gpu/drm/bridge/dbi-spi.c | 261 +++++++++++++++++++++++++++++++
>  3 files changed, 270 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/dbi-spi.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index c7f0dacfb57a..ed38366847c1 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -219,6 +219,14 @@ config DRM_TI_TPD12S015
>  	  Texas Instruments TPD12S015 HDMI level shifter and ESD protection
>  	  driver.
>  
> +config DRM_MIPI_DBI_SPI
> +	tristate "SPI host support for MIPI DBI"
> +	depends on OF && SPI
> +	select DRM_MIPI_DSI
> +	select DRM_MIPI_DBI
> +	help
> +	  Driver to support DBI over SPI.
> +
>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>  
>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 7d7c123a95e4..c2c522c2774f 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>  obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>  obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
> +obj-$(CONFIG_DRM_MIPI_DBI_SPI) += dbi-spi.o
>  obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>  
>  obj-y += analogix/
> diff --git a/drivers/gpu/drm/bridge/dbi-spi.c b/drivers/gpu/drm/bridge/dbi-spi.c
> new file mode 100644
> index 000000000000..1060b8f95fba
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/dbi-spi.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MIPI Display Bus Interface (DBI) SPI support
> + *
> + * Copyright 2016 Noralf Trønnes
> + * Copyright 2020 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_mipi_dsi.h>
> +
> +#include <video/mipi_display.h>
> +
> +struct dbi_spi {
> +	struct mipi_dsi_host host;
> +	struct mipi_dsi_host_ops host_ops;
> +
> +	struct spi_device *spi;
> +	struct gpio_desc *dc;
> +	struct mutex cmdlock;
> +
> +	unsigned int current_bus_type;
> +
> +	/**
> +	 * @tx_buf9: Buffer used for Option 1 9-bit conversion
> +	 */
> +	void *tx_buf9;
> +
> +	/**
> +	 * @tx_buf9_len: Size of tx_buf9.
> +	 */
> +	size_t tx_buf9_len;
> +};
> +
> +static inline struct dbi_spi *host_to_dbi_spi(struct mipi_dsi_host *host)
> +{
> +	return container_of(host, struct dbi_spi, host);
> +}
> +
> +static ssize_t dbi_spi1_transfer(struct mipi_dsi_host *host,
> +				 const struct mipi_dsi_msg *msg)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +	struct spi_device *spi = dbi->spi;
> +	struct spi_transfer tr = {
> +		.bits_per_word = 9,
> +	};
> +	const u8 *src8 = msg->tx_buf;
> +	struct spi_message m;
> +	size_t max_chunk, chunk;
> +	size_t len = msg->tx_len;
> +	bool cmd_byte = true;
> +	unsigned int i;
> +	u16 *dst16;
> +	int ret;
> +
> +	tr.speed_hz = mipi_dbi_spi_cmd_max_speed(spi, len);
> +	dst16 = dbi->tx_buf9;
> +
> +	max_chunk = min(dbi->tx_buf9_len / 2, len);
> +
> +	spi_message_init_with_transfers(&m, &tr, 1);
> +	tr.tx_buf = dst16;
> +
> +	while (len) {
> +		chunk = min(len, max_chunk);
> +
> +		for (i = 0; i < chunk; i++) {
> +			dst16[i] = *src8++;
> +
> +			/* Bit 9: 0 means command, 1 means data */
> +			if (!cmd_byte)
> +				dst16[i] |= BIT(9);
> +
> +			cmd_byte = false;
> +		}
> +
> +		tr.len = chunk * 2;
> +		len -= chunk;
> +
> +		ret = spi_sync(spi, &m);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t dbi_spi3_transfer(struct mipi_dsi_host *host,
> +				 const struct mipi_dsi_msg *msg)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +	struct spi_device *spi = dbi->spi;
> +	const u8 *buf = msg->tx_buf;
> +	unsigned int bpw = 8;
> +	u32 speed_hz;
> +	ssize_t ret;
> +
> +	/* for now we only support sending messages, not receiving */
> +	if (msg->rx_len)
> +		return -EINVAL;
> +
> +	gpiod_set_value_cansleep(dbi->dc, 0);
> +
> +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
> +	ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, buf, 1);
> +	if (ret || msg->tx_len == 1)
> +		return ret;
> +
> +	if (buf[0] == MIPI_DCS_WRITE_MEMORY_START)
> +		bpw = 16;
> +
> +	gpiod_set_value_cansleep(dbi->dc, 1);
> +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, msg->tx_len - 1);
> +
> +	ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw,
> +				    &buf[1], msg->tx_len - 1);
> +	if (ret)
> +		return ret;
> +
> +	return msg->tx_len;
> +}
> +
> +static ssize_t dbi_spi_transfer(struct mipi_dsi_host *host,
> +				const struct mipi_dsi_msg *msg)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +
> +	switch (dbi->current_bus_type) {
> +	case MIPI_DEVICE_TYPE_DBI_SPI_MODE1:
> +		return dbi_spi1_transfer(host, msg);
> +	case MIPI_DEVICE_TYPE_DBI_SPI_MODE3:
> +		return dbi_spi3_transfer(host, msg);
> +	default:
> +		dev_err(&dbi->spi->dev, "Unknown transfer type\n");
> +		return -EINVAL;
> +	}
> +}
> +
> +static int dbi_spi_attach(struct mipi_dsi_host *host,
> +			  struct mipi_dsi_device *dsi)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +
> +	dbi->current_bus_type = dsi->bus_type;
> +
> +	if (dbi->current_bus_type == MIPI_DEVICE_TYPE_DBI_SPI_MODE1) {
> +		dbi->tx_buf9_len = SZ_16K;
> +
> +		dbi->tx_buf9 = kmalloc(dbi->tx_buf9_len, GFP_KERNEL);
> +		if (!dbi->tx_buf9)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dbi_spi_detach(struct mipi_dsi_host *host,
> +			  struct mipi_dsi_device *dsi)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +
> +	kfree(dbi->tx_buf9);
> +	dbi->tx_buf9_len = 0;
> +
> +	return 0; /* Nothing to do */
> +}
> +
> +static void dbi_spi_host_unregister(void *d)
> +{
> +	mipi_dsi_host_unregister(d);
> +}
> +
> +static int dbi_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct mipi_dsi_device_info info = { };
> +	struct mipi_dsi_device *dsi;
> +	struct dbi_spi *dbi;
> +	int ret;
> +
> +	dbi = devm_kzalloc(dev, sizeof(*dbi), GFP_KERNEL);
> +	if (!dbi)
> +		return -ENOMEM;
> +
> +	dbi->host.dev = dev;
> +	dbi->host.ops = &dbi->host_ops;
> +	dbi->spi = spi;
> +	spi_set_drvdata(spi, dbi);
> +
> +	dbi->dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dbi->dc)) {
> +		dev_err(dev, "Failed to get gpio 'dc'\n");
> +		return PTR_ERR(dbi->dc);
> +	}
> +
> +	if (spi_is_bpw_supported(spi, 9))
> +		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE1;
> +	if (dbi->dc)
> +		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE3;
> +
> +	if (!dbi->host.bus_types) {
> +		dev_err(dev, "Neither Type1 nor Type3 are supported\n");
> +		return -EINVAL;
> +	}
> +
> +	dbi->host_ops.transfer = dbi_spi_transfer;
> +	dbi->host_ops.attach = dbi_spi_attach;
> +	dbi->host_ops.detach = dbi_spi_detach;
> +
> +	mutex_init(&dbi->cmdlock);
> +
> +	ret = mipi_dsi_host_register(&dbi->host);
> +	if (ret) {
> +		dev_err(dev, "Unable to register DSI host\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, dbi_spi_host_unregister, &dbi->host);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Register our own node as a MIPI DSI device.
> +	 * This ensures that the panel driver will be probed.
> +	 */
> +	info.channel = 0;
> +	info.node = of_node_get(dev->of_node);
> +
> +	dsi = mipi_dsi_device_register_full(&dbi->host, &info);

Two devices for the same OF node is asking for trouble :-S

Looking at the other patches in this series, it seems that what you need
from the DSI framework is DCS. I think we should then extract the DCS
support to mipi_dcs_* functions, with two different backends, one for
DSI and one for DBI. That way you will be able to support DBI panels
with a single driver instead of splutting the support between two
drivers (this one and the corresponding mipi_dsi_driver's).

> +	if (IS_ERR(dsi)) {
> +		dev_err(dev, "Failed to add DSI device\n");
> +		return PTR_ERR(dsi);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dbi_spi_of_match[] = {
> +	{ .compatible = "adafruit,yx240qv29" },
> +	{ .compatible = "leadtek,ltk035c5444t-spi" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, dbi_spi_of_match);
> +
> +static struct spi_driver dbi_spi_driver = {
> +	.driver = {
> +		.name = "dbi-spi",
> +		.of_match_table = dbi_spi_of_match,
> +	},
> +	.probe = dbi_spi_probe,
> +};
> +module_spi_driver(dbi_spi_driver);
> +
> +MODULE_DESCRIPTION("DBI SPI bus driver");
> +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] drm/bridge: Add SPI DBI host driver

[Paul Cercueil]

Hi Laurent,

Le lun. 27 juil. 2020 à 20:06, Laurent Pinchart 
<laurent.pinchart@ideasonboard.com> a écrit :
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Jul 27, 2020 at 06:46:10PM +0200, Paul Cercueil wrote:
>>  This driver will register a DBI host driver for panels connected 
>> over
>>  SPI.
>> 
>>  DBI types c1 and c3 are supported. C1 is a SPI protocol with 9 bits 
>> per
>>  word, with the data/command information in the 9th (MSB) bit. C3 is 
>> a
>>  SPI protocol with 8 bits per word, with the data/command information
>>  carried by a separate GPIO.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/gpu/drm/bridge/Kconfig   |   8 +
>>   drivers/gpu/drm/bridge/Makefile  |   1 +
>>   drivers/gpu/drm/bridge/dbi-spi.c | 261 
>> +++++++++++++++++++++++++++++++
>>   3 files changed, 270 insertions(+)
>>   create mode 100644 drivers/gpu/drm/bridge/dbi-spi.c
>> 
>>  diff --git a/drivers/gpu/drm/bridge/Kconfig 
>> b/drivers/gpu/drm/bridge/Kconfig
>>  index c7f0dacfb57a..ed38366847c1 100644
>>  --- a/drivers/gpu/drm/bridge/Kconfig
>>  +++ b/drivers/gpu/drm/bridge/Kconfig
>>  @@ -219,6 +219,14 @@ config DRM_TI_TPD12S015
>>   	  Texas Instruments TPD12S015 HDMI level shifter and ESD 
>> protection
>>   	  driver.
>> 
>>  +config DRM_MIPI_DBI_SPI
>>  +	tristate "SPI host support for MIPI DBI"
>>  +	depends on OF && SPI
>>  +	select DRM_MIPI_DSI
>>  +	select DRM_MIPI_DBI
>>  +	help
>>  +	  Driver to support DBI over SPI.
>>  +
>>   source "drivers/gpu/drm/bridge/analogix/Kconfig"
>> 
>>   source "drivers/gpu/drm/bridge/adv7511/Kconfig"
>>  diff --git a/drivers/gpu/drm/bridge/Makefile 
>> b/drivers/gpu/drm/bridge/Makefile
>>  index 7d7c123a95e4..c2c522c2774f 100644
>>  --- a/drivers/gpu/drm/bridge/Makefile
>>  +++ b/drivers/gpu/drm/bridge/Makefile
>>  @@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>   obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>>   obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>>   obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
>>  +obj-$(CONFIG_DRM_MIPI_DBI_SPI) += dbi-spi.o
>>   obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>> 
>>   obj-y += analogix/
>>  diff --git a/drivers/gpu/drm/bridge/dbi-spi.c 
>> b/drivers/gpu/drm/bridge/dbi-spi.c
>>  new file mode 100644
>>  index 000000000000..1060b8f95fba
>>  --- /dev/null
>>  +++ b/drivers/gpu/drm/bridge/dbi-spi.c
>>  @@ -0,0 +1,261 @@
>>  +// SPDX-License-Identifier: GPL-2.0-or-later
>>  +/*
>>  + * MIPI Display Bus Interface (DBI) SPI support
>>  + *
>>  + * Copyright 2016 Noralf Trønnes
>>  + * Copyright 2020 Paul Cercueil <paul@crapouillou.net>
>>  + */
>>  +
>>  +#include <linux/gpio/consumer.h>
>>  +#include <linux/module.h>
>>  +#include <linux/spi/spi.h>
>>  +
>>  +#include <drm/drm_mipi_dbi.h>
>>  +#include <drm/drm_mipi_dsi.h>
>>  +
>>  +#include <video/mipi_display.h>
>>  +
>>  +struct dbi_spi {
>>  +	struct mipi_dsi_host host;
>>  +	struct mipi_dsi_host_ops host_ops;
>>  +
>>  +	struct spi_device *spi;
>>  +	struct gpio_desc *dc;
>>  +	struct mutex cmdlock;
>>  +
>>  +	unsigned int current_bus_type;
>>  +
>>  +	/**
>>  +	 * @tx_buf9: Buffer used for Option 1 9-bit conversion
>>  +	 */
>>  +	void *tx_buf9;
>>  +
>>  +	/**
>>  +	 * @tx_buf9_len: Size of tx_buf9.
>>  +	 */
>>  +	size_t tx_buf9_len;
>>  +};
>>  +
>>  +static inline struct dbi_spi *host_to_dbi_spi(struct mipi_dsi_host 
>> *host)
>>  +{
>>  +	return container_of(host, struct dbi_spi, host);
>>  +}
>>  +
>>  +static ssize_t dbi_spi1_transfer(struct mipi_dsi_host *host,
>>  +				 const struct mipi_dsi_msg *msg)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +	struct spi_device *spi = dbi->spi;
>>  +	struct spi_transfer tr = {
>>  +		.bits_per_word = 9,
>>  +	};
>>  +	const u8 *src8 = msg->tx_buf;
>>  +	struct spi_message m;
>>  +	size_t max_chunk, chunk;
>>  +	size_t len = msg->tx_len;
>>  +	bool cmd_byte = true;
>>  +	unsigned int i;
>>  +	u16 *dst16;
>>  +	int ret;
>>  +
>>  +	tr.speed_hz = mipi_dbi_spi_cmd_max_speed(spi, len);
>>  +	dst16 = dbi->tx_buf9;
>>  +
>>  +	max_chunk = min(dbi->tx_buf9_len / 2, len);
>>  +
>>  +	spi_message_init_with_transfers(&m, &tr, 1);
>>  +	tr.tx_buf = dst16;
>>  +
>>  +	while (len) {
>>  +		chunk = min(len, max_chunk);
>>  +
>>  +		for (i = 0; i < chunk; i++) {
>>  +			dst16[i] = *src8++;
>>  +
>>  +			/* Bit 9: 0 means command, 1 means data */
>>  +			if (!cmd_byte)
>>  +				dst16[i] |= BIT(9);
>>  +
>>  +			cmd_byte = false;
>>  +		}
>>  +
>>  +		tr.len = chunk * 2;
>>  +		len -= chunk;
>>  +
>>  +		ret = spi_sync(spi, &m);
>>  +		if (ret)
>>  +			return ret;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static ssize_t dbi_spi3_transfer(struct mipi_dsi_host *host,
>>  +				 const struct mipi_dsi_msg *msg)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +	struct spi_device *spi = dbi->spi;
>>  +	const u8 *buf = msg->tx_buf;
>>  +	unsigned int bpw = 8;
>>  +	u32 speed_hz;
>>  +	ssize_t ret;
>>  +
>>  +	/* for now we only support sending messages, not receiving */
>>  +	if (msg->rx_len)
>>  +		return -EINVAL;
>>  +
>>  +	gpiod_set_value_cansleep(dbi->dc, 0);
>>  +
>>  +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
>>  +	ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, buf, 1);
>>  +	if (ret || msg->tx_len == 1)
>>  +		return ret;
>>  +
>>  +	if (buf[0] == MIPI_DCS_WRITE_MEMORY_START)
>>  +		bpw = 16;
>>  +
>>  +	gpiod_set_value_cansleep(dbi->dc, 1);
>>  +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, msg->tx_len - 1);
>>  +
>>  +	ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw,
>>  +				    &buf[1], msg->tx_len - 1);
>>  +	if (ret)
>>  +		return ret;
>>  +
>>  +	return msg->tx_len;
>>  +}
>>  +
>>  +static ssize_t dbi_spi_transfer(struct mipi_dsi_host *host,
>>  +				const struct mipi_dsi_msg *msg)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +
>>  +	switch (dbi->current_bus_type) {
>>  +	case MIPI_DEVICE_TYPE_DBI_SPI_MODE1:
>>  +		return dbi_spi1_transfer(host, msg);
>>  +	case MIPI_DEVICE_TYPE_DBI_SPI_MODE3:
>>  +		return dbi_spi3_transfer(host, msg);
>>  +	default:
>>  +		dev_err(&dbi->spi->dev, "Unknown transfer type\n");
>>  +		return -EINVAL;
>>  +	}
>>  +}
>>  +
>>  +static int dbi_spi_attach(struct mipi_dsi_host *host,
>>  +			  struct mipi_dsi_device *dsi)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +
>>  +	dbi->current_bus_type = dsi->bus_type;
>>  +
>>  +	if (dbi->current_bus_type == MIPI_DEVICE_TYPE_DBI_SPI_MODE1) {
>>  +		dbi->tx_buf9_len = SZ_16K;
>>  +
>>  +		dbi->tx_buf9 = kmalloc(dbi->tx_buf9_len, GFP_KERNEL);
>>  +		if (!dbi->tx_buf9)
>>  +			return -ENOMEM;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int dbi_spi_detach(struct mipi_dsi_host *host,
>>  +			  struct mipi_dsi_device *dsi)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +
>>  +	kfree(dbi->tx_buf9);
>>  +	dbi->tx_buf9_len = 0;
>>  +
>>  +	return 0; /* Nothing to do */
>>  +}
>>  +
>>  +static void dbi_spi_host_unregister(void *d)
>>  +{
>>  +	mipi_dsi_host_unregister(d);
>>  +}
>>  +
>>  +static int dbi_spi_probe(struct spi_device *spi)
>>  +{
>>  +	struct device *dev = &spi->dev;
>>  +	struct mipi_dsi_device_info info = { };
>>  +	struct mipi_dsi_device *dsi;
>>  +	struct dbi_spi *dbi;
>>  +	int ret;
>>  +
>>  +	dbi = devm_kzalloc(dev, sizeof(*dbi), GFP_KERNEL);
>>  +	if (!dbi)
>>  +		return -ENOMEM;
>>  +
>>  +	dbi->host.dev = dev;
>>  +	dbi->host.ops = &dbi->host_ops;
>>  +	dbi->spi = spi;
>>  +	spi_set_drvdata(spi, dbi);
>>  +
>>  +	dbi->dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
>>  +	if (IS_ERR(dbi->dc)) {
>>  +		dev_err(dev, "Failed to get gpio 'dc'\n");
>>  +		return PTR_ERR(dbi->dc);
>>  +	}
>>  +
>>  +	if (spi_is_bpw_supported(spi, 9))
>>  +		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE1;
>>  +	if (dbi->dc)
>>  +		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE3;
>>  +
>>  +	if (!dbi->host.bus_types) {
>>  +		dev_err(dev, "Neither Type1 nor Type3 are supported\n");
>>  +		return -EINVAL;
>>  +	}
>>  +
>>  +	dbi->host_ops.transfer = dbi_spi_transfer;
>>  +	dbi->host_ops.attach = dbi_spi_attach;
>>  +	dbi->host_ops.detach = dbi_spi_detach;
>>  +
>>  +	mutex_init(&dbi->cmdlock);
>>  +
>>  +	ret = mipi_dsi_host_register(&dbi->host);
>>  +	if (ret) {
>>  +		dev_err(dev, "Unable to register DSI host\n");
>>  +		return ret;
>>  +	}
>>  +
>>  +	ret = devm_add_action_or_reset(dev, dbi_spi_host_unregister, 
>> &dbi->host);
>>  +	if (ret)
>>  +		return ret;
>>  +
>>  +	/*
>>  +	 * Register our own node as a MIPI DSI device.
>>  +	 * This ensures that the panel driver will be probed.
>>  +	 */
>>  +	info.channel = 0;
>>  +	info.node = of_node_get(dev->of_node);
>>  +
>>  +	dsi = mipi_dsi_device_register_full(&dbi->host, &info);
> 
> Two devices for the same OF node is asking for trouble :-S

There is nothing that prevents you from doing that, and it's been done 
before (on Ingenic SoCs we have one DT node shared by 3 drivers).

> Looking at the other patches in this series, it seems that what you 
> need
> from the DSI framework is DCS. I think we should then extract the DCS
> support to mipi_dcs_* functions, with two different backends, one for
> DSI and one for DBI. That way you will be able to support DBI panels
> with a single driver instead of splutting the support between two
> drivers (this one and the corresponding mipi_dsi_driver's).

Then that would be five different backends, one for DSI, one for 
DBI/i8080, one for DBI/m68k, one for DBI/spi9, one for DBI/spi+gpio. 
Without the SPI/DBI bridge, your panel driver then has to support 
probing from two different buses, as a mipi_dsi_driver, and as a 
mipi_spi_driver, if you want to support both interfaces. That moves a 
lot of the complexity to the panel driver itself. Besides, to transform 
the spi_device to a mipi_dsi_device that the driver manipulates, 
wouldn't the shared code need to register a new DSI device with a 
SPI-to-DBI host anyway? The only difference is that it wouldn't be done 
in a separate driver.

So I fail to see how that would work better. A shared node may look 
scary but I think it's the cleanest solution.

As for backends: Having mipi_dcs_* functions with different backends 
wouldn't really work either, because that assumes that DSI and DBI are 
completely different buses. But one controller can very well support 
both DSI and DBI/i8080, and as such one "DCS host driver" should be 
able to support both. So the distinction should be made within the host 
drivers, which should present a unified DCS API. This is basically what 
this patchset does but with the current DSI API as the "unified DCS 
API".

-Paul

> 
>>  +	if (IS_ERR(dsi)) {
>>  +		dev_err(dev, "Failed to add DSI device\n");
>>  +		return PTR_ERR(dsi);
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct of_device_id dbi_spi_of_match[] = {
>>  +	{ .compatible = "adafruit,yx240qv29" },
>>  +	{ .compatible = "leadtek,ltk035c5444t-spi" },
>>  +	{ }
>>  +};
>>  +MODULE_DEVICE_TABLE(of, dbi_spi_of_match);
>>  +
>>  +static struct spi_driver dbi_spi_driver = {
>>  +	.driver = {
>>  +		.name = "dbi-spi",
>>  +		.of_match_table = dbi_spi_of_match,
>>  +	},
>>  +	.probe = dbi_spi_probe,
>>  +};
>>  +module_spi_driver(dbi_spi_driver);
>>  +
>>  +MODULE_DESCRIPTION("DBI SPI bus driver");
>>  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>  +MODULE_LICENSE("GPL");
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 2/6] drm: dsi: Let host and device specify supported bus

[Paul Cercueil]

Hi Laurent,

Le lun. 27 juil. 2020 à 20:02, Laurent Pinchart 
<laurent.pinchart@ideasonboard.com> a écrit :
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Jul 27, 2020 at 06:46:09PM +0200, Paul Cercueil wrote:
>>  The current MIPI DSI framework can very well be used to support 
>> MIPI DBI
>>  panels. In order to add support for the various bus types supported 
>> by
>>  DBI, the DRM panel drivers should specify the bus type they will 
>> use,
>>  and the DSI host drivers should specify the bus types they are
>>  compatible with.
>> 
>>  The DSI host driver can then use the information provided by the 
>> DBI/DSI
>>  device driver, such as the bus type and the number of lanes, to
>>  configure its hardware properly.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/gpu/drm/drm_mipi_dsi.c |  9 +++++++++
>>   include/drm/drm_mipi_dsi.h     | 12 ++++++++++++
> 
> Use the mipi_dsi_* API for DBI panels will be very confusing to say 
> the
> least. Can we consider a global name refactoring to clarify all this ?

I was thinking that this could be done when the code is cleaned up and 
drivers/gpu/drm/drm_mipi_dbi.c is removed. I'm scared of tree-wide 
patchsets.

-Paul

> 
>>   2 files changed, 21 insertions(+)
>> 
>>  diff --git a/drivers/gpu/drm/drm_mipi_dsi.c 
>> b/drivers/gpu/drm/drm_mipi_dsi.c
>>  index 5dd475e82995..11ef885de765 100644
>>  --- a/drivers/gpu/drm/drm_mipi_dsi.c
>>  +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>>  @@ -281,6 +281,9 @@ int mipi_dsi_host_register(struct mipi_dsi_host 
>> *host)
>>   {
>>   	struct device_node *node;
>> 
>>  +	if (WARN_ON_ONCE(!host->bus_types))
>>  +		host->bus_types = MIPI_DEVICE_TYPE_DSI;
>>  +
>>   	for_each_available_child_of_node(host->dev->of_node, node) {
>>   		/* skip nodes without reg property */
>>   		if (!of_find_property(node, "reg", NULL))
>>  @@ -323,6 +326,12 @@ int mipi_dsi_attach(struct mipi_dsi_device 
>> *dsi)
>>   {
>>   	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>> 
>>  +	if (WARN_ON_ONCE(!dsi->bus_type))
>>  +		dsi->bus_type = MIPI_DEVICE_TYPE_DSI;
>>  +
>>  +	if (!(dsi->bus_type & dsi->host->bus_types))
>>  +		return -EINVAL;
>>  +
>>   	if (!ops || !ops->attach)
>>   		return -ENOSYS;
>> 
>>  diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>  index 360e6377e84b..65d2961fc054 100644
>>  --- a/include/drm/drm_mipi_dsi.h
>>  +++ b/include/drm/drm_mipi_dsi.h
>>  @@ -63,6 +63,14 @@ struct mipi_dsi_packet {
>>   int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
>>   			   const struct mipi_dsi_msg *msg);
>> 
>>  +/* MIPI bus types */
>>  +#define MIPI_DEVICE_TYPE_DSI		BIT(0)
>>  +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE1	BIT(1)
>>  +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE2	BIT(2)
>>  +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE3	BIT(3)
>>  +#define MIPI_DEVICE_TYPE_DBI_M6800	BIT(4)
>>  +#define MIPI_DEVICE_TYPE_DBI_I8080	BIT(5)
>>  +
>>   /**
>>    * struct mipi_dsi_host_ops - DSI bus operations
>>    * @attach: attach DSI device to DSI host
>>  @@ -94,11 +102,13 @@ struct mipi_dsi_host_ops {
>>    * struct mipi_dsi_host - DSI host device
>>    * @dev: driver model device node for this DSI host
>>    * @ops: DSI host operations
>>  + * @bus_types: Bitmask of supported MIPI bus types
>>    * @list: list management
>>    */
>>   struct mipi_dsi_host {
>>   	struct device *dev;
>>   	const struct mipi_dsi_host_ops *ops;
>>  +	unsigned int bus_types;
>>   	struct list_head list;
>>   };
>> 
>>  @@ -162,6 +172,7 @@ struct mipi_dsi_device_info {
>>    * @host: DSI host for this peripheral
>>    * @dev: driver model device node for this peripheral
>>    * @name: DSI peripheral chip type
>>  + * @bus_type: MIPI bus type (MIPI_DEVICE_TYPE_DSI/...)
>>    * @channel: virtual channel assigned to the peripheral
>>    * @format: pixel format for video mode
>>    * @lanes: number of active data lanes
>>  @@ -178,6 +189,7 @@ struct mipi_dsi_device {
>>   	struct device dev;
>> 
>>   	char name[DSI_DEV_NAME_SIZE];
>>  +	unsigned int bus_type;
>>   	unsigned int channel;
>>   	unsigned int lanes;
>>   	enum mipi_dsi_pixel_format format;
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 1/6] dt-bindings: display: Document NewVision NV3052C DT node

[Sam Ravnborg]

Hi Paul.

On Mon, Jul 27, 2020 at 06:46:08PM +0200, Paul Cercueil wrote:
> Add documentation for the Device Tree node for LCD panels based on the
> NewVision NV3052C controller.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Very happy to see work on RG-350 :-)
Some feedback below.

	Sam

> ---
>  .../display/panel/newvision,nv3052c.yaml      | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml b/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
> new file mode 100644
> index 000000000000..751a28800fc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/newvision,nv3052c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NewVision NV3052C TFT LCD panel driver with SPI control bus
> +
> +maintainers:
> +  - Paul Cercueil <paul@crapouillou.net>
> +
> +description: |
> +  This is a driver for 320x240 TFT panels,
The binding describes the HW, not the driver. So please re-phrase this
part.

This datasheet: https://www.phoenixdisplay.com/wp-content/uploads/2019/05/NV3052C-Datasheet-V0.2.pdf
tells that the driver supports additional resoltions.
I guess the 320x240 resolution is limited to the leadtek panel.

> +  accepting a variety of input
> +  streams that get adapted and scaled to the panel. The panel output has
> +  960 TFT source driver pins and 240 TFT gate driver pins, VCOM, VCOML and
> +  VCOMH outputs.
> +
> +  The panel must obey the rules for a SPI slave device as specified in
> +  spi/spi-controller.yaml
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - leadtek,ltk035c5444t-spi
> +
> +      - const: newvision,nv3052c
> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios: true
> +  port: true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
Do the panel need any power?
I had expected to see a power-supply node as mandatory.

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      display@0 {
> +        compatible = "leadtek,ltk035c5444t-spi", "newvision,nv3052c";
> +        reg = <0>;
> +
> +        spi-max-frequency = <15000000>;
> +        spi-3wire;
> +        reset-gpios = <&gpe 2 GPIO_ACTIVE_LOW>;


> +        backlight = <&backlight>;
> +        power-supply = <&vcc>;
These would fail later due to "unevaluatedProperties: false".
Add them above like
  backlight: true
  power-supply: true

as done for reset-gpios for example.

> +
> +        port {
> +          panel_input: endpoint {
> +              remote-endpoint = <&panel_output>;
> +          };
> +        };
> +      };
> +    };
Personally I prefer 4 space indent. But there is no fixed rule (yet)
what to use.

> +
> +...
> -- 
> 2.27.0

Re: [PATCH 1/6] dt-bindings: display: Document NewVision NV3052C DT node

[Maarten ter Huurne]

On Monday, 27 July 2020 21:10:52 CEST Sam Ravnborg wrote:
> > +description: |
> > +  This is a driver for 320x240 TFT panels,
> 
> The binding describes the HW, not the driver. So please re-phrase this
> part.
> 
> This datasheet:
> https://www.phoenixdisplay.com/wp-content/uploads/2019/05/NV3052C-Dat
> asheet-V0.2.pdf tells that the driver supports additional resoltions.
> I guess the 320x240 resolution is limited to the leadtek panel.

The word "driver" is overloaded ;)

I guess "driver IC" would make it clearer.

Bye,
		Maarten

Re: [PATCH 2/6] drm: dsi: Let host and device specify supported bus

[Sam Ravnborg]

Hi Paul.

On Mon, Jul 27, 2020 at 06:46:09PM +0200, Paul Cercueil wrote:
> The current MIPI DSI framework can very well be used to support MIPI DBI
> panels. In order to add support for the various bus types supported by
> DBI, the DRM panel drivers should specify the bus type they will use,
> and the DSI host drivers should specify the bus types they are
> compatible with.
> 
> The DSI host driver can then use the information provided by the DBI/DSI
> device driver, such as the bus type and the number of lanes, to
> configure its hardware properly.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c |  9 +++++++++
>  include/drm/drm_mipi_dsi.h     | 12 ++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 5dd475e82995..11ef885de765 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -281,6 +281,9 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>  {
>  	struct device_node *node;
>  
> +	if (WARN_ON_ONCE(!host->bus_types))
> +		host->bus_types = MIPI_DEVICE_TYPE_DSI;
> +
So all 14 users need to specify bus_types.
Seems doable.

>  	for_each_available_child_of_node(host->dev->of_node, node) {
>  		/* skip nodes without reg property */
>  		if (!of_find_property(node, "reg", NULL))
> @@ -323,6 +326,12 @@ int mipi_dsi_attach(struct mipi_dsi_device *dsi)
>  {
>  	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>  
> +	if (WARN_ON_ONCE(!dsi->bus_type))
> +		dsi->bus_type = MIPI_DEVICE_TYPE_DSI;
We have ~50 users of mipi_dsi_attach() - doable. But a bit more work.

> +
> +	if (!(dsi->bus_type & dsi->host->bus_types))
> +		return -EINVAL;
> +
>  	if (!ops || !ops->attach)
>  		return -ENOSYS;
>  
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 360e6377e84b..65d2961fc054 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -63,6 +63,14 @@ struct mipi_dsi_packet {
>  int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
>  			   const struct mipi_dsi_msg *msg);
>  
> +/* MIPI bus types */
If you define this as an enum then kernel-doc syntax will be picked up.
See for example: enum drm_driver_feature

> +#define MIPI_DEVICE_TYPE_DSI		BIT(0)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE1	BIT(1)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE2	BIT(2)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE3	BIT(3)
> +#define MIPI_DEVICE_TYPE_DBI_M6800	BIT(4)
> +#define MIPI_DEVICE_TYPE_DBI_I8080	BIT(5)
> +
>  /**
>   * struct mipi_dsi_host_ops - DSI bus operations
>   * @attach: attach DSI device to DSI host
> @@ -94,11 +102,13 @@ struct mipi_dsi_host_ops {
>   * struct mipi_dsi_host - DSI host device
>   * @dev: driver model device node for this DSI host
>   * @ops: DSI host operations
> + * @bus_types: Bitmask of supported MIPI bus types
Please add some kind of reference to MIPI_DEVICE_TYPE_* - so the reader
knows for sure this is the bits used here.

>   * @list: list management
>   */
>  struct mipi_dsi_host {
>  	struct device *dev;
>  	const struct mipi_dsi_host_ops *ops;
> +	unsigned int bus_types;
Use u32. Shorter and we know this is 32 bits wide.

>  	struct list_head list;
>  };
>  
> @@ -162,6 +172,7 @@ struct mipi_dsi_device_info {
>   * @host: DSI host for this peripheral
>   * @dev: driver model device node for this peripheral
>   * @name: DSI peripheral chip type
> + * @bus_type: MIPI bus type (MIPI_DEVICE_TYPE_DSI/...)
>   * @channel: virtual channel assigned to the peripheral
>   * @format: pixel format for video mode
>   * @lanes: number of active data lanes
> @@ -178,6 +189,7 @@ struct mipi_dsi_device {
>  	struct device dev;
>  
>  	char name[DSI_DEV_NAME_SIZE];
> +	unsigned int bus_type;
Use u32.

>  	unsigned int channel;
>  	unsigned int lanes;
>  	enum mipi_dsi_pixel_format format;
> -- 
> 2.27.0

Re: [PATCH 3/6] drm/bridge: Add SPI DBI host driver

[Sam Ravnborg]

Hi Paul.

On Mon, Jul 27, 2020 at 06:46:10PM +0200, Paul Cercueil wrote:
> This driver will register a DBI host driver for panels connected over
> SPI.
So this is actually a MIPI DBI host driver.

I personally would love to have added mipi_ in the names - to make it
all more explicit.
But maybe that just because I get confused on all the acronyms.


Some details in the following. Will try to find some more time so I can
grasp the full picture. The following was just my low-level notes for
now.

	Sam
> 
> DBI types c1 and c3 are supported. C1 is a SPI protocol with 9 bits per
> word, with the data/command information in the 9th (MSB) bit. C3 is a
> SPI protocol with 8 bits per word, with the data/command information
> carried by a separate GPIO.

We did not have any define to distingush between DBI_C1 and DBI_c3:
+/* MIPI bus types */
+#define MIPI_DEVICE_TYPE_DSI           BIT(0)
+#define MIPI_DEVICE_TYPE_DBI_SPI_MODE1 BIT(1)
+#define MIPI_DEVICE_TYPE_DBI_SPI_MODE2 BIT(2)
+#define MIPI_DEVICE_TYPE_DBI_SPI_MODE3 BIT(3)
+#define MIPI_DEVICE_TYPE_DBI_M6800     BIT(4)
+#define MIPI_DEVICE_TYPE_DBI_I8080     BIT(5)

Is this on purpose?
I had assumed the host should tell what it supports and the device should
tell what it wanted.
So if the host did not support DBI_C3 and device wants it - then we
could give up early.

> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/Kconfig   |   8 +
>  drivers/gpu/drm/bridge/Makefile  |   1 +
>  drivers/gpu/drm/bridge/dbi-spi.c | 261 +++++++++++++++++++++++++++++++
This is no bridge driver - so does not belong in the bridge
directory.
gpu/drm/drm_mipi_dbi_spi.c?

>  3 files changed, 270 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/dbi-spi.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index c7f0dacfb57a..ed38366847c1 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -219,6 +219,14 @@ config DRM_TI_TPD12S015
>  	  Texas Instruments TPD12S015 HDMI level shifter and ESD protection
>  	  driver.
>  
> +config DRM_MIPI_DBI_SPI
> +	tristate "SPI host support for MIPI DBI"
> +	depends on OF && SPI
> +	select DRM_MIPI_DSI
> +	select DRM_MIPI_DBI
> +	help
> +	  Driver to support DBI over SPI.
> +
>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>  
>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 7d7c123a95e4..c2c522c2774f 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>  obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>  obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
> +obj-$(CONFIG_DRM_MIPI_DBI_SPI) += dbi-spi.o
mipi_dbi_spi.o would be nice...

>  obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>  
>  obj-y += analogix/
> diff --git a/drivers/gpu/drm/bridge/dbi-spi.c b/drivers/gpu/drm/bridge/dbi-spi.c
> new file mode 100644
> index 000000000000..1060b8f95fba
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/dbi-spi.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MIPI Display Bus Interface (DBI) SPI support
> + *
> + * Copyright 2016 Noralf Trønnes
> + * Copyright 2020 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_mipi_dsi.h>
> +
> +#include <video/mipi_display.h>
> +
> +struct dbi_spi {
> +	struct mipi_dsi_host host;
It is very confusing that the mipi_dbi_spi driver uses a dsi_host.
It clashes in my head - and then reviewing it not easy.

> +	struct mipi_dsi_host_ops host_ops;
const?
> +
> +	struct spi_device *spi;
> +	struct gpio_desc *dc;
> +	struct mutex cmdlock;
> +
> +	unsigned int current_bus_type;
> +
> +	/**
> +	 * @tx_buf9: Buffer used for Option 1 9-bit conversion
> +	 */
> +	void *tx_buf9;
> +
> +	/**
> +	 * @tx_buf9_len: Size of tx_buf9.
> +	 */
> +	size_t tx_buf9_len;
> +};
> +
> +static inline struct dbi_spi *host_to_dbi_spi(struct mipi_dsi_host *host)
> +{
> +	return container_of(host, struct dbi_spi, host);
> +}
> +
> +static ssize_t dbi_spi1_transfer(struct mipi_dsi_host *host,
> +				 const struct mipi_dsi_msg *msg)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +	struct spi_device *spi = dbi->spi;
> +	struct spi_transfer tr = {
> +		.bits_per_word = 9,
> +	};
> +	const u8 *src8 = msg->tx_buf;
> +	struct spi_message m;
> +	size_t max_chunk, chunk;
> +	size_t len = msg->tx_len;
> +	bool cmd_byte = true;
> +	unsigned int i;
> +	u16 *dst16;
> +	int ret;
> +
> +	tr.speed_hz = mipi_dbi_spi_cmd_max_speed(spi, len);
> +	dst16 = dbi->tx_buf9;
> +
> +	max_chunk = min(dbi->tx_buf9_len / 2, len);
Hmm, this looks not right. We limit the max_chunk to 8K here.
We learned the other day that we count in bytes.
OR did I miss something?

> +
> +	spi_message_init_with_transfers(&m, &tr, 1);
> +	tr.tx_buf = dst16;
> +
> +	while (len) {
> +		chunk = min(len, max_chunk);
> +
> +		for (i = 0; i < chunk; i++) {
> +			dst16[i] = *src8++;
> +
> +			/* Bit 9: 0 means command, 1 means data */
> +			if (!cmd_byte)
> +				dst16[i] |= BIT(9);
> +
> +			cmd_byte = false;
> +		}
> +
> +		tr.len = chunk * 2;
> +		len -= chunk;
> +
> +		ret = spi_sync(spi, &m);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t dbi_spi3_transfer(struct mipi_dsi_host *host,
> +				 const struct mipi_dsi_msg *msg)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +	struct spi_device *spi = dbi->spi;
> +	const u8 *buf = msg->tx_buf;
> +	unsigned int bpw = 8;
> +	u32 speed_hz;
> +	ssize_t ret;
> +
> +	/* for now we only support sending messages, not receiving */
> +	if (msg->rx_len)
> +		return -EINVAL;
> +
> +	gpiod_set_value_cansleep(dbi->dc, 0);
> +
> +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
> +	ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, buf, 1);
> +	if (ret || msg->tx_len == 1)
> +		return ret;
> +
> +	if (buf[0] == MIPI_DCS_WRITE_MEMORY_START)
> +		bpw = 16;
> +
> +	gpiod_set_value_cansleep(dbi->dc, 1);
> +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, msg->tx_len - 1);
> +
> +	ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw,
> +				    &buf[1], msg->tx_len - 1);
> +	if (ret)
> +		return ret;
> +
> +	return msg->tx_len;
> +}
> +
> +static ssize_t dbi_spi_transfer(struct mipi_dsi_host *host,
> +				const struct mipi_dsi_msg *msg)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +
> +	switch (dbi->current_bus_type) {
> +	case MIPI_DEVICE_TYPE_DBI_SPI_MODE1:
> +		return dbi_spi1_transfer(host, msg);
> +	case MIPI_DEVICE_TYPE_DBI_SPI_MODE3:
> +		return dbi_spi3_transfer(host, msg);
> +	default:
> +		dev_err(&dbi->spi->dev, "Unknown transfer type\n");
> +		return -EINVAL;
> +	}
> +}
> +
> +static int dbi_spi_attach(struct mipi_dsi_host *host,
> +			  struct mipi_dsi_device *dsi)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +
> +	dbi->current_bus_type = dsi->bus_type;
> +
> +	if (dbi->current_bus_type == MIPI_DEVICE_TYPE_DBI_SPI_MODE1) {
> +		dbi->tx_buf9_len = SZ_16K;
> +
> +		dbi->tx_buf9 = kmalloc(dbi->tx_buf9_len, GFP_KERNEL);
> +		if (!dbi->tx_buf9)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dbi_spi_detach(struct mipi_dsi_host *host,
> +			  struct mipi_dsi_device *dsi)
> +{
> +	struct dbi_spi *dbi = host_to_dbi_spi(host);
> +
> +	kfree(dbi->tx_buf9);
> +	dbi->tx_buf9_len = 0;
> +
> +	return 0; /* Nothing to do */
> +}
> +
> +static void dbi_spi_host_unregister(void *d)
> +{
> +	mipi_dsi_host_unregister(d);
> +}
> +
> +static int dbi_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct mipi_dsi_device_info info = { };
> +	struct mipi_dsi_device *dsi;
> +	struct dbi_spi *dbi;
> +	int ret;
> +
> +	dbi = devm_kzalloc(dev, sizeof(*dbi), GFP_KERNEL);
> +	if (!dbi)
> +		return -ENOMEM;
> +
> +	dbi->host.dev = dev;
> +	dbi->host.ops = &dbi->host_ops;
> +	dbi->spi = spi;
> +	spi_set_drvdata(spi, dbi);
> +
> +	dbi->dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dbi->dc)) {
> +		dev_err(dev, "Failed to get gpio 'dc'\n");
> +		return PTR_ERR(dbi->dc);
> +	}
> +
> +	if (spi_is_bpw_supported(spi, 9))
> +		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE1;
> +	if (dbi->dc)
> +		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE3;
> +
> +	if (!dbi->host.bus_types) {
> +		dev_err(dev, "Neither Type1 nor Type3 are supported\n");
> +		return -EINVAL;
> +	}
> +
> +	dbi->host_ops.transfer = dbi_spi_transfer;
> +	dbi->host_ops.attach = dbi_spi_attach;
> +	dbi->host_ops.detach = dbi_spi_detach;
> +
> +	mutex_init(&dbi->cmdlock);
> +
> +	ret = mipi_dsi_host_register(&dbi->host);
> +	if (ret) {
> +		dev_err(dev, "Unable to register DSI host\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, dbi_spi_host_unregister, &dbi->host);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Register our own node as a MIPI DSI device.
> +	 * This ensures that the panel driver will be probed.
> +	 */
> +	info.channel = 0;
> +	info.node = of_node_get(dev->of_node);
> +
> +	dsi = mipi_dsi_device_register_full(&dbi->host, &info);
> +	if (IS_ERR(dsi)) {
> +		dev_err(dev, "Failed to add DSI device\n");
> +		return PTR_ERR(dsi);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dbi_spi_of_match[] = {
> +	{ .compatible = "adafruit,yx240qv29" },
> +	{ .compatible = "leadtek,ltk035c5444t-spi" },
> +	{ }
Would it be better with a fall-back compatible like:
mipi,dbi-spi.
So the nodes must includes this compatible to be registered with
this driver?

> +};
> +MODULE_DEVICE_TABLE(of, dbi_spi_of_match);
> +
> +static struct spi_driver dbi_spi_driver = {
> +	.driver = {
> +		.name = "dbi-spi",
> +		.of_match_table = dbi_spi_of_match,
> +	},
> +	.probe = dbi_spi_probe,
> +};
> +module_spi_driver(dbi_spi_driver);
> +
> +MODULE_DESCRIPTION("DBI SPI bus driver");
> +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.27.0

Re: [PATCH 3/6] drm/bridge: Add SPI DBI host driver

[Paul Cercueil]

Hi Sam,

Le lun. 27 juil. 2020 à 22:31, Sam Ravnborg <sam@ravnborg.org> a 
écrit :
> Hi Paul.
> 
> On Mon, Jul 27, 2020 at 06:46:10PM +0200, Paul Cercueil wrote:
>>  This driver will register a DBI host driver for panels connected 
>> over
>>  SPI.
> So this is actually a MIPI DBI host driver.
> 
> I personally would love to have added mipi_ in the names - to make it
> all more explicit.
> But maybe that just because I get confused on all the acronyms.

I can rename the driver and move it out of drm/bridge/, no problem.

> Some details in the following. Will try to find some more time so I 
> can
> grasp the full picture. The following was just my low-level notes for
> now.
> 
> 	Sam
>> 
>>  DBI types c1 and c3 are supported. C1 is a SPI protocol with 9 bits 
>> per
>>  word, with the data/command information in the 9th (MSB) bit. C3 is 
>> a
>>  SPI protocol with 8 bits per word, with the data/command information
>>  carried by a separate GPIO.
> 
> We did not have any define to distingush between DBI_C1 and DBI_c3:
> +/* MIPI bus types */
> +#define MIPI_DEVICE_TYPE_DSI           BIT(0)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE1 BIT(1)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE2 BIT(2)
> +#define MIPI_DEVICE_TYPE_DBI_SPI_MODE3 BIT(3)
> +#define MIPI_DEVICE_TYPE_DBI_M6800     BIT(4)
> +#define MIPI_DEVICE_TYPE_DBI_I8080     BIT(5)
> 
> Is this on purpose?

I understand the confusion. Here SPI_MODE1/3 actually mean SPI_C1/3. I 
will rename them.

> I had assumed the host should tell what it supports and the device 
> should
> tell what it wanted.
> So if the host did not support DBI_C3 and device wants it - then we
> could give up early.

Well that's exactly what's done here - just with badly named macros :)

>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/gpu/drm/bridge/Kconfig   |   8 +
>>   drivers/gpu/drm/bridge/Makefile  |   1 +
>>   drivers/gpu/drm/bridge/dbi-spi.c | 261 
>> +++++++++++++++++++++++++++++++
> This is no bridge driver - so does not belong in the bridge
> directory.
> gpu/drm/drm_mipi_dbi_spi.c?
> 
>>   3 files changed, 270 insertions(+)
>>   create mode 100644 drivers/gpu/drm/bridge/dbi-spi.c
>> 
>>  diff --git a/drivers/gpu/drm/bridge/Kconfig 
>> b/drivers/gpu/drm/bridge/Kconfig
>>  index c7f0dacfb57a..ed38366847c1 100644
>>  --- a/drivers/gpu/drm/bridge/Kconfig
>>  +++ b/drivers/gpu/drm/bridge/Kconfig
>>  @@ -219,6 +219,14 @@ config DRM_TI_TPD12S015
>>   	  Texas Instruments TPD12S015 HDMI level shifter and ESD 
>> protection
>>   	  driver.
>> 
>>  +config DRM_MIPI_DBI_SPI
>>  +	tristate "SPI host support for MIPI DBI"
>>  +	depends on OF && SPI
>>  +	select DRM_MIPI_DSI
>>  +	select DRM_MIPI_DBI
>>  +	help
>>  +	  Driver to support DBI over SPI.
>>  +
>>   source "drivers/gpu/drm/bridge/analogix/Kconfig"
>> 
>>   source "drivers/gpu/drm/bridge/adv7511/Kconfig"
>>  diff --git a/drivers/gpu/drm/bridge/Makefile 
>> b/drivers/gpu/drm/bridge/Makefile
>>  index 7d7c123a95e4..c2c522c2774f 100644
>>  --- a/drivers/gpu/drm/bridge/Makefile
>>  +++ b/drivers/gpu/drm/bridge/Makefile
>>  @@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>   obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>>   obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>>   obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
>>  +obj-$(CONFIG_DRM_MIPI_DBI_SPI) += dbi-spi.o
> mipi_dbi_spi.o would be nice...
> 
>>   obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>> 
>>   obj-y += analogix/
>>  diff --git a/drivers/gpu/drm/bridge/dbi-spi.c 
>> b/drivers/gpu/drm/bridge/dbi-spi.c
>>  new file mode 100644
>>  index 000000000000..1060b8f95fba
>>  --- /dev/null
>>  +++ b/drivers/gpu/drm/bridge/dbi-spi.c
>>  @@ -0,0 +1,261 @@
>>  +// SPDX-License-Identifier: GPL-2.0-or-later
>>  +/*
>>  + * MIPI Display Bus Interface (DBI) SPI support
>>  + *
>>  + * Copyright 2016 Noralf Trønnes
>>  + * Copyright 2020 Paul Cercueil <paul@crapouillou.net>
>>  + */
>>  +
>>  +#include <linux/gpio/consumer.h>
>>  +#include <linux/module.h>
>>  +#include <linux/spi/spi.h>
>>  +
>>  +#include <drm/drm_mipi_dbi.h>
>>  +#include <drm/drm_mipi_dsi.h>
>>  +
>>  +#include <video/mipi_display.h>
>>  +
>>  +struct dbi_spi {
>>  +	struct mipi_dsi_host host;
> It is very confusing that the mipi_dbi_spi driver uses a dsi_host.
> It clashes in my head - and then reviewing it not easy.

 From now on read all "mipi_dsi_*" as a MIPI DSI/DBI API. Renaming the 
API means a treewide patchset that touches many many files...

> 
>>  +	struct mipi_dsi_host_ops host_ops;
> const?
>>  +
>>  +	struct spi_device *spi;
>>  +	struct gpio_desc *dc;
>>  +	struct mutex cmdlock;
>>  +
>>  +	unsigned int current_bus_type;
>>  +
>>  +	/**
>>  +	 * @tx_buf9: Buffer used for Option 1 9-bit conversion
>>  +	 */
>>  +	void *tx_buf9;
>>  +
>>  +	/**
>>  +	 * @tx_buf9_len: Size of tx_buf9.
>>  +	 */
>>  +	size_t tx_buf9_len;
>>  +};
>>  +
>>  +static inline struct dbi_spi *host_to_dbi_spi(struct mipi_dsi_host 
>> *host)
>>  +{
>>  +	return container_of(host, struct dbi_spi, host);
>>  +}
>>  +
>>  +static ssize_t dbi_spi1_transfer(struct mipi_dsi_host *host,
>>  +				 const struct mipi_dsi_msg *msg)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +	struct spi_device *spi = dbi->spi;
>>  +	struct spi_transfer tr = {
>>  +		.bits_per_word = 9,
>>  +	};
>>  +	const u8 *src8 = msg->tx_buf;
>>  +	struct spi_message m;
>>  +	size_t max_chunk, chunk;
>>  +	size_t len = msg->tx_len;
>>  +	bool cmd_byte = true;
>>  +	unsigned int i;
>>  +	u16 *dst16;
>>  +	int ret;
>>  +
>>  +	tr.speed_hz = mipi_dbi_spi_cmd_max_speed(spi, len);
>>  +	dst16 = dbi->tx_buf9;
>>  +
>>  +	max_chunk = min(dbi->tx_buf9_len / 2, len);
> Hmm, this looks not right. We limit the max_chunk to 8K here.
> We learned the other day that we count in bytes.
> OR did I miss something?

We want to extend 8-bit values into 16-bit values, and we have a 
X-bytes output buffer for that, so the maximum input buffer size we can 
use is (X/2).

This code is the original algorithm in drm_mipi_dbi.c, I didn't change 
anything here (safe for the fix that was merged a couple of days ago to 
drm-misc-fixes).

> 
>>  +
>>  +	spi_message_init_with_transfers(&m, &tr, 1);
>>  +	tr.tx_buf = dst16;
>>  +
>>  +	while (len) {
>>  +		chunk = min(len, max_chunk);
>>  +
>>  +		for (i = 0; i < chunk; i++) {
>>  +			dst16[i] = *src8++;
>>  +
>>  +			/* Bit 9: 0 means command, 1 means data */
>>  +			if (!cmd_byte)
>>  +				dst16[i] |= BIT(9);
>>  +
>>  +			cmd_byte = false;
>>  +		}
>>  +
>>  +		tr.len = chunk * 2;
>>  +		len -= chunk;
>>  +
>>  +		ret = spi_sync(spi, &m);
>>  +		if (ret)
>>  +			return ret;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static ssize_t dbi_spi3_transfer(struct mipi_dsi_host *host,
>>  +				 const struct mipi_dsi_msg *msg)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +	struct spi_device *spi = dbi->spi;
>>  +	const u8 *buf = msg->tx_buf;
>>  +	unsigned int bpw = 8;
>>  +	u32 speed_hz;
>>  +	ssize_t ret;
>>  +
>>  +	/* for now we only support sending messages, not receiving */
>>  +	if (msg->rx_len)
>>  +		return -EINVAL;
>>  +
>>  +	gpiod_set_value_cansleep(dbi->dc, 0);
>>  +
>>  +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
>>  +	ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, buf, 1);
>>  +	if (ret || msg->tx_len == 1)
>>  +		return ret;
>>  +
>>  +	if (buf[0] == MIPI_DCS_WRITE_MEMORY_START)
>>  +		bpw = 16;
>>  +
>>  +	gpiod_set_value_cansleep(dbi->dc, 1);
>>  +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, msg->tx_len - 1);
>>  +
>>  +	ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw,
>>  +				    &buf[1], msg->tx_len - 1);
>>  +	if (ret)
>>  +		return ret;
>>  +
>>  +	return msg->tx_len;
>>  +}
>>  +
>>  +static ssize_t dbi_spi_transfer(struct mipi_dsi_host *host,
>>  +				const struct mipi_dsi_msg *msg)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +
>>  +	switch (dbi->current_bus_type) {
>>  +	case MIPI_DEVICE_TYPE_DBI_SPI_MODE1:
>>  +		return dbi_spi1_transfer(host, msg);
>>  +	case MIPI_DEVICE_TYPE_DBI_SPI_MODE3:
>>  +		return dbi_spi3_transfer(host, msg);
>>  +	default:
>>  +		dev_err(&dbi->spi->dev, "Unknown transfer type\n");
>>  +		return -EINVAL;
>>  +	}
>>  +}
>>  +
>>  +static int dbi_spi_attach(struct mipi_dsi_host *host,
>>  +			  struct mipi_dsi_device *dsi)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +
>>  +	dbi->current_bus_type = dsi->bus_type;
>>  +
>>  +	if (dbi->current_bus_type == MIPI_DEVICE_TYPE_DBI_SPI_MODE1) {
>>  +		dbi->tx_buf9_len = SZ_16K;
>>  +
>>  +		dbi->tx_buf9 = kmalloc(dbi->tx_buf9_len, GFP_KERNEL);
>>  +		if (!dbi->tx_buf9)
>>  +			return -ENOMEM;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int dbi_spi_detach(struct mipi_dsi_host *host,
>>  +			  struct mipi_dsi_device *dsi)
>>  +{
>>  +	struct dbi_spi *dbi = host_to_dbi_spi(host);
>>  +
>>  +	kfree(dbi->tx_buf9);
>>  +	dbi->tx_buf9_len = 0;
>>  +
>>  +	return 0; /* Nothing to do */
>>  +}
>>  +
>>  +static void dbi_spi_host_unregister(void *d)
>>  +{
>>  +	mipi_dsi_host_unregister(d);
>>  +}
>>  +
>>  +static int dbi_spi_probe(struct spi_device *spi)
>>  +{
>>  +	struct device *dev = &spi->dev;
>>  +	struct mipi_dsi_device_info info = { };
>>  +	struct mipi_dsi_device *dsi;
>>  +	struct dbi_spi *dbi;
>>  +	int ret;
>>  +
>>  +	dbi = devm_kzalloc(dev, sizeof(*dbi), GFP_KERNEL);
>>  +	if (!dbi)
>>  +		return -ENOMEM;
>>  +
>>  +	dbi->host.dev = dev;
>>  +	dbi->host.ops = &dbi->host_ops;
>>  +	dbi->spi = spi;
>>  +	spi_set_drvdata(spi, dbi);
>>  +
>>  +	dbi->dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
>>  +	if (IS_ERR(dbi->dc)) {
>>  +		dev_err(dev, "Failed to get gpio 'dc'\n");
>>  +		return PTR_ERR(dbi->dc);
>>  +	}
>>  +
>>  +	if (spi_is_bpw_supported(spi, 9))
>>  +		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE1;
>>  +	if (dbi->dc)
>>  +		dbi->host.bus_types |= MIPI_DEVICE_TYPE_DBI_SPI_MODE3;
>>  +
>>  +	if (!dbi->host.bus_types) {
>>  +		dev_err(dev, "Neither Type1 nor Type3 are supported\n");
>>  +		return -EINVAL;
>>  +	}
>>  +
>>  +	dbi->host_ops.transfer = dbi_spi_transfer;
>>  +	dbi->host_ops.attach = dbi_spi_attach;
>>  +	dbi->host_ops.detach = dbi_spi_detach;
>>  +
>>  +	mutex_init(&dbi->cmdlock);
>>  +
>>  +	ret = mipi_dsi_host_register(&dbi->host);
>>  +	if (ret) {
>>  +		dev_err(dev, "Unable to register DSI host\n");
>>  +		return ret;
>>  +	}
>>  +
>>  +	ret = devm_add_action_or_reset(dev, dbi_spi_host_unregister, 
>> &dbi->host);
>>  +	if (ret)
>>  +		return ret;
>>  +
>>  +	/*
>>  +	 * Register our own node as a MIPI DSI device.
>>  +	 * This ensures that the panel driver will be probed.
>>  +	 */
>>  +	info.channel = 0;
>>  +	info.node = of_node_get(dev->of_node);
>>  +
>>  +	dsi = mipi_dsi_device_register_full(&dbi->host, &info);
>>  +	if (IS_ERR(dsi)) {
>>  +		dev_err(dev, "Failed to add DSI device\n");
>>  +		return PTR_ERR(dsi);
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct of_device_id dbi_spi_of_match[] = {
>>  +	{ .compatible = "adafruit,yx240qv29" },
>>  +	{ .compatible = "leadtek,ltk035c5444t-spi" },
>>  +	{ }
> Would it be better with a fall-back compatible like:
> mipi,dbi-spi.
> So the nodes must includes this compatible to be registered with
> this driver?

Ideally, it would be perfect, but unfortunately we cannot do that.

If a node has the following:
compatible = "adafruit,yx240qv29", "mipi-dbi-spi";

Then Linux will probe only with the first compatible string since it 
has a driver for it. It will use the fallback string only if no driver 
matches the first string.

-Paul

> 
>>  +};
>>  +MODULE_DEVICE_TABLE(of, dbi_spi_of_match);
>>  +
>>  +static struct spi_driver dbi_spi_driver = {
>>  +	.driver = {
>>  +		.name = "dbi-spi",
>>  +		.of_match_table = dbi_spi_of_match,
>>  +	},
>>  +	.probe = dbi_spi_probe,
>>  +};
>>  +module_spi_driver(dbi_spi_driver);
>>  +
>>  +MODULE_DESCRIPTION("DBI SPI bus driver");
>>  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>  +MODULE_LICENSE("GPL");
>>  --
>>  2.27.0

Re: [PATCH 1/6] dt-bindings: display: Document NewVision NV3052C DT node

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Jul 27, 2020 at 06:46:08PM +0200, Paul Cercueil wrote:
> Add documentation for the Device Tree node for LCD panels based on the
> NewVision NV3052C controller.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../display/panel/newvision,nv3052c.yaml      | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml b/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
> new file mode 100644
> index 000000000000..751a28800fc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/newvision,nv3052c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NewVision NV3052C TFT LCD panel driver with SPI control bus

s/driver/driver IC/ (or driver chip, or controller, or any other similar
term) to avoid confusion with device drivers.

Do I understand that the NV3052C also supports control through DSI ?
Shouldn't this appear in the DT bindings ? Do I assume correctly that
the panel will be controlled either through SPI or through DSI, but not
through both ?

> +
> +maintainers:
> +  - Paul Cercueil <paul@crapouillou.net>
> +
> +description: |
> +  This is a driver for 320x240 TFT panels, accepting a variety of input
> +  streams that get adapted and scaled to the panel. The panel output has
> +  960 TFT source driver pins and 240 TFT gate driver pins, VCOM, VCOML and
> +  VCOMH outputs.
> +
> +  The panel must obey the rules for a SPI slave device as specified in
> +  spi/spi-controller.yaml
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - leadtek,ltk035c5444t-spi

According to its datasheet, that panel is 640x480 :-)

I think you need a bit of documentation to explain that two compatible
strings are needed, one matching the panel type, and a second one
matching the chip.

> +
> +      - const: newvision,nv3052c
> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios: true
> +  port: true

The NV3052C requires multiple power supplies, I think this needs to be
taken into account here.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      display@0 {
> +        compatible = "leadtek,ltk035c5444t-spi", "newvision,nv3052c";
> +        reg = <0>;
> +
> +        spi-max-frequency = <15000000>;
> +        spi-3wire;
> +        reset-gpios = <&gpe 2 GPIO_ACTIVE_LOW>;
> +        backlight = <&backlight>;
> +        power-supply = <&vcc>;
> +
> +        port {
> +          panel_input: endpoint {
> +              remote-endpoint = <&panel_output>;
> +          };
> +        };
> +      };
> +    };
> +
> +...

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] drm/panel: Add panel driver for NewVision NV3052C based LCDs

[Sam Ravnborg]

Hi Paul.

While I continue to try to wrap my head around how I think we should
support DBI here are some panel specific comments.

	Sam

On Mon, Jul 27, 2020 at 06:46:11PM +0200, Paul Cercueil wrote:
> This driver supports the NewVision NV3052C based LCDs. Right now, it
> only supports the LeadTek LTK035C5444T 2.4" 640x480 TFT LCD panel, which
> can be found in the Anbernic RG-350M handheld console.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/panel/Kconfig                 |   9 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../gpu/drm/panel/panel-newvision-nv3052c.c   | 523 ++++++++++++++++++
>  3 files changed, 533 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index de2f2a452be5..6a8a51a702d8 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -198,6 +198,15 @@ config DRM_PANEL_NEC_NL8048HL11
>  	  panel (found on the Zoom2/3/3630 SDP boards). To compile this driver
>  	  as a module, choose M here.
>  
> +config DRM_PANEL_NEWVISION_NV3052C
> +	tristate "NewVision NV3052C DSI/SPI RGB panel"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	help
> +	  Say Y here if you want to enable support for the panels built
> +	  around the NewVision NV3052C display controller.
> +
>  config DRM_PANEL_NOVATEK_NT35510
>  	tristate "Novatek NT35510 RGB panel driver"
>  	depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index e45ceac6286f..a0516ced87db 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
>  obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
>  obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
> +obj-$(CONFIG_DRM_PANEL_NEWVISION_NV3052C) += panel-newvision-nv3052c.o
>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT39016) += panel-novatek-nt39016.o
>  obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o
> diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> new file mode 100644
> index 000000000000..2feabef6dc3c
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> @@ -0,0 +1,523 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * NevVision NV3052C IPS LCD panel driver
> + *
> + * Copyright (C) 2020, Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +struct nv3052c_panel_info {
> +	const struct drm_display_mode *display_modes;
> +	unsigned int num_modes;
> +	unsigned int bus_type;
> +	u16 width_mm, height_mm;
> +	u32 bus_format, bus_flags;
> +};
> +
> +struct nv3052c_reg {
> +	u8 cmd;
> +	u8 value;
> +};
> +
> +struct nv3052c {
> +	struct drm_panel drm_panel;
> +	struct mipi_dsi_device *dsi;
> +	struct device *dev;
> +
> +	struct regulator *supply;
> +	const struct nv3052c_panel_info *panel_info;
> +
> +	struct gpio_desc *reset_gpio;
> +
> +	struct backlight_device *backlight;

drm_panel has backlight support.

Just calling drm_panel_of_backlight() after init should do the trick.
All other references to backlight can be dropped, including this
variable in struct nv3052c.

There is one delay loop you may need to keep.
Will comment on it below.



> +};
> +
> +static const struct nv3052c_reg nv3052c_regs[] = {
...
> +	{ 0x36, 0x0a },
> +};
> +
> +static inline struct nv3052c *to_nv3052c(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct nv3052c, drm_panel);
> +}
> +
> +static int nv3052c_prepare(struct drm_panel *drm_panel)
> +{
> +	struct nv3052c *priv = to_nv3052c(drm_panel);
> +	unsigned int i;
> +	int err;
> +
> +	err = regulator_enable(priv->supply);
> +	if (err) {
> +		dev_err(priv->dev, "Failed to enable power supply: %d\n", err);
> +		return err;
> +	}
> +
> +	/* Reset the chip */
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	usleep_range(10, 1000);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +	usleep_range(5000, 20000);
> +
> +	for (i = 0; i < ARRAY_SIZE(nv3052c_regs); i++) {
> +		err = mipi_dsi_dcs_write(priv->dsi, nv3052c_regs[i].cmd,
> +					 &nv3052c_regs[i].value, 1);
> +		if (err) {
> +			dev_err(priv->dev, "Unable to set register: %d\n", err);
> +			goto err_disable_regulator;
> +		}
> +	}
> +
> +	err = mipi_dsi_dcs_exit_sleep_mode(priv->dsi);
> +	if (err) {
> +		dev_err(priv->dev, "Unable to exit sleep mode: %d\n", err);
> +		return err;
> +	}
> +
> +	msleep(100);
> +
> +	return 0;
> +
> +err_disable_regulator:
> +	regulator_disable(priv->supply);
> +	return err;
> +}
> +
> +static int nv3052c_unprepare(struct drm_panel *drm_panel)
> +{
> +	struct nv3052c *priv = to_nv3052c(drm_panel);
> +	int err;
> +
> +	err = mipi_dsi_dcs_enter_sleep_mode(priv->dsi);
> +	if (err) {
> +		dev_err(priv->dev, "Unable to enter sleep mode: %d\n", err);
> +		return err;
> +	}
> +
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +
> +	regulator_disable(priv->supply);
> +
> +	return 0;
> +}
> +
> +static int nv3052c_enable(struct drm_panel *drm_panel)
> +{
> +	struct nv3052c *priv = to_nv3052c(drm_panel);
> +	int err;
> +
> +	err = mipi_dsi_dcs_set_display_on(priv->dsi);
> +	if (err) {
> +		dev_err(priv->dev, "Unable to enable display: %d\n", err);
> +		return err;
> +	}
> +
> +	if (priv->backlight) {
> +		/* Wait for the picture to be ready before enabling backlight */
> +		usleep_range(10000, 20000);
This delay would likely still be relevant.
Just check priv->panel->backlight.

> +
> +		err = backlight_enable(priv->backlight);
Drop this.

> +	}
> +
> +	return err;
> +}
> +
> +static int nv3052c_disable(struct drm_panel *drm_panel)
> +{
> +	struct nv3052c *priv = to_nv3052c(drm_panel);
> +	int err;
> +
> +	backlight_disable(priv->backlight);
Drop this.

> +
> +	err = mipi_dsi_dcs_set_display_off(priv->dsi);
> +	if (err) {
> +		dev_err(priv->dev, "Unable to disable display: %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int nv3052c_get_modes(struct drm_panel *drm_panel,
> +			     struct drm_connector *connector)
> +{
> +	struct nv3052c *priv = to_nv3052c(drm_panel);
> +	const struct nv3052c_panel_info *panel_info = priv->panel_info;
> +	struct drm_display_mode *mode;
> +	unsigned int i;
> +
> +	for (i = 0; i < panel_info->num_modes; i++) {
> +		mode = drm_mode_duplicate(connector->dev,
> +					  &panel_info->display_modes[i]);
> +		if (!mode)
> +			return -ENOMEM;
> +
> +		drm_mode_set_name(mode);
> +
> +		mode->type = DRM_MODE_TYPE_DRIVER;
> +		if (panel_info->num_modes == 1)
> +			mode->type |= DRM_MODE_TYPE_PREFERRED;
> +
> +		drm_mode_probed_add(connector, mode);
> +	}
> +
> +	connector->display_info.bpc = 8;
> +	connector->display_info.width_mm = panel_info->width_mm;
> +	connector->display_info.height_mm = panel_info->height_mm;
> +
> +	drm_display_info_set_bus_formats(&connector->display_info,
> +					 &panel_info->bus_format, 1);
> +	connector->display_info.bus_flags = panel_info->bus_flags;
> +
> +	return panel_info->num_modes;
> +}
> +
> +static const struct drm_panel_funcs nv3052c_funcs = {
> +	.prepare	= nv3052c_prepare,
> +	.unprepare	= nv3052c_unprepare,
> +	.enable		= nv3052c_enable,
> +	.disable	= nv3052c_disable,
> +	.get_modes	= nv3052c_get_modes,
> +};
> +
> +static int nv3052c_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct nv3052c *priv;
> +	int err;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	priv->dsi = dsi;
> +	mipi_dsi_set_drvdata(dsi, priv);
> +
> +	priv->panel_info = of_device_get_match_data(dev);
> +	if (!priv->panel_info)
> +		return -EINVAL;
> +
> +	dsi->bus_type = priv->panel_info->bus_type;
> +
> +	priv->supply = devm_regulator_get(dev, "power");
> +	if (IS_ERR(priv->supply)) {
> +		dev_err(dev, "Failed to get power supply\n");
> +		return PTR_ERR(priv->supply);
> +	}
> +
> +	priv->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset_gpio)) {
> +		dev_err(dev, "Failed to get reset GPIO\n");
> +		return PTR_ERR(priv->reset_gpio);
> +	}
> +
> +	priv->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(priv->backlight)) {
> +		err = PTR_ERR(priv->backlight);
> +		if (err != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get backlight handle\n");
> +		return err;
> +	}
Drop this part, use drm_panel_of_backlight() later.
See other drivers for where it is called.

> +
> +	drm_panel_init(&priv->drm_panel, dev, &nv3052c_funcs,
> +		       DRM_MODE_CONNECTOR_DPI);
> +
> +	err = drm_panel_add(&priv->drm_panel);
> +	if (err < 0) {
> +		dev_err(dev, "Failed to register priv\n");
> +		return err;
> +	}
> +
> +	err = mipi_dsi_attach(dsi);
> +	if (err) {
> +		dev_err(dev, "Failed to attach panel\n");
> +		drm_panel_remove(&priv->drm_panel);
> +		return err;
> +	}
> +
> +	/*
> +	 * We can't call mipi_dsi_maybe_register_tiny_driver(), since the
> +	 * NV3052C does not support MIPI_DCS_WRITE_MEMORY_START.
> +	 */
> +
> +	return 0;
> +}
> +
> +static int nv3052c_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct nv3052c *priv = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&priv->drm_panel);
> +
> +	nv3052c_disable(&priv->drm_panel);
Call drm_panel_disable()

> +	nv3052c_unprepare(&priv->drm_panel);
Call drm_panel_unprepare()

With the above two we keep maintaining the backlight properly.

> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode ltk035c5444t_modes[] = {
> +	{ /* 60 Hz */
> +		.clock = 24000,
> +		.hdisplay = 640,
> +		.hsync_start = 640 + 96,
> +		.hsync_end = 640 + 96 + 16,
> +		.htotal = 640 + 96 + 16 + 48,
> +		.vdisplay = 480,
> +		.vsync_start = 480 + 5,
> +		.vsync_end = 480 + 5 + 2,
> +		.vtotal = 480 + 5 + 2 + 13,
> +		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +	},
> +	{ /* 50 Hz */
> +		.clock = 18000,
> +		.hdisplay = 640,
> +		.hsync_start = 640 + 39,
> +		.hsync_end = 640 + 39 + 2,
> +		.htotal = 640 + 39 + 2 + 39,
> +		.vdisplay = 480,
> +		.vsync_start = 480 + 5,
> +		.vsync_end = 480 + 5 + 2,
> +		.vtotal = 480 + 5 + 2 + 13,
> +		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +	},
> +};
> +
> +static const struct nv3052c_panel_info ltk035c5444t_spi_panel_info = {
> +	.display_modes = ltk035c5444t_modes,
> +	.num_modes = ARRAY_SIZE(ltk035c5444t_modes),
> +	.bus_type = MIPI_DEVICE_TYPE_DBI_SPI_MODE1,
> +	.width_mm = 77,
> +	.height_mm = 64,
> +	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
> +};
> +
> +static const struct of_device_id nv3052c_of_match[] = {
> +	{ .compatible = "leadtek,ltk035c5444t-spi", .data = &ltk035c5444t_spi_panel_info },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, nv3052c_of_match);
> +
> +static struct mipi_dsi_driver nv3052c_driver = {
> +	.driver = {
> +		.name = "nv3052c",
> +		.of_match_table = nv3052c_of_match,
> +	},
> +	.probe = nv3052c_probe,
> +	.remove = nv3052c_remove,
> +};
> +module_mipi_dsi_driver(nv3052c_driver);
> +
> +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.27.0