Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org>]

Daniel Ribeiro wrote:
> Hi!
> 
> You were right, there is nothing wrong with the spi transfer.
> 
> The problem is the hardware. This chip really needs some delay before
> deasserting CS on register writes. Otherwise it just silently ignores
> the register write.
> 
> I no longer think that anything is wrong with spi communication, i can
> reliably read any chip register. The problem only occurs on register
> writes, and not even on all registers.
> 
> On giveback(), if i add a delay _before_ cs_control(CS_DEASSERT), it works.
> If i add it just _after_ cs_control() it does not work.
> 
> I see that there is a .delay_usecs field on spi_transfer:
> 
>  * @delay_usecs: microseconds to delay after this transfer before
>  *      (optionally) changing the chipselect status, then starting
>  *      the next transfer or completing this @spi_message.
> 
> But it is only called for the previous transfer when on RUNNING_STATE,
> so a second spi_transfer is needed on the same spi_message to actually
> use this feature.
> 
> Should i submit a patch to use this delay_usecs field also on
> DONE_STATE, so it actually do something when set on the last
> spi_transfer? Or should i just cheat pxa2xx_spi with:
> 
>         struct spi_transfer t[2];
>         struct spi_message m;
> 
>         spi_message_init(&m);
>         t[0].len = 4;
>         t[0].tx_buf = (u8 *)data;
>         t[0].rx_buf = (u8 *)data;
>         t[0].bits_per_word = 32;
>         t[0].delay_usecs = 300;
>         spi_message_add_tail(&t[0], &m);
> 
>         /* trick pxa2xx_spi so it uses t[0].delay_usecs */
>         t[1].len = 0;
>         spi_message_add_tail(&t[1], &m);
> 
>         return spi_sync(pcap.spi, &m);
> 
> (with the above code my chip works great without changes to pxa2xx_spi.c)
> 
> My opinion is that this field should be used to delay also on
> DONE_STATE: "microseconds to delay after this transfer before changing
> the chipselect status, then starting the next transfer
> *or*completing*this*spi_message*"
> 
> I will be happy to submit a patch to change this if you think the
> delay on DONE_STATE is acceptable.

Wow! Our messages crossed in the ether.  I'm glad we agree now on the
nature of the problem.  Yes, I think that delay_usecs should be honored
on every transfer on which it is non-zero.  As noted in my previous
email, there is a known bug in the driver in this regard, and if you
would like to submit a patch for it, that would be great.  Exactly what
that patch should look like, I'm not sure at the moment.  It is very
late where I live and I would prefer to consider this tomorrow.  I would
like to suggest/review this sort of patch, to make sure it is as
compatible as possible with my unreleased expansion of this driver.

I'm still curious to know how much delay was added by the various
patches you tested.  My hardware is not available at the moment, so I
cannot test, and I would be testing a PXA255, in any case.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/