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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
