Re: [PATCH 4/7] arm64: dts: rockchip: Fix compatible and model for Radxa CM3i

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 17/01/2023 13:52, Jagan Teki wrote:
> On Tue, Jan 17, 2023 at 4:37 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/01/2023 11:55, Jagan Teki wrote:
>>> Fix the model name and compatible strings for Radxa CM3i SoM
>>> and Carrier board based on Radxa recommendation.
>>>
>>> Fixes: 2bf2f4d9f673 ("arm64: dts: rockchip: Add Radxa CM3I E25")
>>> Cc: Chukun Pan <amadeus@jmu.edu.cn>
>>> Reported-by: FUKAUMI Naoki <naoki@radxa.com>
>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3568-radxa-cm3i.dtsi | 2 +-
>>>  arch/arm64/boot/dts/rockchip/rk3568-radxa-e25.dts   | 4 ++--
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-radxa-cm3i.dtsi b/arch/arm64/boot/dts/rockchip/rk3568-radxa-cm3i.dtsi
>>> index 225dbbe4955d..0c9a918b809a 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3568-radxa-cm3i.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-radxa-cm3i.dtsi
>>> @@ -7,7 +7,7 @@
>>>
>>>  / {
>>>       model = "Radxa CM3 Industrial Board";
>>> -     compatible = "radxa,cm3i", "rockchip,rk3568";
>>> +     compatible = "radxa,radxa-cm3i", "rockchip,rk3568";
>>
>> Nope, second radxa is redundant.
> 
> It is not redundant, Product name and vendor name is same. Vendor name
> is Radxa and product name is Radxa CM3i

If this was true, then on their website the product would be called
"Radxa Radxa CM3i", but it is not. It is called Radxa CM3i, to denote
the company name with the model name. Anyway, in the name of model you
can call it. In the context of compatible it is redundant.

> 
>>
>>>
>>>       aliases {
>>>               mmc0 = &sdhci;
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-radxa-e25.dts b/arch/arm64/boot/dts/rockchip/rk3568-radxa-e25.dts
>>> index a4c33310ca42..8ca15b723799 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3568-radxa-e25.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-radxa-e25.dts
>>> @@ -4,8 +4,8 @@
>>>  #include "rk3568-radxa-cm3i.dtsi"
>>>
>>>  / {
>>> -     model = "Radxa E25";
>>> -     compatible = "radxa,e25", "radxa,cm3i", "rockchip,rk3568";
>>> +     model = "Radxa E25 Carrier Board";
>>> +     compatible = "radxa,radxa-e25", "radxa,radxa-cm3i", "rockchip,rk3568";
>>
>> NAK. You just changed it in patch 2. Don't add broken compatibles in one
>> patch (cm3i) and fix them next one. It's like adding intentional bugs
>> and fixing them immediately...
> 
> I did squash this with previous patch but the check patch showing
> warning to separate the dt-bindings patch from actual devicetree file
> change.

Patch 2 was not a bindings patch, so you squashed wrong files.

Best regards,
Krzysztof