Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Corey Minyard <tcminyard@gmail.com>
Date:   Sun, 7 Aug 2022 07:11:38 -0500
From:   Corey Minyard <minyard@acm.org>
To:     Tomer Maimon <tmaimon77@gmail.com>
Cc:     Avi Fishman <avifishman70@gmail.com>,
        Tali Perry <tali.perry1@gmail.com>,
        Joel Stanley <joel@jms.id.au>,
        Patrick Venture <venture@google.com>,
        Nancy Yuen <yuenn@google.com>,
        Benjamin Fair <benjaminfair@google.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        openipmi-developer@lists.sourceforge.net,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>
Subject: Re: [PATCH v2] dt-binding: ipmi: add fallback to npcm845 compatible
Message-ID: <20220807121138.GL3834@minyard.net>
Reply-To: minyard@acm.org
References: <20220804181800.235368-1-tmaimon77@gmail.com>
 <20220805115827.GG3834@minyard.net>
 <CAP6Zq1gfvEpUF-TKhA8EdJqBtwaVvJR3qxtn=8Li4swHB6sDYQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAP6Zq1gfvEpUF-TKhA8EdJqBtwaVvJR3qxtn=8Li4swHB6sDYQ@mail.gmail.com>
Precedence: bulk

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