Re: [PATCH] spi: Added spi master driver for Freescale MPC83xx SPI controller

[David Brownell <david-b@pacbell.net>]

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



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


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

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

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

- Dave