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

[David Brownell]

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