mbox series

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

Message ID 20200727164613.19744-1-paul@crapouillou.net (mailing list archive)
Headers show
Series DBI/DSI, panel drivers, and tinyDRM compat | expand

Message

Paul Cercueil July 27, 2020, 4:46 p.m. UTC
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

Comments

Paul Cercueil July 27, 2020, 5:59 p.m. UTC | #1
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
Paul Cercueil July 27, 2020, 9:10 p.m. UTC | #2
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