David Brownell wrote:
On Friday 23 December 2005 6:15 am, Vitaly Wool wrote:
(PNX0106 and PNX4008, namely) based on spi_bitbang library
Good! Does need work yet though...
Yup...
# Add new SPI master controllers in alphabetical order above this line
Note how it says "above", yet you've added yours ... below! :(
#
+config SPI_PNX
+ tristate "PNX SPI bus support"
+ depends on ARCH_PNX4008 && SPI_BITBANG
... also added the eeprom so it shows in the list of controller drivers!!
Oh sorry. Stupid me... Not that it matters really since this driver's
not yet mature enough anyway.
+config SPI_EEPROM
+ tristate "SPI EEPROM"
+ depends on SPI
+
+#define lock_device( dev ) /* down( &dev->sem ); */
+#define unlock_device( dev ) /* up( &dev->sem ); */
Not clear why you'd want such stuff ... better to just remove
NOPs like that.
They caused problems on 2.6.10... I might have uncommented that but I
saw no reason. This driver is now is in FYI state :)
+static void
+spi_pnx_chipselect(struct spi_device *spi, int is_on)
+{
+ spipnx_arch_cs(spi, is_on);
+}
I see that "arch" hook is actually just toggling a DMA line.
Did you notice how Stephen's PXA code handled that? The
"controller_data" was a board-specific GPIO toggle function,
as provided in the board_info that implicitly defines the
relevant device.
Plus, activating the chip does more than just toggling some
arch-specific GPIO. It has to set the right SPI mode, of
which CPOL must be set _before_ chipselect goes active.
And it has to set the transfer clock, for chips like yours
which are multiplexing devices using software.
Yep, the current way of doing stuff is a sort of abuse to the driver's
model.
It's just "put #ifdefs out of the C dode" thing. We're gonna rewrite
that using controller_data but in our case that's gonna be a structure
with several functions and data needed. We have more arch-specific stuff
to care about than Stephen's PXA code, so we just didn't bother at the
moment; but anyway you're right.
+static int spi_pnx_xfer(struct spi_device *spidev, struct spi_transfer *t)
+{
...
+ if (spidev->max_speed_hz)
+ spi_pnx_set_clock_rate(spidev->max_speed_hz / 1000, regs);
Just do it the one time, when activating that chip's selection.
Nm really in our case, we have to reset the chip at each direction
change due to hardware bug.
...
+ if (t->tx_buf) {
+ dat = (u8 *)t->tx_buf;
+ regs->con |= SPIPNX_CON_RXTX;
+ regs->ier |= SPIPNX_IER_EOT;
+ regs->con &= ~SPIPNX_CON_SHIFT_OFF;
+ regs->frm = len;
+
+ if (dd->dma_mode && len >= FIFO_CHUNK_SIZE) {
+ void *dmasafe = NULL;
+ if (t->tx_dma)
That's not actually correct, since zero might be a legal DMA address.
Unfortunately we have no DMA_ADDR_INVALID to test against, so this
will need to suffice for now. (And I updated the spi_bitbang code
to ensure that address _is_ zeroed when the message didn't set up
the DMA addresses already, matching the default behavior.)
Hmm, DMA_ADDR_INVALID... (u_long)-1 ?
+ params.dma_buffer = t->tx_dma;
+ else {
+ dmasafe = kmalloc(len, SLAB_KERNEL);
+ if (!dmasafe) {
+ len = 0;
+ goto out;
+ }
+ params.dma_buffer = dma_map_single(dev->parent, dmasafe, len, DMA_TO_DEVICE);
+ memcpy(dmasafe, dat, len);
You do know this is incorrect don't you? Call dma_map_single()
if you need a dma address. The caller already guaranteed that the
input address is dma-safe.
Well it didn't :) See eeprom.c.
It's not the way to go however, but it's nice to have ability to add DMA
safety in some controller drivers working with weird device drivers :/
...
+ }
+
+ if (t->rx_buf) {
...
+ }
That RX bit sure looks wrong to me ... as if, given an spi_transfer
with both TX and RX buffers, it will execute them in sequence (half
duplex) rather than concurrently (full duplex). SPI is full duplex,
and this code should be too. If it can't implement that, then it
should be reporting an error if both rx and tx are requested, rather
than doing the wrong thing.
This particular controller is full duplex but it has some hw problems
working in FD mode.
This is a workaround for FD which looks ok to me.
+ if (rc < 0)
+ goto out;
+
+ data = kzalloc(sizeof *data, GFP_KERNEL);
You don't need that kzalloc, it was already done for you by virtue of
your telling spi_alloc_master() to do so. But then you saved that
memory into "pnx" not "data" (bug! wrong size!). What you should be
doing is nesting your bitbang structure inside the "data" thing.
Agreed.
Likewise, mark this __exit. And I suspect you're using some kernel
older than 2.6.15-rc6 ... ;)
Definitely, it was quickly ported from 2.6.10.
I notice your header file was mostly inline function declarations,
even for basic parts like clocking and DMA. That's best as part
of the driver itself, if it's not following some framework like
the <asm/hardware/clock.h> API on ARM.
Well, these all platform-specific functions are gonna be in
controller_data some day... hopefully soon.
Vitaly
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
