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

devicetree: add phandles support for chosen properties #84890

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JarmouniA
Copy link
Collaborator

@JarmouniA JarmouniA commented Jan 29, 2025

This PR adds the necessary Python code to parse chosen properties that have multiple phandles as a value.
Also adds DeviceTree macros to get node at an index from a chosen property.
Other chosen properties are treated as if they are of type Phandles with 1 element, and as such, a '0' index is appended to them in generated DeviceTree header and CMake DTS data.

This will be usefull for adding LVGL multi-display support (#83567) and being able to get the display devices and their number from DT directly to create the buffers statically for all displays.

@JarmouniA JarmouniA force-pushed the dev_chosen_phandles branch 2 times, most recently from aa390e4 to 646a504 Compare January 29, 2025 23:10
@JarmouniA JarmouniA force-pushed the dev_chosen_phandles branch from 646a504 to 82a8e79 Compare January 30, 2025 19:22
This commit adds the necessary Python code to parse chosen properties
that have multiple phandles as a value.
Also adds devicetree macros to get nodes from chosen properties.
Other chosen props are treated as if they are Phandles with 1 element,
and as such, a '0' index is appended to them in generated devicetree
header CMake DTS data.

Signed-off-by: Abderrahmane JARMOUNI <[email protected]>
Copy link
Contributor

@mbolivar mbolivar left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I understand the goal is to support multiple displays, and the current zephyr,display chosen node is not enough.

I'm not sure about the approach in this PR, though. Let's discuss that first before we get into the specifics of the code changes here.

First, I can't find any precedent in linux for multiple-phandle-valued properties in chosen, and I'm reluctant to make zephyr's devicetree dialect even more different than what is in linux. See https://docs.zephyrproject.org/latest/build/dts/design.html#source-compatibility-with-other-operating-systems.

Second, I'm not sure this extension is necessary. It seems like zephyr,display could instead be generalized to a new zephyr,displays chosen node, pointing to a node that contains the relevant phandles property instead, without having to extend the magic features in our chosen node support:

{
    chosen { zephyr,displays = &displays; };
    displays: my-displays { displays = <d1 d2 d3>; };
};

I get that it's a little clunkier syntax-wise, but I'm not sure that avoiding the extra typing alone is worth the extra "magic", yet.

Did you consider that? If so, why was it rejected? Did you consider any other implementations?

Putting a -1 on here for now until we can resolve the approach for enabling the multiple displays feature.

@JarmouniA
Copy link
Collaborator Author

JarmouniA commented Jan 30, 2025

@mbolivar Thank you for the quick and detailed feedback!

First, I can't find any precedent in linux for multiple-phandle-valued properties in chosen, and I'm reluctant to make zephyr's devicetree dialect even more different than what is in linux. See https://docs.zephyrproject.org/latest/build/dts/design.html#source-compatibility-with-other-operating-systems.

Yeah I thought about this too to be honest, seems like a valid thing to consider...

Second, I'm not sure this extension is necessary. It seems like zephyr,display could instead be generalized to a new zephyr,displays chosen node, pointing to a node that contains the relevant phandles property instead, without having to extend the magic features in our chosen node support:

{
    chosen { zephyr,displays = &displays; };
    displays: my-displays { displays = <d1 d2 d3>; };
};

I get that it's a little clunkier syntax-wise, but I'm not sure that avoiding the extra typing alone is worth the extra "magic", yet.

Did you consider that? If so, why was it rejected? Did you consider any other implementations?

No I actually haven't, thanks for the suggestion, will try it and see if it can be used easily for multi-display support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants