All of lore.kernel.org

* Re: [PATCH] spi: Added spi master driver for Freescale MPC83xx SPI controller
From: Kumar Gala @ 2006-04-07 16:44 UTC (permalink / raw)
  To: David Brownell; +Cc: Greg KH, linux-kernel, spi-devel-general
In-Reply-To: <200604070854.41383.david-b@pacbell.net>

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.

^ permalink raw reply