Re: [spi-devel-general] [PATCH v4] spi: Add PPC4xx SPI driver

On Friday 31 October 2008, Stefan Roese wrote:

+static int spi_ppc4xx_setupxfer(struct spi_device *spi, struct spi_transfer *t)
+{
		...

+       if (in_8(&hw->regs->cdm) != cdm)
+               out_8(&hw->regs->cdm, cdm);

... writes to hardware, updating SPI the clock rate ...

+static int spi_ppc4xx_setup(struct spi_device *spi)
+{
	...
+
+       ret = spi_ppc4xx_setupxfer(spi, NULL);

... hence this also writes to the hardware.

Except ... the setup() method may be called for one SPI device
in the middle of a transfer for another device.  This might for
example cause the clock rate to speed up so it's too fast, thus
corrupting a transfer for that other device.  So you should NOT
be calling setupxfer() here.

+       /* write new configration */
+       out_8(&hw->regs->mode, mode);

Or here:  changing from a device using MSB-first mode 3 to
one using LSB-first mode 1.  This could also corrupt a transfer.


It might be better to have the setup() validate its inputs
and store them in the spi->controller_state, and then have
the setup_transfer() pull values from there ... but not make
setup() call setupxfer().  The best model would be that each
chipselect has its own set of speed/mode registers; if the
hardware doesn't work that way, then it can be emulated.

+static int __init spi_ppc4xx_init(void)
+{
+       return of_register_platform_driver(&spi_ppc4xx_of_driver);
+}
+
+static void __exit spi_ppc4xx_exit(void)
+{
+       of_unregister_platform_driver(&spi_ppc4xx_of_driver);
+}
+
+module_init(spi_ppc4xx_init);
+module_exit(spi_ppc4xx_exit);

I prefer to see module_{init,exit} annotations snugged up next
to the functions to which they apply ... just like EXPORT_SYMBOL
annotations, and for the same reasons.

- Dave