Re: [PATCH 9/9 v3] spi: add Broadcom BCM63xx SPI controller driver

Hello Grant,

On Tuesday 31 January 2012 21:19:22 Grant Likely wrote:

On Tue, Jan 31, 2012 at 03:10:48PM +0100, Florian Fainelli wrote:

quoted

This patch adds support for the SPI controller found on the Broadcom
BCM63xx SoCs.

Signed-off-by: Tanguy Bouzeloc <redacted>
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
Changes since v2:
- reworked bcm63xx_spi_setup_transfer to choose closest spi transfer

  frequency

- removed invalid 25Mhz frequency
- fixed some minor checkpatch issues

Changes since v1:
- switched to the devm_* API which frees resources automatically
- switched to dev_pm_ops
- use module_platform_driver
- remove MODULE_VERSION()
- fixed return value when clock is not present using PTR_ERR()
- fixed probe() error path to disable clock in case of failure

 drivers/spi/Kconfig       |    6 +
 drivers/spi/Makefile      |    1 +
 drivers/spi/spi-bcm63xx.c |  486
 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 493
 insertions(+), 0 deletions(-)
 create mode 100644 drivers/spi/spi-bcm63xx.c

Looks okay.  There are a couple of problems that needs to be fixed
below, but otherwise:

Acked-by: Grant Likely <redacted>

Merge this through the same tree as patches 1-8

g.

quoted

+static int __init bcm63xx_spi_probe(struct platform_device *pdev)

__devinit

quoted

+static int __exit bcm63xx_spi_remove(struct platform_device *pdev)

__devexit

quoted

+static const struct dev_pm_ops bcm63xx_spi_pm_ops = {
+	.suspend	= bcm63xx_spi_suspend,
+	.resume		= bcm63xx_spi_resume,
+};
+
+#define BCM63XX_SPI_PM_OPS	(&bcm63xx_spi_pm_ops)
+#else
+#define BCM63XX_SPI_PM_OPS	NULL

A bit ugly.  Do this instead in the else clause and drop the
BCM63XX_SPI_PM_OPS:

#define bcm63xx_spi_pm_ops NULL

This won't work, because driver.pm must be set to a pointer to a struct 
dev_pm_ops, that's why I used this trick to make it build fine in both cases. 
If I follow your advice, with driver.pm = &bcm63xx_spi_pm_ops, it won't build 
for CONFIG_PM=n.

quoted

+#endif
+
+static struct platform_driver bcm63xx_spi_driver = {
+	.driver = {
+		.name	= "bcm63xx-spi",
+		.owner	= THIS_MODULE,
+		.pm	= BCM63XX_SPI_PM_OPS,
+	},
+	.probe		= bcm63xx_spi_probe,
+	.remove		= __exit_p(bcm63xx_spi_remove),

__devexit_p

quoted

+};
+
+module_platform_driver(bcm63xx_spi_driver);
+
+MODULE_ALIAS("platform:bcm63xx_spi");
+MODULE_AUTHOR("Florian Fainelli [off-list ref]");
+MODULE_AUTHOR("Tanguy Bouzeloc [off-list ref]");
+MODULE_DESCRIPTION("Broadcom BCM63xx SPI Controller driver");
+MODULE_LICENSE("GPL");

-- 
Florian