Re: [Buildroot] [PATCH 1/1] configs/orangepi_pc2: add board support

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Javad Rahimi <javad321javad@gmail.com>
Cc: buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH 1/1] configs/orangepi_pc2: add board support
Date: Tue, 14 Mar 2023 09:31:24 +0100	[thread overview]
Message-ID: <20230314093124.7aa2de8b@windsurf> (raw)
In-Reply-To: <20230301083556.2408221-1-javad321javad@gmail.com>

Hello Javad,

On Wed,  1 Mar 2023 12:05:56 +0330
Javad Rahimi <javad321javad@gmail.com> wrote:

> add board support for orangepi pc2
> 
> Signed-off-by: Javad Rahimi <javad321javad@gmail.com>

Thanks for your contribution. For your reference, we used to have a
defconfig for this platform until 2022, at which point it was removed
due to it not being maintained. See the commit that removed it:

  https://gitlab.com/buildroot.org/buildroot/-/commit/eeede611f81d8f83b72e4a6671cd7d1e46b266b0

I suggest that you look at this commit to see what could possibly be
missing in your submission.

More comments below.

> ---
>  configs/orangepi_pc2_defconfig | 41 ++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)

We need an entry in the DEVELOPERS file corresponding to this defconfig
and any other file added by this commit.

We need a readme.txt file somewhere in board/ to document how to use
the defconfig and board. You can read other readme.txt files in board/
to get a feeling of the expected format/content.

> diff --git a/configs/orangepi_pc2_defconfig b/configs/orangepi_pc2_defconfig
> new file mode 100644
> index 0000000000..63ee554caf
> --- /dev/null
> +++ b/configs/orangepi_pc2_defconfig
> @@ -0,0 +1,41 @@
> +BR2_aarch64=y
> +BR2_ARM_FPU_VFPV4=y
> +BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_5_16=y
> +BR2_TOOLCHAIN_BUILDROOT_CXX=y

This is not needed in a minimal defconfig.

> +BR2_TARGET_GENERIC_ISSUE="Welcome to Orangepi PC2"
> +BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV=y

Any specific reason why mdev is needed?

> +BR2_SYSTEM_DHCP="eth0"
> +BR2_ROOTFS_POST_BUILD_SCRIPT="board/orangepi/common/post-build.sh"
> +BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
> +BR2_ROOTFS_POST_SCRIPT_ARGS="-c $(BINARIES_DIR)/genimage.cfg"
> +BR2_LINUX_KERNEL=y
> +BR2_LINUX_KERNEL_CUSTOM_VERSION=y
> +BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="5.16.1"
> +BR2_LINUX_KERNEL_USE_ARCH_DEFAULT_CONFIG=y
> +BR2_LINUX_KERNEL_DTS_SUPPORT=y
> +BR2_LINUX_KERNEL_INTREE_DTS_NAME="allwinner/sun50i-h5-orangepi-pc2"
> +BR2_LINUX_KERNEL_DTB_KEEP_DIRNAME=y
> +BR2_LINUX_KERNEL_INSTALL_TARGET=y
> +BR2_LINUX_KERNEL_NEEDS_HOST_OPENSSL=y
> +BR2_PACKAGE_LINUX_FIRMWARE=y

Why is linux-firmware installed, but without any sub-option enabled?

> +BR2_PACKAGE_LIBNL=y

Why is this needed?

Otherwise, looks good to me. Could you look at the above issues, and
submit a new iteration?

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