Re: [RFC PATCH wireless-next v3 1/4] wifi: aic: add SDIO BSP and build configuration

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/08/2026 14:11, Yanli Yang wrote:
> From: Zhirun Liu <zhirunliu@aicsemi.com>
> 
> Add the build configuration and SDIO board-support layer for the initial
> AIC8800D80 U02 upstream target.  The BSP handles platform power sequencing,
> SDIO transport setup, firmware loading and chip initialization.
> 
> Limit the selectable configuration to the D80 U02 SDIO station baseline and
> use the kernel crypto library for firmware MD5 diagnostics.
> 
> Signed-off-by: Zhirun Liu <zhirunliu@aicsemi.com>
> Signed-off-by: Yanli Yang <yanli.yang@bedmex.com>

You still did not run checkpatch and has obvious issues pointed out by
it, but more importantly - you still did not document bindings for your
device.



...

> +
> +static DEVICE_ATTR_RW(hwinfo);
> +
> +static DEVICE_ATTR_RW(fwdebug);
> +
> +static DEVICE_ATTR_RW(adapt);

Where did you document the sysfs ABI?

> +
> +static struct attribute *aicbsp_attributes[] = {
> +	&dev_attr_hwinfo.attr,
> +	&dev_attr_fwdebug.attr,
> +	&dev_attr_adapt.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group aicbsp_attribute_group = {
> +	.name = "aicbsp_info",
> +	.attrs = aicbsp_attributes,
> +};
> +
> +int testmode = AICBSP_CPMODE_DEFAULT;
> +int adap_test;

Drop, not suitable for module params.

> +module_param(adap_test, int, 0660);
> +
> +#if defined CONFIG_PLATFORM_EXTERNAL && defined CONFIG_CUSTOM_PWF
> +int gpio_pwr;
> +bool gpio_pwr_assert;

You should not have globals. You need properly organize Linux device
driver structures.

> +
> +void nxp_pwr_ctrl(int value)
> +{
> +	pr_info("aic_bsp: %s: %d\n", __func__, value);
> +	gpio_set_value(gpio_pwr, value);
> +}
> +
> +static int aicbsp_pwr_node(void)
> +{
> +	struct device_node *np;
> +	int err;
> +
> +	np = of_find_compatible_node(NULL, NULL, "aic,wifi-power");


Undocumented compatible. But also unlikely to be accepted. Please read
writing bindings, DTS101 slides or some other guidelines for beginners,
how to properly describe hardware in DTS.



> +	if (!np) {
> +		pr_err("aic_bsp: %s np not found\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	gpio_pwr = of_get_named_gpio(np, "wifi-power", 0);

Do not use legacy API. Please open recent Linux drivers and look how
they manage GPIOs.


> +	if (!gpio_is_valid(gpio_pwr)) {
> +		pr_err("aic_bsp: %s is invalid\n", __func__);
> +		return -ENODEV;
> +	}
> +


...


> +
> +	(void)testmode;

Heh?

> +	aicbsp_firmware_list = fw_8800d80_u02;
> +	aicbsp_info.cpmode = AICBSP_CPMODE_WORK;
> +	if (!fw_path)
> +		return -ENOENT;
> +
> +	pr_info("aicbsp 8800d80 u02 wifi firmware: %s\n", fw_path);

Why your driver does not use dev_* calls? And why does it print so much?

Drivers should be silent on success:
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79

...

> +
> +aic8800_fdrv-y := \
> +	rwnx_msg_tx.o \
> +	rwnx_msg_rx.o \
> +	rwnx_utils.o \
> +	rwnx_cmds.o \
> +	rwnx_irqs.o \
> +	rwnx_cfgfile.o \
> +	rwnx_strs.o \
> +	rwnx_rx.o \
> +	rwnx_tx.o \
> +	rwnx_txq.o \
> +	rwnx_main.o \
> +	rwnx_mod_params.o \
> +	rwnx_platform.o \
> +	aicwf_chip_8800d80.o \
> +	aicwf_chip_ops.o \
> +	rwnx_v7.o \
> +	ipc_host.o \
> +	aicwf_compat_8800d80.o \
> +	regdb.o \
> +	aic_priv_cmd.o \
> +	sdio_host.o \
> +	aicwf_txrxif.o \
> +	aicwf_sdio.o \
> +	aicwf_rx_prealloc.o
> +
> +aic8800_fdrv-$(CONFIG_AIC8800_NONSTA_SUPPORT) += rwnx_mesh.o rwnx_tdls.o
> +
> +aic8800_fdrv-$(CONFIG_AIC8800_GENL) += aicwf_genl.o
> +
> +ccflags-y += -I$(srctree)
> +ccflags-y += -I$(src)
> +ccflags-y += -I$(src)/../aic8800_bsp
> +ccflags-y += -DAIC_TRACE_INCLUDE_PATH=$(src)
> +ccflags-y += -DAICWF_SDIO_SUPPORT
> +ccflags-y += -DCONFIG_SDIO_PWRCTRL
> +ccflags-y += -DCONFIG_RWNX_FULLMAC
> +ccflags-y += -DUSE_5G
> +ccflags-y += -DAICWF_RX_REORDER
> +ccflags-y += -DAICWF_ARP_OFFLOAD
> +ccflags-y += -DCONFIG_RX_NETIF_RECV_SKB
> +ccflags-y += -DCONFIG_AIC8800_RX_NETIF_RECV_SKB

I do not understand what is a ll this. Looks like out of tree module
build. We do not define defines via Makefile. Integrate the driver into
Linux.


Best regards,
Krzysztof