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;
Message-ID: <f6283bf7-2c38-8257-e430-d17dd4dd233b@linaro.org>
Date:   Fri, 16 Jun 2023 18:59:16 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH 2/3] dt-bindings: arm: Add SolidRun LX2162A SoM & Clearfog
 Board
Content-Language: en-US
To:     Josua Mayer <josua@solid-run.com>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Cc:     Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Li Yang <leoyang.li@nxp.com>, Marek Vasut <marex@denx.de>,
        Fabio Estevam <festevam@denx.de>,
        Stefan Wahren <stefan.wahren@chargebyte.com>,
        Frieder Schrempf <frieder.schrempf@kontron.de>,
        Marcel Ziswiler <marcel.ziswiler@toradex.com>,
        Andreas Kemnade <andreas@kemnade.info>
References: <20230616110610.32173-1-josua@solid-run.com>
 <20230616110610.32173-3-josua@solid-run.com>
 <a9098664-ac16-eddb-3e2d-78eb08ac973f@linaro.org>
 <5139b72e-cccb-43b3-ba37-35f0011e4a2b@solid-run.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <5139b72e-cccb-43b3-ba37-35f0011e4a2b@solid-run.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 16/06/2023 15:32, Josua Mayer wrote:
> HI Krzysztof,
> 
> Am 16.06.23 um 14:36 schrieb Krzysztof Kozlowski:
>> On 16/06/2023 13:06, Josua Mayer wrote:
>>> Add DT compatible for SolidRun LX2162A SoM and Clearfog board.
>>>
>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>> ---
>>>   Documentation/devicetree/bindings/arm/fsl.yaml | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
>>> index 15d411084065..438a4ece8157 100644
>>> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
>>> @@ -1373,9 +1373,11 @@ properties:
>>>         - description: SolidRun LX2160A based Boards
>>>           items:
>>>             - enum:
>>> +              - solidrun,clearfog
>>>                 - solidrun,clearfog-cx
>>>                 - solidrun,honeycomb
>>>             - const: solidrun,lx2160a-cex7
>>> +          - const: solidrun,lx2162a-som
>>>             - const: fsl,lx2160a
>> You change existing entries, breaking boards and changing the meaning,
>> without any explanation in commit msg. That's not how it is done. Please
>> provide rationale in commit msg.
> 
> I'm sorry. Given your comment I think I did not understand how these 
> entries are supposed to work.
> So perhaps you can provide some guidance based on my explanation?:
> 
> - NXP LX2162 is a smaller physical package of the same LX2160 SoC, with 
> reduced IOs and some silicon blocks disabled.
> - SolidRun LX2162 SoM is essentially a different form factor of LX2160 CEX
> - SolidRun LX2162 Clearfog is the reference platform for the SoM. 
> Despite it's naming similarity to clearfog-cx, it has a different 
> feature set more similar to SolidRun Armada 388 Clearfog Pro
> 
> So I believed I could just add to the existing entry "SolidRun LX2160A 
> based Boards" also the new LX2162 Board & SoM.

But you added much more, didn't you?

> I see now that adding a fourth const messes upthe existing 3-part 
> compatible for those already existing boards.
> 
> Please can you confirm if it would have been more correct to replace 
> "const: solidrun,lx2160a-cex7" with an enum?:
> enum:
>    - solidrun,lx2160a-cex7
>    - solidrun,lx2162a-som
> 
> Finally, is it okay to add a "solidrun,clearfog" given my explanation 
> above, or should it be more specific "solidrun,lx2162a-clearfog"?
> 

Test the binding and test DTS against it:
Please run `make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/

It might point you to answer.

Why do you make solidrun,honeycomb compatible with cex7 and som?

Best regards,
Krzysztof