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:   Mon, 8 Aug 2022 09:23:27 -0500
From:   Corey Minyard <minyard@acm.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Tomer Maimon <tmaimon77@gmail.com>, avifishman70@gmail.com,
        tali.perry1@gmail.com, joel@jms.id.au, venture@google.com,
        yuenn@google.com, benjaminfair@google.com, jic23@kernel.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        openbmc@lists.ozlabs.org, openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v3] dt-binding: ipmi: add fallback to npcm845 compatible
Message-ID: <20220808142327.GR3834@minyard.net>
Reply-To: minyard@acm.org
References: <20220808075452.115907-1-tmaimon77@gmail.com>
 <da83671e-08b9-2d68-e5d3-d9b09c105bb4@linaro.org>
 <20220808122652.GO3834@minyard.net>
 <937b6737-5d55-2185-a4bc-7535fa6da85c@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <937b6737-5d55-2185-a4bc-7535fa6da85c@linaro.org>
Precedence: bulk

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