zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.38k stars 6.36k forks source link

devicetree bindings: improve support for multiple compatibles #19904

Open pabigot opened 4 years ago

pabigot commented 4 years ago

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.

ulfalizer commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

See https://github.com/zephyrproject-rtos/zephyr/pull/20289#issuecomment-554483726 for additional information.

mbolivar commented 4 years ago

@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 commented 6 months ago

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!