SPI chip select problem

[Antony Pavlov]

On 25 June 2012 12:53, Johannes Stezenbach <js at sig21.net> wrote:
> 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.

But I have not found any common code where cs_change is managed.

>
>> 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



-- 
Best regards,
  Antony Pavlov