Re: [PATCH v4] spi: add Broadcom BCM63xx SPI controller driver

[Maxime Bizon <mbizon@freebox.fr>]

On Wed, 2012-02-01 at 11:14 +0100, Florian Fainelli wrote:

Hi Florian,

> +struct bcm63xx_spi {
> +	spinlock_t		lock;

this lock is never actually used

it is referenced only once in device removal path

> +	int			stopping;

this can be removed by changing device removal path to first call
spi_unregister_master. that way the spi stack cannot call spi_transfer
anymore and we don't need to abort these calls.


> +static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
> +{

> [...]

> +	bcm_spi_writew(bs, cmd, SPI_CMD);
> +	wait_for_completion(&bs->done);
> +

bcm63xx_txrx_bufs() is called by bcm63xx_transfer(), and according to
Documentation/spi/spi-summary:

    master->transfer(struct spi_device *spi, struct spi_message *message)
	This must not sleep.  Its responsibility is arrange that the
        transfer happens and its complete() callback is issued.  The two
        will normally happen later, after other transfers complete, and
        if the controller is idle it will need to be kickstarted.

So we cannot do a synchronous wait here, this must be pushed to a
workqueue or kthread.


-- 
Maxime