On 06/08/2024 19:00, 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@xxxxxx>
...
> +
> +#include <linux/mmc/sdio_func.h>
> +#include <linux/mmc/host.h>
> +#include <linux/gpio.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of_irq.h>
> +
> +#include "cc33xx.h"
> +#include "io.h"
> +
> +#ifndef SDIO_VENDOR_ID_TI
> +#define SDIO_VENDOR_ID_TI 0x0097
> +#endif
> +
> +#define SDIO_DEVICE_ID_CC33XX_NO_EFUSE 0x4076
> +#define SDIO_DEVICE_ID_TI_CC33XX 0x4077
> +
> +static bool dump;
Drop.
> +
> +struct cc33xx_sdio_glue {
> + struct device *dev;
> + struct platform_device *core;
> +};
> +
> +static const struct sdio_device_id cc33xx_devices[] = {
> + { SDIO_DEVICE(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_CC33XX) },
> + { SDIO_DEVICE(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_CC33XX_NO_EFUSE) },
> + {}
> +};
> +MODULE_DEVICE_TABLE(sdio, cc33xx_devices);
Keep all tables next to each other - bottom of the driver. Spreading
same type of structures over the file does not help in review.
> +
> +
> +static struct cc33xx_if_operations sdio_ops_gpio_irq = {
Why this is not const?
> + .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_disable_line_irq,
> + .enable_irq = cc33xx_enable_line_irq,
> +};
> +
> +static 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,cc3300", .data = &cc33xx_data },
> + { .compatible = "ti,cc3301", .data = &cc33xx_data },
> + { .compatible = "ti,cc3350", .data = &cc33xx_data },
> + { .compatible = "ti,cc3351", .data = &cc33xx_data },
> + { }
> +};
Eh? What happened here? So devices are compatibles thus make them
compatible in the bindings.
> +
> +static int sdio_cc33xx_probe(struct sdio_func *func,
> + const struct sdio_device_id *id)
> +{
> + struct cc33xx_platdev_data *pdev_data;
> + struct cc33xx_sdio_glue *glue;
> + struct resource res[1];
> + mmc_pm_flag_t mmcflags;
> + int ret = -ENOMEM;
> + int gpio_irq, wakeirq, irq_flags;
> + const char *chip_family;
> +
> + /* We are only able to handle the wlan function */
> + if (func->num != 0x02)
> + return -ENODEV;
> +
> + pdev_data = devm_kzalloc(&func->dev, sizeof(*pdev_data), GFP_KERNEL);
> + if (!pdev_data)
> + return -ENOMEM;
> +
> + glue = devm_kzalloc(&func->dev, sizeof(*glue), GFP_KERNEL);
> + if (!glue)
> + return -ENOMEM;
> +
> + glue->dev = &func->dev;
> +
> + /* Grab access to FN0 for ELP reg. */
> + func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
> +
> + /* Use block mode for transferring over one block size of data */
> + func->card->quirks |= MMC_QUIRK_BLKSZ_FOR_BYTE_MODE;
> +
> + ret = cc33xx_probe_of(&func->dev, &gpio_irq, &wakeirq, pdev_data);
> + if (ret)
> + goto out;
> +
> + /* if sdio can keep power while host is suspended, enable wow */
> + mmcflags = sdio_get_host_pm_caps(func);
> + dev_dbg(glue->dev, "sdio PM caps = 0x%x\n", mmcflags);
> +
> + sdio_set_drvdata(func, glue);
> +
> + /* Tell PM core that we don't need the card to be powered now */
> + pm_runtime_put_noidle(&func->dev);
> +
> + chip_family = "cc33xx";
That's usless assignment.
> +
> + glue->core = platform_device_alloc(chip_family, PLATFORM_DEVID_AUTO);
> + if (!glue->core) {
> + dev_err(glue->dev, "can't allocate platform_device");
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + glue->core->dev.parent = &func->dev;
> +
> + if (gpio_irq) {
> + dev_info(glue->dev, "Using GPIO as IRQ\n");
Drop. Driver should be silent.
> +
> + irq_flags = irqd_get_trigger_type(irq_get_irq_data(gpio_irq));
> +
> + irq_set_status_flags(gpio_irq, IRQ_NOAUTOEN);
> +
> + if (irq_flags & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
> + irq_flags |= IRQF_ONESHOT;
> +
> + ret = request_threaded_irq(gpio_irq, gpio_irq_hard_handler,
> + gpio_irq_thread_handler,
> + irq_flags, glue->core->name, func);
> + if (ret) {
> + dev_err(glue->dev, "can't register GPIO IRQ handler\n");
> + goto out_dev_put;
> + }
> +
> + pdev_data->gpio_irq_num = gpio_irq;
> +
> + if ((mmcflags & MMC_PM_KEEP_POWER) &&
> + (enable_irq_wake(gpio_irq) == 0))
> + pdev_data->pwr_in_suspend = true;
> +
> + pdev_data->if_ops = &sdio_ops_gpio_irq;
> + } else {
> + dev_info(glue->dev, "Using SDIO in-band IRQ\n");
> +
> + pdev_data->if_ops = &sdio_ops_inband_irq;
> + }
> +
> + if (wakeirq > 0) {
> + res[0].start = wakeirq;
> + res[0].flags = IORESOURCE_IRQ |
> + irqd_get_trigger_type(irq_get_irq_data(wakeirq));
> + res[0].name = "wakeirq";
> +
> + ret = platform_device_add_resources(glue->core, res, 1);
> + if (ret) {
> + dev_err(glue->dev, "can't add resources\n");
> + goto out_dev_put;
> + }
> + }
> +
> + ret = platform_device_add_data(glue->core, pdev_data,
> + sizeof(*pdev_data));
> + if (ret) {
> + dev_err(glue->dev, "can't add platform data\n");
> + goto out_dev_put;
> + }
> +
> + ret = platform_device_add(glue->core);
> + if (ret) {
> + dev_err(glue->dev, "can't add platform device\n");
> + goto out_dev_put;
> + }
> + return 0;
> +
> +out_dev_put:
> + platform_device_put(glue->core);
> +
> + if (pdev_data->gpio_irq_num)
> + free_irq(pdev_data->gpio_irq_num, func);
> +
> +out:
> + return ret;
> +}
> +
> +static void sdio_cc33xx_remove(struct sdio_func *func)
> +{
> + struct cc33xx_sdio_glue *glue = sdio_get_drvdata(func);
> + struct platform_device *pdev = glue->core;
> + struct cc33xx_platdev_data *pdev_data = dev_get_platdata(&pdev->dev);
> +
> + /* Undo decrement done above in sdio_cc33xx_probe */
> + pm_runtime_get_noresume(&func->dev);
> +
> + platform_device_unregister(glue->core);
> +
> + if (pdev_data->gpio_irq_num) {
> + free_irq(pdev_data->gpio_irq_num, func);
> + if (pdev_data->pwr_in_suspend)
> + disable_irq_wake(pdev_data->gpio_irq_num);
> + } else {
> + sdio_claim_host(func);
> + sdio_release_irq(func);
> + sdio_release_host(func);
> + }
> +}
> +
> +#ifdef CONFIG_PM
> +static int cc33xx_suspend(struct device *dev)
> +{
> + /* Tell MMC/SDIO core it's OK to power down the card
> + * (if it isn't already), but not to remove it completely
> + */
> + struct sdio_func *func = dev_to_sdio_func(dev);
> + struct cc33xx_sdio_glue *glue = sdio_get_drvdata(func);
> + struct cc33xx *cc = platform_get_drvdata(glue->core);
> + mmc_pm_flag_t sdio_flags;
> + int ret = 0;
> +
> + if (!cc) {
> + dev_err(dev, "no wilink module was probed\n");
> + goto out;
> + }
> +
> + dev_dbg(dev, "cc33xx suspend. keep_device_power: %d\n",
> + cc->keep_device_power);
> +
> + if (cc->keep_device_power) {
> + sdio_flags = sdio_get_host_pm_caps(func);
> +
> + if (!(sdio_flags & MMC_PM_KEEP_POWER)) {
> + dev_err(dev, "can't keep power while host is suspended\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* keep power while host suspended */
> + ret = sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
> + if (ret) {
> + dev_err(dev, "error while trying to keep power\n");
> + goto out;
> + }
> + }
> +out:
> + return ret;
> +}
> +
> +static int cc33xx_resume(struct device *dev)
> +{
> + dev_dbg(dev, "cc33xx resume\n");
I asked to drop such silly confirmation messages. Kernel has already
infrastructure for this.
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops cc33xx_sdio_pm_ops = {
> + .suspend = cc33xx_suspend,
> + .resume = cc33xx_resume,
> +};
> +
> +static struct sdio_driver cc33xx_sdio_driver = {
> + .name = "cc33xx_sdio",
> + .id_table = cc33xx_devices,
> + .probe = sdio_cc33xx_probe,
> + .remove = sdio_cc33xx_remove,
> + .drv = {
> + .pm = &cc33xx_sdio_pm_ops,
> + },
> +};
> +#else
No, that's terrible, you cannot have two drivers! Look how drivers are
doing - there is a wrapper or just use maybe_unused.
> +static struct sdio_driver cc33xx_sdio_driver = {
> + .name = "cc33xx_sdio",
> + .id_table = cc33xx_devices,
> + .probe = sdio_cc33xx_probe,
> + .remove = sdio_cc33xx_remove,
> +};
> +#endif /* CONFIG_PM */
> +
> +static int __init sdio_cc33xx_init(void)
> +{
> + return sdio_register_driver(&cc33xx_sdio_driver);
> +}
> +
> +static void __exit sdio_cc33xx_exit(void)
> +{
> + sdio_unregister_driver(&cc33xx_sdio_driver);
> +}
> +
> +module_init(sdio_cc33xx_init);
> +module_exit(sdio_cc33xx_exit);
Look at other drivers how they do it - there is a wrapper.
> +
> +module_param(dump, bool, 0600);
> +MODULE_PARM_DESC(dump, "Enable sdio read/write dumps.");
This should be rather debug interface, not module param.
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("SDIO transport for Texas Instruments CC33xx WLAN driver");
> +MODULE_AUTHOR("Michael Nemanov <michael.nemanov@xxxxxx>");
> +MODULE_AUTHOR("Sabeeh Khan <sabeeh-khan@xxxxxx>");
Best regards,
Krzysztof
