spi_bitbang cs_change hinting fix

spi_bitbang cs_change hinting fix

[Jan Nikitenko]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

Fix condition for deactivating chipselect after processing of
spi_message - if cs_change==0, chipselect should not be touched and left
active for next spi_message to follow.

Signed-off-by: Jan Nikitenko <jan.nikitenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

---

I am not sure about the meaning of spi message transfer cs_change attribute.

I assume that non-zero cs_change indicate that chipselect should be
toggled between spi_transfers.

What about cs_change in last spi_transfer of spi_message?
Should zero cs_change provide hinting that chipselect should be left
active, because spi_message to follow will be for the same spi device?

If that is so, the handling in spi_bitbang.c would be incorrect, because
chipselect would be deactivated when cs_change==0.

Or is the meaning of cs_change in the last spi_transfer of spi_message
different than in previous spi_transfers in single spi_message?
Like cs_change==0 in last spi_transfer would indicate to deactivate
chipselect and cs_change!=0 in last spi_transfer would provide hinting
that next spi_message will be for the same spi device and chipselect
should be left active?
If that is so, it's quite confusing (and not documented), as the meaning
of cs_change in the last spi_transfer would be opposite than in other
spi_transfers in single spi_message.

The patch here fixes the condition for the case that cs_change meaning
is the first variant described.

 spi_bitbang.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

[-- Attachment #2: spi_bitbang-cs_change-hinting-fix.patch --]
[-- Type: text/plain, Size: 496 bytes --]

diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
index f7f8580..7ff31a3 100644
--- a/drivers/spi/spi_bitbang.c
+++ b/drivers/spi/spi_bitbang.c
@@ -380,7 +380,7 @@ static void bitbang_work(struct work_struct *work)
 		 * cs_change has hinted that the next message will probably
 		 * be for this chip too.
 		 */
-		if (!(status == 0 && cs_change)) {
+		if (!(status == 0 && !cs_change)) {
 			ndelay(nsecs);
 			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
 			ndelay(nsecs);

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 210 bytes --]

_______________________________________________
spi-devel-general mailing list
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: spi_bitbang cs_change hinting fix

[Jan Nikitenko]

Hi David,

Jan Nikitenko wrote:
> Fix condition for deactivating chipselect after processing of
> spi_message - if cs_change==0, chipselect should not be touched and left
> active for next spi_message to follow.
> 
> Signed-off-by: Jan Nikitenko <jan.nikitenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> ---
> 
> I am not sure about the meaning of spi message transfer cs_change attribute.
> 
> I assume that non-zero cs_change indicate that chipselect should be
> toggled between spi_transfers.
> 
> What about cs_change in last spi_transfer of spi_message?
> Should zero cs_change provide hinting that chipselect should be left
> active, because spi_message to follow will be for the same spi device?
> 
> If that is so, the handling in spi_bitbang.c would be incorrect, because
> chipselect would be deactivated when cs_change==0.
> 
> Or is the meaning of cs_change in the last spi_transfer of spi_message
> different than in previous spi_transfers in single spi_message?
> Like cs_change==0 in last spi_transfer would indicate to deactivate
> chipselect and cs_change!=0 in last spi_transfer would provide hinting
> that next spi_message will be for the same spi device and chipselect
> should be left active?
> If that is so, it's quite confusing (and not documented), as the meaning
> of cs_change in the last spi_transfer would be opposite than in other
> spi_transfers in single spi_message.
> 
> The patch here fixes the condition for the case that cs_change meaning
> is the first variant described.
> 
>  spi_bitbang.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
> index f7f8580..7ff31a3 100644
> --- a/drivers/spi/spi_bitbang.c
> +++ b/drivers/spi/spi_bitbang.c
> @@ -380,7 +380,7 @@ static void bitbang_work(struct work_struct *work)
>  		 * cs_change has hinted that the next message will probably
>  		 * be for this chip too.
>  		 */
> -		if (!(status == 0 && cs_change)) {
> +		if (!(status == 0 && !cs_change)) {
>  			ndelay(nsecs);
>  			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
>  			ndelay(nsecs);


Could you, please, comment this issue and clarify the meaning of
cs_change in the last spi_transfer of spi_message?

It seems that I am not the only one confused about that:
http://lkml.org/lkml/2006/2/28/2
SPI: cs_change usage

Thanks and best regards,
Jan

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: spi_bitbang cs_change hinting fix

[David Brownell]

On Tuesday 11 March 2008, Jan Nikitenko wrote:
> I am not sure about the meaning of spi message transfer cs_change attribute.
> 
> I assume that non-zero cs_change indicate that chipselect should be
> toggled between spi_transfers.
> 
> What about cs_change in last spi_transfer of spi_message?
> Should zero cs_change provide hinting that chipselect should be left
> active, because spi_message to follow will be for the same spi device?

>From <linux/spi/spi.h>:

 * (i) If the transfer isn't the last one in the message, this flag is
 * used to make the chipselect briefly go inactive in the middle of the
 * message.  Toggling chipselect in this way may be needed to terminate
 * a chip command, letting a single spi_message perform all of group of
 * chip transactions together.

So you don't need to "assume" that first point.  For the second:

 * (ii) When the transfer is the last one in the message, the chip may
 * stay selected until the next transfer.  On multi-device SPI busses
 * with nothing blocking messages going to other devices, this is just
 * a performance hint; starting a message to another device deselects
 * this one. 

Yes, hinting.  However, in the case where there's some way to
establish exclusive access to that bus, it can do more:

 		But in other cases, this can be used to ensure correctness.
 * Some devices need protocol transactions to be built from a series of
 * spi_message submissions, where the content of one message is determined
 * by the results of previous messages and where the whole transaction
 * ends when the chipselect goes intactive.

(Where "other cases" means the converse of "multi-device SPI busses"
with "nothing blocking messages" ... either single-device busses, or
ones with such blocking.)


> If that is so, the handling in spi_bitbang.c would be incorrect, because
> chipselect would be deactivated when cs_change==0.

It's correct as-is:

    if (!(status == 0 && cs_change)) {
        ... set chipselect to INACTIVE
    }

That is, it always goes inactive unless the message as a whole
succeeded (status == 0) *and* cs_change == 0.


> Or is the meaning of cs_change in the last spi_transfer of spi_message
> different than in previous spi_transfers in single spi_message?

I don't understand your "or" here.  It's explicit that it means
something different there ... end-of-message chipselect semantics
are *always* different from semantics internal to the message.  All
that flag does is -- consistently!! -- toggle the default behavior.


> Like cs_change==0 in last spi_transfer would indicate to deactivate
> chipselect and cs_change!=0 in last spi_transfer would provide hinting
> that next spi_message will be for the same spi device and chipselect
> should be left active?
>
> If that is so, it's quite confusing (and not documented), as the meaning
> of cs_change in the last spi_transfer would be opposite than in other
> spi_transfers in single spi_message.

I'll disagree about lack of documentation.  It's quite clear there
are two cases -- (i) not-the-last, and (ii) the-last.  And that the
normal behavior, cs_change==0, is (i) stay selected, (ii) deselect.
But cs_change==1 toggles that to (i) deselect, (ii) stay selected.

Confusing?  Maybe; but if you remember that what it changes is the
default chipselect *behavior* not its value, then it should be easy
enough to keep straight.  Having more bits to affect a single behavior
would IMO be a lot more confusing than just one bit toggling it.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Register now and save $200. Hurry, offer ends at 11:59 p.m., 
Monday, April 7! Use priority code J8TLD2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone