Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boards: shields: add rk055hdmipi4m shield definition #55493

Merged
merged 5 commits into from Jul 11, 2023

Conversation

danieldegrasse
Copy link
Collaborator

@danieldegrasse danieldegrasse commented Mar 6, 2023

Note- the PR depends on #53907, and contains commits from it. Those commits will be dropped once that PR merges

Add rk055hdmipi4m shield definition. This shield contains a 1280x720
display driven by an RM68200 IC, with a GT911 touch controller. The
display is supported by a 40 pin FFC connector present on NXP's RT1170
and RT595 EVKs.

Comment on lines 6 to 7
config DISPLAY
default y
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be set at application level, not as part of default configuration.
Same policy used in boards apply here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieldegrasse This does not seem to be addressed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^

@danieldegrasse
Copy link
Collaborator Author

@erwango Thank you for taking a look- requested changes should be addressed

erwango
erwango previously approved these changes May 15, 2023
backlight-gpios = <&mipi_connector 0 GPIO_ACTIVE_HIGH>;
};

&mipi_dsi {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.zephyrproject.org/latest/hardware/porting/shields.html#board-compatibility, do we consider mipi_dsi to be well-known connector? Should it not be zephyr_mipi_dsi?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mipi_dsi is the node definition being used for shields that currently define mipi displays: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/shields/st_b_lcd40_dsi1_mb1166/st_b_lcd40_dsi1_mb1166.overlay. Since we have a MIPI-DSI API, I think it is fair to define this nodelabel as a well known connector.

The alternative here would be to provide no shield overlay, and only board specific overlays within the shield, since the connection between these displays and the EVK does not use the Arduino headers, and is made via a 40 pin flat flexible cable. I think that defining this cable's pins as a GPIO nexus makes sense though, as the interface is shared between multiple NXP EVKs, and defining the shield this way allows more reuse than board specific overlays

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative here would be to provide no shield overlay, and only board specific overlays within the shield, since the connection between these displays and the EVK does not use the Arduino headers, and is made via a 40 pin flat flexible cable. I think that defining this cable's pins as a GPIO nexus makes sense though, as the interface is shared between multiple NXP EVKs, and defining the shield this way allows more reuse than board specific overlays

I know that it does not use Arduino headers or other well-known connectors. But IMO mipi_dsi is too SoC specific, there could be mipi_dsi0 and mipi_dsiX, therefore I think it make sense to use zephyr_mipi_dsi nodelabel at the board level as reference to default interface, like we do it using zephyr_udc0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm fine using that. We should do the same to the LCD controller, since the bindings for that are also generic with #53907

Do these node labels look ok to you?
zephyr_lcdif- for LCD interface controller
zephyr_mipi_dsi- for MIPI DSI controller
nxp_mipi_connector- for 40 pin connector (present on NXP EVKs, but any board could implement this)
nxp_mipi_i2c- for I2C interface exposed on the mipi connector. Used with the touch controller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfischer-no ping- do these names look good to you?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm fine using that. We should do the same to the LCD controller, since the bindings for that are also generic with #53907

Do these node labels look ok to you? zephyr_lcdif- for LCD interface controller zephyr_mipi_dsi- for MIPI DSI controller

LGTM, not sure if you need to prefix lcdif if it is referenced by mipi dsi node. @erwango ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not sure if you need to prefix lcdif if it is referenced by mipi dsi node. @erwango ?

TBH, I think I'm missing some background on this topic, so looks fine to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The display pipeline here works as follows:

LCDIF writes DPI packets to MIPI, which serializes those packets as DSI data and sends them to the RK055HDMIPI4M. This allows the display to be constantly refreshed, which is required as this is a video mode display. I believe this same pipeline is possible on ST devices using the LTDC and the MIPI-DSI. The only property here that is NXP specific is the nxp,lcdif. This is used by our MIPI driver to extract LCD configuration parameters, and avoids the display controller having to redeclare these parameters when using the mipi_dsi_attach function.

Comment on lines 6 to 7
config DISPLAY
default y
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^

@danieldegrasse
Copy link
Collaborator Author

@erwango @jfischer-no ping- can you revisit this PR?

Add rk055hdmipi4m shield definition. This shield contains a 1080x720
display driven by an RM68200 IC, with a GT911 touch controller. The
display is supported by a 40 pin FFC connector present on NXP's RT1170
and RT595 EVKs.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Remove display definition from RT1170 EVK, as this can now be handled
via a shield. Add gpio nexus defining GPIO routing to the FFC connector.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Remove display definition from RT595 EVK, as this is now supported by
the shield. Add gpio nexus for the FFC connector on this EVK.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Enable logging by default in kscan sample, so that user will see output
even if they do not have a keyboard connected that follows the keymap.
Add a specific testcase for boards that support the RK055HDMIPI4M
display.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Add a specific testcase for boards that support the RK055HDMIPI4M
display.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late minute question

samples/drivers/kscan/sample.yaml Show resolved Hide resolved
@dleach02 dleach02 merged commit 40849e4 into zephyrproject-rtos:main Jul 11, 2023
19 checks passed
@danieldegrasse danieldegrasse deleted the feature/rm68200_shield branch July 11, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants