[Buildroot] [PATCH 1/2] package/freescale-imx/imx-m4-demos: new package
Thomas Petazzoni
thomas.petazzoni at bootlin.com
Wed Aug 5 20:34:34 UTC 2020
Hello Fabrice,
Thanks a lot for your patch series. Overall it looks pretty good, but a
few changes are needed.
First a minor detail: your commit title doesn't match the actual
package name being added. See below for other comments.
On Fri, 31 Jul 2020 14:13:32 +0200
Fabrice Goucem <fabrice.goucem at oss.nxp.com> wrote:
> DEVELOPERS | 1 +
> board/freescale/common/imx/post-image.sh | 14 ++++-
I think this part (changes to post-image.sh) should go into a separate
commit, after the package addition.
> diff --git a/board/freescale/common/imx/post-image.sh b/board/freescale/common/imx/post-image.sh
> index 06ccaac3a4..dc651ac584 100755
> --- a/board/freescale/common/imx/post-image.sh
> +++ b/board/freescale/common/imx/post-image.sh
> @@ -30,6 +30,18 @@ linux_image()
> fi
> }
>
> +#
> +# mcore_image prints all available MCORE demo file names for the genimage
> +# configuration file
> +#
> +mcore_image()
> +{
> + if grep -Eq "^BR2_PACKAGE_IMX_MCORE_DEMOS=y$" ${BR2_CONFIG}; then
> + echo -n ", "
> + for f in $(ls ${BINARIES_DIR}/mcore_*); do echo -n "$(basename $f), "; done
If I look at this, it seems like having the list finish with a comma is
fine. If that's the case, then I think we should modify the
linux_image() function so that it echoes a string that is
comma-terminated, so that the mcore_image() doesn't have to worry about
this, and looks like dtb_list() in how it handles the comma.
> diff --git a/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash
> new file mode 100644
> index 0000000000..13f3092c88
> --- /dev/null
> +++ b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash
> @@ -0,0 +1,10 @@
> +# locally computed
> +sha256 cc00d3b936d49b2794a2a99e10129437e70caba3fd26b8379b8c50dd22f73254 imx7d-sabresd-m4-freertos-1.0.1.bin
> +sha256 a8fbe1180b3d20e933a410cd76e60baac7a9f54e8b2bae8b6b8a3e1af550eca6 imx7ulp-m4-demo-2.8.0.bin
> +sha256 2dbfab7fbbe89e89a2881d77c84a6c257699dc73ee6462a813bdd5ad09836b04 imx8mm-m4-demo-2.8.0.bin
> +sha256 e877c7462b6ea87c498563842f42352d204eb28a65f35f7dc1fec643f84abb66 imx8mn-m7-demo-2.8.0.bin
> +sha256 ac88568f63a794530339775a6e49e7928d3d09bcf4ba5edacea1841989e674b0 imx8mq-m4-demo-2.8.0.bin
> +sha256 d06a636b84cd559483091cbdb07b5ce9e15a534bca31d4cb756b33b696c2160b imx8qm-m4-demo-2.8.0.bin
> +sha256 7800cdbebe07f426cdac50b0e295d64215164a767e79ca58bd917445c50e345f imx8qx-m4-demo-2.8.0.bin
> +
> +# no hash for license file as it is different for each package listed above
Hm, that's is not great, but I don't really have a good/simple solution
for this.
> +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7),y)
> +IMX_MCORE_DEMOS_VERSION = 1.0.1
> +IMX_MCORE_DEMOS_SOURCE = imx7d-sabresd-m4-freertos-$(IMX_MCORE_DEMOS_VERSION).bin
> +EXT = bin
Variables in Buildroot are global, so a variable named just "EXT" is
not good. It should be IMX_MCORE_DEMOS_FILE_EXT or something like that.
> +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7ULP),y)
This BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7ULP option is only
introduced in the second patch of the series. I think it would make
more sense to introduce the i.MX7ULP platform as a separate patch
earlier in the series.
> +# Note: firmware names are copied to binaries directory with an "mcore_" prefix, for easier post image scripting
Please wrap long lines.
> +define IMX_MCORE_DEMOS_INSTALL_IMAGES_CMDS
> + for f in $(@D)/*.$(EXT); do cp "$$f" $(BINARIES_DIR)/mcore_$$(basename "$$f"); done
Could you try (untested):
$(foreach f,$(wildcard $(@D)/*.$(IMX_MCORE_DEMOS_FILE_EXT), \
$(INSTALL) -D -m 0644 $(f) $(BINARIES_DIR)/mcore_$(notdir $(f))
)
This is a bit more make-style :-)
Thanks!
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
More information about the buildroot
mailing list