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

<?xml version="1.0" encoding="UTF-8" standalone="yes"?> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"><html xmlns="http://www.w3.org/1999/xhtml"><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /><title>LKML: Vitaly Wool: Re: [PATCH] SPI: add support for PNX/SPI controller</title><link href="/css/message.css" rel="stylesheet" type="text/css" /><link href="/css/wrap.css" rel="alternate stylesheet" type="text/css" title="wrap" /><link href="/css/nowrap.css" rel="stylesheet" type="text/css" title="nowrap" /><link href="/favicon.ico" rel="shortcut icon" /><script src="/js/simple-calendar.js" type="text/javascript"></script><script src="/js/styleswitcher.js" type="text/javascript"></script><link rel="alternate" type="application/rss+xml" title="lkml.org : last 100 messages" href="/rss.php" /><link rel="alternate" type="application/rss+xml" title="lkml.org : last messages by Vitaly Wool" href="/groupie.php?aid=29396" /><script> var _paq = window._paq = window._paq || []; /* tracker methods like "setCustomDimension" should be called before "trackPageView" */ _paq.push(["setDoNotTrack", true]); _paq.push(["disableCookies"]); _paq.push(['trackPageView']); _paq.push(['enableLinkTracking']); (function() { var u="//m.lkml.org/"; _paq.push(['setTrackerUrl', u+'matomo.php']); _paq.push(['setSiteId', '1']); var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0]; g.async=true; g.src=u+'matomo.js'; s.parentNode.insertBefore(g,s); })(); </script></head><body onload="es.jasper.simpleCalendar.init();" itemscope="itemscope" itemtype="http://schema.org/BlogPosting"><table border="0" cellpadding="0" cellspacing="0"><tr><td width="180" align="center"><a href="/"><img style="border:0;width:135px;height:32px" src="/images/toprowlk.gif" alt="lkml.org" /></a></td><td width="32">聽</td><td class="nb"><div><a class="nb" href="/lkml"> [lkml]</a> 聽 <a class="nb" href="/lkml/2005"> [2005]</a> 聽 <a class="nb" href="/lkml/2005/12"> [Dec]</a> 聽 <a class="nb" href="/lkml/2005/12/27"> [27]</a> 聽 <a class="nb" href="/lkml/last100"> [last100]</a> 聽 <a href="/rss.php"><img src="/images/rss-or.gif" border="0" alt="RSS Feed" /></a></div><div>Views: <a href="#" class="nowrap" onclick="setActiveStyleSheet('wrap');return false;">[wrap]</a><a href="#" class="wrap" onclick="setActiveStyleSheet('nowrap');return false;">[no wrap]</a> 聽 <a class="nb" href="/lkml/mheaders/2005/12/27/122" onclick="this.href='/lkml/headers'+'/2005/12/27/122';">[headers]</a>聽 <a href="/lkml/bounce/2005/12/27/122">[forward]</a>聽 </div></td><td width="32">聽</td></tr><tr><td valign="top"><div class="es-jasper-simpleCalendar" baseurl="/lkml/"></div><div class="threadlist">Messages in this thread</div><ul class="threadlist"><li class="root"><a href="/lkml/2005/12/23/57">First message in thread</a></li><li><a href="/lkml/2005/12/23/57">Vitaly Wool</a><ul><li><a href="/lkml/2005/12/27/106">David Brownell</a><ul><li class="origin"><a href="">Vitaly Wool</a></li></ul></li></ul></li></ul></td><td width="32" rowspan="2" class="c" valign="top"><img src="/images/icornerl.gif" width="32" height="32" alt="/" /></td><td class="c" rowspan="2" valign="top" style="padding-top: 1em"><table><tr><td><table><tr><td class="lp">Date</td><td class="rp" itemprop="datePublished">Tue, 27 Dec 2005 23:21:11 +0300</td></tr><tr><td class="lp">From</td><td class="rp" itemprop="author">Vitaly Wool <></td></tr><tr><td class="lp">Subject</td><td class="rp" itemprop="name">Re: [PATCH] SPI: add support for PNX/SPI controller</td></tr></table></td><td></td></tr></table><pre itemprop="articleBody">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 majordomo@vger.kernel.org More majordomo info at <a href="http://vger.kernel.org/majordomo-info.html">http://vger.kernel.org/majordomo-info.html</a> Please read the FAQ at <a href="http://www.tux.org/lkml/">http://www.tux.org/lkml/</a> </pre></td><td width="32" rowspan="2" class="c" valign="top"><img src="/images/icornerr.gif" width="32" height="32" alt="\" /></td></tr><tr><td align="right" valign="bottom"> 聽 </td></tr><tr><td align="right" valign="bottom">聽</td><td class="c" valign="bottom" style="padding-bottom: 0px"><img src="/images/bcornerl.gif" width="32" height="32" alt="\" /></td><td class="c">聽</td><td class="c" valign="bottom" style="padding-bottom: 0px"><img src="/images/bcornerr.gif" width="32" height="32" alt="/" /></td></tr><tr><td align="right" valign="top" colspan="2"> 聽 </td><td class="lm">Last update: 2005-12-27 21:25 聽聽 [from the cache] 漏2003-2020 <a href="http://blog.jasper.es/">Jasper Spaans</a>|hosted at <a href="https://www.digitalocean.com/?refcode=9a8e99d24cf9">Digital Ocean</a> and my Meterkast|<a href="http://blog.jasper.es/categories.html#lkml-ref">Read the blog</a></td><td>聽</td></tr></table><script language="javascript" src="/js/styleswitcher.js" type="text/javascript"></script></body></html>

CINXE.COM

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