| Message ID | 20220808075452.115907-1-tmaimon77@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v3] dt-binding: ipmi: add fallback to npcm845 compatible | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 8 lines checked |
| robh/patch-applied | fail | build log |
On 08/08/2022 09:54, Tomer Maimon wrote: > Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible > string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules. > > Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible") > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Mon, Aug 08, 2022 at 11:11:16AM +0300, Krzysztof Kozlowski wrote: > On 08/08/2022 09:54, Tomer Maimon wrote: > > Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible > > string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules. > > > > Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible") > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Ok, I think I understand how this is supposed to work. It's not altogether clear from the device tree documentation. It says in Documentation/devicetree/bindings/writing-bindings.rst: - DO make 'compatible' properties specific. DON'T use wildcards in compatible strings. DO use fallback compatibles when devices are the same as or a subset of prior implementations. DO add new compatibles in case there are new features or bugs. AFAICT, there are no new features or bugs, just a new SOC with the same device. In general usage I have seen, you would just use the same compatible. However, if I understand this, that last sentence should say: DO add new compatibles in case there is a new version of hardware with the possibility of new features and/or bugs. Also, the term "specific" is, ironically, vague. Specific to what? It would be nice to have something added to "Typical cases and caveats" that says: - If you are writing a binding for a new device that is the same as, or a superset of another existing device, add a new specific compatible for the new device followed by a compatible for the existing device. That way, if the device has new bugs or new specific features are added, you can add workarounds without modifying the device tree. Anyway, I have added this to my tree with your ack. -corey > > > Best regards, > Krzysztof
On 08/08/2022 15:26, Corey Minyard wrote: > On Mon, Aug 08, 2022 at 11:11:16AM +0300, Krzysztof Kozlowski wrote: >> On 08/08/2022 09:54, Tomer Maimon wrote: >>> Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible >>> string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules. >>> >>> Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible") >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> >> >> >> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Ok, I think I understand how this is supposed to work. It's not > altogether clear from the device tree documentation. It says in > Documentation/devicetree/bindings/writing-bindings.rst: > > - DO make 'compatible' properties specific. DON'T use wildcards in compatible > strings. DO use fallback compatibles when devices are the same as or a subset > of prior implementations. DO add new compatibles in case there are new > features or bugs. This documentation is short, so it explains what should be done, not exactly why it should be done. If we wanted "why" this would not be set of 4 sentences but twice more... > > AFAICT, there are no new features or bugs, just a new SOC with the same > device. In general usage I have seen, you would just use the same > compatible. Most of the usages are like shown here. There are several other cases. It's the same with poor or good code - you will always find both patterns. > However, if I understand this, that last sentence should say: > > DO add new compatibles in case there is a new version of hardware with > the possibility of new features and/or bugs. > > Also, the term "specific" is, ironically, vague. Specific to what? To me it is rather clear. Specific as in first meanings of the word (1, 3, 4 and 5): https://en.wiktionary.org/wiki/specific nuvoton,npcm7xx-kcs-bmc would not be definite (allows more meanings), unique (in terms of devices it expresses), distinctive (as two different devices use the same) or serving to identify one thing (again - two SoCs). What other meaning do you think of? > > It would be nice to have something added to "Typical cases and caveats" > that says: > > - If you are writing a binding for a new device that is the same as, or > a superset of another existing device, add a new specific compatible > for the new device followed by a compatible for the existing device. > That way, if the device has new bugs or new specific features are > added, you can add workarounds without modifying the device tree. > > Anyway, I have added this to my tree with your ack. Fantastic, thanks! Best regards, Krzysztof
On Mon, Aug 08, 2022 at 03:38:45PM +0300, Krzysztof Kozlowski wrote: > On 08/08/2022 15:26, Corey Minyard wrote: > > On Mon, Aug 08, 2022 at 11:11:16AM +0300, Krzysztof Kozlowski wrote: > >> On 08/08/2022 09:54, Tomer Maimon wrote: > >>> Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible > >>> string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules. > >>> > >>> Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible") > >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > >> > >> > >> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > Ok, I think I understand how this is supposed to work. It's not > > altogether clear from the device tree documentation. It says in > > Documentation/devicetree/bindings/writing-bindings.rst: > > > > - DO make 'compatible' properties specific. DON'T use wildcards in compatible > > strings. DO use fallback compatibles when devices are the same as or a subset > > of prior implementations. DO add new compatibles in case there are new > > features or bugs. > > This documentation is short, so it explains what should be done, not > exactly why it should be done. If we wanted "why" this would not be set > of 4 sentences but twice more... > > > > > AFAICT, there are no new features or bugs, just a new SOC with the same > > device. In general usage I have seen, you would just use the same > > compatible. > > Most of the usages are like shown here. There are several other cases. > It's the same with poor or good code - you will always find both patterns. It is true, but lack of specified good examples makes it hard for people like me to know what is right and wrong. > > > However, if I understand this, that last sentence should say: > > > > DO add new compatibles in case there is a new version of hardware with > > the possibility of new features and/or bugs. > > > > Also, the term "specific" is, ironically, vague. Specific to what? > > To me it is rather clear. Specific as in first meanings of the word (1, > 3, 4 and 5): > https://en.wiktionary.org/wiki/specific Everything is always clear when you understand something :). The really hard part about technical documentation is forgetting what you know and approaching it from a newbie's context. > > nuvoton,npcm7xx-kcs-bmc would not be definite (allows more meanings), > unique (in terms of devices it expresses), distinctive (as two different > devices use the same) or serving to identify one thing (again - two SoCs). > > What other meaning do you think of? It is not the definition of specific that is vague, it is what "specific" applies to. Is it specific to a SOC? Specific to a board? Specific to a particular device implementation? Specific to a rev of the silicon? I will say that when I read that sentence, it means nothing to me. If it said "DO make compatible properties as specific as possible to the particular hardware implementation of the device" that would have more meaning, but still leaves the reader wondering exactly how to do this. For instance, should I put board/rev specific compatibles in? Would it be: "mycompany,myboard-rev1-npcm845-kcs-bmc", "mycompany,myboard-npcm845-kcs-bmc", "nuvoton,npcm845-revb-kcs-bmc", "nuvoton,npcm845-kcs-bmc", "nuvoton,npcm750-kcs-bmc" That's about as specific as you can get with fallbacks for everything, but it is too specific? How far do you go? There might be wiring differences on specific board, maybe that makes a difference. That's where good (and identified bad) examples and rationale come in. Tell the user why something is being done and it's easier to understand what to do in different situations. It's not a matter of number of sentences. Just like code, shorter is not always better. Anyway, I have ranted for too long. Thank you for clearing this up for me. -corey > > > > > It would be nice to have something added to "Typical cases and caveats" > > that says: > > > > - If you are writing a binding for a new device that is the same as, or > > a superset of another existing device, add a new specific compatible > > for the new device followed by a compatible for the existing device. > > That way, if the device has new bugs or new specific features are > > added, you can add workarounds without modifying the device tree. > > > > Anyway, I have added this to my tree with your ack. > > Fantastic, thanks! > > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt b/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt index cbc10a68ddef..4fda76e63396 100644 --- a/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt +++ b/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt @@ -7,7 +7,7 @@ used to perform in-band IPMI communication with their host. Required properties: - compatible : should be one of "nuvoton,npcm750-kcs-bmc" - "nuvoton,npcm845-kcs-bmc" + "nuvoton,npcm845-kcs-bmc", "nuvoton,npcm750-kcs-bmc" - interrupts : interrupt generated by the controller - kcs_chan : The KCS channel number in the controller
Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules. Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible") Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)