Re: [Buildroot] [PATCH] rtl8822cs: new package

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Viacheslav Bocharov via buildroot <buildroot@buildroot.org>
Cc: Viacheslav Bocharov <adeep@lexina.in>
Subject: Re: [Buildroot] [PATCH] rtl8822cs: new package
Date: Thu, 13 Jul 2023 23:25:18 +0200	[thread overview]
Message-ID: <20230713232518.7aed05dc@windsurf> (raw)
In-Reply-To: <20221125102650.2804226-1-adeep@lexina.in>

Hello Viacheslav,

On Fri, 25 Nov 2022 13:26:50 +0300
Viacheslav Bocharov via buildroot <buildroot@buildroot.org> wrote:

> This package adds the rtl88822cs WiFi driver.
>    repo: https://github.com/jethome-ru/rtl88x2cs.git
>    branch: tune_for_jethub
> 
> Driver is known to support Realtek RTL8822CS SDIO WiFi/BT chip.
> 
> Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>

Sorry for the super long delay, but good news: I have now applied your
patch! I made some changes, which I will explain below for your
reference.

First, the commit title should have been:

	package/rtl8822cs: new package

> diff --git a/package/rtl8822cs/Config.in b/package/rtl8822cs/Config.in
> new file mode 100644
> index 0000000000..c887ae7e02
> --- /dev/null
> +++ b/package/rtl8822cs/Config.in
> @@ -0,0 +1,12 @@
> +comment "RTL8822CS needs a Linux kernel to be built"

rtl8822cs in lower-case, like the package name.

> +	depends on !BR2_s390x
> +	depends on !BR2_LINUX_KERNEL
> +
> +config BR2_PACKAGE_RTL8822CS
> +	bool "Realtek RTL8822CS SDIO Wi-Fi driver"

Just:

	bool "rtl8822cs"

like the package name.

> +	depends on !BR2_s390x
> +	depends on BR2_LINUX_KERNEL
> +	help
> +          Realtek RTL8822CS Wi-Fi driver as a kernel module (JetHome repository)

Line was slightly too long, so I wrapped it. You can run "make
check-package" to get this kind of sanity checking.

> +
> +          https://github.com/jethome-ru/rtl88x2cs/
> diff --git a/package/rtl8822cs/rtl8822cs.hash b/package/rtl8822cs/rtl8822cs.hash
> new file mode 100644
> index 0000000000..11633e1633
> --- /dev/null
> +++ b/package/rtl8822cs/rtl8822cs.hash
> @@ -0,0 +1 @@
> +sha256 4cd97adcf44dc4196fce6f87e68370ad588b19bbc38b246615f9c05739f7bd00  rtl8822cs-db8dc6c7ae1a75af3f6d7fa4f05456c76f5cab3e.tar.gz

         ^^ we want two spaces as a separator

Also, we want a comment that says where the hash comes from. In this case:

# Locally calculated

> diff --git a/package/rtl8822cs/rtl8822cs.mk b/package/rtl8822cs/rtl8822cs.mk
> new file mode 100644
> index 0000000000..799a07823d
> --- /dev/null
> +++ b/package/rtl8822cs/rtl8822cs.mk
> @@ -0,0 +1,26 @@
> +################################################################################
> +#
> +# Realtek RTL8822CS driver

Just:

# rtl8822cs

(i.e, just the package name)

> +#
> +################################################################################
> +
> +RTL8822CS_VERSION = db8dc6c7ae1a75af3f6d7fa4f05456c76f5cab3e

I updated that to a newer commit that has fixes to build with Linux >= 6.3.

> +RTL8822CS_SITE = $(call github,jethome-ru,rtl88x2cs,$(RTL8822CS_VERSION))
> +RTL8822CS_LICENSE = GPL-2.0

It would be nice to ask the maintainer of this github repo to add a
license file.

> +
> +RTL8822CS_MODULE_MAKE_OPTS = \
> +	CONFIG_RTL8822CS=m \
> +	KVER=$(LINUX_VERSION_PROBED) \
> +	KSRC=$(LINUX_DIR)
> +
> +define RTL8822CS_LINUX_CONFIG_FIXUPS
> +	$(call KCONFIG_ENABLE_OPT,CONFIG_NET)
> +	$(call KCONFIG_ENABLE_OPT,CONFIG_WIRELESS)
> +	$(call KCONFIG_ENABLE_OPT,CONFIG_CFG80211)
> +	$(call KCONFIG_ENABLE_OPT,CONFIG_MAC80211)
> +	$(call KCONFIG_ENABLE_OPT,CONFIG_MMC)
> +endef
> +
> +

One too many newline here.

As I said: I fixed all those minor details and applied to our master
branch.

Thanks for your contribution!

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