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

[Florian Fainelli <florian@openwrt.org>]

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:
> > This patch adds support for the SPI controller found on the Broadcom
> > BCM63xx SoCs.
> > 
> > Signed-off-by: Tanguy Bouzeloc <tanguy.bouzeloc@efixo.com>
> > 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 <grant.likely@secretlab.ca>
> 
> Merge this through the same tree as patches 1-8
> 
> g.
> 
> > +static int __init bcm63xx_spi_probe(struct platform_device *pdev)
> 
> __devinit
> 
> > +static int __exit bcm63xx_spi_remove(struct platform_device *pdev)
> 
> __devexit
> 
> > +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.

> 
> > +#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
> 
> > +};
> > +
> > +module_platform_driver(bcm63xx_spi_driver);
> > +
> > +MODULE_ALIAS("platform:bcm63xx_spi");
> > +MODULE_AUTHOR("Florian Fainelli <florian@openwrt.org>");
> > +MODULE_AUTHOR("Tanguy Bouzeloc <tanguy.bouzeloc@efixo.com>");
> > +MODULE_DESCRIPTION("Broadcom BCM63xx SPI Controller driver");
> > +MODULE_LICENSE("GPL");

-- 
Florian