Message-ID: <5020193f-64f5-48ff-8597-8e6a5ca0fb36@kernel.org>
Date: Wed, 7 Aug 2024 09:12:49 +0200
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 v3 03/17] wifi: cc33xx: Add sdio.c, io.c, io.h

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@...com>


...

> +
> +#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@...com>");
> +MODULE_AUTHOR("Sabeeh Khan <sabeeh-khan@...com>");

Best regards,
Krzysztof

Powered by blists - more mailing lists