Hi,
On Mon, Jun 25, 2012 at 11:45:06AM +0400, Antony Pavlov wrote:
> I have added spi controller driver for one of my MIPS boards and
> found, that there is a problem with chip select.
>
> During initialisation we call *_spi_setup() method. It switch chip
> select and frequency for every probing spi slave chip.
> But after initialisation __we never__ call this method. So if I have
> more than 1 spi slave chip, I can use only last of them.
>
> There is the 'cs_change' flag for *_spi_transfer() method, but this
> flag does not used at all!
altera_spi.c and mic_spi.c use it, but often this is not needed (e.g. for
SPI flashes) so to keep the code simple and small it might be better
to not implement cs_change handling.
> I have made quick-and-dirty patch:
>
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -196,6 +196,8 @@ EXPORT_SYMBOL(spi_register_master);
>
> int spi_sync(struct spi_device *spi, struct spi_message *message)
> {
> + spi->master->setup(spi);
> +
> return spi->master->transfer(spi, message);
> }
I noticed this issue, too, but since barebox spi code is similar
to linux code I'd like to point out linux drivers must not modify the
chip select in ->setup(). Since bare box only does synchronous transfers
it isn't an issue, but in linux ->setup() is called when
a new message is queued, at this time the previous message
might still be transferring. Thus the only purpose of ->setup()
is to do error checking on the provided parameters.
Chip select, frequency etc. must be set in ->transfer().
In the interest of portability/similarity barebox drivers should
do the same as linux.
That said, your proposed patch still looks OK.
BTW, ->cleanup() is also never called, but so far no driver needs it...
Johannes
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
