* spi_bitbang cs_change hinting fix
@ 2008-03-11 10:05 Jan Nikitenko
[not found] ` <47D6597F.4040503-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 3+ messages in thread
From: Jan Nikitenko @ 2008-03-11 10:05 UTC (permalink / raw)
To: David Brownell; +Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
[-- 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: spi_bitbang cs_change hinting fix
[not found] ` <47D6597F.4040503-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2008-03-27 8:30 ` Jan Nikitenko
2008-04-07 5:09 ` David Brownell
1 sibling, 0 replies; 3+ messages in thread
From: Jan Nikitenko @ 2008-03-27 8:30 UTC (permalink / raw)
To: David Brownell; +Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: spi_bitbang cs_change hinting fix
[not found] ` <47D6597F.4040503-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2008-03-27 8:30 ` Jan Nikitenko
@ 2008-04-07 5:09 ` David Brownell
1 sibling, 0 replies; 3+ messages in thread
From: David Brownell @ 2008-04-07 5:09 UTC (permalink / raw)
To: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-04-07 5:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-11 10:05 spi_bitbang cs_change hinting fix Jan Nikitenko
[not found] ` <47D6597F.4040503-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2008-03-27 8:30 ` Jan Nikitenko
2008-04-07 5:09 ` David Brownell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).