-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
dts: sensor: remove lis3dh binding #31450
Conversation
f6e05e9
to
8cd460a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a comment to the lis2dh binding/driver that it works for lis3dh?
8cd460a
to
c0788c9
Compare
Yes. I've done that in a second commit, which @mbolivar-nordic should confirm. This may be a path to finally resolving #19904. I think there were other proposals to address that issue but I don't see them in tree; if they're in progress I can try to follow that standard instead. Also noticed, since #19904 reminded me, that this also has to be done for lis2dh12, though the in-tree devicetree sources for that were correct. |
c0788c9
to
3d6d8c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avisconti please review
@pabigot @mhelm
Thanks and sorry if I lost some info in the meantime |
@avisconti Thanks for pointing to #30104; I couldn't find it. I don't think the intent was ever to have a separate binding file for every compatible; that would be the wrong path. All the way back to #19904 what we've really needed was to have multiple compatibles identified in the binding file. So that's the path I'm following, hopefully it's not inconsistent with where #30104 ends up going. I can add the other devices to this, I just wasn't aware of everything the current driver supports. Is that list documented somewhere that I should have found? Basically I want to be able to |
…bles While there once was a separate driver for st,lis3dh support for that variant was merged into lis2dh. That driver only recognizes devices that are identified as st,lis2dh. Remove the lis3dh binding, and update in-tree devicetree nodes to add the compatible required by the driver. Similarly for the st,lis2dh12 binding. Also update the board/shield devicetree files so that nodes that are implemented by the st,lis2dh driver use the standard ordering of supported compatibles: most specific to least specific. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Add a standard documentation block to the st,lis2dh binding to identify its companion driver and variant devices also supported by that driver. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
3d6d8c4
to
76431e7
Compare
I've updated this with some significant changes:
This should be reviewed as an alternative or adjunct to #30104 per #31450 (comment) |
First of all let me explain to you what is what I'm asking for at the first place. I was requested by my management to provide more visibility to lis2dh12, which is what our marketing is promoting. As a matter of fact lis2dh12 is supported in zephyr through lis2dh, yet there are users that are asking us for support, because they just look under drivers/sensor and they do not find lis2dh12. So, my original request was to create a proper documentation for such cases, where a single driver is supporting multiple devices. After that I left the discussion with the understanding that we would have used the bindings to provide a proper documentation to all the devices (not only sensors) for which a driver is already existing. Now, I do not have anything against this PR. I like it. But I think we would need to come up to a solution to what my original request was. So, if bindings are going to be deleted what we are going to use instead? Maybe provide scripts that look for "compatible" strings and provide a list of devices? I think we should come up with a solution in #30104 if this PR will be accepted (which I think should). So, I'm not approving it yet, but just to give a chance to at least have a new proposal for documenting all devices supported. |
@avisconti Thanks, that helps. I proposed this to address #31253, but that's a low priority bug. Your needs are different, and I'm fine with having other infrastructure make this more visible and delaying this until there's a plan in place. I just don't want to see bindings that do nothing but include the same properties as another device when both are supported by the same driver and we can use the standard compatible property ordering to describe what's going on. |
Sure! Let's see what @mbolivar-nordic and @erwango have to say about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @avisconti explained, we should be able to document what hardware pieces are supported despite code factorization. And I still consider that compatibles should be the base to document that.
I'm fine with this PR as long as #30104 (or the next iteration) is able to take it into account and provide an adequate documentation out of it.
The LIS3DH sensor is supported by the same driver, and should be | ||
specified in devicetree with: | ||
|
||
compatible = "st,lis3dh", "st,lis2dh"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine now. But eventually, I'd prefer we have a new field in this compatible that provide the list of compatible compatibles and thanks to #30104 or alike, we should end up having a "st,lis3dh" entry in the doc (that would redirect to this binding and explaining how to use it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like:
compatible-list = "st,lsm303agr-accel", "st,lsm303dlhc-accel", "st,lis2dh12", "st,lis3dh", "st,lis2dh";
Something like that @erwango?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe:
zephyr,driver-kconfig = "LIS2DH";
to complete the coupling between compatible and (default) driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compatible-list = "st,lsm303agr-accel", "st,lsm303dlhc-accel", "st,lis2dh12", "st,lis3dh", "st,lis2dh";
For instance. and that would be part of binding meta data.
zephyr,driver-kconfig = "LIS2DH";
Still meta-data rigth ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new field in this compatible that provide the list of compatible compatibles
Wow, that's quite meta ;)
compatible-list = "st,lsm303agr-accel", "st,lsm303dlhc-accel", "st,lis2dh12", "st,lis3dh", "st,lis2dh";
Where does this go; in the YAML? Not in DT, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, all this is intended to be YAML; I got the key/value syntax wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in YAML. It was just a brainstorming.
The main problem I was trying to solve in #30104 is the way to associate a driver implementation with its Kconfig options and DTS compatibles. I tried to do this in Doxygen, but ran into various issues that make that approach impractical in my opinion. So what I would like to do instead is to basically take the Doxygen comments in this commit: And move them into .rst files in the drivers/ directory, the same way we have sample-specific documentation in the samples/ directory. That will let us write free-form text to document our drivers that lets us bind together the Kconfig and DT compatibles. We can put that somewhere in the reference pages. |
To summarize my high-level "requirements": Given a sensor like LIS2DH12 I want a way to quickly identify:
I'd expect to find all of that in one of the places revealed by the first search, either the generic sensor YAML, the specific sensor YAML if the specific sensor has additional properties, or the driver implementation file.
That'd work too. |
OK, let me try to see if I get your proposal. Bascally we need to provide a .rst file inside each driver folder (like driver/sensors/lis2dh) in which we are free to document whatever we feel is important to document. THis may be done with the help pof doxygen (even if I feel it is not 100% needed). But at the end in the zephyrproject documentation pages we will have a "driver supported" page beside to "Sample and Demos" and "Supported Boards". Is that what you mean? |
YEs, and this is commented in the proper way in this commit itself provided by you. I mean, inside the lis2dh yaml file. Moreover, if I'm getting properly what @mbolivar-nordic is proposing, we will also have a .rst file in lis2dh driver which will describe all the compatibles required for each sensor supported by lis2dh driver. |
Let's target the generated documentation option. Based on my short community support experience, grepping the tree doesn't answer most of the questions we usually get (or if it would be an option we'd get much less questions).
And these would be sorted by compatible in the documentation ? |
If the free-form comments I put in the YAML of the base driver compatible are instead in RST, they don't have to be replicated in the YAML. In fact, my proposal for fixing this PR would be to move them to such a file. If we go that way, I don't think we need the Edit: Actually, if we do have the driver Kconfig, we might be able to link from the generated YAML documentation to the corresponding generated driver documentation. |
Yes, I think that having them in a single place reduce the future info maintenance.
compatible-list was proposed to just have a way to automatically generate/document a list of devices supported by a given driver. I think we do not need it anymore if we go in the per-driver .rst direction. Concerning the other attribute for KConfig I'm not sure. Let's think about it. Anyway, I have the feeling that we are not all 100% aligned. Am I wrong? |
Which means we'd need .rst file per specific sensor, even of support is provided through generic driver ? |
No, I assumed one |
Then for I2C, I assume we'd go for I2C.rst ( as opposed to I2C_STM32.rst, I2C_NRF.rst, ...). This means in the general doc, we'd have 2 sets entries:
This would solve the issue of having bindings that do not content Zephyr specific Kconfig symbols. |
? No, I was proposing using the driver Kconfig, not the subsystem Kconfig, so probably Ugh. Something to revisit in 2.6. |
I'm not quite for this proposal. To me, these symbols shouldn't be user selectable, user should only care to enable I2C and the matching I2C_VENDOR should be selected, either through Kconfig dependencies, either thanks to identification of enabled i2c compatibles in board dts. |
I do think a user should always be able to tell, given a compatible, what driver supports it, and vice-versa. |
Yes, I agree. In linux the compatible string is in the driver itself. In zephyr case is in YAML. So, I guess that going with something like lis2dh.rst, i2c_stm32_v1.rst, ... would be appropriate. |
I'm affraid this would be misleading. Documenting things as proposed: <st,stm32-i2c-v1.yaml, CONFIG_I2C_STM32_V1, i2c_ll_stm32_v1.rst> puts into light vendor symbol that user shouldn't care is majority of cases and omit the important portability information ("driver requires I2C=y"). |
I think that we are talking about different requirements. Usually one may want to know if a give sensor is supported, but usually does not care whether a particular I2C controller is supported or not. In that case usually one may just want to know if a particular SoC or even board is supported. So, in this case would a single I2C.rst file that is documenting all compatibles for all I2C bus controllers better? What I feel is really necessary is to have a single page that lists all devices supported and what is the driver associated. This should be easily extracted from the compatibles. How to do that I don't know exactly. Moreover the idea of having a rst for each driver does not solve the immediate doubt whether lis2dh12 is supported or not, because you have to know in advance that is lis2dh. So, I'm coming back with the original idea to automatically extract from the bindings the list of compatibles and create such a page. |
Yes.
No. Maybe the role of doxygen is confusing because #30104 used it? TL;DR I tried to use doxygen in #30104 but it didn't/doesn't work. So I do not think we should use doxygen for this. See #30104 for details.
Yes, but...
No, I'm not proposing that location; what I wrote was:
And by that I meant here: https://docs.zephyrproject.org/latest/reference/index.html. This is where the APIs are documented. Thinking about it more I believe we should have a list of drivers that we link to from the documentation for each API. So for example https://docs.zephyrproject.org/latest/reference/peripherals/adc.html#adc-api would link to the drivers implementing the API, or perhaps link to an index page of all the drivers. |
@mbolivar-nordic But there is a second part which is currently not covered yet, and that is how a user may find the information if a given device is currently supported or not, and by which driver. And the example I had in mind was lis2dh12, which is supported by lis2dh. If we only document lis2dh driver the user has to guess that lis2dh driver is covering more than one device. Instead, I think we should have also a page where all supported devices are listed. At the end is a compatible list. |
I imagined that a driver's documentation would include a list of compatibles it supported, so they could be found via freetext search on the docs site. But I have no objections to a generated index of the same; I think Sphinx has the support we need for auto-generating indices. |
That would be great. I'm not sure how many people would grep the device name in the source code to see if some driver is supporting it. |
It was one of my requirements. But yes, I suspect others might prefer a more friendly approach. |
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
I'm not working to solve this anymore and this is far too old to merge. |
While there once was a separate driver for st,lis3dh support for that variant was merged into lis2dh. That driver only recognizes devices that are identified as st,lis2dh. Remove the lis3dh binding, and update in-tree devicetree nodes to add the compatible required by the driver.
Also st,lis2dh12 same thing. Also propose standard documentation block to relate compatibles with bindings and drivers.
(NB: Prior to this I verified that neither of the affected in-tree boards could build the lis2dh sample because the required devicetree binding macros were not generated.)
Fixes #31253.