Re: [PATCH v5 1/4] arm64: dts: rockchip: add overlay test for WolfVision PF5

From: Quentin Schulz <quentin.schulz@cherry.de>
To: Dragan Simic <dsimic@manjaro.org>,
	Quentin Schulz <foss+kernel@0leil.net>
Cc: Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>, Jagan Teki <jagan@edgeble.ai>,
	Niklas Cassel <cassel@kernel.org>,
	Michael Riesch <michael.riesch@wolfvision.net>,
	Jonas Karlman <jonas@kwiboo.se>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 1/4] arm64: dts: rockchip: add overlay test for WolfVision PF5
Date: Mon, 10 Feb 2025 18:56:47 +0100	[thread overview]
Message-ID: <9615521e-8a2a-44d7-99ea-5571f5de1e57@cherry.de> (raw)
In-Reply-To: <ef9133d217bc35f611ef390da8042510@manjaro.org>

Hi Dragan,

On 2/10/25 9:46 AM, Dragan Simic wrote:
> Hello Quentin,
> 
> Please see a few comments below.
> 
> On 2025-02-07 16:19, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> The WolfVision PF5 can have a PF5 Visualizer display and PF5 IO Expander
>> board connected to it. Therefore, let's generate an overlay test so the
>> application of the two overlays are validated against the base DTB.
>>
>> Suggested-by: Michael Riesch <michael.riesch@wolfvision.net>
>> Reviewed-by: Michael Riesch <michael.riesch@wolfvision.net>
>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> ---
>>  arch/arm64/boot/dts/rockchip/Makefile | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile
>> b/arch/arm64/boot/dts/rockchip/Makefile
>> index
>> def1222c1907eb16b23cff6d540174a4e897abc9..534e70a649eeada7f9b6f12596b83f5c47b184b4
>> 100644
>> --- a/arch/arm64/boot/dts/rockchip/Makefile
>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
>> @@ -170,3 +170,25 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s- 
>> orangepi-5.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-orangepi-5b.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5a.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5c.dtb
>> +
>> +# Overlay application tests
>> +#
>> +# A .dtbo must have its own
>> +#
>> +#  dtb-$(CONFIG_ARCH_ROCKCHIP) += <overlay>.dtbo
>> +#
>> +# entry, and at least one overlay application test - ideally 
>> reflecting how it
>> +# will be used in real life -:
> 
> Hmm, what's "-:" actually doing in the line right above?  I mean,
> it's a minor nitpick, so might be worth addressing only if there
> will be the v6...  Also, "test - ideally" might be replaced by
> "test, ideally", because splicing sentences together using em/en
> dashes is generally frowned upon. :)
> 

That was supposed to be an em-dash yes.

, and at least one overlay application test (ideally reflecting how it 
will be used in real life):

Would that work? I don't like the : following "ideally reflecting how it 
will be used in real life" as it applies to "overlay application test" 
and not the end of the sentence.

>> +#
>> +#  dtb-$(CONFIG_ARCH_ROCKCHIP) += <name of overlay application test>.dtb
>> +#  <name of overlay application test>-dtbs := <base>.dtb
>> <overlay-1>.dtbo [<overlay-2>.dtbo ...]
> 
> As another minor nitpick, I'd suggest that
> 
>      "<name of overlay application test>.dtb"
> 
> is replaced with
> 
>      "<name-of-overlay-application-test>.dtb"
> 

OK.

Cheers,
Quentin

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip