Re: [RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Gabor Juhos <juhosg@openwrt.org>]

Hi Grant,

>> <...>
>> +#include <asm/mach-ath79/ath79_spi_platform.h>
>> +
>> +#define DRV_DESC	"SPI controller driver for Atheros AR71XX/AR724X/AR91X"
> 
> Used exactly once.  Don't bother with a #define

Ok.

>> +#define DRV_NAME	"ath79-spi"
>> +
>> +struct ath79_spi {
>> +	struct	spi_bitbang	bitbang;
>> +	u32			ioc_base;
>> +	u32			reg_ctrl;
>> +
>> +	void __iomem		*base;
>> +
>> +	struct platform_device	*pdev;
>> +};
>> +
>> +static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
>> +{
>> +	return __raw_readl(sp->base + reg);
>> +}
>> +
>> +static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
>> +{
>> +	__raw_writel(val, sp->base + reg);
>> +}
> 
> This is suspect.  Why is __raw_{readl,writel} being used instead of
> ioread32/iowrite32?  The __raw versions don't provide any kind of
> ordering barriers.

Mainly because the resulting code is smaller, and the performance is a bit
better with the use of the __raw versions. The controller is embedded into the
SoC and the registers are memory mapped, so i think it is safe to access them
with __raw_{readl,writel}. However I can change it if that is the preferred method.

>> <...>
>> +static int ath79_spi_probe(struct platform_device *pdev)
> 
> __devinit

I will add this.

> 
>> +{
>> +	struct spi_master *master;
>> +	struct ath79_spi *sp;
>> +	struct ath79_spi_platform_data *pdata;
>> +	struct resource	*r;
>> +	int ret;
>> +
>> +	master = spi_alloc_master(&pdev->dev, sizeof(*sp));
>> +	if (master == NULL) {
>> +		dev_err(&pdev->dev, "failed to allocate spi master\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	sp = spi_master_get_devdata(master);
>> +	platform_set_drvdata(pdev, sp);
>> +
>> +	pdata = pdev->dev.platform_data;
>> +
>> +	master->setup = ath79_spi_setup;
>> +	master->cleanup = ath79_spi_cleanup;
>> +	if (pdata) {
>> +		master->bus_num = pdata->bus_num;
>> +		master->num_chipselect = pdata->num_chipselect;
>> +	} else {
>> +		master->bus_num = 0;
> 
> Use -1 so that a bus number can be dynamically assigned

All right.

>> <...>
>> +static int ath79_spi_remove(struct platform_device *pdev)
> 
> __devexit
> 
>> +{
>> +	struct ath79_spi *sp = platform_get_drvdata(pdev);
>> +
>> +	spi_bitbang_stop(&sp->bitbang);
>> +	iounmap(sp->base);
>> +	platform_set_drvdata(pdev, NULL);
>> +	spi_master_put(sp->bitbang.master);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver ath79_spi_drv = {
>> +	.probe		= ath79_spi_probe,
>> +	.remove		= ath79_spi_remove,
> 
> __devexit_p(ath79_spi_remove),
> 

I will add these annotations as well.

Thank you,
Gabor