question on spi_bitbang

question on spi_bitbang

[Kumar Gala]

I'm looking at using spi_bitbang for a SPI driver and was trying to  
understand were the right point is to handle MODE switches.

There are 4 function pointers provided for each mode.  My controller  
HW has a mode register which allows setting clock polarity and clock  
phase.  I assume what I want is in my chipselect() function is to set  
my mode register and have the four function pointers set to the same  
function.

Is this the right usage model, or should I set the mode register as  
part of the txrx_word[mode] function?

- kumar

Re: question on spi_bitbang

[David Brownell]

On Friday 31 March 2006 9:31 am, Kumar Gala wrote:
> I'm looking at using spi_bitbang for a SPI driver and was trying to  
> understand were the right point is to handle MODE switches.
> 
> There are 4 function pointers provided for each mode.

That's if you are indeed "bit banging", or your controller is the
type that's basically a wrapper around a shift register:  each
txrx_word() function transfers (or bitbangs) a 1-32 bit word in
the relevant SPI mode (0-3).

There's also a higher level txrx_bufs() routine for buffer-at-a-time
access, better suited to DMA, FIFOs, and half-duplex hardware.


> My controller   
> HW has a mode register which allows setting clock polarity and clock  
> phase.  I assume what I want is in my chipselect() function is to set  
> my mode register and have the four function pointers set to the same  
> function.

I don't know how your particular hardware works, but if you have a
real SPI controller it would probably be more natural to have your
setup() function handle that mode register earlier, out of the main
transfer loop ... unless that mode register is shared among all
chipselects, in which case you'd use the setup_transfer() call for
that, inside the transfer loop.  (That call hasn't yet been merged
into the mainline kernel yet; it's in the MM tree.)

The chipselect() call should only affect the chipselect signal and,
when you're activating a chip, its initial clock polarity.  Though
if you're not using the latest from the MM tree, that's also your
hook for ensuring that the SPI mode is set up right.

- Dave

Re: [spi-devel-general] Re: question on spi_bitbang

[Stephen Street]

On Fri, 2006-03-31 at 10:11 -0800, David Brownell wrote:
> I don't know how your particular hardware works, but if you have a
> real SPI controller it would probably be more natural to have your
> setup() function handle that mode register earlier, out of the main
> transfer loop ... unless that mode register is shared among all
> chipselects, in which case you'd use the setup_transfer() call for
> that, inside the transfer loop.  (That call hasn't yet been merged
> into the mainline kernel yet; it's in the MM tree.)
> 
Is setup_transfer() a change to framework API or just the bit_bang
driver?

> The chipselect() call should only affect the chipselect signal and,
> when you're activating a chip, its initial clock polarity.  Though
> if you're not using the latest from the MM tree, that's also your
> hook for ensuring that the SPI mode is set up right.
> 
Ditto?

-Stephen

Re: [spi-devel-general] Re: question on spi_bitbang

[David Brownell]

On Friday 31 March 2006 10:19 am, Stephen Street wrote:
> On Fri, 2006-03-31 at 10:11 -0800, David Brownell wrote:
> > I don't know how your particular hardware works, but if you have a
> > real SPI controller it would probably be more natural to have your
> > setup() function handle that mode register earlier, out of the main
> > transfer loop ... unless that mode register is shared among all
> > chipselects, in which case you'd use the setup_transfer() call for
> > that, inside the transfer loop.  (That call hasn't yet been merged
> > into the mainline kernel yet; it's in the MM tree.)
> > 
> Is setup_transfer() a change to framework API or just the bit_bang
> driver?

Just bitbang.


> > The chipselect() call should only affect the chipselect signal and,
> > when you're activating a chip, its initial clock polarity.  Though
> > if you're not using the latest from the MM tree, that's also your
> > hook for ensuring that the SPI mode is set up right.
> 
> Ditto?

Ditto.  Though it should also be OK, come to think of it, to keep
doing SPI mode selection in chipselect(); that shouldn't break.

- Dave

Re: question on spi_bitbang

[Kumar Gala]

On Mar 31, 2006, at 12:11 PM, David Brownell wrote:

> On Friday 31 March 2006 9:31 am, Kumar Gala wrote:
>> I'm looking at using spi_bitbang for a SPI driver and was trying to
>> understand were the right point is to handle MODE switches.
>>
>> There are 4 function pointers provided for each mode.
>
> That's if you are indeed "bit banging", or your controller is the
> type that's basically a wrapper around a shift register:  each
> txrx_word() function transfers (or bitbangs) a 1-32 bit word in
> the relevant SPI mode (0-3).
>
> There's also a higher level txrx_bufs() routine for buffer-at-a-time
> access, better suited to DMA, FIFOs, and half-duplex hardware.

My controller is just a shift register that I can set the  
characteristics of (bit length for example, reverse data).

>> My controller
>> HW has a mode register which allows setting clock polarity and clock
>> phase.  I assume what I want is in my chipselect() function is to set
>> my mode register and have the four function pointers set to the same
>> function.
>
> I don't know how your particular hardware works, but if you have a
> real SPI controller it would probably be more natural to have your
> setup() function handle that mode register earlier, out of the main
> transfer loop ... unless that mode register is shared among all
> chipselects, in which case you'd use the setup_transfer() call for
> that, inside the transfer loop.  (That call hasn't yet been merged
> into the mainline kernel yet; it's in the MM tree.)

The mode register is shared between chipselects so I'll go pull the  
patches from -mm for setup_transfer().  That sounds like the right  
place for setting my mode register.

> The chipselect() call should only affect the chipselect signal and,
> when you're activating a chip, its initial clock polarity.  Though
> if you're not using the latest from the MM tree, that's also your
> hook for ensuring that the SPI mode is set up right.

Why deal with just clock polarity and not clock phase as well in  
chipselect()?

It sounds like with the new patch, I'll end up setting txrx_word[] to  
the same function for all modes.

- kumar

Re: question on spi_bitbang

[Kumar Gala]

On Mar 31, 2006, at 1:07 PM, Kumar Gala wrote:

>
> On Mar 31, 2006, at 12:11 PM, David Brownell wrote:
>
>> On Friday 31 March 2006 9:31 am, Kumar Gala wrote:
>>> I'm looking at using spi_bitbang for a SPI driver and was trying to
>>> understand were the right point is to handle MODE switches.
>>>
>>> There are 4 function pointers provided for each mode.
>>
>> That's if you are indeed "bit banging", or your controller is the
>> type that's basically a wrapper around a shift register:  each
>> txrx_word() function transfers (or bitbangs) a 1-32 bit word in
>> the relevant SPI mode (0-3).
>>
>> There's also a higher level txrx_bufs() routine for buffer-at-a-time
>> access, better suited to DMA, FIFOs, and half-duplex hardware.
>
> My controller is just a shift register that I can set the  
> characteristics of (bit length for example, reverse data).
>
>>> My controller
>>> HW has a mode register which allows setting clock polarity and clock
>>> phase.  I assume what I want is in my chipselect() function is to  
>>> set
>>> my mode register and have the four function pointers set to the same
>>> function.
>>
>> I don't know how your particular hardware works, but if you have a
>> real SPI controller it would probably be more natural to have your
>> setup() function handle that mode register earlier, out of the main
>> transfer loop ... unless that mode register is shared among all
>> chipselects, in which case you'd use the setup_transfer() call for
>> that, inside the transfer loop.  (That call hasn't yet been merged
>> into the mainline kernel yet; it's in the MM tree.)
>
> The mode register is shared between chipselects so I'll go pull the  
> patches from -mm for setup_transfer().  That sounds like the right  
> place for setting my mode register.

Now that I look at setup_transfer() more I dont think its what I  
want.  It's only going to get called if the transfer is different  
from the spi_device() settings.  I would seem I would want to  
configure all of my settings in chipselect().

>> The chipselect() call should only affect the chipselect signal and,
>> when you're activating a chip, its initial clock polarity.  Though
>> if you're not using the latest from the MM tree, that's also your
>> hook for ensuring that the SPI mode is set up right.
>
> Why deal with just clock polarity and not clock phase as well in  
> chipselect()?
>
> It sounds like with the new patch, I'll end up setting txrx_word[]  
> to the same function for all modes.
>
> - kumar
> -
> To unsubscribe from this list: send the line "unsubscribe linux- 
> kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [spi-devel-general] Re: question on spi_bitbang

[David Brownell]

On Friday 31 March 2006 11:07 am, Kumar Gala wrote:
> My controller is just a shift register that I can set the  
> characteristics of (bit length for example, reverse data).

I've got a patch somewhere to enable LSB-first transfers in the API,
though without an implementation, if you're interested.  I'll post it
as an RFC at some point.


> > The chipselect() call should only affect the chipselect signal and,
> > when you're activating a chip, its initial clock polarity.  Though
> > if you're not using the latest from the MM tree, that's also your
> > hook for ensuring that the SPI mode is set up right.
> 
> Why deal with just clock polarity and not clock phase as well in  
> chipselect()?

You could, but the point is that you _must_ set the initial polarity
before setting the chipselect.  Most SPI devices support modes 0 and 3,
and make the choice based on the clock polarity when chipselect goes
active.  Changing polarity later would start a transfer.  :)


> It sounds like with the new patch, I'll end up setting txrx_word[] to  
> the same function for all modes.

Yes, it does sound like that.  If that works for you, I'd like to see
that go into 2.6.17 kernels.

- Dave

Re: [spi-devel-general] Re: question on spi_bitbang

[Kumar Gala]

On Mar 31, 2006, at 1:32 PM, David Brownell wrote:

> On Friday 31 March 2006 11:07 am, Kumar Gala wrote:
>> My controller is just a shift register that I can set the
>> characteristics of (bit length for example, reverse data).
>
> I've got a patch somewhere to enable LSB-first transfers in the API,
> though without an implementation, if you're interested.  I'll post it
> as an RFC at some point.

The controllers capable so if/when we have something at a higher  
level I can look at adding support for it.

>>> The chipselect() call should only affect the chipselect signal and,
>>> when you're activating a chip, its initial clock polarity.  Though
>>> if you're not using the latest from the MM tree, that's also your
>>> hook for ensuring that the SPI mode is set up right.
>>
>> Why deal with just clock polarity and not clock phase as well in
>> chipselect()?
>
> You could, but the point is that you _must_ set the initial polarity
> before setting the chipselect.  Most SPI devices support modes 0  
> and 3,
> and make the choice based on the clock polarity when chipselect goes
> active.  Changing polarity later would start a transfer.  :)

Makes sense about needing to set polarity in the chipselect() before  
the actual chip select.  I just now completely confused on when I  
need to things.

My confusion is about the order of which various things occur.  setup 
(), chipselect() and transfer() vs what's happening in bitbang_work 
().  I don't see how we handle the fact that two different devices  
may require setup() to be called when we switch between them.

>> It sounds like with the new patch, I'll end up setting txrx_word[] to
>> the same function for all modes.
>
> Yes, it does sound like that.  If that works for you, I'd like to see
> that go into 2.6.17 kernels.

I'm not sure I understand what you'd like to see go into 2.6.17.

- kumar

Re: [spi-devel-general] Re: question on spi_bitbang

[David Brownell]

On Friday 31 March 2006 12:00 pm, Kumar Gala wrote:

> My confusion is about the order of which various things occur.  setup 
> (), chipselect() and transfer() vs what's happening in bitbang_work 
> ().  I don't see how we handle the fact that two different devices  
> may require setup() to be called when we switch between them.

In your case, it sounds like setup() will just store data, and you'll
want a different method to actually grab that data and use it to stuff
your controller registers before actually transferring data.  The
transfer() method just queues transfers (and maybe kicks them off).

Remember that setup() is generally a one-time thing.  Fancier hardware
will use it to store clock, mode, wordsize, and other parameters into
a hardware register so that starting a transfer is very quick.  In your
case, there's no such register, so starting transfers is slower.


One thing to keep in mind is that while I believe the spi_bitbang code
ought to support controllers like the one you're working with, I don't
know that anyone has done that yet.  So patches might be necessary.

At the top of <linux/spi/spi_bitbang.h> are verbal sketches of three
types of "bitbang" drivers.  Implementations of two of them now seem
to be working (word-at-a-time with GPIO bitbanging, "spi_butterfly"
being one of a few examples; and transfer-at-a-time, "omap_uwire").
Your hardware would be of the third type.



> >> It sounds like with the new patch, I'll end up setting txrx_word[] to
> >> the same function for all modes.
> >
> > Yes, it does sound like that.  If that works for you, I'd like to see
> > that go into 2.6.17 kernels.
> 
> I'm not sure I understand what you'd like to see go into 2.6.17.

The patch allowing the per-transfer overrides, which you were going to
grab from the MM tree.

That would support SPI drivers for things like bitmapped displays, some
of which need oddball things like 9-bit commands followed by 8-bit data.

- Dave

Re: [spi-devel-general] Re: question on spi_bitbang

[Kumar Gala]

On Mar 31, 2006, at 2:36 PM, David Brownell wrote:

> On Friday 31 March 2006 12:00 pm, Kumar Gala wrote:
>
>> My confusion is about the order of which various things occur.  setup
>> (), chipselect() and transfer() vs what's happening in bitbang_work
>> ().  I don't see how we handle the fact that two different devices
>> may require setup() to be called when we switch between them.
>
> In your case, it sounds like setup() will just store data, and you'll
> want a different method to actually grab that data and use it to stuff
> your controller registers before actually transferring data.  The
> transfer() method just queues transfers (and maybe kicks them off).
>
> Remember that setup() is generally a one-time thing.  Fancier hardware
> will use it to store clock, mode, wordsize, and other parameters into
> a hardware register so that starting a transfer is very quick.  In  
> your
> case, there's no such register, so starting transfers is slower.
>
> One thing to keep in mind is that while I believe the spi_bitbang code
> ought to support controllers like the one you're working with, I don't
> know that anyone has done that yet.  So patches might be necessary.

What I'm looking at is the following:

* use spi_bitbang_setup() as is
* have my chipselect do:
	if (BITBANG_CS_INACTIVE)
		deassert GPIO pin for CS
	else
		set HW mode register (polarity, phase, bit length)
		assert GPIO pin for CS
* setup_transfer()
	* set HW mode register (bit length)
	* call bitbang_setup_transfer()

> At the top of <linux/spi/spi_bitbang.h> are verbal sketches of three
> types of "bitbang" drivers.  Implementations of two of them now seem
> to be working (word-at-a-time with GPIO bitbanging, "spi_butterfly"
> being one of a few examples; and transfer-at-a-time, "omap_uwire").
> Your hardware would be of the third type.
>
>
>
>>>> It sounds like with the new patch, I'll end up setting txrx_word 
>>>> [] to
>>>> the same function for all modes.
>>>
>>> Yes, it does sound like that.  If that works for you, I'd like to  
>>> see
>>> that go into 2.6.17 kernels.
>>
>> I'm not sure I understand what you'd like to see go into 2.6.17.
>
> The patch allowing the per-transfer overrides, which you were going to
> grab from the MM tree.
>
> That would support SPI drivers for things like bitmapped displays,  
> some
> of which need oddball things like 9-bit commands followed by 8-bit  
> data.

Right, I dont think I need the support of setup_transfer() for my  
devices aren't
changing their settings mid message.  However, I do think some  
changes are needed to the patch

- kumar

Re: [spi-devel-general] Re: question on spi_bitbang

[David Brownell]

On Friday 31 March 2006 12:52 pm, Kumar Gala wrote:

> What I'm looking at is the following:
> 
> * use spi_bitbang_setup() as is
> * have my chipselect do:
> 	if (BITBANG_CS_INACTIVE)
> 		deassert GPIO pin for CS
> 	else
> 		set HW mode register (polarity, phase, bit length)
> 		assert GPIO pin for CS
> * setup_transfer()
> 	* set HW mode register (bit length)
> 	* call bitbang_setup_transfer()

And export bitbang_setup_transfer()?  I guess that makes sense,
but you should probably rename it then to match the convention for
the other exported symbols.

Once that's all working, please submit the relevant patch.

- Dave

Re: [spi-devel-general] Re: question on spi_bitbang

[Kumar Gala]

On Mar 31, 2006, at 3:15 PM, David Brownell wrote:

> On Friday 31 March 2006 12:52 pm, Kumar Gala wrote:
>
>> What I'm looking at is the following:
>>
>> * use spi_bitbang_setup() as is
>> * have my chipselect do:
>> 	if (BITBANG_CS_INACTIVE)
>> 		deassert GPIO pin for CS
>> 	else
>> 		set HW mode register (polarity, phase, bit length)
>> 		assert GPIO pin for CS
>> * setup_transfer()
>> 	* set HW mode register (bit length)
>> 	* call bitbang_setup_transfer()
>
> And export bitbang_setup_transfer()?  I guess that makes sense,
> but you should probably rename it then to match the convention for
> the other exported symbols.
>
> Once that's all working, please submit the relevant patch.

Will do.

So I give a new question.  Any issue with adding a rx & tx completion  
to spi_bitbang?  In my HW I get an interrupt when the transmitter is  
done transmitting and one when the receiver is done receiving.  I  
need some way to synchronize and wait for both events to occur before  
continuing on in txrx_word().

- kumar

Re: [spi-devel-general] Re: question on spi_bitbang

[David Brownell]

On Friday 31 March 2006 2:11 pm, Kumar Gala wrote:

> So I give a new question.  Any issue with adding a rx & tx completion  
> to spi_bitbang?

What do you mean?

> In my HW I get an interrupt when the transmitter is   
> done transmitting and one when the receiver is done receiving.  I  
> need some way to synchronize and wait for both events to occur before  
> continuing on in txrx_word().

You can't return from txrx_word() before the RX event, since the
return value is the word that was shifted in.  So if you use IRQs
to synchronize there (rather than polling a status register), all
that would be internal to your code.

- Dave

Re: [spi-devel-general] Re: question on spi_bitbang

[Kumar Gala]

On Mar 31, 2006, at 4:20 PM, David Brownell wrote:

> On Friday 31 March 2006 2:11 pm, Kumar Gala wrote:
>
>> So I give a new question.  Any issue with adding a rx & tx completion
>> to spi_bitbang?
>
> What do you mean?
>
>> In my HW I get an interrupt when the transmitter is
>> done transmitting and one when the receiver is done receiving.  I
>> need some way to synchronize and wait for both events to occur before
>> continuing on in txrx_word().
>
> You can't return from txrx_word() before the RX event, since the
> return value is the word that was shifted in.  So if you use IRQs
> to synchronize there (rather than polling a status register), all
> that would be internal to your code.

Your right, I just put this in my struct that wraps spi_bitbang.

It's too bad we dont have a better solution for spi_bitbang having to  
be first.

I've got a working driver w/o using the setup_transfer() mods, I'll  
look at fixing that up next.

- kumar