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 bindings: improve support for multiple compatibles #19904

Open
pabigot opened this issue Oct 17, 2019 · 5 comments
Open

devicetree bindings: improve support for multiple compatibles #19904

pabigot opened this issue Oct 17, 2019 · 5 comments
Assignees
Labels
area: Devicetree Enhancement Changes/Updates/Additions to existing features

Comments

@pabigot
Copy link
Collaborator

pabigot commented Oct 17, 2019

The st,lis12dh driver supports multiple variants: SPI and I2C buses, and at least two variant chips st,lis3dh and st,lis2dh12. PR #19901 adds a reference to the latter.

In the original submission checkpatch complained about:

-:11: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "st,lis2dh12" appears un-documented -- check ./dts/bindings/
#11: FILE: boards/arm/nrf52_pca20020/nrf52_pca20020.dts:123:
+		compatible = "st,lis2dh12", "st,lis2dh";

It appears that to make this work we need to duplicate the YAML with the name of the alternative. This means that the property descriptions are now replicated in four different binding files.

In #19624 it is proposed to add another property, which would have to be added to all those files. In an attempt to simplify that I prepared https://github.com/pabigot/zephyr/commits/pr/20191017b where the top commit refactors the properties to a file that's included by all the others.

This results in the following build failure:

-- Overlaying /mnt/nordic/zp/zephyr/dts/common/common.dts
devicetree error: both /mnt/nordic/zp/zephyr/dts/bindings/sensor/st,lis2dh-i2c.yaml and /mnt/nordic/zp/zephyr/dts/bindings/sensor/st,lis2dh-spi.yaml have 'compatible: st,lis2dh'
CMake Error at /mnt/nordic/zp/zephyr/cmake/dts.cmake:177 (message):
  new extractor failed with return code: 1

It's not obvious to me how the Linux checkpatch is satisfied by having st,lis2dh12 appear as a compatible in a file st,lis2dh12-i2c.yaml but without that file it complains. Nor is it obvious to me how putting the properties into a common include file causes breakage of the unique-compatible rule when duplicating them in separate files does not.

Nonetheless, having to replicate all properties in multiple binding files just to allow specification of variants to get past checkpatch is not a very maintainable solution. Something should be done.

@pabigot pabigot added Enhancement Changes/Updates/Additions to existing features area: Devicetree labels Oct 17, 2019
@ulfalizer
Copy link
Collaborator

ulfalizer commented Oct 22, 2019

Looks like scripts/checkpatch.pl is just doing a recursive grep for each compatible string in dts/bindings/. Maybe it could be hacked around with a comment like

# Mention this compatible just to make checkpatch.pl happy
# st,lis2dh12

Would that get awkward?

@pabigot
Copy link
Collaborator Author

pabigot commented Oct 23, 2019

I don't think it'd get awkward, but it should probably be handled by an explicit entry exposed in the YAML either by making the compatible key accept a list, or adding an also-compatible key with non-primary entries.

I think we may have touched on this before somewhere, regarding the change in how compatible works in devicetree between the original text model and DTSchema. Though in both cases the binding description is expected to have a list of acceptable compatibles.

If things like st,lis3dh-i2c.yaml exist only to satisfy this checkpatch requirement (which seems to be the case) I'd be in favor of moving to multiple compatibles in a single binding, however that gets done.

@pabigot
Copy link
Collaborator Author

pabigot commented Nov 15, 2019

See #20289 (comment) for additional information.

@mbolivar mbolivar changed the title devicetree bindings: improve support for compatible compatibles devicetree bindings: improve support for multiple compatibles Jan 28, 2020
@mbolivar
Copy link
Contributor

@pabigot I think the title of this issue was in error, so I've tried to fix it. Please revert if I'm wrong.

@zephyrbot
Copy link
Collaborator

Hi @galak,

This issue, marked as an Enhancement, was opened a while ago and did not get any traction. It was just assigned to you based on the labels. If you don't consider yourself the right person to address this issue, please re-assing it to the right person.

Please take a moment to review if the issue is still relevant to the project. If it is, please provide feedback and direction on how to move forward. If it is not, has already been addressed, is a duplicate, or is no longer relevant, please close it with a short comment explaining the reason.

@pabigot you are also encouraged to help moving this issue forward by providing additional information and confirming this request/issue is still relevant to you.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

5 participants