netdev - Re: [PATCH v4 04/17] wifi: cc33xx: Add sdio.c, io.c, io.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9aff2bed-bca4-482b-83ea-4cd945812817@kernel.org>
Date: Tue, 29 Oct 2024 18:34:18 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Michael Nemanov <michael.nemanov@...com>, Kalle Valo <kvalo@...nel.org>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-wireless@...r.kernel.org,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: Sabeeh Khan <sabeeh-khan@...com>
Subject: Re: [PATCH v4 04/17] wifi: cc33xx: Add sdio.c, io.c, io.h

On 29/10/2024 18:23, Michael Nemanov wrote:
> sdio.c implements SDIO transport functions. These are bound into
> struct cc33xx_if_operations and accessed via io.h in order to abstract
> multiple transport interfaces such as SPI in the future.
> The CC33xx driver supports the SDIO in-band IRQ option so the IRQ from
> the device received here as well.
> Unlike wl1xxx products, there is no longer mapping between
> HW and SDIO / SPI address space of any kind.
> There are only 3 valid addresses for control, data and status
> transactions each with a predefined structure.
> 
> Signed-off-by: Michael Nemanov <michael.nemanov@...com>
> ---
>  drivers/net/wireless/ti/cc33xx/io.c   | 129 +++++++
>  drivers/net/wireless/ti/cc33xx/io.h   |  26 ++
>  drivers/net/wireless/ti/cc33xx/sdio.c | 530 ++++++++++++++++++++++++++
>  3 files changed, 685 insertions(+)
>  create mode 100644 drivers/net/wireless/ti/cc33xx/io.c
>  create mode 100644 drivers/net/wireless/ti/cc33xx/io.h
>  create mode 100644 drivers/net/wireless/ti/cc33xx/sdio.c
> 
> diff --git a/drivers/net/wireless/ti/cc33xx/io.c b/drivers/net/wireless/ti/cc33xx/io.c
> new file mode 100644
> index 000000000000..59696004efe9
> --- /dev/null
> +++ b/drivers/net/wireless/ti/cc33xx/io.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022-2024 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#include "cc33xx.h"
> +#include "debug.h"
> +#include "io.h"
> +#include "tx.h"
> +
> +bool cc33xx_set_block_size(struct cc33xx *cc)
> +{
> +	if (cc->if_ops->set_block_size) {
> +		cc->if_ops->set_block_size(cc->dev, CC33XX_BUS_BLOCK_SIZE);
> +		cc33xx_debug(DEBUG_CC33xx,
> +			     "Set BLKsize to %d", CC33XX_BUS_BLOCK_SIZE);
> +		return true;
> +	}
> +
> +	cc33xx_debug(DEBUG_CC33xx, "Could not set BLKsize");
> +	return false;
> +}
> +
> +void cc33xx_disable_interrupts_nosync(struct cc33xx *cc)
> +{
> +	cc->if_ops->disable_irq(cc->dev);
> +}
> +
> +void cc33xx_irq(void *cookie);

Why do you need forward declaration of non-static function? If you need
it, it means you had W=1 warning which you fixed incorrect way.

Regardless, be sure this code has 0 warnings on clang with W=1.


> +void cc33xx_enable_interrupts(struct cc33xx *cc)
> +{
> +	cc->if_ops->enable_irq(cc->dev);
> +
> +	cc33xx_irq(cc);
> +}

...


> +static const struct cc33xx_if_operations sdio_ops_inband_irq = {
> +	.interface_claim	= cc33xx_sdio_claim,
> +	.interface_release	= cc33xx_sdio_release,
> +	.read			= cc33xx_sdio_raw_read,
> +	.write			= cc33xx_sdio_raw_write,
> +	.power			= cc33xx_sdio_set_power,
> +	.set_block_size	= cc33xx_sdio_set_block_size,
> +	.set_irq_handler	= cc33xx_set_irq_handler,
> +	.disable_irq		= cc33xx_sdio_disable_irq,
> +	.enable_irq		= cc33xx_sdio_enable_irq,
> +};
> +
> +#ifdef CONFIG_OF
> +static const struct cc33xx_family_data cc33xx_data = {
> +	.name = "cc33xx",
> +	.cfg_name = "ti-connectivity/cc33xx-conf.bin",
> +	.nvs_name = "ti-connectivity/cc33xx-nvs.bin",
> +};
> +
> +static const struct of_device_id cc33xx_sdio_of_match_table[] = {
> +	{ .compatible = "ti,cc33xx", .data = &cc33xx_data },

This is supposed to be your base variant.

> +	{ }
> +};

Missing MODULE_DEVICE_TABLE... or you do not autoload this based on OF
matching? That's a bit surprising, I don't remember how SDIO bus handles
it. But for other buses this is unexpected and usually not correct.

> +

...

> +
> +static struct sdio_driver cc33xx_sdio_driver = {
> +	.name		= "cc33xx_sdio",
> +	.id_table	= cc33xx_devices,
> +	.probe		= sdio_cc33xx_probe,
> +	.remove		= sdio_cc33xx_remove,
> +#ifdef CONFIG_PM
> +	.drv = {
> +		.pm = &cc33xx_sdio_pm_ops,
> +	},
> +#endif /* CONFIG_PM */
> +};
> +
> +MODULE_DEVICE_TABLE(sdio, cc33xx_devices);

This is always next to the table.

> +
> +module_sdio_driver(cc33xx_sdio_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("SDIO transport for Texas Instruments CC33xx WLAN driver");
> +MODULE_AUTHOR("Michael Nemanov <michael.nemanov@...com>");
> +MODULE_AUTHOR("Sabeeh Khan <sabeeh-khan@...com>");

Best regards,
Krzysztof