Re: [PATCH v3 4/4] arm64: dts: amlogic: add fbx8am DT overlays

[Neil Armstrong <neil.armstrong@linaro.org>]

Marc,

On 20/02/2024 11:01, Krzysztof Kozlowski wrote:
> On 19/02/2024 11:32, Marc Gonzalez wrote:
>> On 14/02/2024 10:47, Neil Armstrong wrote:
>>
>>> On 13/02/2024 18:14, Marc Gonzalez wrote:
>>>
>>>> Add support for two variants of the fbx8am board.
>>>>
>>>> Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr>
>>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>>>> ---
>>>>    arch/arm64/boot/dts/amlogic/Makefile                       |  6 ++++++
>>>>    arch/arm64/boot/dts/amlogic/meson-g12a-fbx8am-brcm.dtso    | 35 ++++++++++++++++++++++++++++++++++
>>>>    arch/arm64/boot/dts/amlogic/meson-g12a-fbx8am-realtek.dtso | 25 ++++++++++++++++++++++++
>>>>    3 files changed, 66 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/amlogic/Makefile b/arch/arm64/boot/dts/amlogic/Makefile
>>>> index cc8b34bd583d8..1ab160bf928ae 100644
>>>> --- a/arch/arm64/boot/dts/amlogic/Makefile
>>>> +++ b/arch/arm64/boot/dts/amlogic/Makefile
>>>> @@ -8,6 +8,8 @@ dtb-$(CONFIG_ARCH_MESON) += meson-axg-jethome-jethub-j100.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-axg-jethome-jethub-j110-rev-2.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-axg-jethome-jethub-j110-rev-3.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-axg-s400.dtb
>>>> +dtb-$(CONFIG_ARCH_MESON) += meson-g12a-fbx8am-brcm.dtb
>>>> +dtb-$(CONFIG_ARCH_MESON) += meson-g12a-fbx8am-realtek.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-g12a-radxa-zero.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-g12a-sei510.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-g12a-u200.dtb
>>>> @@ -80,3 +82,7 @@ dtb-$(CONFIG_ARCH_MESON) += meson-sm1-odroid-hc4.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-sm1-sei610.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-sm1-x96-air-gbit.dtb
>>>>    dtb-$(CONFIG_ARCH_MESON) += meson-sm1-x96-air.dtb
>>>> +
>>>> +# Overlays
>>>> +meson-g12a-fbx8am-brcm-dtbs	:= meson-g12a-fbx8am.dtb meson-g12a-fbx8am-brcm.dtbo
>>>> +meson-g12a-fbx8am-realtek-dtbs	:= meson-g12a-fbx8am.dtb meson-g12a-fbx8am-realtek.dtbo
>>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-fbx8am-brcm.dtso b/arch/arm64/boot/dts/amlogic/meson-g12a-fbx8am-brcm.dtso
>>>> new file mode 100644
>>>> index 0000000000000..ed79809b15859
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-fbx8am-brcm.dtso
>>>> @@ -0,0 +1,35 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>> +// Copyright (c) 2024 Freebox SAS
>>>> +
>>>> +/dts-v1/;
>>>> +/plugin/;
>>>> +
>>>> +#include <dt-bindings/gpio/gpio.h>
>>>> +#include <dt-bindings/gpio/meson-g12a-gpio.h>
>>>> +
>>>> +/ {
>>>> +	compatible = "freebox,fbx8am-brcm", "freebox,fbx8am", "amlogic,g12a";
>>>
>>> This looks fine but I'm unsure if those new compatible should be documented or not,
>>> since they are in an overlay
> 
> All compatibles in entire Linux source code shall be documented, so
> please document any new ones appearing in overlays.

You can either add a follow-up patch to:
- document those new compatibles
- remove those compatibles

As you want, I plan to send the ARM64 DT PR next week.

Neil


> 
>>
>> Rob, Krzysztof, Conor,
>>
>> Just want to make sure this was done "right".
>>
>> Is it acceptable for an overlay (DTSO) to define a more specific compatible prop?
> 
> I think it is fine and some DTSO already do it. User-space might want to
> identify that specific overlay is applied or not. Of course this will
> not work when multiple overlays are applied and each changes compatible,
> thus I would say this is in general specific to particular cases of
> exclusive overlays. In such case, why not making it new board DTS?
> 
> If this is not exclusive overlay, then new compatible looks wrong.
> 
> Best regards,
> Krzysztof
>