| Message ID | 20250207-pre-ict-jaguar-v5-1-a70819ea0692@cherry.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: minimal support for Pre-ICT tester adapter for RK3588 Jaguar + add overlay tests | expand |
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. :) > +# > +# 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" for the sake of consistency and, obviously, for clear indication of the space characters not being applicable. Regarding using "-" or "_" characters there, perhaps we should follow what Git uses in its man pages, which is the "-" character (see e.g. git-switch(1)). > +# > +# This will make the <base>.dtb have symbols (like when DTC_FLAGS has > -@ passed) > +# and generate a new DTB (<name of overlay application test>.dtb) > which is the > +# result of the application of <overlay-1>.dtbo and other listed > overlays on top > +# of <base>.dtb. > + > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-vz-2-uhd.dtb > +rk3568-wolfvision-pf5-vz-2-uhd-dtbs := rk3568-wolfvision-pf5.dtb \ > + rk3568-wolfvision-pf5-display-vz.dtbo \ > + rk3568-wolfvision-pf5-io-expander.dtbo Otherwise, it's looking good to me, thanks for the patch! It was already discussed and commented in detail in the v4 of this series, [*] so please feel free to include: Reviewed-by: Dragan Simic <dsimic@manjaro.org> [*] https://lore.kernel.org/linux-rockchip/a3b98e3d3a2571ee75e59418bb3b6960@manjaro.org/T/#u
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
Hello Quentin, On 2025-02-10 18:56, Quentin Schulz wrote: > On 2/10/25 9:46 AM, Dragan Simic wrote: >> 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. I see. To explain it a bit further, here's how hyphens, en and em dashes should look like when an unproportional font is used: - When it comes to hyphens, it's a somewhat-limited option. - Using en dashes -- as visible here -- is a bit more involved. - If you use em dashes---like here---it gets borderline ugly. > , 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. It works, although I'd suggest that a comma is added after "ideally". Technically, it would be better not to use parentheses here, but it's still fine. Though, here's another option for the wording: , and at least one overlay application test that represents the overlay's intended real-life use: >>> +# >>> +# 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. Thanks.
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 -: +# +# dtb-$(CONFIG_ARCH_ROCKCHIP) += <name of overlay application test>.dtb +# <name of overlay application test>-dtbs := <base>.dtb <overlay-1>.dtbo [<overlay-2>.dtbo ...] +# +# This will make the <base>.dtb have symbols (like when DTC_FLAGS has -@ passed) +# and generate a new DTB (<name of overlay application test>.dtb) which is the +# result of the application of <overlay-1>.dtbo and other listed overlays on top +# of <base>.dtb. + +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-vz-2-uhd.dtb +rk3568-wolfvision-pf5-vz-2-uhd-dtbs := rk3568-wolfvision-pf5.dtb \ + rk3568-wolfvision-pf5-display-vz.dtbo \ + rk3568-wolfvision-pf5-io-expander.dtbo