bindings: nxp,dai-sai: shorten property description and add configuration examples #85094
Conversation
The property descriptions contain a lot of unneeded and driver- specific information. As such, make them shorter while removing driver-specific information. Signed-off-by: Laurentiu Mihalcea <[email protected]>
Add some examples depicting how the SAI node may be configured. Signed-off-by: Laurentiu Mihalcea <[email protected]>
zephyrbot
requested review from
dbaluta,
decsny,
dleach02,
iuliana-prodan,
juimonen,
kv2019i,
lgirdwood,
manuargue,
mmahadevan108 and
Raymond0225
|
Thanks @LaurentiuM1234! I think next time we can place this kind of information in the commit message, as users will usually browse git log when trying to understand parts of the code! |
Not to be the contrarian here but I hate having to search commit messages to find information that could easily just be a comment in the code. |
Sure, but this is not the case. We are now talking about adding extra information in a document that needs to be rapidly read by someone when creating a dts node. In general, I agree with you. Comments are preferred over messages in the git log. |
I will go on the record here to say the main reason that these descriptions needed to change is because they are driver specific. The DT bindings are supposed to be able to be used in any software environment, with any driver, even outside of the context of Zephyr, or by multiple drivers within Zephyr. The hardware description should not have to change when a software change is made. cc @mbolivar to double check agreement about this principle now that he is back, as it is something I have been harping on while he was gone |
| value so use with caution. If unsure, it's better to simply not | ||
| use this property, in which case the reported value will be | ||
| DEFAULT_FIFO_DEPTH. | ||
| description: Size (in FIFO words) of the TX/RX FIFO. |
There was a problem hiding this comment.
I find some of the description about how to pick the value (between 0 to DEFAULT_FIFO_DEPTH) useful to include here.
|
Unfortunately, I think we're bound to lose some information here. The bindings should simply not contain any driver-related information. This is what the PR attempts to fix. I've added some examples meant to alleviate this problem but they might not be as useful as the previously overly verbose information to someone looking at this binding for the first time (EDIT: or dealing with the IP for the first time) I don't think looking through the commit history is ideal but it's better than nothing? Alternatively, we could try to move some of the documentation to the driver but, again, it's not really ideal as we might end up with something like https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/interrupt_controller/intc_nxp_irqsteer.c, which I wouldn't really want as it makes navigating through the code harder (the SAI driver already has ~1k lines of code). Anyhow, please feel free to block this if you believe some descriptions could be better formulated or if you have a better idea on how we should proceed with this while still sticking to the rules of writing DT bindings. |
|
@LaurentiuM1234 I understand the goal of not putting driver specifics in the bindings. Unfortunately we don't really have a better option in zephyr and we have had to make some pragmatic choices in the past to put things in the bindings even if they apply to the upstream driver. Note that Zephyr is not alone in this; even linux mentions specific drivers from some bindings: In general I'd be inclined to leave this as-is if you were a random person, but since you work for the vendor, I'm fine with deferring to your company's community members on whether or not to merge this. Hence my +1. |
|
There are already examples for NXP of having multiple drivers for the same IP in zephyr, nd having totally orthogonal bindings, like edma, we want to avoid this type of thing |
|
Got it, thanks @decsny and noted. Glad y'all are making use of this layer of indirection for its intended purpose. |
Property descriptions contain a lot of driver-specific and unneeded information. Make them shorter.
Also, add some configuration examples. This should be more helpful than having overly verbose property descriptions.