boards: shield: support st_b_lcd40_dsi1_mb1166 A09 #70350

erian747 · 2024-03-17T20:51:03Z

Adds support for the A09 version of STM32H747I Dsiocery shield MB1166

Changes:
Add display driver NT35510 mounted on the MB1166-A09 version

Add new shield revision in boards/shields/st_b_lcd40_dsi1_mb1166

Update STM32 DSI driver with a few more device tree options required
to support NT35510

Tested on a STM32H747I Discovery board with a mounted MB1166-A09 shield
NOT tested with the older revision of shield as i don't own any
Old shield configuration compiles ok but maybe it would be good if someone having an old shield
could make a quick test and see nothing has been broken after restructuring in shield folder

github-actions · 2024-03-17T20:51:41Z

Hello @erian747, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

erwango · 2024-03-19T14:48:29Z

Thanks @erian747 for this contribution. Please rebase on top of main branch to fix reported conflicts

ajarmouni-st · 2024-03-19T15:00:16Z

@erian747 The conflicts are most likely related to HWMv2, checkout https://docs.zephyrproject.org/latest/hardware/porting/board_porting.html for more info.

erian747 · 2024-03-19T22:35:46Z

Thanks!
Yes, i had to rebase and adapt some file-names to match HWMv2, seems to build ok now

ajarmouni-st · 2024-03-20T08:08:44Z

@erian747 you have compliance issue in CI (you can run the checks locally before pushing using
/scripts/ci/check_compliance.py --commits HEAD~n..HEAD with n the number of commits you want to push)

Error: Git conflict markers and whitespace errors are not allowed in added changes

 2dd8cb355585e464aded9600c108daeb14eb46e0: dts/bindings/mipi-dsi/st,stm32-mipi-dsi.yaml:53: trailing whitespace.
Error: Check YAML files with YAMLLint.

 YAMLLint (trailing-spaces):trailing spaces
File:dts/bindings/mipi-dsi/st,stm32-mipi-dsi.yaml
Line:53
Column:62
Error: See https://docs.zephyrproject.org/latest/contribute/guidelines.html#coding-style for more details.

 Missing newline at end of 'boards/shields/st_b_lcd40_dsi1_mb1166/boards/st_b_lcd40_dsi1_mb1166/stm[32](https://github.com/zephyrproject-rtos/zephyr/actions/runs/8350265354/job/22869993152?pr=70350#step:9:33)h747i_disco_stm32h747xx_m7.overlay'. Check your text editor settings.
 Missing newline at end of 'boards/shields/st_b_lcd40_dsi1_mb1166/boards/st_b_lcd40_dsi1_mb1166_a09/stm32h747i_disco_stm32h747xx_m7.conf'. Check your text editor settings.
Error: See https://docs.zephyrproject.org/latest/contribute/guidelines.html#coding-style for more details.

 BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
File:drivers/display/display_nt[35](https://github.com/zephyrproject-rtos/zephyr/actions/runs/8350265354/job/22869993152?pr=70350#step:9:36)510.c
Line:186
 C99_COMMENTS: do not use C99 // comments
File:drivers/display/display_nt35510.c
Line:187
 C99_COMMENTS: do not use C99 // comments
File:drivers/display/display_nt35510.c
Line:191
 C99_COMMENTS: do not use C99 // comments
File:drivers/display/display_nt35510.c
Line:195
 C99_COMMENTS: do not use C99 // comments
File:drivers/display/display_nt35510.c
Line:199
 BLOCK_COMMENT_STYLE: Block comments use * on subsequent lines
File:drivers/display/display_nt35510.c
Line:206
 BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
File:drivers/display/display_nt35510.c
Line:207
 LINE_SPACING: Missing a blank line after declarations
File:drivers/display/display_nt35510.c
Line:239
 LONG_LINE_COMMENT: line length of 104 exceeds 100 columns
File:drivers/display/display_nt35510.h
Line:73
 LONG_LINE_COMMENT: line length of 108 exceeds 100 columns
File:drivers/display/display_nt35510.h
Line:74
 LONG_LINE_COMMENT: line length of 108 exceeds 100 columns
File:drivers/display/display_nt35510.h
Line:75
 LONG_LINE: line length of 105 exceeds 100 columns
File:drivers/mipi_dsi/dsi_stm32.c
Line:495
 LONG_LINE: line length of 105 exceeds 100 columns
File:drivers/mipi_dsi/dsi_stm32.c
Line:498
 TRAILING_WHITESPACE: trailing whitespace
File:dts/bindings/mipi-dsi/st,stm32-mipi-dsi.yaml
Line:53
Error: Process completed with exit code 1.

danieldegrasse · 2024-03-20T14:31:31Z

drivers/display/Kconfig.nt35510

+
+if NT35510
+
+config DISPLAY_NT35510_INIT_PRIORITY


Why is the custom init priority needed here? Is this because of the dependency this display will have on the LTDC and the MIPI DSI?

Yes, it was discussed here #68527 (comment)

danieldegrasse · 2024-03-20T14:40:57Z

drivers/display/display_nt35510.c

+	return ret;
+}
+
+static int nt35510_write(const struct device *dev, uint16_t x, uint16_t y,


These stub functions aren't needed- the display API will return -ENOSYS if they are not present

Ok, so if i understand you correctly, you mean i can remove
the stub and let write function pointer be initialized to NULL?

Yes, exactly. Just remove the function definition and entry in the API structure.

danieldegrasse · 2024-03-20T14:41:25Z

drivers/display/display_nt35510.c

+static int nt35510_set_brightness(const struct device *dev, const uint8_t brightness)
+{
+	return nt35510_write_reg(dev, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, &brightness, 1);
+	return 0;


Second return not needed

Indeed, will fix :)

danieldegrasse · 2024-03-20T14:41:58Z

drivers/display/display_nt35510.c

+{
+	struct nt35510_data *data = dev->data;
+
+	data->dsi_pixel_format = pixel_format;


Should there be a check here to verify the requested pixel format is actually supported?

Yes you are right, and "enum display_pixel_format" must also be mapped into MIPI_DSI_PIXFMT_xxx values
Also realized that resulting color is hardcoded in mipi_dsi_stm32_attach (dsi_stm32.c):
"vcfg->ColorCoding = STM32_DSI_INIT_PIXEL_FORMAT" which comes from LTDC color configuration
But i will fix the verification and mapping to DSI colors in display_nt35510.c

danieldegrasse · 2024-03-20T14:48:22Z

drivers/display/display_nt35510.c

+	static const uint8_t nt35510_caset_landscape[] = {0x00, 0x00, 0x03, 0x1F};
+	static const uint8_t nt35510_raset_landscape[] = {0x00, 0x00, 0x01, 0xDF};
+
+	ret = nt35510_write_reg(dev, 0xF0, nt35510_reg, 5);   /* LV2:  Page 1 enable */


I'd prefer to avoid using this method of checking the return code- it makes it difficult for a developer to see where the write actually failed when debugging (since they would need to check the value of ret each time).

Maybe put these initialization commands into a const array, and loop over that array? Something like the following:

struct nt35510_init_cmd { uint8_t reg; uint8_t cmd_len; uint8_t cmd[] }; static const struct nt35510_init_cmd init_cmds[] = { { .reg = 0xF0, /* LV2: Page 1 enable */ .cmd_len = 5, .cmd = {0x03, 0x03, 0x03}, }; .... struct nt35510_init_cmd *cmd; for (int i = 0; i < ARRAY_SIZE(init_cmds); i++) { cmd = init_cmds[i]; ret = nt35510_write_reg(dev, cmd->reg, cmd->cmd, cmd->cmd_len); if (ret < 0) { return ret; } }

Agree, will possibly occupy less program space too
I will borrow your example above and extend it with a delay struct member needed for those two
delays required in initialization sequence

danieldegrasse · 2024-03-20T14:48:58Z

dts/bindings/display/frida,nt35510.yaml

+      - 180
+      - 270
+    description: |
+      Display rotation (CW) in degrees. Defaults to 0, display default.


Nit- spell clockwise out, no need to abbreviate in documentation

drivers/display/display_nt35510.h

drivers/display/display_nt35510.c

boards/shields/st_b_lcd40_dsi1_mb1166/doc/index.rst

ajarmouni-st · 2024-03-20T15:04:16Z

drivers/display/Kconfig.nt35510

+
+if NT35510
+
+config DISPLAY_NT35510_INIT_PRIORITY


Yes, it was discussed here #68527 (comment)

To support the NT35510 display, some additional options needs to be configurable in the STM32 DSI peripheral Signed-off-by: Erik Andersson <erian747@gmail.com>

For now DSI settings are hard-coded for the specific LCD module used on the STM32H747I Discovery board Signed-off-by: Erik Andersson <erian747@gmail.com>

Add support for the A09 version of MB1166 which have a NT35510 panel controller instead of an OTM8009a as in prior versions Fixes zephyrproject-rtos#60888 Signed-off-by: Erik Andersson <erian747@gmail.com>

ajarmouni-st · 2024-03-22T10:32:48Z

Hi @erian747 just tested your PR on an stm32h747i-disco kit with otm8009a controller & it works fine with no apparent issues.
However, can you add a commit with the following patch so that the new shield revision is also covered in CI. Thanks!

diff --git a/samples/drivers/display/sample.yaml b/samples/drivers/display/sample.yaml
index c6188d37d1..50038edf93 100644
--- a/samples/drivers/display/sample.yaml
+++ b/samples/drivers/display/sample.yaml
@@ -137,6 +137,7 @@ tests:
     harness_config:
       fixture: fixture_display
   sample.display.st_b_lcd40_dsi1_mb1166:
+    filter: dt_compat_enabled("orisetech,otm8009a")
     platform_allow: stm32h747i_disco/stm32h747xx/m7
     extra_args: SHIELD=st_b_lcd40_dsi1_mb1166
     tags:
@@ -164,3 +165,13 @@ tests:
     harness_config:
       fixture: fixture_display
     tags: display
+  sample.display.st_b_lcd40_dsi1_mb1166_a09:
+    filter: dt_compat_enabled("frida,nt35510")
+    platform_allow: stm32h747i_disco/stm32h747xx/m7
+    extra_args: SHIELD=st_b_lcd40_dsi1_mb1166_a09
+    tags:
+      - display
+      - shield
+    harness: console
+    harness_config:
+      fixture: fixture_display
diff --git a/samples/modules/lvgl/demos/sample.yaml b/samples/modules/lvgl/demos/sample.yaml
index 4184f7df1a..5a9d668d02 100644
--- a/samples/modules/lvgl/demos/sample.yaml
+++ b/samples/modules/lvgl/demos/sample.yaml
@@ -25,6 +25,7 @@ tests:
     extra_configs:
       - CONFIG_LV_Z_DEMO_WIDGETS=y
   sample.modules.lvgl.demos.st_b_lcd40_dsi1_mb1166:
+    filter: dt_compat_enabled("orisetech,otm8009a")
     platform_allow: stm32h747i_disco/stm32h747xx/m7
     extra_args: SHIELD=st_b_lcd40_dsi1_mb1166
     harness: console
@@ -40,3 +41,20 @@ tests:
       - shield
       - lvgl
       - gui
+  sample.modules.lvgl.demos.st_b_lcd40_dsi1_mb1166_a09:
+    filter: dt_compat_enabled("frida,nt35510")
+    platform_allow: stm32h747i_disco/stm32h747xx/m7
+    extra_args: SHIELD=st_b_lcd40_dsi1_mb1166_a09
+    harness: console
+    harness_config:
+      fixture: fixture_display
+    extra_configs:
+      - CONFIG_LV_Z_DEMO_BENCHMARK=y
+    modules:
+      - lvgl
+    tags:
+      - samples
+      - display
+      - shield
+      - lvgl
+      - gui
diff --git a/samples/subsys/display/lvgl/sample.yaml b/samples/subsys/display/lvgl/sample.yaml
index c0ab07d48e..d8bfb5de9b 100644
--- a/samples/subsys/display/lvgl/sample.yaml
+++ b/samples/subsys/display/lvgl/sample.yaml
@@ -41,6 +41,7 @@ tests:
     integration_platforms:
       - mimxrt1170_evk/mimxrt1176/cm7
   sample.subsys.display.lvgl.st_b_lcd40_dsi1_mb1166:
+    filter: dt_compat_enabled("orisetech,otm8009a")
     platform_allow: stm32h747i_disco/stm32h747xx/m7
     extra_args: SHIELD=st_b_lcd40_dsi1_mb1166
     harness: console
@@ -54,3 +55,18 @@ tests:
       - shield
       - lvgl
       - gui
+  sample.subsys.display.lvgl.st_b_lcd40_dsi1_mb1166_a09:
+    filter: dt_compat_enabled("frida,nt35510")
+    platform_allow: stm32h747i_disco/stm32h747xx/m7
+    extra_args: SHIELD=st_b_lcd40_dsi1_mb1166_a09
+    harness: console
+    harness_config:
+      fixture: fixture_display
+    modules:
+      - lvgl
+    tags:
+      - samples
+      - display
+      - shield
+      - lvgl
+      - gui

Add tests for shield st_b_lcd40_dsi1_mb1166_a09 in: samples/drivers/display samples/modules/lvgl/demos samples/subsys/display/lvgl Signed-off-by: Erik Andersson <erian747@gmail.com>

erian747 · 2024-03-22T14:23:45Z

Applied patch from @ajarmouni-st
Don't know if commit message is 100% correct, but let me now in that case and i can change it

danieldegrasse · 2024-03-22T14:51:34Z

drivers/display/display_nt35510.c

+struct nt35510_init_cmd {
+	uint8_t reg;
+	uint8_t cmd_len;
+	uint8_t cmd[5];


Don't know if you care to do this, but you can probably save flash space by making this cmd field a pointer, and then declaring the init command arrays separately (like so):

static const uint8_t lv2_cmds[] = {0x55, 0xaa, 0x52, 0x08, 0x01}; static const struct nt35510_init_cmd init_cmds[] = { /* LV2: Page 1 enable */ {.reg = 0xf0, .cmd_len = sizeof(lv2_cmds), .cmd = lv2_cmds},

Yes i also taught in that direction to avoid a fixed size array of parameter data (cmd[5])
But a bit cumbersome of having lots of pointers and extra separate const arrays

One other option that might can be considered is the format that follows in example code below
Most compact variant and still enough human readable in my opinion, if formatted correctly
and comments are used

What do you think?

/* * LCD controller initialization data * First byte is command ( or register ) (CMD) * Second byte is N number of parameter bytes following * Next is N bytes of parameter data * * [CMD] [N] [DATA[0]...[DATA[N] ] * Directly after last data byte next command / register follows * Sequence ends when N equals 255 */ static const uint8_t init_cmds[] = { /* LV2: Page 1 enable */ 0xf0, 5, 0x55, 0xaa, 0x52, 0x08, 0x01, /* AVDD: 5.2V */ 0xb0, 3, 0x03, 0x03, 0x03, /* AVDD: Ratio */ 0xb6, 3, 0x46, 0x46, 0x46, /* AVEE: -5.2V */ 0xb1, 3, 0x03, 0x03, 0x03, /* AVEE: Ratio */ 0xb7, 3, 0x36, 0x36, 0x36, /* Example EOL */ 0xff,0xff

We could do something like this, my concern is implementing parsing code for that case seems unnecessarily complicated. Truthfully I'm not very worried about the extra few bytes used by this approach- the parts this display will be targeting generally have a lot of flash space.

If you want to implement this method or the one I suggested above, feel free to- but I'm also fine with the implementation as is

Yes that's true, a few bytes extra here would not be critical in a system with a high resolution display :)
Then i leave it as it is right now

danieldegrasse

Display changes look good to me

ajarmouni-st

Great work, thanks!

erian747 · 2024-03-22T16:37:49Z

Great work, thanks!

Thank you, now that i have learnt atleast i little bit of "ways of working" with Zephyr, so i can
make more PR:s in the future that are better tested locally before submitted :)

..._lcd40_dsi1_mb1166/boards/st_b_lcd40_dsi1_mb1166_a09/stm32h747i_disco_stm32h747xx_m7.overlay

erian747 · 2024-03-25T08:40:18Z

Yes, im pretty sure Motherboard and Shield revisions are changed independently of each other Den mån 25 mars 2024 09:30Abderrahmane ***@***.***> skrev:

…

***@***.**** commented on this pull request. ------------------------------ On boards/shields/st_b_lcd40_dsi1_mb1166/boards/st_b_lcd40_dsi1_mb1166_a09/stm32h747i_disco_stm32h747xx_m7.overlay <#70350 (comment)> : I don't think so, the board is still the same, only the display shield was revised. — Reply to this email directly, view it on GitHub <#70350 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC4NPBXEI3IB3NGTHN5WIRDYZ7OCXAVCNFSM6AAAAABE2P54B6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNJXGEYTCMRVG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Explain the different versions of the MB1166 shield and remove note about that version A09 is not supported Signed-off-by: Erik Andersson <erian747@gmail.com>

github-actions · 2024-04-03T01:04:49Z

Hi @erian747!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

erian747 requested review from erwango, ABOSTM and FRASTM as code owners March 17, 2024 20:51

zephyrbot added platform: STM32 ST Micro STM32 area: MIPI-DSI area: Shields Shields (add-on boards) area: Devicetree Binding PR modifies or adds a Device Tree binding area: Display labels Mar 17, 2024

zephyrbot requested review from avisconti, danieldegrasse, Desvauxm-st, galak, gautierg-st, GeorgeCGV, jfischer-no and kartben March 17, 2024 20:51

zephyrbot assigned erwango Mar 17, 2024

erwango requested a review from ajarmouni-st March 18, 2024 08:09

erian747 force-pushed the add_display_driver_nt35510 branch from 093edc6 to 2353ffd Compare March 19, 2024 21:20

danieldegrasse reviewed Mar 20, 2024

View reviewed changes

ajarmouni-st reviewed Mar 20, 2024

View reviewed changes

erian747 added 3 commits March 21, 2024 22:08

drivers: dsi: stm32: add additional dt options

fdb13a8

To support the NT35510 display, some additional options needs to be configurable in the STM32 DSI peripheral Signed-off-by: Erik Andersson <erian747@gmail.com>

drivers: display: add driver for frida,nt35510

dc97114

For now DSI settings are hard-coded for the specific LCD module used on the STM32H747I Discovery board Signed-off-by: Erik Andersson <erian747@gmail.com>

board: shield: st_b_lcd40_dsi1_mb1166 A09 support

c61c6c0

Add support for the A09 version of MB1166 which have a NT35510 panel controller instead of an OTM8009a as in prior versions Fixes zephyrproject-rtos#60888 Signed-off-by: Erik Andersson <erian747@gmail.com>

erian747 force-pushed the add_display_driver_nt35510 branch from 2353ffd to c61c6c0 Compare March 21, 2024 21:14

samples: ci: add st_b_lcd40_dsi1_mb1166_a09 shield

4c1c662

Add tests for shield st_b_lcd40_dsi1_mb1166_a09 in: samples/drivers/display samples/modules/lvgl/demos samples/subsys/display/lvgl Signed-off-by: Erik Andersson <erian747@gmail.com>

zephyrbot added the area: Samples Samples label Mar 22, 2024

zephyrbot requested a review from nashif March 22, 2024 14:21

danieldegrasse reviewed Mar 22, 2024

View reviewed changes

danieldegrasse previously approved these changes Mar 22, 2024

View reviewed changes

ajarmouni-st previously approved these changes Mar 22, 2024

View reviewed changes

erwango requested changes Mar 25, 2024

View reviewed changes

..._lcd40_dsi1_mb1166/boards/st_b_lcd40_dsi1_mb1166_a09/stm32h747i_disco_stm32h747xx_m7.overlay Outdated Show resolved Hide resolved

doc: boards: update stm32h747i_disco documentation

a8faffa

Explain the different versions of the MB1166 shield and remove note about that version A09 is not supported Signed-off-by: Erik Andersson <erian747@gmail.com>

erian747 dismissed stale reviews from ajarmouni-st and danieldegrasse via a8faffa March 25, 2024 18:32

erian747 requested a review from erwango March 27, 2024 21:15

ajarmouni-st approved these changes Mar 28, 2024

View reviewed changes

ajarmouni-st requested a review from danieldegrasse April 2, 2024 15:39

erwango approved these changes Apr 2, 2024

View reviewed changes

danieldegrasse approved these changes Apr 2, 2024

View reviewed changes

nashif merged commit 7d9fa5f into zephyrproject-rtos:main Apr 3, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boards: shield: support st_b_lcd40_dsi1_mb1166 A09 #70350

boards: shield: support st_b_lcd40_dsi1_mb1166 A09 #70350

erian747 commented Mar 17, 2024

github-actions bot commented Mar 17, 2024

erwango commented Mar 19, 2024

ajarmouni-st commented Mar 19, 2024

erian747 commented Mar 19, 2024

ajarmouni-st commented Mar 20, 2024

danieldegrasse Mar 20, 2024

ajarmouni-st Mar 20, 2024

danieldegrasse Mar 20, 2024

erian747 Mar 20, 2024

danieldegrasse Mar 20, 2024

danieldegrasse Mar 20, 2024

erian747 Mar 20, 2024

danieldegrasse Mar 20, 2024

erian747 Mar 20, 2024

danieldegrasse Mar 20, 2024

erian747 Mar 20, 2024

danieldegrasse Mar 20, 2024

ajarmouni-st Mar 20, 2024

ajarmouni-st commented Mar 22, 2024

erian747 commented Mar 22, 2024

danieldegrasse Mar 22, 2024

erian747 Mar 22, 2024 •

edited

danieldegrasse Mar 22, 2024

erian747 Mar 22, 2024

danieldegrasse left a comment

ajarmouni-st left a comment

erian747 commented Mar 22, 2024

erian747 commented Mar 25, 2024 via email

github-actions bot commented Apr 3, 2024

boards: shield: support st_b_lcd40_dsi1_mb1166 A09 #70350

boards: shield: support st_b_lcd40_dsi1_mb1166 A09 #70350

Conversation

erian747 commented Mar 17, 2024

github-actions bot commented Mar 17, 2024

erwango commented Mar 19, 2024

ajarmouni-st commented Mar 19, 2024

erian747 commented Mar 19, 2024

ajarmouni-st commented Mar 20, 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

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

ajarmouni-st commented Mar 22, 2024

erian747 commented Mar 22, 2024

Choose a reason for hiding this comment

erian747 Mar 22, 2024 • edited

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

ajarmouni-st left a comment

Choose a reason for hiding this comment

erian747 commented Mar 22, 2024

erian747 commented Mar 25, 2024 via email

github-actions bot commented Apr 3, 2024

erian747 Mar 22, 2024 •

edited