Re: [Buildroot] [PATCH 1/4] package/adsp-ldr: new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Philip,

Thanks for this contribution! See below some feedback.

On Mon, 18 Aug 2025 14:22:30 +0000
Philip Molloy <philip@philipmolloy.com> wrote:

> From: Philip Molloy <philip.molloy@analog.com>
> 
> Signed-off-by: Philip Molloy <philip.molloy@analog.com>
> Signed-off-by: Philip Molloy <philip@philipmolloy.com>
> ---
>  package/Config.in              |  1 +
>  package/adsp-ldr/Config.in     |  9 +++++++++
>  package/adsp-ldr/adsp-ldr.hash |  3 +++
>  package/adsp-ldr/adsp-ldr.mk   | 19 +++++++++++++++++++
>  4 files changed, 32 insertions(+)

I see in PATCH 4/4 you add an entry in the DEVELOPERS file for this
package. You should squash PATCH 4/4 into this patch, so that the
addition in DEVELOPERS comes in the patch adding the package.


> diff --git a/package/Config.in b/package/Config.in
> index 579b5ffc87..63aee051e9 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -470,6 +470,7 @@ endmenu
>  	source "package/acpica/Config.in"
>  	source "package/acpid/Config.in"
>  	source "package/acpitool/Config.in"
> +	source "package/adsp-ldr/Config.in"

This will not be needed (see below).

> diff --git a/package/adsp-ldr/Config.in b/package/adsp-ldr/Config.in
> new file mode 100644
> index 0000000000..a1806df576
> --- /dev/null
> +++ b/package/adsp-ldr/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_ADSP_LDR
> +	bool "adsp-ldr"
> +	depends on BR2_arm || BR2_aarch64

This file is not relevant: your package is a host only package. In
fact, if you enable this option and try to build, you should get a
failure as your .mk file does not create a target variant of this
package (which is correct).

> diff --git a/package/adsp-ldr/adsp-ldr.mk b/package/adsp-ldr/adsp-ldr.mk
> new file mode 100644
> index 0000000000..9fdf3c3c4e
> --- /dev/null
> +++ b/package/adsp-ldr/adsp-ldr.mk
> @@ -0,0 +1,19 @@
> +################################################################################
> +#
> +# adsp-ldr
> +#
> +################################################################################
> +
> +ADSP_LDR_VERSION = 1.0.0
> +ADSP_LDR_SITE = $(call github,analogdevicesinc,adsp-ldr,v$(ADSP_LDR_VERSION))
> +ADSP_LDR_LICENSE = GPL-3.0
> +ADSP_LDR_LICENSE_FILES = LICENSE

Are you sure that this license file is correct? Indeed, all source
files seem to have a BSD-3-Clause license, for example:
https://github.com/analogdevicesinc/adsp-ldr/blob/main/src/ldr/lfd.c.

> +ADSP_LDR_SUBDIR = src/ldr
> +
> +define HOST_ADSP_LDR_INSTALL_CMDS
> +	$(INSTALL) -m 755 -D $(@D)/src/ldr/ldr \
> +		$(TARGET_CROSS)ldr

Why do you install it with a $(TARGET_CROSS) prefix?

Also, you seem to be the author of this repo. Why is the ldr code
burried in src/ldr, with not much outside of this folder? It would make
a lot more sense to have the code directly at the project root, and
avoid having to pass ADSP_LDR_SUBDIR = src/ldr.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot