Re: SPI chip select problem

[Johannes Stezenbach <js@sig21.net>]

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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox