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

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

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.

> 
> 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