Re: [PATCH] SPI: add support for PNX/SPI controller

Vitaly Wool
2005-12-27T20:21:47Z

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