Renesas: Smartbond: Add MIPI DBI Driver Support #68426

ioannis-karachalios · 2024-02-01T14:51:00Z

No description provided.

zephyrbot · 2024-02-01T14:51:53Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

danieldegrasse · 2024-02-01T15:57:33Z

west.yml

Please add more description to the commit body here- it seems from the diff that the manifest revision is just being updated to include LCD controller driver fixes.

OK. Correct, the manifest revision points to LCDC fixes (which will otherwise raise compile errors).

danieldegrasse · 2024-02-01T16:02:18Z

boards/arm/da1469x_dk_pro/da1469x_dk_pro-pinctrl.dtsi

Please do not include the pin control changes in the commit adding the driver- group them with changes for the board

danieldegrasse · 2024-02-01T16:05:23Z

drivers/mipi_dbi/mipi_dbi_smartbond.c

+};
+
+/* Mark the device is is progress and so it's not allowed to enter the sleep state. */
+static void mipi_dbi_pm_get(const struct device *dev)


Not sure if this will work for your use case, but pm_policy_state_lock_get might also be helpful here

Given our pm_state_set SoC implementation, this should not work here. Thanks for pointing this out, either way.

danieldegrasse · 2024-02-01T16:06:52Z

drivers/mipi_dbi/mipi_dbi_smartbond.c

+#include <zephyr/logging/log.h>
+LOG_MODULE_REGISTER(smartbond_mipi_dbi, CONFIG_MIPI_DBI_LOG_LEVEL);
+
+#define SMARTBOND_IRQN		DT_INST_IRQN(0)


Generally speaking, I'd prefer that this driver be multi-instance, unless there is a good reason not to make it support multiple device instances. These properties should likely be part of the dev->config structure

danieldegrasse · 2024-02-01T16:08:10Z

drivers/mipi_dbi/mipi_dbi_smartbond.c

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


Could you use gpio_is_ready_dt here?

danieldegrasse · 2024-02-01T16:19:43Z

drivers/mipi_dbi/mipi_dbi_smartbond.c

+	/* Provide synchronization between task return and ISR firing */
+	struct k_sem sync_sem;
+	/* Flag indicating whether or not an underflow took place */
+	bool underflow_flag;


Just a question- does this need to be volatile? I suppose the compiler will likely always read this from memory, but might be worth being explicit since this is touched in an ISR.

The compiler should read this symbol from memory. Technically speaking, symbols that are also written within ISR should be marked as volatile. Will fix it.

danieldegrasse · 2024-02-01T16:28:20Z

drivers/mipi_dbi/mipi_dbi_smartbond.c

+#endif
+};
+
+#define SMARTBOND_MIPI_DBI_INIT(inst)	\


I mentioned this above- but we should make this driver support multiple instances. The macros here are already instance based, so if there is a compelling reason to keep the driver single instance we need to use single instance macros here as well.

From architecture point of view, our SoC integrates a single LCDC IP block. The only reason that multiple instances should be supported is to cover use cases where more than one display device is to be driven. Note sure if this scenario has a meaning in a real application. @danieldegrasse what do you think?

I am always pro multi instance drivers, you never know what the hardware team will come up with for the next MCU 😉. That being said- if you would prefer to keep this a single instance driver I am ok with that- we just need to align the driver macros to avoid using DT_INST_FOREACH_STATUS_OKAY. We can just call SMARTBOND_MIPI_DBI_INIT(0) in this driver to initialize the driver for instance 0 of the compatible.

danieldegrasse · 2024-02-01T16:36:43Z

dts/bindings/mipi-dbi/renesas,smartbond-mipi-dbi.yaml

+  dma-prefetch:
+    type: int
+    enum:
+      - 0 # Prefetch mechanism is disabled


You can use an enum type here of string and then use DT_INST_ENUM_IDX in the code to get the value you need, this will be easier for the user to follow

danieldegrasse · 2024-02-01T16:46:14Z

modules/lvgl/Kconfig

I think we want this change in a separate PR, since it touches generic code.

I was unsure so I decided to push these changes here. Will open a separate PR.

danieldegrasse · 2024-02-01T16:47:57Z

samples/modules/lvgl/demos/boards/da1469x_dk_pro.conf

+CONFIG_INPUT_FT5336_INTERRUPT=y
+CONFIG_LV_Z_POINTER_INPUT_MSGQ_COUNT=70
+# Frame buffer must always be 4-byte aligned. This is imposed by LCDC.
+CONFIG_LV_Z_VDB_ALIGN=4


You might consider setting these values in the board's Kconfig.defconfig, since they will be needed by customers using LVGL as well.

zephyr/boards/arm/mimxrt1060_evk/Kconfig.defconfig

Line 61 in 7f5b332

if LVGL

danieldegrasse · 2024-02-08T15:36:50Z

@ioannis-karachalios can you take a look at the CI/compliance failures?

ioannis-karachalios · 2024-02-08T15:40:02Z

@ioannis-karachalios can you take a look at the CI/compliance failures?

Sure. I was just waiting for your complete feedback.

danieldegrasse · 2024-02-08T15:53:51Z

Sure. I was just waiting for your complete feedback.

Understood. I'll continue giving feedback, but I would start addressing the failures. It is difficult to give a final review when CI is failing, since the contents of the PR will necessarily have to change in order for it to merge.

danieldegrasse · 2024-02-08T15:55:15Z

samples/drivers/display/boards/da1469x_dk_pro.conf

+#
+CONFIG_HEAP_MEM_POOL_SIZE=4096
+CONFIG_LOG_MODE_MINIMAL=y
+CONFIG_MIPI_DBI_LOG_LEVEL_DBG=y


Are the log settings needed for customers running the sample? Further, does the heap size need adjustment? It seems like the default (larger) heap size would work ok.

danieldegrasse · 2024-02-08T15:58:08Z

samples/drivers/display/boards/da1469x_dk_pro.overlay

+    };
+ };
+
+ &mipi_dbi {


Is the display used here an external module, or integrated with the board? If integrated, it might make sense to define it in the board devicetree instead of as an overlay

I was sort of unsure how to handle this as the specific display module is neither integrated with the board nor promoted for the specific display interface. Therefore, I decided to move its configuration in overlay files. However, I will need to safeguard it against other display interfaces/modules as the same LCD controller might also be used to drive display panels. To be honest, I have not encountered related use cases so, I guess it would be ok to introduce a Kconfig choice in our board's Kconfig.defconfig which should select the display node/module. Let me know if there are other approaches for this to work.

Is this a case where the module could be a shield? Generally anything using the Arduino interface is a great candidate for this. If the interface is completely nonstandard, keeping the display as an overlay is probably best. NXP has used shields to define displays when there is a shared interface used between multiple display types (we use a 40 pin FPC for several modules) so this could be another option.

Yes, it could be that case as our board exhibits Arduino expansion headers. I tried to add a <BOARD>.overlay file, namely da1469x_dk_pro.overlay, under /boards/shields/adafruit_2_8_tft_touch_v2/boards/ but I cannot build with our native MIPI DBI host controller and despite the changes I have made in our board's overlay. Every time I build with -DSHIELD=adafruit_2_8_tft_touch_v2 it's always the MIPI DBI SPI interface the selected bus type. Any hints?

To be clear here- the LCD controller pins are exposed on the arduino header, in the same location as the SPI pins are usually exposed? I'm not certain what the best way to handle this is, because the adafruit_2_8_tft_touch_v2 shield assumes a standard SPI peripheral is exposed via the SPI pins on the arduino header (so it will use the MIPI DBI SPI driver). This might be a case where using a board overlay is the best we can do.

Yes, the LCD controller's pins are exposed in the same location as the SPI pins. So, my understanding is that we should stick with my initial approach, correct? In that case, I would like to ask what is the best way to add support for multiple LCD interfaces within the same <BOARD>.overlay file. A tried to use Kconfig symbols along with pre-processor directives but it seems that Kconfigs are not interpreted correctly (not sure if it's my fault or it's that such a scheme is not allowed in overlays.)

Yes, I think the initial approach is the best option. Kconfig symbols won't be handled correctly within a DTS file, no- Kconfig is processed after DTS in the build system, so DTS can't use Kconfig symbols.

In that case, I would like to ask what is the best way to add support for multiple LCD interfaces within the same <BOARD>.overlay file

If the goal here is to allow the user to choose which interface they are using via a switch at build time, then a single board overlay won't work. You could provide multiple board overlay files (either within the board folder, or within the display sample) that configure a different display interface for the board. Setting DTC_OVERLAY_FILE within the build command would then direct the build system to use the specific overlay for whichever display the user wanted

Understood.

danieldegrasse · 2024-03-04T23:45:34Z

drivers/mipi_dbi/mipi_dbi_smartbond.c

+
+static struct mipi_dbi_driver_api mipi_dbi_smartbond_driver_api = {
+#if MIPI_DBI_SMARTBOND_IS_RESET_AVAILABLE
+	.reset = mipi_dbi_smnartbond_reset,


Nit- typo in this function name

danieldegrasse · 2024-03-04T23:49:08Z

boards/renesas/da1469x_dk_pro/da1469x_dk_pro-pinctrl.dtsi

@@ -26,20 +26,20 @@

 	i2c2_default: i2c2_default {
 		group1 {
-			pinmux = <SMARTBOND_PINMUX(I2C2_SDA, 0, 19)>,
-				<SMARTBOND_PINMUX(I2C2_SCL, 0, 18)>;
+			pinmux = <SMARTBOND_PINMUX(I2C2_SDA, 0, 28)>,


Can you split these pin control fixes into separate commits? It should be clear in the git history why these are needed

danieldegrasse · 2024-03-04T23:50:26Z

boards/renesas/da1469x_dk_pro/Kconfig.defconfig

+
+# LCDC imposes display buffer be word aligned
+config LV_Z_VDB_ALIGN
+	default 4


This should not be needed- the default value is already 4: https://github.com/zephyrproject-rtos/zephyr/blob/main/modules/lvgl/Kconfig.memory#L66

Correct. The reason I also chose to add this Kconfig here is because buffer alignment is critical for LCD controller's operation. Nevertheless, I will remove it since the HAL API should catch any 'disrespect' to this requirement.

danieldegrasse · 2024-03-04T23:53:31Z

samples/modules/lvgl/demos/boards/da1469x_dk_pro_mipi_dbi.overlay

Just a suggestion here, since you are reusing a similar overlay to define this display between multiple samples. You might consider putting this overlay in the boards dts folder. For example, see how an overlay is included for many NXP boards to switch to a new ethernet driver: https://github.com/zephyrproject-rtos/zephyr/blob/3203bd660c6c8968e3047fb16b5721645f7ca696/boards/nxp/mimxrt1060_evk/dts/nxp%2Cenet-experimental.overlay

Thanks for the suggestion. I'd prefer to proceed with this kind of structural changes at a later point.

Add support for the MIPI DBI host controller. Signed-off-by: Ioannis Karachalios <[email protected]>

Update SoC DTS configurations to support the Renesas MIPI DBI host controller. Signed-off-by: Ioannis Karachalios <[email protected]>

danieldegrasse

Sorry- just thought about this. Can you add a testcase entry like the following

zephyr/samples/drivers/display/sample.yaml

Line 110 in a14ba6a

sample.display.dummy:

So that twister will build this sample with the MIPI DBI driver in the future?

Re-assign peripherals' default pins functionality to avoid conflicts with LCD controller's pins usage. The PRO DevKit exhibits a display socket which is routed to dedicated pins. Signed-off-by: Ioannis Karachalios <[email protected]>

Update board's DTS configurations to support the Renesas MIPI DBI host controller. Signed-off-by: Ioannis Karachalios <[email protected]>

Add overlay file to support the pro devkit. To build the sample code, one should explicitly select the overlay file at 'west build' invokation via the DTC_OVERLAY_FILE system variable. Signed-off-by: Ioannis Karachalios <[email protected]>

ioannis-karachalios · 2024-03-05T21:00:59Z

@danieldegrasse many thanks for reviewing this PR.

zephyrbot added area: Display Controller area: Devicetree Binding PR modifies or adds a Device Tree binding area: Samples Samples area: LVGL Light and Versatile Graphics Library Support platform: Renesas SmartBond Renesas Electronics Corporation, SmartBond labels Feb 1, 2024

zephyrbot requested review from andrzej-kaczmarek, brgl, danieldegrasse, faxe1008, galak, kartben, nashif and pdgendt February 1, 2024 14:51

zephyrbot assigned kartben Feb 1, 2024

zephyrbot added manifest manifest-hal_renesas DNM This PR should not be merged (Do Not Merge) labels Feb 1, 2024

kartben assigned andrzej-kaczmarek and unassigned kartben Feb 1, 2024

danieldegrasse reviewed Feb 1, 2024

View reviewed changes

danieldegrasse reviewed Feb 8, 2024

View reviewed changes

ioannis-karachalios force-pushed the da1469x-mipi-dbi branch 4 times, most recently from 964304c to 287ce0e Compare February 14, 2024 14:57

ioannis-karachalios requested a review from danieldegrasse February 14, 2024 15:30

ioannis-karachalios force-pushed the da1469x-mipi-dbi branch from 0482fb0 to 7af44c7 Compare February 27, 2024 09:51

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Feb 27, 2024

ioannis-karachalios force-pushed the da1469x-mipi-dbi branch 2 times, most recently from 3502f80 to c2f6269 Compare March 3, 2024 10:07

zephyrbot removed manifest manifest-hal_renesas labels Mar 3, 2024

ioannis-karachalios force-pushed the da1469x-mipi-dbi branch 3 times, most recently from 2b067e9 to 9b6041d Compare March 4, 2024 21:38

danieldegrasse reviewed Mar 4, 2024

View reviewed changes

ioannis-karachalios force-pushed the da1469x-mipi-dbi branch 2 times, most recently from df81b65 to b71a17c Compare March 5, 2024 09:12

ioannis-karachalios added 2 commits March 5, 2024 11:16

drivers: mipi_dbi: smartbond: Add support for MIPI DBI driver class.

862e3d7

Add support for the MIPI DBI host controller. Signed-off-by: Ioannis Karachalios <[email protected]>

dts: renesas: smartbond: Add support for the MIPI DBI host controller.

fdbef6a

Update SoC DTS configurations to support the Renesas MIPI DBI host controller. Signed-off-by: Ioannis Karachalios <[email protected]>

ioannis-karachalios force-pushed the da1469x-mipi-dbi branch 5 times, most recently from 9fc4504 to 13b9bd4 Compare March 5, 2024 09:44

ioannis-karachalios requested a review from danieldegrasse March 5, 2024 11:14

danieldegrasse reviewed Mar 5, 2024

View reviewed changes

ioannis-karachalios added 4 commits March 5, 2024 21:59

boards: renesas: smartbond: Add support for the MIPI DBI host driver

2f38761

Update board's DTS configurations to support the Renesas MIPI DBI host controller. Signed-off-by: Ioannis Karachalios <[email protected]>

ioannis-karachalios force-pushed the da1469x-mipi-dbi branch from 13b9bd4 to bd8cdad Compare March 5, 2024 20:14

danieldegrasse approved these changes Mar 5, 2024

View reviewed changes

andrzej-kaczmarek approved these changes Mar 5, 2024

View reviewed changes

fabiobaltieri merged commit 9b88dba into zephyrproject-rtos:main Mar 6, 2024
26 checks passed

Renesas: Smartbond: Add MIPI DBI Driver Support #68426

Renesas: Smartbond: Add MIPI DBI Driver Support #68426

Conversation

ioannis-karachalios commented Feb 1, 2024

zephyrbot commented Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldegrasse commented Feb 8, 2024

ioannis-karachalios commented Feb 8, 2024

danieldegrasse commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannis-karachalios Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannis-karachalios Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldegrasse left a comment

Choose a reason for hiding this comment

ioannis-karachalios commented Mar 5, 2024

zephyrbot commented Feb 1, 2024 •

edited

Loading

ioannis-karachalios Feb 14, 2024 •

edited

Loading

ioannis-karachalios Mar 5, 2024 •

edited

Loading