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

[Vitaly Wool <vwool@ru.mvista.com>]

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