[RFC] Introduce MIPI DBI driver class #64587

danieldegrasse · 2023-10-30T18:24:57Z

Introduction

This PR introduces a MIPI DBI driver class. This driver class is based off the MIPI DBI definitions present in Linux: https://elixir.bootlin.com/linux/latest/source/include/drm/drm_mipi_dbi.h, which is used by the tinydrm driver to enable TFT SPI displays.

Although the driver included in the PR simply uses a SPI peripheral to emulate a MIPI DBI Type C interface, peripherals such as the LCD controller on the Renesas DA1469x implement MIPI DBI Type C directly in hardware. Furthermore, the MIPI driver API will pave the way for supporting Type B (8080 based) interfaces in Zephyr, which are supported by several NXP peripherals (such as the SEMC or FlexIO)

At a high level, this PR includes the following changes:

MIPI DBI API headers
MIPI DBI Mode C implementation using the SPI and GPIO APIs
Updates to the ILI9XXX driver (and devicetree definitions) in order to use the MIPI DBI API

Problem Description

Currently, Zephyr supports SPI based displays using the SPI API directly. Most SPI displays use a data format often called 3/4 Wire SPI. The interface for 4 wire SPI utilizes the following signals:

SCLK: serial clock
DOUT/DIN: bidirectional data line
CS: chip select
C/D: command/data select
RST: optional reset signal

For SPI display controllers, a SPI device can easily drive SCLK, DOUT, and CS. Most display controllers only need data sent to them to support displaying data, so this scheme works well. The C/D and RST pins can be implemented using a GPIO.

However, LCD controller specific IP that implements support for SPI 3/4 wire mode does not map well into the current solution for SPI displays. This IP often supports setting the C/D and RST pins directly, and sometimes will not support reading and writing data simultaneously via SPI (making it a poor fit for the SPI API). Therefore, I would like to introduce the MIPI DBI API in order to better abstract LCD controllers specifically designed for 3/4 wire SPI mode.

Detailed RFC

The MIPI DBI API includes the following 4 functions, loosely based on the API definition in Linux:

mipi_dbi_command_write: Write a command (and optionally a data buffer) to the display
mipi_dbi_command_read: Write a command, and read the display's response into a buffer
mipi_dbi_write_display: Write display buffer data to a controller. This API is included because some MIPI DBI peripherals need awareness of the display buffer descriptor or pixel format in use.
mipi_dbi_reset: reset the attached display via the RST pin

The command_write, command_read, and write_display APIs all take a pointer to a mipi_dbi_config structure. This approach was chosen in order to map well onto the SPI APIs in zephyr. The alternative to this would be to provide a discrete mipi_dbi_attach function, like Linux does. I chose to deviate from Linux here because mapping well onto the SPI API will enable this MIPI DBI abstraction layer to consume less ROM/RAM space, since no mipi_dbi_attach function is needed.

It should be noted that the MIPI DBI is somewhat similar to the MIPI DSI API. I believe we should have a distinct driver interface here for a few reasons:

MIPI DBI is fundamentally a different interface than MIPI DSI, although the command sets are very similar (both display types use the DCS command set)
MIPI DBI displays are often used on smaller devices, so design goals for this API will likely be slightly different

Alternatives

Another option I considered here was somehow extending the SPI API to support these 3/4 wire display peripherals. The reasons I chose not to go down this path were:

Linux already solved this issue with the MIPI DBI layer. There is no need to reinvent the wheel
8080 devices would not be possible to support with a SPI API extension
SPI 3/4 LCD peripherals likely will not map well onto the SPI API. For example, they may not support simultaneous TX and RX, or may require that the command/data pin be driven for any transaction, even when not interfacing with a display.

bjarki-andreasen

Overall it looks really good, please address the potential issue with the GPIO polarity properties :)

dts/bindings/mipi-dbi/mipi-dbi-host.yaml

drivers/mipi_dbi/mipi_dbi_spi.c

drivers/Kconfig

drivers/mipi_dbi/mipi_dbi_spi.c

tests/drivers/build_all/display/app.overlay

drivers/mipi_dbi/mipi_dbi_spi.c

dts/bindings/display/ilitek,ili9xxx-common.yaml

fabiobaltieri · 2024-01-31T11:12:53Z

DNM'ing for now, since it's flagged as RFC, @danieldegrasse please untag once it's ready, I imagine other long time reviewers should approve first.

Thanks for working on this!

danieldegrasse · 2024-01-31T14:39:13Z

DNM'ing for now, since it's flagged as RFC, @danieldegrasse please untag once it's ready, I imagine other long time reviewers should approve first.

I am ok with the state of the PR- @ioannis-karachalios, can you provide a review here? If the API looks good to you, I am ok to move forwards with it.

ioannis-karachalios · 2024-01-31T14:43:43Z

DNM'ing for now, since it's flagged as RFC, @danieldegrasse please untag once it's ready, I imagine other long time reviewers should approve first.

I am ok with the state of the PR- @ioannis-karachalios, can you provide a review here? If the API looks good to you, I am ok to move forwards with it.

No further comments from my side. The API looks good to me.

fabiobaltieri · 2024-01-31T15:38:17Z

@bjarki-trackunit anything to add?

bjarki-andreasen

Looks good, I have two suggestions, non of which are blocking :)

bjarki-andreasen · 2024-01-31T15:53:39Z

drivers/mipi_dbi/mipi_dbi_spi.c

+		return -ENODEV;
+	}
+
+	if (config->cmd_data.port) {


You should check that the GPIO is ready as well if it is included, you can use

if (!gpio_is_ready_dt(&config->cmd_data)) { return -ENODEV; }

bjarki-andreasen · 2024-01-31T15:56:08Z

drivers/mipi_dbi/mipi_dbi_spi.c

+	const struct mipi_dbi_spi_config *config = dev->config;
+	int ret;
+
+	if (config->reset.port == NULL) {


you can make this a bit prettier with an inline function

static inline bool mipi_dbi_has_pin(const struct gpio_dt_spec *spec) { return spec->port != NULL; }

fabiobaltieri · 2024-01-31T16:01:42Z

Cool, let's merge this and keep those for a followup since we are close to RC1.

kartben · 2024-02-01T12:46:24Z

As per #68408, it looks like this PR should have also included release notes/migration guide for out-of-tree users to know what changes they need to make?

ajarmouni-st · 2024-02-01T12:59:22Z

As per #68408, it looks like this PR should have also included release notes/migration guide for out-of-tree users to know what changes they need to make?

@kartben Have you seen this https://github.com/zephyrproject-rtos/zephyr/pull/64587/files#diff-46acdbef30e5a958f187ab7343b38d74fcf91152d28ae99b267cd4bc80b92cc3 ?

kartben · 2024-02-01T13:38:50Z

As per #68408, it looks like this PR should have also included release notes/migration guide for out-of-tree users to know what changes they need to make?

@kartben Have you seen this https://github.com/zephyrproject-rtos/zephyr/pull/64587/files#diff-46acdbef30e5a958f187ab7343b38d74fcf91152d28ae99b267cd4bc80b92cc3 ?

I am so sorry, you are right of course. I just somehow missed it when going through the contents of this PR.

diegoherranz · 2024-02-02T15:45:00Z

I've just seen this and adapted my out-of-tree board DTS to use it.
It's nice to standardise displays further.

One thing I find quite confusing is the definition of the reset pin. Namely the fact that it is defined as active low in the binding, so that the DTS must be set as GPIO_ACTIVE_HIGH for it to behave as active low. Is it just me?

Thanks.

danieldegrasse · 2024-02-02T16:21:50Z

Glad you were able to transition your board- hopefully the changes weren't too much churn :)

One thing I find quite confusing is the definition of the reset pin. Namely the fact that it is defined as active low in the binding, so that the DTS must be set as GPIO_ACTIVE_HIGH for it to behave as active low. Is it just me?

This was intentional- the reset pin is set low by the MIPI DBI driver. GPIO_ACTIVE_HIGH indicates that the GPIO logic itself is active when high, not that the reset pin is active high. If you needed to invert the reset pin polarity you could theoretically do so using the GPIO_ACTIVE_LOW flag on the pin.

This is a similar pattern to how other reset pins in tree work, see the GT911 touchscreen driver for example: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/input/input_gt911.c#L259

diegoherranz · 2024-02-02T16:49:19Z

Glad you were able to transition your board- hopefully the changes weren't too much churn :)

It was straightforward. And it seems to work fine :)

This was intentional- the reset pin is set low by the MIPI DBI driver. GPIO_ACTIVE_HIGH indicates that the GPIO logic itself is active when high, not that the reset pin is active high. If you needed to invert the reset pin polarity you could theoretically do so using the GPIO_ACTIVE_LOW flag on the pin.

To me, if the binding is defined as "reset", the driver should set the pin to 1 to enter reset and to 0 to release from reset (and then you use GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW as needed). Otherwise it should be called reset_n, reset_b or similar. As you say, maybe it's done like that in other in-tree drivers, but I still find confusing that I need to define my active-low reset pin as GPIO_ACTIVE_HIGH 😄

Edit: actually after re-reading the gt911 code you linked, isn't that doing what I'm saying (i.e. 1 while in reset and 0 to release from reset)?

danieldegrasse · 2024-02-02T18:04:18Z

Edit: actually after re-reading the gt911 code you linked, isn't that doing what I'm saying (i.e. 1 while in reset and 0 to release from reset)?

So it does, probably should have read my own example closer 🤦. It does not really seem like we have a clear story for this in Zephyr, since there are some places where we drive an active low reset pin to 0 to reset the device: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/video/ov2640.c#L997 (hopefully I've read this one right). The reason I lean towards GPIO_ACTIVE_HIGH here is because it is the default flag for GPIO pins, so there is no need for the user to explicitly define it.

I'm totally fine to rename the pin to reset_n or something in order to clear up the confusion here, though, if you think that would be useful

nordicjm · 2024-02-05T07:40:22Z

The above is what I would call a bug, reset should be set to 1 to reset it then set to 0, and is also how it is in linux from what I can see and zephyr should be using the same style as linux unless there is a very good reason not to, @mbolivar-ampere @galak @gmarull for comment

gmarull · 2024-02-05T09:01:57Z

The above is what I would call a bug, reset should be set to 1 to reset it then set to 0

agreed

fabiobaltieri · 2024-02-05T09:37:43Z

The above is what I would call a bug, reset should be set to 1 to reset it then set to 0

Well yeah that's the whole intent of the logical "active low" flag, setting to 1 asserts the condition (i.e. enter reset state) whatever that means on the hardware, that's just how the APi is meant to be used.

nordicjm · 2024-02-05T09:40:35Z

@danieldegrasse since everyone seems to be agree here, can you submit a PR fixing the issue so that it sets it to 1 (which would be the GPIO_ACTIVE state) then sets it back to 0 (which would be the GPIO_INACTIVE state). If there are other drivers not following that then issues should be raised against them

danieldegrasse · 2024-02-05T15:49:37Z

@danieldegrasse since everyone seems to be agree here, can you submit a PR fixing the issue so that it sets it to 1 (which would be the GPIO_ACTIVE state) then sets it back to 0 (which would be the GPIO_INACTIVE state). If there are other drivers not following that then issues should be raised against them

Sure- I wasn't aware that Linux had a convention here, thanks for highlighting this. #68563 should fix the issue. Just a note- I've changed the reset pin, but left the MIPI DBI command/data pin as GPIO_ACTIVE_HIGH (which is a change from how it was previously implemented on the ILI9xxx), because I don't think there is really a active/inactive state for the pin. On most displays, the pin being low indicates a command, and high indicates data. I've made sure the description of the pin indicates it will be set to logic 0 for a command and logic 1 for data, so I think using GPIO_ACTIVE_HIGH there should be ok

diegoherranz · 2024-02-06T09:00:35Z

Sure- I wasn't aware that Linux had a convention here, thanks for highlighting this. #68563 should fix the issue

Thanks. I've provided a comment in that PR.

I've changed the reset pin, but left the MIPI DBI command/data pin as GPIO_ACTIVE_HIGH (which is a change from how it was previously implemented on the ILI9xxx), because I don't think there is really a active/inactive state for the pin. On most displays, the pin being low indicates a command, and high indicates data. I've made sure the description of the pin indicates it will be set to logic 0 for a command and logic 1 for data, so I think using GPIO_ACTIVE_HIGH there should be ok

Previously, it was defined as cmd-data-gpios, now as dc-gpios, so the "logical" polarity has been reversed. But I think that's OK and, having as look at the linux kernel, I can find dc-gpios, but not cmd-data-gpios. So, it looks good to me.

Thanks.

dberlin · 2024-07-15T20:24:35Z

Hi - the ili9xxx changes here broke the bl5340_dvk. The change moved the SPI device from spi4 to spi2 (Unintentionally I assume).

This breaks display entirely.
Changing it back to spi4 makes it work again.

IE

diff --git a/boards/lairdconnect/bl5340_dvk/bl5340_dvk_nrf5340_cpuapp_common.dtsi b/boards/lairdconnect/bl5340_dvk/bl5340_dvk_nrf5340_cpuapp_common.dtsi
index ff29e1f176e..b10199649d0 100644
--- a/boards/lairdconnect/bl5340_dvk/bl5340_dvk_nrf5340_cpuapp_common.dtsi
+++ b/boards/lairdconnect/bl5340_dvk/bl5340_dvk_nrf5340_cpuapp_common.dtsi
@@ -112,7 +112,7 @@
 		compatible = "zephyr,mipi-dbi-spi";
 		reset-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
 		dc-gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
-		spi-dev = <&spi2>;
+		spi-dev = <&spi4>;
 		write-only;
 		#address-cells = <1>;
 		#size-cells = <0>;

Fixes it.
I don't have a perfectly clean zephyr git client, so if you can fix it, that'd be awesome.
Otherwise, i can make one.

danieldegrasse · 2024-07-17T10:46:58Z

Hi - the ili9xxx changes here broke the bl5340_dvk. The change moved the SPI device from spi4 to spi2 (Unintentionally I assume).

This breaks display entirely.
Changing it back to spi4 makes it work again.

Yes, this was by mistake. Thank you for catching this! I made an issue to track this here: #75991

danieldegrasse requested review from nashif, carlescufi, galak, MaureenHelm, dcpleung and erwango as code owners October 30, 2023 18:24

zephyrbot added Release Notes To be mentioned in the release notes platform: Laird Connectivity Laird Connectivity area: Shields Shields (add-on boards) area: Display platform: ITE ITE area: Devicetree Binding PR modifies or adds a Device Tree binding labels Oct 30, 2023

zephyrbot requested review from avisconti, Dino-Li, greg-leach, GTLin08, henrikbrixandersen, jackrosenthal, jfischer-no, kartben and rerickson1 October 30, 2023 18:25

zephyrbot assigned erwango Oct 30, 2023

danieldegrasse changed the title ~~include: drivers: mipi_dsi: split MIPI DCS values into separate header~~ [RFC] Introduce MIPI DBI driver class Oct 30, 2023

danieldegrasse added the RFC Request For Comments: want input from the community label Oct 30, 2023

bjarki-andreasen requested changes Oct 31, 2023

View reviewed changes

dts/bindings/mipi-dbi/mipi-dbi-host.yaml Outdated Show resolved Hide resolved

drivers/mipi_dbi/mipi_dbi_spi.c Outdated Show resolved Hide resolved

drivers/mipi_dbi/mipi_dbi_spi.c Outdated Show resolved Hide resolved

zephyrbot added the area: Process label Oct 31, 2023

bjarki-andreasen previously approved these changes Nov 1, 2023

View reviewed changes

bjarki-andreasen requested review from fabiobaltieri and removed request for GTLin08 November 1, 2023 04:11

fabiobaltieri reviewed Nov 2, 2023

View reviewed changes

ioannis-karachalios approved these changes Jan 31, 2024

View reviewed changes

bjarki-andreasen approved these changes Jan 31, 2024

View reviewed changes

fabiobaltieri removed the DNM This PR should not be merged (Do Not Merge) label Jan 31, 2024

fabiobaltieri merged commit 0fc0c0f into zephyrproject-rtos:main Jan 31, 2024
30 of 31 checks passed

danieldegrasse deleted the feature/mipi-dbi branch January 31, 2024 16:14

This was referenced Jan 31, 2024

drivers: mipi_dbi: mipi_dbi_spi: cleanup pin checks #68372

Merged

drivers: display: fix ili9xxx write size #68378

Merged

danieldegrasse mentioned this pull request Jun 5, 2024

drivers: display: gc9x01: convert to MIPI DBI API #73686

Merged

dberlin mentioned this pull request Jul 15, 2024

Add BL5340PA LairdCP/sdk-zephyr#3

Merged

danieldegrasse mentioned this pull request Jul 17, 2024

Display does not work on bl5340_dvk #75991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Introduce MIPI DBI driver class #64587

[RFC] Introduce MIPI DBI driver class #64587

danieldegrasse commented Oct 30, 2023 •

edited

Loading

bjarki-andreasen left a comment

fabiobaltieri commented Jan 31, 2024 •

edited

Loading

danieldegrasse commented Jan 31, 2024

ioannis-karachalios commented Jan 31, 2024

fabiobaltieri commented Jan 31, 2024

bjarki-andreasen left a comment

bjarki-andreasen Jan 31, 2024

bjarki-andreasen Jan 31, 2024

fabiobaltieri commented Jan 31, 2024

kartben commented Feb 1, 2024

ajarmouni-st commented Feb 1, 2024 •

edited

Loading

kartben commented Feb 1, 2024

diegoherranz commented Feb 2, 2024 •

edited

Loading

danieldegrasse commented Feb 2, 2024

diegoherranz commented Feb 2, 2024 •

edited

Loading

danieldegrasse commented Feb 2, 2024

nordicjm commented Feb 5, 2024 •

edited

Loading

gmarull commented Feb 5, 2024

fabiobaltieri commented Feb 5, 2024

nordicjm commented Feb 5, 2024

danieldegrasse commented Feb 5, 2024

diegoherranz commented Feb 6, 2024

dberlin commented Jul 15, 2024

danieldegrasse commented Jul 17, 2024 •

edited

Loading

[RFC] Introduce MIPI DBI driver class #64587

[RFC] Introduce MIPI DBI driver class #64587

Conversation

danieldegrasse commented Oct 30, 2023 • edited Loading

Introduction

Problem Description

Detailed RFC

Alternatives

bjarki-andreasen left a comment

Choose a reason for hiding this comment

fabiobaltieri commented Jan 31, 2024 • edited Loading

danieldegrasse commented Jan 31, 2024

ioannis-karachalios commented Jan 31, 2024

fabiobaltieri commented Jan 31, 2024

bjarki-andreasen left a comment

Choose a reason for hiding this comment

bjarki-andreasen Jan 31, 2024

Choose a reason for hiding this comment

bjarki-andreasen Jan 31, 2024

Choose a reason for hiding this comment

fabiobaltieri commented Jan 31, 2024

kartben commented Feb 1, 2024

ajarmouni-st commented Feb 1, 2024 • edited Loading

kartben commented Feb 1, 2024

diegoherranz commented Feb 2, 2024 • edited Loading

danieldegrasse commented Feb 2, 2024

diegoherranz commented Feb 2, 2024 • edited Loading

danieldegrasse commented Feb 2, 2024

nordicjm commented Feb 5, 2024 • edited Loading

gmarull commented Feb 5, 2024

fabiobaltieri commented Feb 5, 2024

nordicjm commented Feb 5, 2024

danieldegrasse commented Feb 5, 2024

diegoherranz commented Feb 6, 2024

dberlin commented Jul 15, 2024

danieldegrasse commented Jul 17, 2024 • edited Loading

danieldegrasse commented Oct 30, 2023 •

edited

Loading

fabiobaltieri commented Jan 31, 2024 •

edited

Loading

ajarmouni-st commented Feb 1, 2024 •

edited

Loading

diegoherranz commented Feb 2, 2024 •

edited

Loading

diegoherranz commented Feb 2, 2024 •

edited

Loading

nordicjm commented Feb 5, 2024 •

edited

Loading

danieldegrasse commented Jul 17, 2024 •

edited

Loading