spi-bitbang inverted logic?

spi-bitbang inverted logic?

[Guennadi Liakhovetski]

Hi all

I stumbled across this code in spi-bitbang.c:

		list_for_each_entry (t, &m->transfers, transfer_list) {
			...
			cs_change = t->cs_change;
			...
			if (!cs_change)
				continue;
			...
			/* sometimes a short mid-message deselect of the chip
			 * may be needed to terminate a mode or command
			 */
			ndelay(nsecs);
			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
			ndelay(nsecs);
		}
		...

		/* normally deactivate chipselect ... unless no error and
		 * cs_change has hinted that the next message will probably
		 * be for this chip too.
		 */
		if (!(status == 0 && cs_change)) {
			ndelay(nsecs);
			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
			ndelay(nsecs);
		}

So, IIUC, on the first occurrance cs_change is interpreted as "true == 
have to disable CD," whereas the second one does the opposite. Shouldn't 
the latter one be inverted?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here 
http://p.sf.net/sfu/sfd2d-msazure

Re: spi-bitbang inverted logic?

[Grant Likely]

On Thu, 15 Mar 2012 12:09:42 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> Hi all
> 
> I stumbled across this code in spi-bitbang.c:
> 
> 		list_for_each_entry (t, &m->transfers, transfer_list) {
> 			...
> 			cs_change = t->cs_change;
> 			...
> 			if (!cs_change)
> 				continue;
> 			...
> 			/* sometimes a short mid-message deselect of the chip
> 			 * may be needed to terminate a mode or command
> 			 */
> 			ndelay(nsecs);
> 			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> 			ndelay(nsecs);
> 		}
> 		...
> 
> 		/* normally deactivate chipselect ... unless no error and
> 		 * cs_change has hinted that the next message will probably
> 		 * be for this chip too.
> 		 */
> 		if (!(status == 0 && cs_change)) {
> 			ndelay(nsecs);
> 			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> 			ndelay(nsecs);
> 		}
> 
> So, IIUC, on the first occurrance cs_change is interpreted as "true == 
> have to disable CD," whereas the second one does the opposite. Shouldn't 
> the latter one be inverted?

Actually, I suspect that cs_change is being abused here to allow multiple
messages to operate over a single cs_change assertion.  It does look dodgy,
but I think you'll need to audit the users of cs_change to ensure that every
'normal' message has cs_change asserted for the last transfer in a message.

g.


------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here 
http://p.sf.net/sfu/sfd2d-msazure

Re: spi-bitbang inverted logic?

[Jassi Brar]

On Thu, Mar 15, 2012 at 4:39 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi all
>
> I stumbled across this code in spi-bitbang.c:
>
>                list_for_each_entry (t, &m->transfers, transfer_list) {
>                        ...
>                        cs_change = t->cs_change;
>                        ...
>                        if (!cs_change)
>                                continue;
>                        ...
>                        /* sometimes a short mid-message deselect of the chip
>                         * may be needed to terminate a mode or command
>                         */
>                        ndelay(nsecs);
>                        bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
>                        ndelay(nsecs);
>                }
>                ...
>
>                /* normally deactivate chipselect ... unless no error and
>                 * cs_change has hinted that the next message will probably
>                 * be for this chip too.
>                 */
>                if (!(status == 0 && cs_change)) {
>                        ndelay(nsecs);
>                        bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
>                        ndelay(nsecs);
>                }
>
> So, IIUC, on the first occurrance cs_change is interpreted as "true ==
> have to disable CD," whereas the second one does the opposite. Shouldn't
> the latter one be inverted?
>
As documented in include/linux/spi/spi.h the behavior of cs_change changes
depending upon whether the transfer is last or not in a message.
I remember using the signal pattern output by the bitbang driver as
the reference,
while developing one for s3c64xx and I didn't see any behavior against what is
documented.

-jassi
------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here 
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: spi-bitbang inverted logic?

[Guennadi Liakhovetski]

On Fri, 16 Mar 2012, Jassi Brar wrote:

> On Thu, Mar 15, 2012 at 4:39 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Hi all
> >
> > I stumbled across this code in spi-bitbang.c:
> >
> >                list_for_each_entry (t, &m->transfers, transfer_list) {
> >                        ...
> >                        cs_change = t->cs_change;
> >                        ...
> >                        if (!cs_change)
> >                                continue;
> >                        ...
> >                        /* sometimes a short mid-message deselect of the chip
> >                         * may be needed to terminate a mode or command
> >                         */
> >                        ndelay(nsecs);
> >                        bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> >                        ndelay(nsecs);
> >                }
> >                ...
> >
> >                /* normally deactivate chipselect ... unless no error and
> >                 * cs_change has hinted that the next message will probably
> >                 * be for this chip too.
> >                 */
> >                if (!(status == 0 && cs_change)) {
> >                        ndelay(nsecs);
> >                        bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> >                        ndelay(nsecs);
> >                }
> >
> > So, IIUC, on the first occurrance cs_change is interpreted as "true ==
> > have to disable CD," whereas the second one does the opposite. Shouldn't
> > the latter one be inverted?
> >
> As documented in include/linux/spi/spi.h the behavior of cs_change changes
> depending upon whether the transfer is last or not in a message.

Right, I looked at Documentation/spi/spi-summary, which is not providing a 
very detailed explanation.

> I remember using the signal pattern output by the bitbang driver as
> the reference,
> while developing one for s3c64xx and I didn't see any behavior against what is
> documented.

Ok, I actually suspected that, just didn't compare to other drivers. And 
the comment in mmc_spi.c:

 * - We tell the controller to keep the chipselect active from the
 *   beginning of an mmc_host_ops.request until the end.  So beware
 *   of SPI controller drivers that mis-handle the cs_change flag!

is also not extremely optimistic... But anyway, my yesterday's 
patch-series

http://sourceforge.net/mailarchive/forum.php?thread_name=Pine.LNX.4.64.1203152230190.2988%40axis700.grange&forum_name=spi-devel-general

preserves that behaviour. But what I still don't understand:

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

So, is this just an optional "hint" or something, that must be obeyed to 
to guarantee functionality? At least, my above proposal offers a method to 
distinguish between those two cases.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here 
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: spi-bitbang inverted logic?

[Jassi Brar]

On Fri, Mar 16, 2012 at 2:04 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
>
>  * - We tell the controller to keep the chipselect active from the
>  *   beginning of an mmc_host_ops.request until the end.  So beware
>  *   of SPI controller drivers that mis-handle the cs_change flag!
>
> is also not extremely optimistic...
>
I had actually tested MMC over bitbanging SPI to compare working of
the spi-s3c64xx.c

>But anyway, my yesterday's patch-series
>
> http://sourceforge.net/mailarchive/forum.php?thread_name=Pine.LNX.4.64.1203152230190.2988%40axis700.grange&forum_name=spi-devel-general
>
> preserves that behaviour. But what I still don't understand:
>
>  * ...  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.  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.
>
> So, is this just an optional "hint" or something, that must be obeyed to
> to guarantee functionality? At least, my above proposal offers a method to
> distinguish between those two cases.
>
Message to a different device unconditionally de-selects the current
device(obviously).
Other than mmc_spi.c, I never came across a protocol driver using it.
But as quoted above, some devices actually count upon it.
So implementing it is always a safe bet.

-jassi

------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here 
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general