On 10/18/2012 04:47 AM, Laxman Dewangan wrote:Tegra20/Tegra30 supports the spi interface through its SLINK
controller. Add spi driver for SLINK controller.
+static inline void tegra_slink_writel(struct tegra_slink_data *tspi,Is that really necessary on every single write, or only for certain
+ unsigned long val, unsigned long reg)
+{
+ writel(val, tspi->base + reg);
+
+ /* Read back register to make sure that register writes completed */
+ if (reg != SLINK_TX_FIFO)
+ readl(tspi->base + SLINK_MAS_DATA);
specific operations? This seems rather heavy-weight.
Status gets updated together. There is no steps of updating status.+ val_write = SLINK_RDY;Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
+ val_write |= (val& SLINK_FIFO_ERROR);
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
+ if (bits_per_word == 8 || bits_per_word == 16) {Spaces required around all operators. Does this pass checkpatch? No:
+ tspi->is_packed = 1;
+ tspi->words_per_32bit = 32/bits_per_word;
total: 1405 errors, 11 warnings, 1418 lines checked
+ bits_per_word = t->bits_per_word ? t->bits_per_word :That logic is repeated a few times. Shouldn't there be a macro to avoid
+ tspi->cur_spi->bits_per_word;
cut/paste. Perhaps even in the SPI core rather than this driver.
+ struct tegra_slink_data *tspi, struct spi_transfer *t)Are there no cases where we can simply DMA straight into the client
buffer? I suppose it would be limited to cache-line-aligned
cache-line-size-aligned buffers which is probably unlikely in general:-(
+static int tegra_slink_unprepare_transfer(struct spi_master *master)Does this driver actually implement any runtime PM ops? I certainly
+{
+ struct tegra_slink_data *tspi = spi_master_get_devdata(master);
+
+ pm_runtime_put_sync(tspi->dev);
+ return 0;
+}
don't see any in struct dev_pm_ops tegra_slink_dev_pm_ops. Related, why
do tegra_slink_clk_{en,dis}able exist; shouldn't those functions be the
implementation of the runtime PM ops? Related, why are
tegra_slink_clk_{en,dis}able called all over the place, rather than
relying on runtime PM API calls, or tegra_slink_{,un}prepare_transfer
having been called?
So that all implies that we wait for the very first interrupt from the
SPI peripheral for a transfer, and then wait for the DMA controller to
complete all DMA (which would probably entail actually waiting for DMA
to drain the RX FIFO in the RX case). Does it make sense to simply
return from the ISR if not all conditions that we will wait on have
occurred, and so avoid blocking this ISR thread? I suppose since this
thread gets blocked rather than spinning, it's not a big deal though.
+ spin_lock_irqsave(&tspi->lock, flags);Do we /have/ to reset the SPI block; can't we just disable it in the
+ if (err) {
+ dev_err(tspi->dev,
+ "DmaXfer: ERROR bit set 0x%x\n", tspi->status_reg);
+ dev_err(tspi->dev,
+ "DmaXfer 0x%08x:0x%08x:0x%08x\n", tspi->command_reg,
+ tspi->command2_reg, tspi->dma_control_reg);
+ tegra_periph_reset_assert(tspi->clk);
+ udelay(2);
+ tegra_periph_reset_deassert(tspi->clk);
control register, clear all status, and re-program it from scratch?
If at all possible, I would like to avoid introducing any new use of
tegra_periph_reset_{,de}assert, since that API has no standard subsystem
equivalent (or if it does, isn't hooked into the standard subsystem
yet), and hence means this driver relies on a header file currently in
arch/arm/mach-tegra/include/mach, and we need to move or delete all such
headers in order to support single zImage.