Re: [PATCH] spi: add driver for BCM2835

[Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2218 bytes --]

On Tue, Mar 05, 2013 at 07:49:02PM -0700, Stephen Warren wrote:

> +Optional properties:
> +- brcm,realtime: Boolean. Indicates the driver should operate with realtime
> +  priority to minimise the transfer latency on the bus.

This isn't obviously something that ought to be in DT, it'll depend on
the OS, kernel version and so on.  Indeed I don't think this is used any
more as the generic pump code Linus did handles it already in a runtime
tunable way?

> +static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
> +{

> +	if (cs & BCM2835_SPI_CS_DONE) {

> +		}
> +	} else if (cs & BCM2835_SPI_CS_RXR) {
> +		/* read 12 bytes of data */

I'd feel happier if these were independent statements in case both are
asserted simultaneously.

> +	return IRQ_HANDLED;


What if neither of the statuses asserted?

> +	switch (bpw) {
> +	case 8:
> +		break;
> +	default:
> +		dev_err(&spi->dev, "unsupported bits_per_word=%d\n", bpw);
> +		return -EINVAL;
> +	}

> +	if (!(spi->mode & SPI_NO_CS) &&
> +			(spi->chip_select > spi->master->num_chipselect)) {
> +		dev_err(&spi->dev,
> +				"invalid chipselect %u\n",
> +				spi->chip_select);
> +		return -EINVAL;
>` +	}

This seems like stuff the core should be able to do for you.

> +	list_for_each_entry(tfr, &mesg->transfers, transfer_list) {
> +		err = bcm2835_spi_check_transfer(spi, tfr);
> +		if (err)
> +			goto out;
> +
> +		err = bcm2835_spi_start_transfer(spi, tfr);
> +		if (err)
> +			goto out;
> +
> +		timeout = wait_for_completion_timeout(&bs->done,
> +				msecs_to_jiffies(BCM2835_SPI_TIMEOUT_MS));
> +		if (!timeout) {
> +			err = -ETIMEDOUT;
> +			goto out;
> +		}

But I wanted to transfer 10G in a single message at 1kHz!  :P

> +	/* initialise the hardware */
> +	clk_prepare_enable(bs->clk);
> +	bcm2835_wr(bs, BCM2835_SPI_CS,
> +		   BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);

It'd be nice to only enable the clock during transfers.

> +static int bcm2835_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *master = platform_get_drvdata(pdev);
> +	struct bcm2835_spi *bs = spi_master_get_devdata(master);
> +
> +	spi_unregister_master(master);
> +	free_irq(bs->irq, master);

Should these be the other way around?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel