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 <[email protected]>
---
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"
- interrupts : interrupt generated by the controller
- kcs_chan : The KCS channel number in the controller
--
2.33.0
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 <[email protected]>
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 :(
You forgot the fixes tag:
Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible")
Best regards,
Krzysztof
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 <[email protected]>
> ---
> 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
<[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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/[email protected]/
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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/[email protected]/
>
> 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
On 07/08/2022 14:11, Corey Minyard 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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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"
No, because it is too generic. Compatibles must be specific.
>
> 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.
It's quite common to have generic and specific compatibles for
compatible devices and a driver which can match to both of them. I don't
understand where is exactly the problem here?
>
> 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. T
Again no, because recommended (also writing bindings document) is always
to have a specific compatible.
> hen 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.
Best regards,
Krzysztof
On 05/08/2022 13:58, Corey Minyard 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 <[email protected]>
>> ---
>> 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
No, because you propose to use wildcards as compatible which is not
correct. Compatibles must be specific.
Best regards,
Krzysztof
On 07/08/2022 18:05, Corey Minyard wrote:
>> 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/[email protected]/
>>
>> 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.
To be clear - NAK. It's not a specific compatible, therefore it is not
appropriate at all.
> 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.\
There is a point. It's exactly how we do in all DT bindings. It is
recommended by writing bindings document.
Best regards,
Krzysztof
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
> <[email protected]> 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 <[email protected]>
>>
>> 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.
Best regards,
Krzysztof
On 07/08/2022 16:54, Tomer Maimon wrote:
> On Sun, 7 Aug 2022 at 15:11, Corey Minyard <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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
No, please don't. It will be NAKed. :)
> 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/[email protected]/
>
> I think we should use a fallback to describe the NPCM8XX KCS in the
> dt-binding document.
Best regards,
Krzysztof
Hi Krzysztof,
Thanks for your reply.
On Mon, 8 Aug 2022 at 09:26, Krzysztof Kozlowski
<[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> >>
> >> 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