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

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

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

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