Messages in this thread | | | From | Kumar Gala <> | Subject | Re: [PATCH] spi: Added spi master driver for Freescale MPC83xx SPI controller | Date | Fri, 7 Apr 2006 11:44:36 -0500 |
| |
On Apr 7, 2006, at 10:54 AM, David Brownell wrote:
> >>> Hmm, it will of course be overridden as soon as needed, but >>> shouldn't >>> that default be "inactive low" clock? SPI mode 0 that is. That >>> stands >>> out mostly because you were interpreting CPOL=0 as inactive high, >>> and >>> that's not my understanding of how that signal works... >> >> I'll change it, not sure what I was thinking but SPI mode 0 being the >> default makes sense. > > The default doesn't really matter, since it will be overridden ... > I was > more concerned about CPOL=0 being misinterpreted ...
Gotcha, I see that I am misinterpreting CPOL/CPHA so I'll fix that up.
>>> ... here you use "__be32" not "u32", and no "__iomem" >>> annotation. So >>> this is inconsistent with the declaration above. Note that if you >>> just made this "&bank->regname" you'd be having the compiler do any >>> offset calculation magic, and the source code will be more obvious. >> >> Yep, I know what you mean. > > Good rule of thumb: run "sparse -Wbitwise" on your drivers, and > have it > tell you about goofed up things. (Assuming the asm-ppc headers are > safe > to run that on!) It's nice having tools tell you about bugs before > you > run into them "live", and GCC only goes so far.
Yeah, forgot to run sparse the first time.
>>>> +static >>>> +int mpc83xx_spi_setup_transfer(struct spi_device *spi, struct >>>> spi_transfer *t) >>>> +{ >>>> + struct mpc83xx_spi *mpc83xx_spi; >>>> + u32 regval; >>>> + u32 len = t->bits_per_word - 1; >>>> + >>>> + if (len == 32) >>>> + len = 0; >>> >>> So the hardware handles 1-33 bit words? It'd be good to filter >>> the spi_setup() path directly then, returning EINVAL for illegal >>> word lengths (and clock speeds). >> >> Uhh, no. The HW supports 4-bit to 32-bit words. However the >> encoding of 32-bit is 0 in the register field, and 8-bit is a value >> of 7, etc.. (bit encodings 1 & 2 are invalid). > > So that test should be "len == 31" too ...
Good catch, I had it testing bits_per_word directly before. Actually, we support 4-bit to 16-bit, and 32-bit transfers.
>> I'm not following you on spi_setup(), but I think you mean to error >> change bits_per_word there and return EINVAL if its not one we >> support. > > Yes, but do it early: provide your own code to implement spi_setup > (), which > makes a range test and then either fails immediately or else > delegates the > rest of the work to spi_bitbang_setup() ... rather than only using > that as > the default. > > Your current code would claim to accept transfers with 64 bit words, > but it wouldn't actually handle them correctly...
Right. However, do you think its really necessary to implement my own spi_setup(), spi_bitbang_setup() is going to call my setup_transfer() which will do the range checking for me (and I'll need range checking in transfer_setup() anyways).
>>> I guess I'm surprised you're not using txrx_buffers() and having >>> that whole thing be IRQ driven, so the per-word cost eliminates >>> the task scheduling. You already paid for IRQ handling ... why >>> not have it store the rx byte into the buffer, and write the tx >>> byte froom the other buffer? That'd be cheaper than what you're >>> doing now ... in both time and code. Only wake up a task at >>> the end of a given spi_transfer(). >> >> I dont follow you at all here. What are you suggesting I do? > > Don't do word-at-a-time I/O with spi_bitbang; you're using IRQs, and > that's oriented towards polling. Don't fill bitbang->txrx_word[]; > don't > use the default spi_bitbang_setup(). > > Instead, provide your own setup(), and provide bitbang->txrx_buffers. > > Then when the generic not-really-bitbang core calls your > txrx_buffers(), > your code would record the "current" spi_transfer buffer pair and > length > then kickstart the I/O by writing the first byte from the TX buffer > (or maybe zero if there is none). Wait on some completion event; > return > when the whole transfer has completed (or stopped after an error). > > Then the rest will be IRQ driven; you'll care only about "rx word > ready" > or whatever. When you get that IRQ, read the word ... and if there's > an RX buffer, store it in the next location (else discard it). > Decrement > the length (by 1, 2, or 4 bytes). If length is nonzero, kickstart the > next step by writing the next word from the TX buffer (or zero). When > length is zero, trigger txrx_buffers() completion. Return from IRQ > handler. > > See for example how bitbang_txrx_8() works; you'd basically be > doing that > as an irq-driven copy, instead of polling txrx_word(). The first > version > of your IRQ handler might be easier if it only handles 4-8 bit words, > leaving 9-16 bits (and 17-32 bits) till later.
Maybe I'm missing where the polling is occurring now, but I felt like I was effectively doing what you are describing.
- 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/
| |