| Message ID | 20220804181800.235368-1-tmaimon77@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] dt-binding: ipmi: add fallback to npcm845 compatible | expand |
On Thu, Aug 04, 2022 at 09:18:00PM +0300, 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. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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" This is just wrong. The compatible is supposed to identify the device, not the board the device is on. I think compatible here should be "npcm7xx-kcs-bmc", and just use that everywhere. It's fine if that is used on a board named npcm845. -corey > - interrupts : interrupt generated by the controller > - kcs_chan : The KCS channel number in the controller > > -- > 2.33.0 >
Hi Krzysztof, Thanks for your review. On Fri, 5 Aug 2022 at 09:36, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 04/08/2022 20:18, 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. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > Your previous commit adding that compatible was simply wrong and not > matching the driver and it is not the first time. I think all Nuvoton > patches need much more careful review :( Will do and sorry about all the mess... > > You forgot the fixes tag: > > Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible") Will add the tag next version. > > > > > Best regards, > Krzysztof Best regards, Tomer
Hi Corey, Thanks for your comment. On Fri, 5 Aug 2022 at 14:58, Corey Minyard <minyard@acm.org> wrote: > > On Thu, Aug 04, 2022 at 09:18:00PM +0300, 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. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > 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" > > This is just wrong. The compatible is supposed to identify the device, > not the board the device is on. I think compatible here should be > "npcm7xx-kcs-bmc", and just use that everywhere. It's fine if that is > used on a board named npcm845. The NPCM8XX is not a board, The Nuvoton NPCM8XX is a fourth-generation BMC SoC device family. > > -corey > > > - interrupts : interrupt generated by the controller > > - kcs_chan : The KCS channel number in the controller > > > > -- > > 2.33.0 > > Best regards, Tomer
On Sun, Aug 07, 2022 at 11:03:56AM +0300, Tomer Maimon wrote: > Hi Corey, > > Thanks for your comment. > > On Fri, 5 Aug 2022 at 14:58, Corey Minyard <minyard@acm.org> wrote: > > > > On Thu, Aug 04, 2022 at 09:18:00PM +0300, 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. > > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > --- > > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > 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" > > > > This is just wrong. The compatible is supposed to identify the device, > > not the board the device is on. I think compatible here should be > > "npcm7xx-kcs-bmc", and just use that everywhere. It's fine if that is > > used on a board named npcm845. > The NPCM8XX is not a board, The Nuvoton NPCM8XX is a fourth-generation > BMC SoC device family. Ok, but same principle applies. If the device is exactly the same, then you would only use one of the "npcm7xx-kcs-bmc" and put that in both device trees. You can use "nuvoton,npcm750-kcs-bmc", it's really not that important. Or even "nuvoton,npcm-kcs-bmc" If the device has a minor difference that can be expressed in a parameter, then create a parameter for it. If the device has enough differences that a parameter or two doesn't cover it, then you put either nuvoton,npcm750-kcs-bmc or nuvoton,npcm750-kcs-bmc in the device tree. Not both. Then you need two entries in the of_device_id array and you use the data field or something to express the difference. Since there appears to be no difference, just put "nuvoton,npcm750-kcs-bmc" in the npcm845 and I will drop the patch adding all this. Then a patch can be added saying it applies to both the 7xx and 8xx series of BMC SOCs. If you want to change the name, then a patch will be needed for that, but then you will need multiple entries in your device tree, but you would not document it as such, as there would only be one that applies for this kernel. I'm pretty sure the only reason to have muliple compatible entries in a device tree is to cover multiple kernels where the name changed. -corey > > > > -corey > > > > > - interrupts : interrupt generated by the controller > > > - kcs_chan : The KCS channel number in the controller > > > > > > -- > > > 2.33.0 > > > > > Best regards, > > Tomer
On Sun, 7 Aug 2022 at 15:11, Corey Minyard <minyard@acm.org> wrote: > > On Sun, Aug 07, 2022 at 11:03:56AM +0300, Tomer Maimon wrote: > > Hi Corey, > > > > Thanks for your comment. > > > > On Fri, 5 Aug 2022 at 14:58, Corey Minyard <minyard@acm.org> wrote: > > > > > > On Thu, Aug 04, 2022 at 09:18:00PM +0300, 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. > > > > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > > --- > > > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > 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" > > > > > > This is just wrong. The compatible is supposed to identify the device, > > > not the board the device is on. I think compatible here should be > > > "npcm7xx-kcs-bmc", and just use that everywhere. It's fine if that is > > > used on a board named npcm845. > > The NPCM8XX is not a board, The Nuvoton NPCM8XX is a fourth-generation > > BMC SoC device family. > > Ok, but same principle applies. > > If the device is exactly the same, then you would only use one of the > "npcm7xx-kcs-bmc" and put that in both device trees. You can use > "nuvoton,npcm750-kcs-bmc", it's really not that important. Or even > "nuvoton,npcm-kcs-bmc" If we use "nuvoton, npcm-kcs-bmc" we should take care of backward dts compatibility, and I am not sure we like to change NPCM KCS driver. > > If the device has a minor difference that can be expressed in a > parameter, then create a parameter for it. > > If the device has enough differences that a parameter or two doesn't > cover it, then you put either nuvoton,npcm750-kcs-bmc or > nuvoton,npcm750-kcs-bmc in the device tree. Not both. Then you need > two entries in the of_device_id array and you use the data field or > something to express the difference. > > Since there appears to be no difference, just put > "nuvoton,npcm750-kcs-bmc" in the npcm845 and I will drop the patch > adding all this. Then a patch can be added saying it applies to both > the 7xx and 8xx series of BMC SOCs. If you want to change the name, > then a patch will be needed for that, but then you will need multiple > entries in your device tree, but you would not document it as such, as > there would only be one that applies for this kernel. It little bit confusing to use nuvoton,npcm750-kcs-bmc that are related to NPCM7XX for NPCM8XX KCS. We can use the generic name "nuvoton, npcm-kcs-bmc" as you suggested above but we should take care of backward dts compatibility, and I am not sure we like to change NPCM KCS driver. We had a disscation with Arnd, Arnd asked us to use a fallback as we did here if NPCM8XX device module is similar to NPCM7XX module: https://lore.kernel.org/lkml/20220522155046.260146-5-tmaimon77@gmail.com/ I think we should use a fallback to describe the NPCM8XX KCS in the dt-binding document. > > I'm pretty sure the only reason to have muliple compatible entries in a > device tree is to cover multiple kernels where the name changed. > > -corey > > > > > > > -corey > > > > > > > - interrupts : interrupt generated by the controller > > > > - kcs_chan : The KCS channel number in the controller > > > > > > > > -- > > > > 2.33.0 > > > > > > > > Best regards, > > > > Tomer Best regards, Tomer
On Sun, Aug 07, 2022 at 05:54:28PM +0300, Tomer Maimon wrote: > On Sun, 7 Aug 2022 at 15:11, Corey Minyard <minyard@acm.org> wrote: > > > > On Sun, Aug 07, 2022 at 11:03:56AM +0300, Tomer Maimon wrote: > > > Hi Corey, > > > > > > Thanks for your comment. > > > > > > On Fri, 5 Aug 2022 at 14:58, Corey Minyard <minyard@acm.org> wrote: > > > > > > > > On Thu, Aug 04, 2022 at 09:18:00PM +0300, 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. > > > > > > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > > > --- > > > > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > 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" > > > > > > > > This is just wrong. The compatible is supposed to identify the device, > > > > not the board the device is on. I think compatible here should be > > > > "npcm7xx-kcs-bmc", and just use that everywhere. It's fine if that is > > > > used on a board named npcm845. > > > The NPCM8XX is not a board, The Nuvoton NPCM8XX is a fourth-generation > > > BMC SoC device family. > > > > Ok, but same principle applies. > > > > If the device is exactly the same, then you would only use one of the > > "npcm7xx-kcs-bmc" and put that in both device trees. You can use > > "nuvoton,npcm750-kcs-bmc", it's really not that important. Or even > > "nuvoton,npcm-kcs-bmc" > If we use "nuvoton, npcm-kcs-bmc" we should take care of backward dts > compatibility, and I am not sure we like to change NPCM KCS driver. > > > > If the device has a minor difference that can be expressed in a > > parameter, then create a parameter for it. > > > > If the device has enough differences that a parameter or two doesn't > > cover it, then you put either nuvoton,npcm750-kcs-bmc or > > nuvoton,npcm750-kcs-bmc in the device tree. Not both. Then you need > > two entries in the of_device_id array and you use the data field or > > something to express the difference. > > > > Since there appears to be no difference, just put > > "nuvoton,npcm750-kcs-bmc" in the npcm845 and I will drop the patch > > adding all this. Then a patch can be added saying it applies to both > > the 7xx and 8xx series of BMC SOCs. If you want to change the name, > > then a patch will be needed for that, but then you will need multiple > > entries in your device tree, but you would not document it as such, as > > there would only be one that applies for this kernel. > > It little bit confusing to use nuvoton,npcm750-kcs-bmc that are > related to NPCM7XX for NPCM8XX KCS. A little, but it's not unusual. > We can use the generic name "nuvoton, npcm-kcs-bmc" as you suggested > above but we should take care of backward dts compatibility, and I am > not sure we like to change NPCM KCS driver. > > We had a disscation with Arnd, Arnd asked us to use a fallback as we > did here if NPCM8XX device module is similar to NPCM7XX module: > https://lore.kernel.org/lkml/20220522155046.260146-5-tmaimon77@gmail.com/ > > I think we should use a fallback to describe the NPCM8XX KCS in the > dt-binding document. I'm ok with that option. I guess I should have mentioned it. Add nuvoton,npcm-kcs-bmc to the driver's of_device_id table. Then use that in that compatible string in the device tree. Leave the 750 compatible string in the table for backwards compatibility. There's no point in having a bunch of those strings if they are all the same. If a new one comes out that is different, we can handle that when the time comes. -corey > > > > I'm pretty sure the only reason to have muliple compatible entries in a > > device tree is to cover multiple kernels where the name changed. > > > > -corey > > > > > > > > > > -corey > > > > > > > > > - interrupts : interrupt generated by the controller > > > > > - kcs_chan : The KCS channel number in the controller > > > > > > > > > > -- > > > > > 2.33.0 > > > > > > > > > > > Best regards, > > > > > > Tomer > > Best regards, > > Tomer
Hi Krzysztof, Thanks for your reply. On Mon, 8 Aug 2022 at 09:26, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 07/08/2022 09:51, Tomer Maimon wrote: > > Hi Krzysztof, > > > > Thanks for your review. > > > > On Fri, 5 Aug 2022 at 09:36, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 04/08/2022 20:18, 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. > >>> > >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > >> > >> Your previous commit adding that compatible was simply wrong and not > >> matching the driver and it is not the first time. I think all Nuvoton > >> patches need much more careful review :( > > Will do and sorry about all the mess... > >> > >> You forgot the fixes tag: > >> > >> Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible") > > Will add the tag next version. > > You received a bit different review from Corey, so to be clear: > 1. Your approach is correct, assuming the devices are really compatible. > 2. Add a fixes tag and send a v3, to get my ack. Will do. > > Best regards, > Krzysztof Best regards, Tomer
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. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)