[<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
Powered by blists - more mailing lists