lkml.org 
[lkml]   [2006]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] spi: Added spi master driver for Freescale MPC83xx SPI controller
Date

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/

\
 
 \ /
  Last update: 2006-04-07 18:47    [W:0.059 / U:0.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site