spi: add spi_tegra driver
Subsystems (2)
PATCH 1/1 spi: add spi_tegra driver
From: Erik Gilling <[email protected]>
---
This patch depends on the Tegra APB DMA driver:
[PATCH] [ARM] tegra: Add APB DMA support
--- drivers/spi/Kconfig | 6 + drivers/spi/Makefile | 1 + drivers/spi/spi_tegra.c | 651 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 658 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/spi_tegra.c
drivers/spi/Kconfig +6 -0
@@ -298,6 +298,12 @@ config SPI_STMP3XXX help SPI driver for Freescale STMP37xx/378x SoC SSP interface +config SPI_TEGRA + tristate "Nvidia Tegra SPI controller" + depends on ARCH_TEGRA + help + SPI driver for NVidia Tegra SoCs + config SPI_TXX9 tristate "Toshiba TXx9 SPI controller" depends on GENERIC_GPIO && CPU_TX49XX
drivers/spi/Makefile +1 -0
@@ -39,6 +39,7 @@ obj-$(CONFIG_SPI_PPC4xx) += spi_ppc4xx.o obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx_hw.o obj-$(CONFIG_SPI_S3C64XX) += spi_s3c64xx.o +obj-$(CONFIG_SPI_TEGRA) += spi_tegra.o obj-$(CONFIG_SPI_TXX9) += spi_txx9.o obj-$(CONFIG_SPI_XILINX) += xilinx_spi.o obj-$(CONFIG_SPI_XILINX_OF) += xilinx_spi_of.o
drivers/spi/spi_tegra.c new file +651 -0
651 lines not on this page · raw message
Signature · 1 line
1.7.1
PATCH 1/1 spi: add spi_tegra driver
Quoted 19 lines · Colin Cross
From: Erik Gilling <[email protected]>
CC spi-devel-general at lists.sourceforge.netSigned-off-by Erik Gilling <[email protected]>---
This patch depends on the Tegra APB DMA driver:
[PATCH] [ARM] tegra: Add APB DMA support--- drivers/spi/Kconfig | 6 + drivers/spi/Makefile | 1 + drivers/spi/spi_tegra.c | 651 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 658 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/spi_tegra.cdrivers/spi/Kconfig +0
-0
drivers/spi/Kconfig config SPI_STMP3XXX · line 306
Quoted 9 lines
drivers/spi/Kconfig +6
-0@@ -298,6 +298,12 @@ config SPI_STMP3XXX help SPI driver for Freescale STMP37xx/378x SoC SSP interface +config SPI_TEGRA + tristate "Nvidia Tegra SPI controller" + depends on ARCH_TEGRA + help + SPI driver for NVidia Tegra SoCs +
[...]
Since you say this driver depends on the APB DMA driver, should this not be expressed as a Kconfig dependency? Something like:
select TEGRA_SYSTEM_DMA
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100730/2c9c6a2d/attachment-0001.sig>
PATCH 1/1 spi: add spi_tegra driver
Quoted 5 lines · Thierry Reding
Since you say this driver depends on the APB DMA driver, should this not be expressed as a
Kconfigdependency? Something like:? ? ? ?select
TEGRA_SYSTEM_DMA
yep, will do.
PATCH 1/1 spi: add spi_tegra driver
In reply to Colin CrossHi Colin,
Looks like a pretty nice driver. Some comments/questions below.
Cheers,
g.
Quoted 4 lines · Colin Cross
From: Erik Gilling <[email protected]>
CC spi-devel-general at lists.sourceforge.netSigned-off-by Erik Gilling <[email protected]>
You should also add your s-o-b line when passing on patches.
Quoted 3 lines
---
This patch depends on the Tegra APB DMA driver:
[PATCH] [ARM] tegra: Add APB DMA support
What is the status of the APB DMA driver? Is it going to be merged in 2.6.36?
Quoted 42 lines
---
?drivers/spi/Kconfig ? ? | ? ?6 + ?drivers/spi/Makefile ? ?| ? ?1 + ?drivers/spi/spi_tegra.c | ?651 +++++++++++++++++++++++++++++++++++++++++++++++?3 files changed, 658 insertions(+), 0 deletions(-) ?create mode 100644
drivers/spi/spi_tegra.cdrivers/spi/Kconfig +0
-0@@ -298,6 +298,12 @@ config SPI_STMP3XXX? ? ? ?help
? ? ? ? ?SPI driver for Freescale STMP37xx/378x SoC SSP interfacedrivers/spi/Kconfig +6
-0+config SPI_TEGRA + ? ? ? tristate "Nvidia Tegra SPI controller" + ? ? ? depends on ARCH_TEGRA + ? ? ? help + ? ? ? ? SPI driver for NVidia Tegra SoCs +?config
SPI_TXX9
? ? ? ?tristate "Toshiba TXx9 SPI controller"
? ? ? ?depends onGENERIC_GPIO&&CPU_TX49XXdrivers/spi/Makefile +0
-0@@ -39,6 +39,7 @@ obj-$(CONFIG_SPI_PPC4xx) ? ? ? ? ? ? ?+= spi_ppc4xx.o?obj-$(CONFIG_SPI_S3C24XX_GPIO) ? ? ? ? += spi_s3c24xx_gpio.o ?obj-$(CONFIG_SPI_S3C24XX) ? ? ? ? ? ? ?+= spi_s3c24xx_hw.o ?obj-$(CONFIG_SPI_S3C64XX) ? ? ? ? ? ? ?+= spi_s3c64xx.o +obj-$(CONFIG_SPI_TEGRA) ? ? ? ? ? ? ? ? ? ? ? ?+= spi_tegra.o ?obj-$(CONFIG_SPI_TXX9) ? ? ? ? ? ? ? ? += spi_txx9.o ?obj-$(CONFIG_SPI_XILINX) ? ? ? ? ? ? ? += xilinx_spi.o ?obj-$(CONFIG_SPI_XILINX_OF) ? ? ? ? ? ?+= xilinx_spi_of.o
drivers/spi/spi_tegra.c new file +0
-0
drivers/spi/spi_tegra.c line 2
Quoted 2 lines
drivers/spi/spi_tegra.c new file +2
-0@@ -0,0 +1,651 @@ +/* +* drivers/spi/spi_tegra.c
I find it is more useful, and it is my preference, to have a one-line description of what the file actually is here. Showing the filename is just redundant and doesn't tell a reader anything new.
Quoted 8 lines
drivers/spi/spi_tegra.c new file +8
-0+* +* Copyright (C) 2010 Google, Inc. +* +* Author: +* ? ? Erik Gilling <[email protected]> +* +* This software is licensed under the terms of the GNU General Public +* License version 2, as published by the Free Software Foundation, and
drivers/spi/spi_tegra.c line 20
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+* may be copied, distributed, and modified under those terms. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the +* GNU General Public License for more details. +* +*/ + +#undef DEBUG
trim
Quoted 6 lines
drivers/spi/spi_tegra.c new file +6
-0+#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/dma-mapping.h>
drivers/spi/spi_tegra.c
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+#include <linux/dmapool.h> +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/delay.h> + +#include <linux/spi/spi.h> + +#include <mach/dma.h> + +#define DRIVER_NAME ? ? ? ? ? ?"spi_tegra"
Used only twice in 2 side-by-side locations. I'd trim, or at least move to where it is actually used.
drivers/spi/spi_tegra.c line 38
Quoted 2 lines
drivers/spi/spi_tegra.c new file +2
-0+ +#define BUSY_TIMEOUT ? ? ? ? ? 1000
Used exactly once. May be better defined and documented at the usage site
Quoted 99 lines
drivers/spi/spi_tegra.c new file +99
-0+ +#define SLINK_COMMAND ? ? ? ? ?0x000 +#define ? BIT_LENGTH(x) ? ? ? ? ? ? ? ? ? ? ? ?(((x) & 0x1f) << 0) +#define ? WORD_SIZE(x) ? ? ? ? ? ? ? ? (((x) & 0x1f) << 5) +#define ? BOTH_EN ? ? ? ? ? ? ? ? ? ? ?(1 << 10) +#define ? CS_SW ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 11) +#define ? CS_VALUE ? ? ? ? ? ? ? ? ? ? (1 << 12) +#define ? CS_POLARITY ? ? ? ? ? ? ? ? ?(1 << 13) +#define ? IDLE_SDA_DRIVE_LOW ? ? ? ? ? (0 << 16) +#define ? IDLE_SDA_DRIVE_HIGH ? ? ? ? ?(1 << 16) +#define ? IDLE_SDA_PULL_LOW ? ? ? ? ? ?(2 << 16) +#define ? IDLE_SDA_PULL_HIGH ? ? ? ? ? (3 << 16) +#define ? IDLE_SDA_MASK ? ? ? ? ? ? ? ? ? ? ? ?(3 << 16) +#define ? CS_POLARITY1 ? ? ? ? ? ? ? ? (1 << 20) +#define ? CK_SDA ? ? ? ? ? ? ? ? ? ? ? (1 << 21) +#define ? CS_POLARITY2 ? ? ? ? ? ? ? ? (1 << 22) +#define ? CS_POLARITY3 ? ? ? ? ? ? ? ? (1 << 23) +#define ? IDLE_SCLK_DRIVE_LOW ? ? ? ? ?(0 << 24) +#define ? IDLE_SCLK_DRIVE_HIGH ? ? ? ? (1 << 24) +#define ? IDLE_SCLK_PULL_LOW ? ? ? ? ? (2 << 24) +#define ? IDLE_SCLK_PULL_HIGH ? ? ? ? ?(3 << 24) +#define ? IDLE_SCLK_MASK ? ? ? ? ? ? ? (3 << 24) +#define ? M_S ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 28) +#define ? WAIT ? ? ? ? ? ? ? ? ? ? ? ? (1 << 29) +#define ? GO ? ? ? ? ? ? ? ? ? ? ? ? ? (1 << 30) +#define ? ENB ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 31) + +#define SLINK_COMMAND2 ? ? ? ? 0x004 +#define ? LSBFE ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 0) +#define ? SSOE ? ? ? ? ? ? ? ? ? ? ? ? (1 << 1) +#define ? SPIE ? ? ? ? ? ? ? ? ? ? ? ? (1 << 4) +#define ? BIDIROE ? ? ? ? ? ? ? ? ? ? ?(1 << 6) +#define ? MODFEN ? ? ? ? ? ? ? ? ? ? ? (1 << 7) +#define ? INT_SIZE(x) ? ? ? ? ? ? ? ? ?(((x) & 0x1f) << 8) +#define ? CS_ACTIVE_BETWEEN ? ? ? ? ? ?(1 << 17) +#define ? SS_EN_CS(x) ? ? ? ? ? ? ? ? ?(((x) & 0x3) << 18) +#define ? SS_SETUP(x) ? ? ? ? ? ? ? ? ?(((x) & 0x3) << 20) +#define ? FIFO_REFILLS_0 ? ? ? ? ? ? ? (0 << 22) +#define ? FIFO_REFILLS_1 ? ? ? ? ? ? ? (1 << 22) +#define ? FIFO_REFILLS_2 ? ? ? ? ? ? ? (2 << 22) +#define ? FIFO_REFILLS_3 ? ? ? ? ? ? ? (3 << 22) +#define ? FIFO_REFILLS_MASK ? ? ? ? ? ?(3 << 22) +#define ? WAIT_PACK_INT(x) ? ? ? ? ? ? (((x) & 0x7) << 26) +#define ? SPC0 ? ? ? ? ? ? ? ? ? ? ? ? (1 << 29) +#define ? TXEN ? ? ? ? ? ? ? ? ? ? ? ? (1 << 30) +#define ? RXEN ? ? ? ? ? ? ? ? ? ? ? ? (1 << 31) + +#define SLINK_STATUS ? ? ? ? ? 0x008 +#define ? COUNT(val) ? ? ? ? ? ? ? ? ? (((val) >> 0) & 0x1f) +#define ? WORD(val) ? ? ? ? ? ? ? ? ? ?(((val) >> 5) & 0x1f) +#define ? BLK_CNT(val) ? ? ? ? ? ? ? ? (((val) >> 0) & 0xffff) +#define ? MODF ? ? ? ? ? ? ? ? ? ? ? ? (1 << 16) +#define ? RX_UNF ? ? ? ? ? ? ? ? ? ? ? (1 << 18) +#define ? TX_OVF ? ? ? ? ? ? ? ? ? ? ? (1 << 19) +#define ? TX_FULL ? ? ? ? ? ? ? ? ? ? ?(1 << 20) +#define ? TX_EMPTY ? ? ? ? ? ? ? ? ? ? (1 << 21) +#define ? RX_FULL ? ? ? ? ? ? ? ? ? ? ?(1 << 22) +#define ? RX_EMPTY ? ? ? ? ? ? ? ? ? ? (1 << 23) +#define ? TX_UNF ? ? ? ? ? ? ? ? ? ? ? (1 << 24) +#define ? RX_OVF ? ? ? ? ? ? ? ? ? ? ? (1 << 25) +#define ? TX_FLUSH ? ? ? ? ? ? ? ? ? ? (1 << 26) +#define ? RX_FLUSH ? ? ? ? ? ? ? ? ? ? (1 << 27) +#define ? SCLK ? ? ? ? ? ? ? ? ? ? ? ? (1 << 28) +#define ? ERR ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 29) +#define ? RDY ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 30) +#define ? BSY ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 31) + +#define SLINK_MAS_DATA ? ? ? ? 0x010 +#define SLINK_SLAVE_DATA ? ? ? 0x014 + +#define SLINK_DMA_CTL ? ? ? ? ?0x018 +#define ? DMA_BLOCK_SIZE(x) ? ? ? ? ? ?(((x) & 0xffff) << 0) +#define ? TX_TRIG_1 ? ? ? ? ? ? ? ? ? ?(0 << 16) +#define ? TX_TRIG_4 ? ? ? ? ? ? ? ? ? ?(1 << 16) +#define ? TX_TRIG_8 ? ? ? ? ? ? ? ? ? ?(2 << 16) +#define ? TX_TRIG_16 ? ? ? ? ? ? ? ? ? (3 << 16) +#define ? TX_TRIG_MASK ? ? ? ? ? ? ? ? (3 << 16) +#define ? RX_TRIG_1 ? ? ? ? ? ? ? ? ? ?(0 << 18) +#define ? RX_TRIG_4 ? ? ? ? ? ? ? ? ? ?(1 << 18) +#define ? RX_TRIG_8 ? ? ? ? ? ? ? ? ? ?(2 << 18) +#define ? RX_TRIG_16 ? ? ? ? ? ? ? ? ? (3 << 18) +#define ? RX_TRIG_MASK ? ? ? ? ? ? ? ? (3 << 18) +#define ? PACKED ? ? ? ? ? ? ? ? ? ? ? (1 << 20) +#define ? PACK_SIZE_4 ? ? ? ? ? ? ? ? ?(0 << 21) +#define ? PACK_SIZE_8 ? ? ? ? ? ? ? ? ?(1 << 21) +#define ? PACK_SIZE_16 ? ? ? ? ? ? ? ? (2 << 21) +#define ? PACK_SIZE_32 ? ? ? ? ? ? ? ? (3 << 21) +#define ? PACK_SIZE_MASK ? ? ? ? ? ? ? (3 << 21) +#define ? IE_TXC ? ? ? ? ? ? ? ? ? ? ? (1 << 26) +#define ? IE_RXC ? ? ? ? ? ? ? ? ? ? ? (1 << 27) +#define ? DMA_EN ? ? ? ? ? ? ? ? ? ? ? (1 << 31) + +#define SLINK_STATUS2 ? ? ? ? ?0x01c +#define ? TX_FIFO_EMPTY_COUNT(val) ? ? (((val) & 0x3f) >> 0) +#define ? RX_FIFO_FULL_COUNT(val) ? ? ?(((val) & 0x3f) >> 16) + +#define SLINK_TX_FIFO ? ? ? ? ?0x100 +#define SLINK_RX_FIFO ? ? ? ? ?0x180 +
drivers/spi/spi_tegra.c line 147
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+static const unsigned long spi_tegra_req_sels[] = { + ? ? ? TEGRA_DMA_REQ_SEL_SL2B1, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B2, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B3, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B4, +}; + +static inline unsigned bytes_per_word (u8 bits) +{ + ? ? ? WARN_ON((bits < 1) || bits > 32);
This condition has already been tested for in spi_tegra_start_transfer()
drivers/spi/spi_tegra.c line 155
Quoted 8 lines
drivers/spi/spi_tegra.c new file +8
-0+ ? ? ? if (bits <= 8) + ? ? ? ? ? ? ? return 1; + ? ? ? else if (bits <= 16) + ? ? ? ? ? ? ? return 2; + ? ? ? else if (bits <= 24) + ? ? ? ? ? ? ? return 3; + ? ? ? else + ? ? ? ? ? ? ? return 4;
return ((bits - 1) / 8) + 1
Also, this function is used in exactly 1 place. Consider moving it down to nearer where it is called, or open coding it instead.
Quoted 24 lines
drivers/spi/spi_tegra.c new file +24
-0+} + +#define BB_LEN ? ? ? ? ? ? ? ? 32 + +struct spi_tegra_data { + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct platform_device ?*pdev; + ? ? ? spinlock_t ? ? ? ? ? ? ?lock; + + ? ? ? struct clk ? ? ? ? ? ? ?*clk; + ? ? ? void __iomem ? ? ? ? ? ?*base; + ? ? ? unsigned long ? ? ? ? ? phys; + + ? ? ? u32 ? ? ? ? ? ? ? ? ? ? cur_speed; + + ? ? ? struct list_head ? ? ? ?queue; + ? ? ? struct spi_transfer ? ? *cur; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_pos; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_len; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_bytes_per_word; + + ? ? ? /* The tegra spi controller has a bug which causes the first word + ? ? ? ?* in PIO transactions to be garbage. ?Since packed DMA transactions + ? ? ? ?* require transfers to be 4 byte aligned we need a bounce buffer
drivers/spi/spi_tegra.c line 189
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+? ? ? ?* for the generic case. +? ? ? ?*/ + ? ? ? struct tegra_dma_req ? ?rx_dma_req; + ? ? ? struct tegra_dma_channel *rx_dma; + ? ? ? u32 ? ? ? ? ? ? ? ? ? ? *rx_bb; + ? ? ? dma_addr_t ? ? ? ? ? ? ?rx_bb_phys; +}; + + +static unsigned long spi_readl (struct spi_tegra_data *tspi,unsigned long reg)
To avoid global namespace collisions, please prefix all static symbols with something like tegra_spi_.
drivers/spi/spi_tegra.c line 199
Quoted 9 lines
drivers/spi/spi_tegra.c new file +9
-0+{ + ? ? ? return readl(tspi->base + reg); +} + +static void spi_writel (struct spi_tegra_data *tspi,unsigned long val, + ? ? ? ? ? ? ? ? ? ? ?unsigned long reg) +{ + ? ? ? writel(val, tspi->base + reg); +}
Are the accessor redirects really necessary? Is there any likelyhood that anything other than readl/writel will ever be used to access the device regs?
Quoted 72 lines
drivers/spi/spi_tegra.c new file +72
-0+ +static void spi_tegra_go (struct spi_tegra_data *tspi) +{ + ? ? ? unsigned long val; + + ? ? ? wmb(); + + ? ? ? val = spi_readl(tspi, SLINK_DMA_CTL); + ? ? ? val &= ~DMA_BLOCK_SIZE(~0) & ~DMA_EN; + ? ? ? val |= DMA_BLOCK_SIZE(tspi->rx_dma_req.size / 4 - 1); + ? ? ? spi_writel(tspi, val, SLINK_DMA_CTL); + + ? ? ? tegra_dma_enqueue_req(tspi->rx_dma, &tspi->rx_dma_req); + + ? ? ? val |= DMA_EN; + ? ? ? spi_writel(tspi, val, SLINK_DMA_CTL); +} + +static unsigned spi_tegra_fill_tx_fifo (struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? unsigned len = min(t->len - tspi->cur_pos, BB_LEN * + ? ? ? ? ? ? ? ? ? ? ? ? ?tspi->cur_bytes_per_word); + ? ? ? u8 *tx_buf = (u8 *)t->tx_buf + tspi->cur_pos; + ? ? ? int i, j; + ? ? ? unsigned long val; + + ? ? ? val = spi_readl(tspi, SLINK_COMMAND); + ? ? ? val &= ~WORD_SIZE(~0); + ? ? ? val |= WORD_SIZE(len / tspi->cur_bytes_per_word - 1); + ? ? ? spi_writel(tspi, val, SLINK_COMMAND); + + ? ? ? for (i = 0; i < len; i += tspi->cur_bytes_per_word) { + ? ? ? ? ? ? ? val = 0; + ? ? ? ? ? ? ? for (j = 0; j < tspi->cur_bytes_per_word; j++) + ? ? ? ? ? ? ? ? ? ? ? val |= tx_buf[i + j] << j * 8; + + ? ? ? ? ? ? ? spi_writel(tspi, val, SLINK_TX_FIFO); + ? ? ? } + + ? ? ? tspi->rx_dma_req.size = len / tspi->cur_bytes_per_word * 4; + + ? ? ? return len; +} + +static unsigned spi_tegra_drain_rx_fifo (struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? unsigned len = tspi->cur_len; + ? ? ? u8 *rx_buf = (u8 *)t->rx_buf + tspi->cur_pos; + ? ? ? int i, j; + ? ? ? unsigned long val; + + ? ? ? for (i = 0; i < len; i += tspi->cur_bytes_per_word) { + ? ? ? ? ? ? ? val = tspi->rx_bb[i / tspi->cur_bytes_per_word]; + ? ? ? ? ? ? ? for (j = 0; j < tspi->cur_bytes_per_word; j++) + ? ? ? ? ? ? ? ? ? ? ? rx_buf[i + j] = (val >> (j * 8)) & 0xff; + ? ? ? } + + ? ? ? return len; +} + +static int spi_tegra_start_transfer (struct spi_device *spi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? u32 speed; + ? ? ? u8 bits_per_word; + ? ? ? unsigned long val; + + ? ? ? speed = t->speed_hz ? t->speed_hz : spi->max_speed_hz; + ? ? ? bits_per_word = t->bits_per_word ? t->bits_per_word ?:
drivers/spi/spi_tegra.c line 280
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? ? ? ? ? spi->bits_per_word; + + ? ? ? if (bits_per_word < 1 || bits_per_word > 32) + ? ? ? ? ? ? ? return -EINVAL; + + ? ? ? if (t->len == 0) + ? ? ? ? ? ? ? return -EINVAL; + + ? ? ? if (!t->rx_buf && !t->tx_buf) + ? ? ? ? ? ? ? return -EINVAL;
Looks to me like this validation should be performed before accepting the transfer (see comments below)
Quoted 102 lines
drivers/spi/spi_tegra.c new file +102
-0+ + ? ? ? tspi->cur_bytes_per_word = bytes_per_word(bits_per_word); + + ? ? ? if (speed != tspi->cur_speed) + ? ? ? ? ? ? ? clk_set_rate(tspi->clk, speed); + + ? ? ? if (tspi->cur_speed == 0) + ? ? ? ? ? ? ? clk_enable(tspi->clk); + + ? ? ? tspi->cur_speed = speed; + + ? ? ? val = spi_readl(tspi, SLINK_COMMAND2); + ? ? ? if (t->rx_buf) + ? ? ? ? ? ? ? val |= RXEN; + ? ? ? else + ? ? ? ? ? ? ? val &= ~RXEN; + + ? ? ? if (t->tx_buf) + ? ? ? ? ? ? ? val |= TXEN; + ? ? ? else + ? ? ? ? ? ? ? val &= ~TXEN; + + ? ? ? val &= ~SS_EN_CS(~0); + ? ? ? val |= SS_EN_CS(spi->chip_select); + ? ? ? val |= SPIE; + + ? ? ? spi_writel(tspi, val, SLINK_COMMAND2); + + ? ? ? val = spi_readl(tspi, SLINK_COMMAND); + ? ? ? val &= ~BIT_LENGTH(~0); + ? ? ? val |= BIT_LENGTH(bits_per_word - 1); + + ? ? ? /* FIXME: should probably control CS manually so that we can be sure + ? ? ? ?* it does not go low between transfer and to support delay_usecs + ? ? ? ?* correctly. + ? ? ? ?*/ + ? ? ? val &= ~IDLE_SCLK_MASK & ~CK_SDA & ~CS_SW; + + ? ? ? if (spi->mode & SPI_CPHA) + ? ? ? ? ? ? ? val |= CK_SDA; + + ? ? ? if (spi->mode & SPI_CPOL) + ? ? ? ? ? ? ? val |= IDLE_SCLK_DRIVE_HIGH; + ? ? ? else + ? ? ? ? ? ? ? val |= IDLE_SCLK_DRIVE_LOW; + + ? ? ? val |= M_S; + + ? ? ? spi_writel(tspi, val, SLINK_COMMAND); + + ? ? ? spi_writel(tspi, RX_FLUSH | TX_FLUSH, SLINK_STATUS); + + ? ? ? tspi->cur = t; + ? ? ? tspi->cur_pos = 0; + ? ? ? tspi->cur_len = spi_tegra_fill_tx_fifo(tspi, t); + + ? ? ? spi_tegra_go(tspi); + + ? ? ? return 0; +} + +static void spi_tegra_start_message (struct spi_device *spi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_message *m) +{ + ? ? ? struct spi_transfer *t; + + ? ? ? m->actual_length = 0; + + ? ? ? t = list_first_entry(&m->transfers, struct spi_transfer, transfer_list); + ? ? ? spi_tegra_start_transfer(spi, t); +} + +static void tegra_spi_rx_dma_complete (struct tegra_dma_req *req) +{ + ? ? ? struct spi_tegra_data *tspi = req->dev; + ? ? ? unsigned long flags; + ? ? ? struct spi_message *m; + ? ? ? struct spi_device *spi; + ? ? ? int complete = 0; + ? ? ? int timeout = 0; + ? ? ? unsigned long val; + + ? ? ? /* the SPI controller may come back with both the BSY and RDY bits + ? ? ? ?* set. ?In this case we need to wait for the BSY bit to clear so + ? ? ? ?* that we are sure the DMA is finished. + ? ? ? ?*/ + ? ? ? while (timeout++ < BUSY_TIMEOUT) { + ? ? ? ? ? ? ? if (!(spi_readl(tspi, SLINK_STATUS) & BSY)) + ? ? ? ? ? ? ? ? ? ? ? break; + ? ? ? } + + ? ? ? val = spi_readl(tspi, SLINK_STATUS); + ? ? ? val |= RDY; + ? ? ? spi_writel(tspi, val, SLINK_STATUS); + + ? ? ? spin_lock_irqsave(&tspi->lock, flags); + + ? ? ? m = list_first_entry(&tspi->queue, struct spi_message, queue); + ? ? ? spi = m->state; + + ? ? ? tspi->cur_pos += spi_tegra_drain_rx_fifo(tspi, tspi->cur); + ? ? ? m->actual_length += tspi->cur_pos;
drivers/spi/spi_tegra.c line 392
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ + ? ? ? if (tspi->cur_pos < tspi->cur->len) { + ? ? ? ? ? ? ? tspi->cur_len = spi_tegra_fill_tx_fifo(tspi, tspi->cur); + ? ? ? ? ? ? ? spi_tegra_go(tspi); + ? ? ? } else if (!list_is_last(&tspi->cur->transfer_list, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&m->transfers)) { + ? ? ? ? ? ? ? tspi->cur = ?list_first_entry(&tspi->cur->transfer_list, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? transfer_list); + ? ? ? ? ? ? ? spi_tegra_start_transfer(spi, tspi->cur);
Not checking the returned error code. If an error occurs, then the callback needs to be called to notify that the message did not complete. However, this comment may be moot if the validation is moved out of spi_tegra_start_transfer().
Quoted 71 lines
drivers/spi/spi_tegra.c new file +71
-0+ ? ? ? } else { + ? ? ? ? ? ? ? complete = 1; + ? ? ? } + + ? ? ? if (complete) { + ? ? ? ? ? ? ? list_del(&m->queue); + + ? ? ? ? ? ? ? m->status = 0; + ? ? ? ? ? ? ? m->complete(m->context); + + ? ? ? ? ? ? ? if (!list_empty(&tspi->queue)) { + ? ? ? ? ? ? ? ? ? ? ? m = list_first_entry(&tspi->queue, struct spi_message, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?queue); + ? ? ? ? ? ? ? ? ? ? ? spi = m->state; + ? ? ? ? ? ? ? ? ? ? ? spi_tegra_start_message(spi, m); + ? ? ? ? ? ? ? } else { + ? ? ? ? ? ? ? ? ? ? ? clk_disable(tspi->clk); + ? ? ? ? ? ? ? ? ? ? ? tspi->cur_speed = 0; + ? ? ? ? ? ? ? } + ? ? ? } + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); +} + +static int spi_tegra_setup (struct spi_device *spi) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? unsigned long cs_bit; + ? ? ? unsigned long val; + ? ? ? unsigned long flags; + + ? ? ? dev_dbg(&spi->dev, "setup %d bpw, %scpol, %scpha, %dHz\n", + ? ? ? ? ? ? ? spi->bits_per_word, + ? ? ? ? ? ? ? spi->mode & SPI_CPOL ? "" : "~", + ? ? ? ? ? ? ? spi->mode & SPI_CPHA ? "" : "~", + ? ? ? ? ? ? ? spi->max_speed_hz); + + + ? ? ? switch (spi->chip_select) { + ? ? ? case 0: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY; + ? ? ? ? ? ? ? break; + + ? ? ? case 1: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY1; + ? ? ? ? ? ? ? break; + + ? ? ? case 2: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY2; + ? ? ? ? ? ? ? break; + + ? ? ? case 4: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY3; + ? ? ? ? ? ? ? break; + + ? ? ? default: + ? ? ? ? ? ? ? return -EINVAL; + ? ? ? } + + ? ? ? spin_lock_irqsave(&tspi->lock, flags); + + ? ? ? val = spi_readl(tspi, SLINK_COMMAND); + ? ? ? if (spi->mode & SPI_CS_HIGH) + ? ? ? ? ? ? ? val |= cs_bit; + ? ? ? else + ? ? ? ? ? ? ? val &= ~cs_bit; + ? ? ? spi_writel(tspi, val, SLINK_COMMAND); + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); + + ? ? ? return 0;
drivers/spi/spi_tegra.c line 473
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+} + +static int spi_tegra_transfer (struct spi_device *spi,struct spi_message *m) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? unsigned long flags; + ? ? ? int was_empty; + + ? ? ? if (list_empty(&m->transfers) || !m->complete) + ? ? ? ? ? ? ? return -EINVAL;
It's a good idea to move most of the validation of the message from spi_tegra_start_message() and spi_tegra_start_transfer() to here so that the validation occurs outside of the spinlock. It also means the driver doesn't discover badly formed messages part way through the transfer.
Quoted 51 lines
drivers/spi/spi_tegra.c new file +51
-0+ + ? ? ? m->state = spi; + + ? ? ? spin_lock_irqsave(&tspi->lock, flags); + ? ? ? was_empty = list_empty(&tspi->queue); + ? ? ? list_add_tail(&m->queue, &tspi->queue); + + ? ? ? if (was_empty) + ? ? ? ? ? ? ? spi_tegra_start_message(spi, m); + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); + + ? ? ? return 0; +} + +static void spi_tegra_cleanup (struct spi_device *spi) +{ + ? ? ? dev_dbg(&spi->dev, "cleanup\n"); +} + +static int __init spi_tegra_probe (struct platform_device *pdev) +{ + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct spi_tegra_data ? *tspi; + ? ? ? struct resource ? ? ? ? *r; + ? ? ? int ret; + + ? ? ? master = spi_alloc_master(&pdev->dev, sizeof *tspi); + ? ? ? if (master == NULL) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "master allocation failed\n"); + ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? } + + ? ? ? /* the spi->mode bits understood by this driver: */ + ? ? ? master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH; + + ? ? ? if (pdev->id != -1) + ? ? ? ? ? ? ? master->bus_num = pdev->id; + + ? ? ? master->setup = spi_tegra_setup; + ? ? ? master->transfer = spi_tegra_transfer; + ? ? ? master->cleanup = spi_tegra_cleanup; + ? ? ? master->num_chipselect = 4; + + ? ? ? dev_set_drvdata(&pdev->dev, master); + ? ? ? tspi = spi_master_get_devdata(master); + ? ? ? tspi->master = master; + ? ? ? tspi->pdev = pdev; + ? ? ? spin_lock_init(&tspi->lock); + + ? ? ? r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drivers/spi/spi_tegra.c line 534
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? if (r == NULL) { + ? ? ? ? ? ? ? ret = -ENODEV; + ? ? ? ? ? ? ? goto err0; + ? ? ? } + + ? ? ? if (!request_mem_region(r->start, (r->end - r->start) + 1, + ? ? ? ? ? ? ? ? ? ? ? dev_name(&pdev->dev))) { + ? ? ? ? ? ? ? ret = -EBUSY; + ? ? ? ? ? ? ? goto err0; + ? ? ? }
I believe the platform bus does this for you already by calling insert_resource() on all the platform bus ranges. See if /proc/iomem is any different with or without this call to verify.
Quoted 57 lines
drivers/spi/spi_tegra.c new file +57
-0+ + ? ? ? tspi->phys = r->start; + ? ? ? tspi->base = ioremap(r->start, r->end - r->start + 1); + ? ? ? if (!tspi->base) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can't ioremap iomem\n"); + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto err1; + ? ? ? } + + ? ? ? tspi->clk = clk_get(&pdev->dev, NULL); + ? ? ? if (IS_ERR_OR_NULL(tspi->clk)) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not get clock\n"); + ? ? ? ? ? ? ? ret = PTR_ERR(tspi->clk); + ? ? ? ? ? ? ? goto err2; + ? ? ? } + + ? ? ? INIT_LIST_HEAD(&tspi->queue); + + ? ? ? tspi->rx_dma = tegra_dma_allocate_channel(TEGRA_DMA_MODE_ONESHOT); + ? ? ? if (IS_ERR(tspi->rx_dma)) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not allocate rx dma channel\n"); + ? ? ? ? ? ? ? ret = PTR_ERR(tspi->rx_dma); + ? ? ? ? ? ? ? goto err3; + ? ? ? } + + ? ? ? tspi->rx_bb = dma_alloc_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&tspi->rx_bb_phys, GFP_KERNEL); + ? ? ? if (!tspi->rx_bb) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not allocate rx bounce buffer\n"); + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto err4; + ? ? ? } + + ? ? ? tspi->rx_dma_req.complete = tegra_spi_rx_dma_complete; + ? ? ? tspi->rx_dma_req.to_memory = 1; + ? ? ? tspi->rx_dma_req.dest_addr = tspi->rx_bb_phys; + ? ? ? tspi->rx_dma_req.dest_bus_width = 32; + ? ? ? tspi->rx_dma_req.source_addr = tspi->phys + SLINK_RX_FIFO; + ? ? ? tspi->rx_dma_req.source_bus_width = 32; + ? ? ? tspi->rx_dma_req.source_wrap = 4; + ? ? ? tspi->rx_dma_req.req_sel = spi_tegra_req_sels[pdev->id]; + ? ? ? tspi->rx_dma_req.dev = tspi; + + ? ? ? ret = spi_register_master(master); + + ? ? ? if (ret < 0) + ? ? ? ? ? ? ? goto err5; + + ? ? ? return ret; + +err5: + ? ? ? dma_free_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? tspi->rx_bb, tspi->rx_bb_phys); +err4: + ? ? ? tegra_dma_free_channel(tspi->rx_dma); +err3: + ? ? ? clk_put(tspi->clk);
drivers/spi/spi_tegra.c line 601
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+err2: + ? ? ? iounmap(tspi->base); +err1: + ? ? ? release_mem_region(r->start, (r->end - r->start) + 1); +err0: + ? ? ? spi_master_put(master); + ? ? ? return ret; +} + +static int __exit spi_tegra_remove(struct platform_device *pdev)
__devexit
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+{ + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct spi_tegra_data ? *tspi; + ? ? ? struct resource ? ? ? ? *r; + + ? ? ? master = dev_get_drvdata(&pdev->dev); + ? ? ? tspi = spi_master_get_devdata(master); + + ? ? ? tegra_dma_free_channel(tspi->rx_dma); +
drivers/spi/spi_tegra.c line 621
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? dma_free_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? tspi->rx_bb, tspi->rx_bb_phys); + + ? ? ? clk_put(tspi->clk); + ? ? ? iounmap(tspi->base); + + ? ? ? spi_master_put(master); + + ? ? ? r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ? ? ? release_mem_region(r->start, (r->end - r->start) + 1);
Ditto to comment earlier.
drivers/spi/spi_tegra.c
Quoted 13 lines
drivers/spi/spi_tegra.c new file +13
-0+ + ? ? ? return 0; +} + + +MODULE_ALIAS ("platform:" DRIVER_NAME ); + +static struct platform_driver spi_tegra_driver = { + ? ? ? .driver = { + ? ? ? ? ? ? ? .name = ? ? ? ? DRIVER_NAME, + ? ? ? ? ? ? ? .owner = ? ? ? ?THIS_MODULE, + ? ? ? }, + ? ? ? .remove = ? ? ? __exit_p(spi_tegra_remove),
__devexit_p
drivers/spi/spi_tegra.c line 643
Quoted 8 lines
drivers/spi/spi_tegra.c new file +8
-0+}; + + +static int __init spi_tegra_init (void ) +{ + ? ? ? return platform_driver_probe(&spi_tegra_driver, spi_tegra_probe); +} +subsys_initcall (spi_tegra_init);
Is module_init() not sufficient?
Quoted 17 lines
drivers/spi/spi_tegra.c new file +8
-1+ +static void __exit spi_tegra_exit(void) +{ + ? ? ? platform_driver_unregister(&spi_tegra_driver); +} +module_exit (spi_tegra_exit); + +MODULE_LICENSE ("GPL" ); --1.7.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 2 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.
PATCH 1/1 spi: add spi_tegra driver
Quoted 1 line · Grant Likely
What is the status of the APB DMA driver? ?Is it going to be merged in 2.6.36?
Since we were too late on these patches, they will go into the 37 merge window. The APB DMA driver will go in then along with SPI.
Quoted 5 lines
Quoted 1 line
+ *
drivers/spi/spi_tegra.cI find it is more useful, and it is my preference, to have a one-line description of what the file actually is here. ?Showing the filename is just redundant and doesn't tell a reader anything new.
I'll change it so it matches the rest of the files in the directory
Quoted 4 lines
Quoted 1 line
+#define
DRIVER_NAME? ? ? ? ? ?"spi_tegra"Used only twice in 2 side-by-side locations. ?I'd trim, or at least move to where it is actually used.
done
Quoted 3 lines
Quoted 1 line
+#define
BUSY_TIMEOUT? ? ? ? ? 1000Used exactly once. ?May be better defined and documented at the usage site
done
Quoted 16 lines
drivers/spi/spi_tegra.c
Quoted 4 lines
drivers/spi/spi_tegra.c new file +4
-0+ +static inline unsigned bytes_per_word (u8 bits) +{ + ? ? ? WARN_ON((bits < 1) || bits > 32);This condition has already been tested for in
spi_tegra_start_transfer()Quoted 8 lines
+ ? ? ? if (bits <= 8)
+ ? ? ? ? ? ? ? return 1;
+ ? ? ? else if (bits <= 16)
+ ? ? ? ? ? ? ? return 2;
+ ? ? ? else if (bits <= 24)
+ ? ? ? ? ? ? ? return 3;
+ ? ? ? else
+ ? ? ? ? ? ? ? return 4;
Quoted 4 lines
return ((bits - 1) / 8) + 1
Also, this function is used in exactly 1 place. ?Consider moving it down to nearer where it is called, or open coding it instead.
moved to spi_tegra_start_transfer
Quoted 4 lines
Quoted 1 line
+static unsigned long
spi_readl(struct spi_tegra_data *tspi, unsigned long reg)To avoid global namespace collisions, please prefix all static symbols with something like
tegra_spi_.
changed
Quoted 5 lines
Quoted 5 lines
drivers/spi/spi_tegra.c new file +5
-0+{ + ? ? ? return readl(tspi->base + reg); +} + +static void spi_writel(struct spi_tegra_data *tspi, unsigned long val,
Quoted 8 lines
drivers/spi/spi_tegra.c line 157
Quoted 4 lines
+ ? ? ? ? ? ? ? ? ? ? ?unsigned long reg)
drivers/spi/spi_tegra.c new file +3
-0+{ + ? ? ? writel(val, tspi->base + reg); +}Are the accessor redirects really necessary? ?Is there any likelyhood that anything other than readl/writel will ever be used to access the device regs?
They are there to make reset of the code easier to read and less error probe by eliminating the tspi->base + reg from every call.
Quoted 8 lines
Quoted 8 lines
(quoted diff) +8
-0+ + ? ? ? speed = t->speed_hz ? t->speed_hz : spi->max_speed_hz; + ? ? ? bits_per_word = t->bits_per_word ? t->bits_per_word ?: + ? ? ? ? ? ? ? spi->bits_per_word; + + ? ? ? if (bits_per_word < 1 || bits_per_word > 32) + ? ? ? ? ? ? ? return -EINVAL; +
Quoted 8 lines
Quoted 5 lines
+ ? ? ? if (
t->len== 0)
+ ? ? ? ? ? ? ? return-EINVAL;
+
+ ? ? ? if (!t->rx_buf && !t->tx_buf)
+ ? ? ? ? ? ? ? return-EINVAL;Looks to me like this validation should be performed before accepting the transfer (see comments below)
moved
Quoted 9 lines
Quoted 4 lines
+ ? ? ? ? ? ? ?
tspi->cur= ?list_first_entry(&tspi->cur->transfer_list,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? structspi_transfer,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?transfer_list);
+ ? ? ? ? ? ? ?spi_tegra_start_transfer(spi, tspi->cur);Not checking the returned error code. ?If an error occurs, then the callback needs to be called to notify that the message did not complete. ?However, this comment may be moot if the validation is moved out of
spi_tegra_start_transfer().
moved
Quoted 6 lines
Quoted 6 lines
drivers/spi/spi_tegra.c new file +6
-0+static int spi_tegra_transfer (struct spi_device *spi,struct spi_message *m) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? unsigned long flags; + ? ? ? int was_empty; +
Quoted 8 lines
Quoted 2 lines
+ ? ? ? if (
list_empty(&m->transfers)|| !m->complete)
+ ? ? ? ? ? ? ? return-EINVAL;It's a good idea to move most of the validation of the message from
spi_tegra_start_message()andspi_tegra_start_transfer()to here so that the validation occurs outside of the spinlock. ?It also means the driver doesn't discover badly formed messages part way through the transfer.
moved
Quoted 9 lines
Quoted 5 lines
+ ? ? ? if (!request_mem_region(r->start, (
r->end-r->start) + 1,
+ ? ? ? ? ? ? ? ? ? ? ?dev_name(&pdev->dev))) {
+ ? ? ? ? ? ? ? ret =-EBUSY;
+ ? ? ? ? ? ? ? goto err0;
+ ? ? ? }I believe the platform bus does this for you already by calling
insert_resource()on all the platform bus ranges. ?See if/proc/iomemis any different with or without this call to verify.
You're right. Didn't realize this
Quoted 3 lines
Quoted 1 line
+static int
__exitspi_tegra_remove(struct platform_device *pdev)
__devexit
done
Quoted 4 lines
Quoted 2 lines
+ ? ? ? r =
platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ ? ? ?release_mem_region(r->start, (r->end - r->start) + 1);Ditto to comment earlier.
done
Quoted 3 lines
Quoted 1 line
+ ? ? ? .remove = ? ? ?
__exit_p(spi_tegra_remove),
__devexit_p
done
Quoted 3 lines
Quoted 1 line
+subsys_initcall(spi_tegra_init);
Is
module_init()not sufficient?
All the other spi drivers use subsys_initcall
New patch coming soon.
Thanks,
Erik
PATCH 1/1 spi: add spi_tegra driver
Quoted 11 lines · Erik Gilling
Quoted 9 lines
Quoted 5 lines
+ ? ? ? if (!request_mem_region(r->start, (
r->end-r->start) + 1,
+ ? ? ? ? ? ? ? ? ? ? ?dev_name(&pdev->dev))) {
+ ? ? ? ? ? ? ? ret =-EBUSY;
+ ? ? ? ? ? ? ? goto err0;
+ ? ? ? }I believe the platform bus does this for you already by calling
insert_resource()on all the platform bus ranges. ?See if/proc/iomemis any different with or without this call to verify.You're right. Didn't realize this
There is a big difference between what the platform bus does and what drivers do.
The platform bus adds the resources to the resource tree as they stand without marking them _busy_. This means that they can be sub-divided and other resources registered within their range.
request_mem_region() adds resources to the resource tree marking them busy, detecting overlapping resources and indicating whether two drivers are trying to claim the same resource region. In other words, it's a preventative measure against two device drivers accessing the same region.
You're absolutely right to do what you're doing above, please continue to do so.
PATCH 1/1 spi: add spi_tegra driver
Quoted 20 lines · Russell King - ARM Linux
Quoted 11 lines · Erik Gilling
Quoted 9 lines
Quoted 5 lines
+ ? ? ? if (!request_mem_region(r->start, (
r->end-r->start) + 1,
+ ? ? ? ? ? ? ? ? ? ? ?dev_name(&pdev->dev))) {
+ ? ? ? ? ? ? ? ret =-EBUSY;
+ ? ? ? ? ? ? ? goto err0;
+ ? ? ? }I believe the platform bus does this for you already by calling
insert_resource()on all the platform bus ranges. ?See if/proc/iomemis any different with or without this call to verify.You're right. ?Didn't realize this
There is a big difference between what the platform bus does and what drivers do.
The platform bus adds the resources to the resource tree as they stand without marking them _busy_. ?This means that they can be sub-divided and other resources registered within their range.
Quoted 9 lines
request_mem_region()adds resources to the resource tree marking them busy, detecting overlapping resources and indicating whether two drivers are trying to claim the same resource region. ?In other words, it's a preventative measure against two device drivers accessing the same region.You're absolutely right to do what you're doing above, please continue to do so.
Thanks for the explanation. I'll add it back in.
Cheers,
Erik
PATCH v2 1/1 spi: add spi_tegra driver
In reply to Colin Crossv2 changes:
from Thierry Reding:
* add "select TEGRA_SYSTEM_DMA" to Kconfig
from Grant Likely:
* add oneline description to header
* inline references to DRIVER_NAME
* inline references to BUSY_TIMEOUT
* open coded bytes_per_word()
* spi_readl/writel -> spi_tegra_readl/writel
* move transfer validation to spi_tegra_transfer
* don't request_mem_region iomem as platform bus does that for us
* __exit -> __devexit
--- drivers/spi/Kconfig | 7 + drivers/spi/Makefile | 1 + drivers/spi/spi_tegra.c | 638 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 646 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/spi_tegra.c
drivers/spi/Kconfig +7 -0
@@ -298,6 +298,13 @@ config SPI_STMP3XXX help SPI driver for Freescale STMP37xx/378x SoC SSP interface +config SPI_TEGRA + tristate "Nvidia Tegra SPI controller" + depends on ARCH_TEGRA + select TEGRA_SYSTEM_DMA + help + SPI driver for NVidia Tegra SoCs + config SPI_TXX9 tristate "Toshiba TXx9 SPI controller" depends on GENERIC_GPIO && CPU_TX49XX
drivers/spi/Makefile +1 -0
@@ -39,6 +39,7 @@ obj-$(CONFIG_SPI_PPC4xx) += spi_ppc4xx.o obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx_hw.o obj-$(CONFIG_SPI_S3C64XX) += spi_s3c64xx.o +obj-$(CONFIG_SPI_TEGRA) += spi_tegra.o obj-$(CONFIG_SPI_TXX9) += spi_txx9.o obj-$(CONFIG_SPI_XILINX) += xilinx_spi.o obj-$(CONFIG_SPI_XILINX_OF) += xilinx_spi_of.o
drivers/spi/spi_tegra.c new file +638 -0
638 lines not on this page · raw message
Signature · 1 line
1.6.5.6
PATCH v2 1/1 spi: add spi_tegra driver
Hi Erik,
Thanks for the quick respin. I've got some more comments below, most of them minor, but there are some questions about the locking model. I don't see any problem with me picking this driver up into my next-spi tree once the .26 merge window closes.
g.
On Wed, Aug 11, 2010 at 6:47 PM, Erik Gilling <[email protected]> wrote:
Missing patch description. Give reviewers and future readers some more details about what hardware this patch adds support for.
Quoted 70 lines
v2 changes:
?from Thierry Reding:
? ?* add "selectTEGRA_SYSTEM_DMA" toKconfig
?from Grant Likely:
? ?* add oneline description to header
? ?* inline references toDRIVER_NAME
? ?* inline references toBUSY_TIMEOUT
? ?* open codedbytes_per_word()
? ?* spi_readl/writel -> spi_tegra_readl/writel
? ?* move transfer validation tospi_tegra_transfer
? ?* don'trequest_mem_regioniomem as platform bus does that for us ? ?*__exit->__devexitSigned-off-by Erik Gilling <[email protected]>Cc Thierry Reding <[email protected]>Cc Grant Likely <[email protected]>---
?drivers/spi/Kconfig ? ? | ? ?7 + ?drivers/spi/Makefile ? ?| ? ?1 + ?drivers/spi/spi_tegra.c | ?638 +++++++++++++++++++++++++++++++++++++++++++++++?3 files changed, 646 insertions(+), 0 deletions(-) ?create mode 100644
drivers/spi/spi_tegra.cdrivers/spi/Kconfig +0
-0@@ -298,6 +298,13 @@ config SPI_STMP3XXX? ? ? ?help
? ? ? ? ?SPI driver for Freescale STMP37xx/378x SoC SSP interfacedrivers/spi/Kconfig +7
-0+config SPI_TEGRA + ? ? ? tristate "Nvidia Tegra SPI controller" + ? ? ? depends on ARCH_TEGRA + ? ? ? select TEGRA_SYSTEM_DMA + ? ? ? help + ? ? ? ? SPI driver for NVidia Tegra SoCs +?config
SPI_TXX9
? ? ? ?tristate "Toshiba TXx9 SPI controller"
? ? ? ?depends onGENERIC_GPIO&&CPU_TX49XXdrivers/spi/Makefile +0
-0@@ -39,6 +39,7 @@ obj-$(CONFIG_SPI_PPC4xx) ? ? ? ? ? ? ?+= spi_ppc4xx.o?obj-$(CONFIG_SPI_S3C24XX_GPIO) ? ? ? ? += spi_s3c24xx_gpio.o ?obj-$(CONFIG_SPI_S3C24XX) ? ? ? ? ? ? ?+= spi_s3c24xx_hw.o ?obj-$(CONFIG_SPI_S3C64XX) ? ? ? ? ? ? ?+= spi_s3c64xx.o +obj-$(CONFIG_SPI_TEGRA) ? ? ? ? ? ? ? ? ? ? ? ?+= spi_tegra.o ?obj-$(CONFIG_SPI_TXX9) ? ? ? ? ? ? ? ? += spi_txx9.o ?obj-$(CONFIG_SPI_XILINX) ? ? ? ? ? ? ? += xilinx_spi.o ?obj-$(CONFIG_SPI_XILINX_OF) ? ? ? ? ? ?+= xilinx_spi_of.o
drivers/spi/spi_tegra.c new file +10
-0@@ -0,0 +1,638 @@ +/* +* Driver for Nvidia TEGRA spi controller. +* +* Copyright (C) 2010 Google, Inc. +* +* Author: +* ? ? Erik Gilling <[email protected]> +* +* This software is licensed under the terms of the GNU General Public +* License version 2, as published by the Free Software Foundation, and
drivers/spi/spi_tegra.c line 20
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+* may be copied, distributed, and modified under those terms. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the +* GNU General Public License for more details. +* +*/ + +#undef DEBUG
Still need to trim this #undef
Quoted 30 lines
drivers/spi/spi_tegra.c new file +30
-0+#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/dma-mapping.h> +#include <linux/dmapool.h> +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/delay.h> + +#include <linux/spi/spi.h> + +#include <mach/dma.h> + +#define SLINK_COMMAND ? ? ? ? ?0x000 +#define ? BIT_LENGTH(x) ? ? ? ? ? ? ? ? ? ? ? ?(((x) & 0x1f) << 0) +#define ? WORD_SIZE(x) ? ? ? ? ? ? ? ? (((x) & 0x1f) << 5) +#define ? BOTH_EN ? ? ? ? ? ? ? ? ? ? ?(1 << 10) +#define ? CS_SW ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 11) +#define ? CS_VALUE ? ? ? ? ? ? ? ? ? ? (1 << 12) +#define ? CS_POLARITY ? ? ? ? ? ? ? ? ?(1 << 13) +#define ? IDLE_SDA_DRIVE_LOW ? ? ? ? ? (0 << 16) +#define ? IDLE_SDA_DRIVE_HIGH ? ? ? ? ?(1 << 16) +#define ? IDLE_SDA_PULL_LOW ? ? ? ? ? ?(2 << 16) +#define ? IDLE_SDA_PULL_HIGH ? ? ? ? ? (3 << 16) +#define ? IDLE_SDA_MASK ? ? ? ? ? ? ? ? ? ? ? ?(3 << 16) +#define ? CS_POLARITY1 ? ? ? ? ? ? ? ? (1 << 20) +#define ? CK_SDA ? ? ? ? ? ? ? ? ? ? ? (1 << 21) +#define ? CS_POLARITY2 ? ? ? ? ? ? ? ? (1 << 22)
drivers/spi/spi_tegra.c line 60
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+#define ? CS_POLARITY3 ? ? ? ? ? ? ? ? (1 << 23) +#define ? IDLE_SCLK_DRIVE_LOW ? ? ? ? ?(0 << 24) +#define ? IDLE_SCLK_DRIVE_HIGH ? ? ? ? (1 << 24) +#define ? IDLE_SCLK_PULL_LOW ? ? ? ? ? (2 << 24) +#define ? IDLE_SCLK_PULL_HIGH ? ? ? ? ?(3 << 24) +#define ? IDLE_SCLK_MASK ? ? ? ? ? ? ? (3 << 24) +#define ? M_S ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 28) +#define ? WAIT ? ? ? ? ? ? ? ? ? ? ? ? (1 << 29) +#define ? GO ? ? ? ? ? ? ? ? ? ? ? ? ? (1 << 30) +#define ? ENB ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 31)
It's a good idea to prefix all these register #defines with something driver specific to avoid the possibility of collisions with the global namespace.
Quoted 82 lines
drivers/spi/spi_tegra.c new file +82
-0+ +#define SLINK_COMMAND2 ? ? ? ? 0x004 +#define ? LSBFE ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 0) +#define ? SSOE ? ? ? ? ? ? ? ? ? ? ? ? (1 << 1) +#define ? SPIE ? ? ? ? ? ? ? ? ? ? ? ? (1 << 4) +#define ? BIDIROE ? ? ? ? ? ? ? ? ? ? ?(1 << 6) +#define ? MODFEN ? ? ? ? ? ? ? ? ? ? ? (1 << 7) +#define ? INT_SIZE(x) ? ? ? ? ? ? ? ? ?(((x) & 0x1f) << 8) +#define ? CS_ACTIVE_BETWEEN ? ? ? ? ? ?(1 << 17) +#define ? SS_EN_CS(x) ? ? ? ? ? ? ? ? ?(((x) & 0x3) << 18) +#define ? SS_SETUP(x) ? ? ? ? ? ? ? ? ?(((x) & 0x3) << 20) +#define ? FIFO_REFILLS_0 ? ? ? ? ? ? ? (0 << 22) +#define ? FIFO_REFILLS_1 ? ? ? ? ? ? ? (1 << 22) +#define ? FIFO_REFILLS_2 ? ? ? ? ? ? ? (2 << 22) +#define ? FIFO_REFILLS_3 ? ? ? ? ? ? ? (3 << 22) +#define ? FIFO_REFILLS_MASK ? ? ? ? ? ?(3 << 22) +#define ? WAIT_PACK_INT(x) ? ? ? ? ? ? (((x) & 0x7) << 26) +#define ? SPC0 ? ? ? ? ? ? ? ? ? ? ? ? (1 << 29) +#define ? TXEN ? ? ? ? ? ? ? ? ? ? ? ? (1 << 30) +#define ? RXEN ? ? ? ? ? ? ? ? ? ? ? ? (1 << 31) + +#define SLINK_STATUS ? ? ? ? ? 0x008 +#define ? COUNT(val) ? ? ? ? ? ? ? ? ? (((val) >> 0) & 0x1f) +#define ? WORD(val) ? ? ? ? ? ? ? ? ? ?(((val) >> 5) & 0x1f) +#define ? BLK_CNT(val) ? ? ? ? ? ? ? ? (((val) >> 0) & 0xffff) +#define ? MODF ? ? ? ? ? ? ? ? ? ? ? ? (1 << 16) +#define ? RX_UNF ? ? ? ? ? ? ? ? ? ? ? (1 << 18) +#define ? TX_OVF ? ? ? ? ? ? ? ? ? ? ? (1 << 19) +#define ? TX_FULL ? ? ? ? ? ? ? ? ? ? ?(1 << 20) +#define ? TX_EMPTY ? ? ? ? ? ? ? ? ? ? (1 << 21) +#define ? RX_FULL ? ? ? ? ? ? ? ? ? ? ?(1 << 22) +#define ? RX_EMPTY ? ? ? ? ? ? ? ? ? ? (1 << 23) +#define ? TX_UNF ? ? ? ? ? ? ? ? ? ? ? (1 << 24) +#define ? RX_OVF ? ? ? ? ? ? ? ? ? ? ? (1 << 25) +#define ? TX_FLUSH ? ? ? ? ? ? ? ? ? ? (1 << 26) +#define ? RX_FLUSH ? ? ? ? ? ? ? ? ? ? (1 << 27) +#define ? SCLK ? ? ? ? ? ? ? ? ? ? ? ? (1 << 28) +#define ? ERR ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 29) +#define ? RDY ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 30) +#define ? BSY ? ? ? ? ? ? ? ? ? ? ? ? ?(1 << 31) + +#define SLINK_MAS_DATA ? ? ? ? 0x010 +#define SLINK_SLAVE_DATA ? ? ? 0x014 + +#define SLINK_DMA_CTL ? ? ? ? ?0x018 +#define ? DMA_BLOCK_SIZE(x) ? ? ? ? ? ?(((x) & 0xffff) << 0) +#define ? TX_TRIG_1 ? ? ? ? ? ? ? ? ? ?(0 << 16) +#define ? TX_TRIG_4 ? ? ? ? ? ? ? ? ? ?(1 << 16) +#define ? TX_TRIG_8 ? ? ? ? ? ? ? ? ? ?(2 << 16) +#define ? TX_TRIG_16 ? ? ? ? ? ? ? ? ? (3 << 16) +#define ? TX_TRIG_MASK ? ? ? ? ? ? ? ? (3 << 16) +#define ? RX_TRIG_1 ? ? ? ? ? ? ? ? ? ?(0 << 18) +#define ? RX_TRIG_4 ? ? ? ? ? ? ? ? ? ?(1 << 18) +#define ? RX_TRIG_8 ? ? ? ? ? ? ? ? ? ?(2 << 18) +#define ? RX_TRIG_16 ? ? ? ? ? ? ? ? ? (3 << 18) +#define ? RX_TRIG_MASK ? ? ? ? ? ? ? ? (3 << 18) +#define ? PACKED ? ? ? ? ? ? ? ? ? ? ? (1 << 20) +#define ? PACK_SIZE_4 ? ? ? ? ? ? ? ? ?(0 << 21) +#define ? PACK_SIZE_8 ? ? ? ? ? ? ? ? ?(1 << 21) +#define ? PACK_SIZE_16 ? ? ? ? ? ? ? ? (2 << 21) +#define ? PACK_SIZE_32 ? ? ? ? ? ? ? ? (3 << 21) +#define ? PACK_SIZE_MASK ? ? ? ? ? ? ? (3 << 21) +#define ? IE_TXC ? ? ? ? ? ? ? ? ? ? ? (1 << 26) +#define ? IE_RXC ? ? ? ? ? ? ? ? ? ? ? (1 << 27) +#define ? DMA_EN ? ? ? ? ? ? ? ? ? ? ? (1 << 31) + +#define SLINK_STATUS2 ? ? ? ? ?0x01c +#define ? TX_FIFO_EMPTY_COUNT(val) ? ? (((val) & 0x3f) >> 0) +#define ? RX_FIFO_FULL_COUNT(val) ? ? ?(((val) & 0x3f) >> 16) + +#define SLINK_TX_FIFO ? ? ? ? ?0x100 +#define SLINK_RX_FIFO ? ? ? ? ?0x180 + +static const unsigned long spi_tegra_req_sels[] = { + ? ? ? TEGRA_DMA_REQ_SEL_SL2B1, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B2, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B3, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B4, +}; + +static inline unsigned bytes_per_word (u8 bits) +{
drivers/spi/spi_tegra.c line 152
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? WARN_ON((bits < 1) || bits > 32); + ? ? ? if (bits <= 8) + ? ? ? ? ? ? ? return 1; + ? ? ? else if (bits <= 16) + ? ? ? ? ? ? ? return 2; + ? ? ? else if (bits <= 24) + ? ? ? ? ? ? ? return 3; + ? ? ? else + ? ? ? ? ? ? ? return 4; +}
No longer used, can be removed.
Quoted 27 lines
drivers/spi/spi_tegra.c new file +27
-0+ +#define BB_LEN ? ? ? ? ? ? ? ? 32 + +struct spi_tegra_data { + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct platform_device ?*pdev; + ? ? ? spinlock_t ? ? ? ? ? ? ?lock; + + ? ? ? struct clk ? ? ? ? ? ? ?*clk; + ? ? ? void __iomem ? ? ? ? ? ?*base; + ? ? ? unsigned long ? ? ? ? ? phys; + + ? ? ? u32 ? ? ? ? ? ? ? ? ? ? cur_speed; + + ? ? ? struct list_head ? ? ? ?queue; + ? ? ? struct spi_transfer ? ? *cur; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_pos; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_len; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_bytes_per_word; + + ? ? ? /* The tegra spi controller has a bug which causes the first word + ? ? ? ?* in PIO transactions to be garbage. ?Since packed DMA transactions + ? ? ? ?* require transfers to be 4 byte aligned we need a bounce buffer + ? ? ? ?* for the generic case. + ? ? ? ?*/ + ? ? ? struct tegra_dma_req ? ?rx_dma_req; + ? ? ? struct tegra_dma_channel *rx_dma;
drivers/spi/spi_tegra.c line 189
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? u32 ? ? ? ? ? ? ? ? ? ? *rx_bb; + ? ? ? dma_addr_t ? ? ? ? ? ? ?rx_bb_phys; +}; + + +static unsigned long spi_tegra_readl (struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long reg) +{ + ? ? ? return readl(tspi->base + reg); +}
static inline
drivers/spi/spi_tegra.c line 196
Quoted 6 lines
drivers/spi/spi_tegra.c new file +6
-0+ +static void spi_tegra_writel (struct spi_tegra_data *tspi,unsigned long val, + ? ? ? ? ? ? ? ? ? ? ?unsigned long reg) +{ + ? ? ? writel(val, tspi->base + reg); +}
ditto
Quoted 88 lines
drivers/spi/spi_tegra.c new file +88
-0+ +static void spi_tegra_go(struct spi_tegra_data *tspi) +{ + ? ? ? unsigned long val; + + ? ? ? wmb(); + + ? ? ? val = spi_tegra_readl(tspi, SLINK_DMA_CTL); + ? ? ? val &= ~DMA_BLOCK_SIZE(~0) & ~DMA_EN; + ? ? ? val |= DMA_BLOCK_SIZE(tspi->rx_dma_req.size / 4 - 1); + ? ? ? spi_tegra_writel(tspi, val, SLINK_DMA_CTL); + + ? ? ? tegra_dma_enqueue_req(tspi->rx_dma, &tspi->rx_dma_req); + + ? ? ? val |= DMA_EN; + ? ? ? spi_tegra_writel(tspi, val, SLINK_DMA_CTL); +} + +static unsigned spi_tegra_fill_tx_fifo(struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? unsigned len = min(t->len - tspi->cur_pos, BB_LEN * + ? ? ? ? ? ? ? ? ? ? ? ? ?tspi->cur_bytes_per_word); + ? ? ? u8 *tx_buf = (u8 *)t->tx_buf + tspi->cur_pos; + ? ? ? int i, j; + ? ? ? unsigned long val; + + ? ? ? val = spi_tegra_readl(tspi, SLINK_COMMAND); + ? ? ? val &= ~WORD_SIZE(~0); + ? ? ? val |= WORD_SIZE(len / tspi->cur_bytes_per_word - 1); + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND); + + ? ? ? for (i = 0; i < len; i += tspi->cur_bytes_per_word) { + ? ? ? ? ? ? ? val = 0; + ? ? ? ? ? ? ? for (j = 0; j < tspi->cur_bytes_per_word; j++) + ? ? ? ? ? ? ? ? ? ? ? val |= tx_buf[i + j] << j * 8; + + ? ? ? ? ? ? ? spi_tegra_writel(tspi, val, SLINK_TX_FIFO); + ? ? ? } + + ? ? ? tspi->rx_dma_req.size = len / tspi->cur_bytes_per_word * 4; + + ? ? ? return len; +} + +static unsigned spi_tegra_drain_rx_fifo(struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? unsigned len = tspi->cur_len; + ? ? ? u8 *rx_buf = (u8 *)t->rx_buf + tspi->cur_pos; + ? ? ? int i, j; + ? ? ? unsigned long val; + + ? ? ? for (i = 0; i < len; i += tspi->cur_bytes_per_word) { + ? ? ? ? ? ? ? val = tspi->rx_bb[i / tspi->cur_bytes_per_word]; + ? ? ? ? ? ? ? for (j = 0; j < tspi->cur_bytes_per_word; j++) + ? ? ? ? ? ? ? ? ? ? ? rx_buf[i + j] = (val >> (j * 8)) & 0xff; + ? ? ? } + + ? ? ? return len; +} + +static void spi_tegra_start_transfer(struct spi_device *spi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? u32 speed; + ? ? ? u8 bits_per_word; + ? ? ? unsigned long val; + + ? ? ? speed = t->speed_hz ? t->speed_hz : spi->max_speed_hz; + ? ? ? bits_per_word = t->bits_per_word ? t->bits_per_word ?: + ? ? ? ? ? ? ? spi->bits_per_word; + + ? ? ? tspi->cur_bytes_per_word = (bits_per_word - 1) / 8 + 1; + + ? ? ? if (speed != tspi->cur_speed) + ? ? ? ? ? ? ? clk_set_rate(tspi->clk, speed); + + ? ? ? if (tspi->cur_speed == 0) + ? ? ? ? ? ? ? clk_enable(tspi->clk); + + ? ? ? tspi->cur_speed = speed; + + ? ? ? val = spi_tegra_readl(tspi, SLINK_COMMAND2); + ? ? ? if (t->rx_buf) + ? ? ? ? ? ? ? val |= RXEN; + ? ? ? else
drivers/spi/spi_tegra.c line 293
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? ? ? ? ? val &= ~RXEN; + + ? ? ? if (t->tx_buf) + ? ? ? ? ? ? ? val |= TXEN; + ? ? ? else + ? ? ? ? ? ? ? val &= ~TXEN; + + ? ? ? val &= ~SS_EN_CS(~0); + ? ? ? val |= SS_EN_CS(spi->chip_select); + ? ? ? val |= SPIE;
Nit: This would be more concise:
+ ? ? ? val &= ~(SS_EN_CS(~0) | RXEN | TXEN);
+ ? ? ? if (t->rx_buf)
+ ? ? ? ? ? ? ? val |= RXEN;
+ ? ? ? if (t->tx_buf)
+ ? ? ? ? ? ? ? val |= TXEN;
+ ? ? ? val |= SS_EN_CS(spi->chip_select);
+ ? ? ? val |= SPIE;
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND2); + + ? ? ? val = spi_tegra_readl(tspi, SLINK_COMMAND); + ? ? ? val &= ~BIT_LENGTH(~0); + ? ? ? val |= BIT_LENGTH(bits_per_word - 1); + + ? ? ? /* FIXME: should probably control CS manually so that we can be sure +? ? ? ?* it does not go low between transfer and to support delay_usecs +? ? ? ?* correctly.
drivers/spi/spi_tegra.c line 313
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+? ? ? ?*/ + ? ? ? val &= ~IDLE_SCLK_MASK & ~CK_SDA & ~CS_SW; + + ? ? ? if (spi->mode & SPI_CPHA) + ? ? ? ? ? ? ? val |= CK_SDA; + + ? ? ? if (spi->mode & SPI_CPOL) + ? ? ? ? ? ? ? val |= IDLE_SCLK_DRIVE_HIGH; + ? ? ? else + ? ? ? ? ? ? ? val |= IDLE_SCLK_DRIVE_LOW;
nit: IDLE_SCLK_DRIVE_LOW == 0. The driver will be more concise if the above 2 lines are dropped.
drivers/spi/spi_tegra.c
Quoted 4 lines
drivers/spi/spi_tegra.c new file +4
-0+ + ? ? ? val |= M_S; + + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND);
The read/modify/write register blocks are quite common. Would it be clearer and more concise to have a spi_tegra_clrsetbits() static inline?
Quoted 30 lines
drivers/spi/spi_tegra.c new file +30
-0+ + ? ? ? spi_tegra_writel(tspi, RX_FLUSH | TX_FLUSH, SLINK_STATUS); + + ? ? ? tspi->cur = t; + ? ? ? tspi->cur_pos = 0; + ? ? ? tspi->cur_len = spi_tegra_fill_tx_fifo(tspi, t); + + ? ? ? spi_tegra_go(tspi); +} + +static void spi_tegra_start_message (struct spi_device *spi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_message *m) +{ + ? ? ? struct spi_transfer *t; + + ? ? ? m->actual_length = 0; + + ? ? ? t = list_first_entry(&m->transfers, struct spi_transfer, transfer_list); + ? ? ? spi_tegra_start_transfer(spi, t); +} + +static void tegra_spi_rx_dma_complete (struct tegra_dma_req *req) +{ + ? ? ? struct spi_tegra_data *tspi = req->dev; + ? ? ? unsigned long flags; + ? ? ? struct spi_message *m; + ? ? ? struct spi_device *spi; + ? ? ? int complete = 0; + ? ? ? int timeout = 0; + ? ? ? unsigned long val;
drivers/spi/spi_tegra.c line 357
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ + ? ? ? /* the SPI controller may come back with both the BSY and RDY bits + ? ? ? ?* set. ?In this case we need to wait for the BSY bit to clear so + ? ? ? ?* that we are sure the DMA is finished. ?1000 reads was empirically + ? ? ? ?* determined to be long enough. + ? ? ? ?*/ + ? ? ? while (timeout++ < 1000) { + ? ? ? ? ? ? ? if (!(spi_tegra_readl(tspi, SLINK_STATUS) & BSY)) + ? ? ? ? ? ? ? ? ? ? ? break; + ? ? ? }
What happens if the BSY bit is still set when the loop exits?
drivers/spi/spi_tegra.c line 361
Quoted 4 lines
drivers/spi/spi_tegra.c new file +4
-0+ + ? ? ? val = spi_tegra_readl(tspi, SLINK_STATUS); + ? ? ? val |= RDY; + ? ? ? spi_tegra_writel(tspi, val, SLINK_STATUS);
The spin lock isn't held at this point. Will that make this read-modify-write operation risky?
drivers/spi/spi_tegra.c line 363
Quoted 2 lines
drivers/spi/spi_tegra.c new file +2
-0+ + ? ? ? spin_lock_irqsave(&tspi->lock, flags);
Is there any condition/event which could cause the spi controller to go busy before obtaining the spinlock?
Quoted 11 lines
drivers/spi/spi_tegra.c new file +11
-0+ + ? ? ? m = list_first_entry(&tspi->queue, struct spi_message, queue); + ? ? ? spi = m->state; + + ? ? ? tspi->cur_pos += spi_tegra_drain_rx_fifo(tspi, tspi->cur); + ? ? ? m->actual_length += tspi->cur_pos; + + ? ? ? if (tspi->cur_pos < tspi->cur->len) { + ? ? ? ? ? ? ? tspi->cur_len = spi_tegra_fill_tx_fifo(tspi, tspi->cur); + ? ? ? ? ? ? ? spi_tegra_go(tspi); + ? ? ? } else if (!list_is_last(&tspi->cur->transfer_list,
drivers/spi/spi_tegra.c line 384
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&m->transfers)) { + ? ? ? ? ? ? ? tspi->cur = ?list_first_entry(&tspi->cur->transfer_list, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? transfer_list); + ? ? ? ? ? ? ? spi_tegra_start_transfer(spi, tspi->cur); + ? ? ? } else { + ? ? ? ? ? ? ? complete = 1; + ? ? ? } + + ? ? ? if (complete) {
Looks like the above else clause is the only thing that can set complete. You could delete the above three lines to get the same behaviour.
Quoted 39 lines
drivers/spi/spi_tegra.c new file +39
-0+ ? ? ? ? ? ? ? list_del(&m->queue); + + ? ? ? ? ? ? ? m->status = 0; + ? ? ? ? ? ? ? m->complete(m->context); + + ? ? ? ? ? ? ? if (!list_empty(&tspi->queue)) { + ? ? ? ? ? ? ? ? ? ? ? m = list_first_entry(&tspi->queue, struct spi_message, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?queue); + ? ? ? ? ? ? ? ? ? ? ? spi = m->state; + ? ? ? ? ? ? ? ? ? ? ? spi_tegra_start_message(spi, m); + ? ? ? ? ? ? ? } else { + ? ? ? ? ? ? ? ? ? ? ? clk_disable(tspi->clk); + ? ? ? ? ? ? ? ? ? ? ? tspi->cur_speed = 0; + ? ? ? ? ? ? ? } + ? ? ? } + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); +} + +static int spi_tegra_setup (struct spi_device *spi) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? unsigned long cs_bit; + ? ? ? unsigned long val; + ? ? ? unsigned long flags; + + ? ? ? dev_dbg(&spi->dev, "setup %d bpw, %scpol, %scpha, %dHz\n", + ? ? ? ? ? ? ? spi->bits_per_word, + ? ? ? ? ? ? ? spi->mode & SPI_CPOL ? "" : "~", + ? ? ? ? ? ? ? spi->mode & SPI_CPHA ? "" : "~", + ? ? ? ? ? ? ? spi->max_speed_hz); + + + ? ? ? switch (spi->chip_select) { + ? ? ? case 0: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY; + ? ? ? ? ? ? ? break; + + ? ? ? case 1:
drivers/spi/spi_tegra.c line 433
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? ? ? ? ? cs_bit = CS_POLARITY1; + ? ? ? ? ? ? ? break; + + ? ? ? case 2: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY2; + ? ? ? ? ? ? ? break; + + ? ? ? case 4: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY3; + ? ? ? ? ? ? ? break;
CS_POLARITY is named a little oddly as it seems to indicate the bit, not the polarity of the bit. Would this better be named CS_BIT0..CS_BIT3?
Quoted 104 lines
drivers/spi/spi_tegra.c new file +104
-0+ + ? ? ? default: + ? ? ? ? ? ? ? return -EINVAL; + ? ? ? } + + ? ? ? spin_lock_irqsave(&tspi->lock, flags); + + ? ? ? val = spi_tegra_readl(tspi, SLINK_COMMAND); + ? ? ? if (spi->mode & SPI_CS_HIGH) + ? ? ? ? ? ? ? val |= cs_bit; + ? ? ? else + ? ? ? ? ? ? ? val &= ~cs_bit; + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND); + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); + + ? ? ? return 0; +} + +static int spi_tegra_transfer(struct spi_device *spi, struct spi_message *m) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? struct spi_transfer *t; + ? ? ? unsigned long flags; + ? ? ? int was_empty; + + ? ? ? if (list_empty(&m->transfers) || !m->complete) + ? ? ? ? ? ? ? return -EINVAL; + + ? ? ? list_for_each_entry(t, &m->transfers, transfer_list) { + ? ? ? ? ? ? ? if (t->bits_per_word < 0 || t->bits_per_word > 32) + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL; + + ? ? ? ? ? ? ? if (t->len == 0) + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL; + + ? ? ? ? ? ? ? if (!t->rx_buf && !t->tx_buf) + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL; + ? ? ? } + + ? ? ? m->state = spi; + + ? ? ? spin_lock_irqsave(&tspi->lock, flags); + ? ? ? was_empty = list_empty(&tspi->queue); + ? ? ? list_add_tail(&m->queue, &tspi->queue); + + ? ? ? if (was_empty) + ? ? ? ? ? ? ? spi_tegra_start_message(spi, m); + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); + + ? ? ? return 0; +} + +static void spi_tegra_cleanup(struct spi_device *spi) +{ + ? ? ? dev_dbg(&spi->dev, "cleanup\n"); +} + +static int __init spi_tegra_probe(struct platform_device *pdev) +{ + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct spi_tegra_data ? *tspi; + ? ? ? struct resource ? ? ? ? *r; + ? ? ? int ret; + + ? ? ? master = spi_alloc_master(&pdev->dev, sizeof *tspi); + ? ? ? if (master == NULL) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "master allocation failed\n"); + ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? } + + ? ? ? /* the spi->mode bits understood by this driver: */ + ? ? ? master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH; + + ? ? ? if (pdev->id != -1) + ? ? ? ? ? ? ? master->bus_num = pdev->id; + + ? ? ? master->setup = spi_tegra_setup; + ? ? ? master->transfer = spi_tegra_transfer; + ? ? ? master->cleanup = spi_tegra_cleanup; + ? ? ? master->num_chipselect = 4; + + ? ? ? dev_set_drvdata(&pdev->dev, master); + ? ? ? tspi = spi_master_get_devdata(master); + ? ? ? tspi->master = master; + ? ? ? tspi->pdev = pdev; + ? ? ? spin_lock_init(&tspi->lock); + + ? ? ? r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ? ? ? if (r == NULL) { + ? ? ? ? ? ? ? ret = -ENODEV; + ? ? ? ? ? ? ? goto err0; + ? ? ? } + + ? ? ? tspi->phys = r->start; + ? ? ? tspi->base = ioremap(r->start, r->end - r->start + 1); + ? ? ? if (!tspi->base) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can't ioremap iomem\n"); + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto err1; + ? ? ? } + + ? ? ? tspi->clk = clk_get(&pdev->dev, NULL);
drivers/spi/spi_tegra.c line 547
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? if (IS_ERR_OR_NULL(tspi->clk)) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not get clock\n"); + ? ? ? ? ? ? ? ret = PTR_ERR(tspi->clk); + ? ? ? ? ? ? ? goto err2; + ? ? ? } + + ? ? ? INIT_LIST_HEAD(&tspi->queue); + + ? ? ? tspi->rx_dma = tegra_dma_allocate_channel(TEGRA_DMA_MODE_ONESHOT); + ? ? ? if (IS_ERR(tspi->rx_dma)) {
As an aside, and commenting on the tegra DMA api... Is the IS_ERR() pattern in the tegra_dma_allocate_channel() return really a good idea here? Personally I find that pattern isn't very useful, especially when the results is either "yes, allocated" or "no, not-alocated", and it is very easy to write code that tests the wrong thing on the return value because the compiler cannot catch it. In other words, if it isn't vital to return a specific error code, the using the PTR_ERR() pattern adds unnecessary risk to the code because developers are used to seeing and writing this pattern:
tspi->rx_dma = tegra_dma_allocate_channel(TEGRA_DMA_MODE_ONESHOT); if (!tspi->rx_dma)
return -ENOMEM;
The compiler won't catch it if the return value is an PTR_ERR.
(I know that some subsystems make heavy use of it, particularly filesystems and the block layer which do need to return specific error codes to userspace. My argument is that the specific error code is probably irrelevant when allocating DMA channels)
Quoted 31 lines
drivers/spi/spi_tegra.c new file +31
-0+ ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not allocate rx dma channel\n"); + ? ? ? ? ? ? ? ret = PTR_ERR(tspi->rx_dma); + ? ? ? ? ? ? ? goto err3; + ? ? ? } + + ? ? ? tspi->rx_bb = dma_alloc_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&tspi->rx_bb_phys, GFP_KERNEL); + ? ? ? if (!tspi->rx_bb) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not allocate rx bounce buffer\n"); + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto err4; + ? ? ? } + + ? ? ? tspi->rx_dma_req.complete = tegra_spi_rx_dma_complete; + ? ? ? tspi->rx_dma_req.to_memory = 1; + ? ? ? tspi->rx_dma_req.dest_addr = tspi->rx_bb_phys; + ? ? ? tspi->rx_dma_req.dest_bus_width = 32; + ? ? ? tspi->rx_dma_req.source_addr = tspi->phys + SLINK_RX_FIFO; + ? ? ? tspi->rx_dma_req.source_bus_width = 32; + ? ? ? tspi->rx_dma_req.source_wrap = 4; + ? ? ? tspi->rx_dma_req.req_sel = spi_tegra_req_sels[pdev->id]; + ? ? ? tspi->rx_dma_req.dev = tspi; + + ? ? ? ret = spi_register_master(master); + + ? ? ? if (ret < 0) + ? ? ? ? ? ? ? goto err5; + + ? ? ? return ret; + +err5:
drivers/spi/spi_tegra.c line 588
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? dma_free_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? tspi->rx_bb, tspi->rx_bb_phys); +err4: + ? ? ? tegra_dma_free_channel(tspi->rx_dma); +err3: + ? ? ? clk_put(tspi->clk); +err2: + ? ? ? iounmap(tspi->base); +err1: + ? ? ? release_mem_region(r->start, (r->end - r->start) + 1);
Since this version doesn't request_mem_region(), this release is dangling. (However, as rmk pointed out the request_mem_region() was completely fine and the driver should continue to use it. Sorry for the noise).
Quoted 31 lines
drivers/spi/spi_tegra.c new file +31
-0+err0: + ? ? ? spi_master_put(master); + ? ? ? return ret; +} + +static int __devexit spi_tegra_remove(struct platform_device *pdev) +{ + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct spi_tegra_data ? *tspi; + + ? ? ? master = dev_get_drvdata(&pdev->dev); + ? ? ? tspi = spi_master_get_devdata(master); + + ? ? ? tegra_dma_free_channel(tspi->rx_dma); + + ? ? ? dma_free_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? tspi->rx_bb, tspi->rx_bb_phys); + + ? ? ? clk_put(tspi->clk); + ? ? ? iounmap(tspi->base); + + ? ? ? spi_master_put(master); + + ? ? ? return 0; +} + +MODULE_ALIAS ("platform:spi_tegra" ); + +static struct platform_driver spi_tegra_driver = { + ? ? ? .driver = { + ? ? ? ? ? ? ? .name = ? ? ? ? "spi_tegra",
drivers/spi/spi_tegra.c line 630
Quoted 10 lines
drivers/spi/spi_tegra.c new file +10
-0+ ? ? ? ? ? ? ? .owner = ? ? ? ?THIS_MODULE, + ? ? ? }, + ? ? ? .remove = ? ? ? __devexit_p(spi_tegra_remove), +}; + +static int __init spi_tegra_init (void ) +{ + ? ? ? return platform_driver_probe(&spi_tegra_driver, spi_tegra_probe); +} +subsys_initcall (spi_tegra_init);
Unless there is a specific reason that this driver needs to be initialized earlier, it should use the module_init() call. In which case the reason it is initialized early should be documented with a comment. (for reference: 4 of the mainlined spi bus drivers use subsys_initcall() whereas 30 of them use module_init().. 1 uses device_initcall() which is the same as module_init. None of the ones using subsys_initcall() give a reason why.)
g.
PATCH v2 1/1 spi: add spi_tegra driver
Quoted 2 lines · Grant Likely
Missing patch description. ?Give reviewers and future readers some more details about what hardware this patch adds support for.
I'll add one
Quoted 3 lines
Quoted 1 line
+#undef DEBUG
Still need to trim this #undef
Done.
Quoted 3 lines
It's a good idea to prefix all these register #defines with something driver specific to avoid the possibility of collisions with the global namespace.
I'll add SLINK_ to all the bit definitions.
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6
-0+static inline unsigned bytes_per_word(u8 bits) +{ + ? ? ? WARN_ON((bits < 1) || bits > 32); + ? ? ? if (bits <= 8) + ? ? ? ? ? ? ? return 1; + ? ? ? else if (bits <= 16)
Quoted 8 lines
Quoted 6 lines
+ ? ? ? ? ? ? ? return 2;
+ ? ? ? else if (bits <= 24)
+ ? ? ? ? ? ? ? return 3;
+ ? ? ? else
+ ? ? ? ? ? ? ? return 4;
+}No longer used, can be removed.
done
Quoted 7 lines
drivers/spi/spi_tegra.c line 190
Quoted 5 lines
drivers/spi/spi_tegra.c new file +5
-0+static unsigned long spi_tegra_readl (struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long reg) +{ + ? ? ? return readl(tspi->base + reg); +}static inline
done
Quoted 8 lines
drivers/spi/spi_tegra.c line 196
Quoted 6 lines
drivers/spi/spi_tegra.c new file +6
-0+ +static void spi_tegra_writel (struct spi_tegra_data *tspi,unsigned long val, + ? ? ? ? ? ? ? ? ? ? ?unsigned long reg) +{ + ? ? ? writel(val, tspi->base + reg); +}ditto
done
Quoted 17 lines
Quoted 5 lines
+ ? ? ? val =
spi_tegra_readl(tspi, SLINK_COMMAND2);
+ ? ? ? if (t->rx_buf)
+ ? ? ? ? ? ? ? val |= RXEN;
+ ? ? ? else
+ ? ? ? ? ? ? ? val &= ~RXEN;Review
Quoted 9 lines
(quoted diff) +9
-0+ + ? ? ? if (t->tx_buf) + ? ? ? ? ? ? ? val |= TXEN; + ? ? ? else + ? ? ? ? ? ? ? val &= ~TXEN; + + ? ? ? val &= ~SS_EN_CS(~0); + ? ? ? val |= SS_EN_CS(spi->chip_select); + ? ? ? val |= SPIE;Nit: This would be more concise:
Quoted 7 lines
+ ? ? ? val &= ~(SS_EN_CS(~0) | RXEN | TXEN);
+ ? ? ? if (t->rx_buf)
+ ? ? ? ? ? ? ? val |= RXEN;
+ ? ? ? if (t->tx_buf)
+ ? ? ? ? ? ? ? val |= TXEN;
+ ? ? ? val |=SS_EN_CS(spi->chip_select);
+ ? ? ? val |= SPIE;
done
Quoted 7 lines
Quoted 4 lines
+ ? ? ? if (
spi->mode&SPI_CPOL)
+ ? ? ? ? ? ? ? val |=IDLE_SCLK_DRIVE_HIGH;
+ ? ? ? else
+ ? ? ? ? ? ? ? val |=IDLE_SCLK_DRIVE_LOW;nit:
IDLE_SCLK_DRIVE_LOW== 0. ?The driver will be more concise if the above 2 lines are dropped.
I did it this way because it's not obvious from this code which sets the bit and which does not without decoding the #defines. I expect the complier optimizes this out.
Quoted 8 lines
drivers/spi/spi_tegra.c
Quoted 4 lines
drivers/spi/spi_tegra.c new file +4
-0+ + ? ? ? val |= M_S; + + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND);The read/modify/write register blocks are quite common. ?Would it be clearer and more concise to have a
spi_tegra_clrsetbits()static inline?
More concise, yes. Clearer? I don't think so. It seems that clrsetbits is mostly used in ppc code.
Quoted 11 lines
Quoted 9 lines
+ ? ? ? /* the SPI controller may come back with both the BSY and RDY bits
+ ? ? ? ?* set. ?In this case we need to wait for the BSY bit to clear so
+ ? ? ? ?* that we are sure the DMA is finished. ?1000 reads was empirically
+ ? ? ? ?* determined to be long enough.
+ ? ? ? ?*/
+ ? ? ? while (timeout++ < 1000) {
+ ? ? ? ? ? ? ? if (!(spi_tegra_readl(tspi,SLINK_STATUS) & BSY))
+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? }What happens if the BSY bit is still set when the loop exits?
Given how I've seen the hardware act, I don't think it's possible for BSY to still be set after 1000 reads. I forget the average number of loops but 1000 was very conservative. Worst case some of the data might be corrupt. I'll put an error message in and set status to -EIO.
Quoted 7 lines
Quoted 4 lines
+
+ ? ? ? val =spi_tegra_readl(tspi, SLINK_STATUS);
+ ? ? ? val |= RDY;
+ ? ? ?spi_tegra_writel(tspi, val, SLINK_STATUS);The spin lock isn't held at this point. ?Will that make this read-modify-write operation risky?
move spinlock up.
Quoted 5 lines
Quoted 2 lines
+
+ ? ? ?spin_lock_irqsave(&tspi->lock, flags);Is there any condition/event which could cause the spi controller to go busy before obtaining the spinlock?
No. The controller only goes busy when a transaction is started by calling spi_tegra_go().
Quoted 17 lines
Quoted 17 lines
(quoted diff) +17
-0+ + ? ? ? m = list_first_entry(&tspi->queue, struct spi_message, queue); + ? ? ? spi = m->state; + + ? ? ? tspi->cur_pos += spi_tegra_drain_rx_fifo(tspi, tspi->cur); + ? ? ? m->actual_length += tspi->cur_pos; + + ? ? ? if (tspi->cur_pos < tspi->cur->len) { + ? ? ? ? ? ? ? tspi->cur_len = spi_tegra_fill_tx_fifo(tspi, tspi->cur); + ? ? ? ? ? ? ? spi_tegra_go(tspi); + ? ? ? } else if (!list_is_last(&tspi->cur->transfer_list, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&m->transfers)) { + ? ? ? ? ? ? ? tspi->cur = ?list_first_entry(&tspi->cur->transfer_list, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? transfer_list); + ? ? ? ? ? ? ? spi_tegra_start_transfer(spi, tspi->cur); + ? ? ? } else {
Quoted 8 lines
Quoted 4 lines
+ ? ? ? ? ? ? ? complete = 1;
+ ? ? ? }
+
+ ? ? ? if (complete) {Looks like the above else clause is the only thing that can set complete. ?You could delete the above three lines to get the same behaviour.
deleted
Quoted 18 lines
Quoted 18 lines
+ ? ? ? ? ? ? ?
list_del(&m->queue);(quoted diff) +15
-0+ + ? ? ? ? ? ? ? m->status = 0; + ? ? ? ? ? ? ? m->complete(m->context); + ? ? ? switch (spi->chip_select) { + ? ? ? case 0: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY; + ? ? ? ? ? ? ? break; + + ? ? ? case 1: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY1; + ? ? ? ? ? ? ? break; + + ? ? ? case 2: + ? ? ? ? ? ? ? cs_bit = CS_POLARITY2; + ? ? ? ? ? ? ? break;
Quoted 8 lines
Quoted 4 lines
+
+ ? ? ? case 4:
+ ? ? ? ? ? ? ?cs_bit=CS_POLARITY3;
+ ? ? ? ? ? ? ? break;
CS_POLARITYis named a little oddly as it seems to indicate the bit, not the polarity of the bit. ?Would this better be named CS_BIT0..CS_BIT3?
That's how Nvidia named them in the TRM.
Quoted 17 lines
Quoted 2 lines
+ ? ? ?
tspi->rx_dma=tegra_dma_allocate_channel(TEGRA_DMA_MODE_ONESHOT);
+ ? ? ? if (IS_ERR(tspi->rx_dma)) {As an aside, and commenting on the tegra DMA api... Is the
IS_ERR()pattern in thetegra_dma_allocate_channel()return really a good idea here? ?Personally I find that pattern isn't very useful, especially when the results is either "yes, allocated" or "no, not-alocated", and it is very easy to write code that tests the wrong thing on the return value because the compiler cannot catch it. ?In other words, if it isn't vital to return a specific error code, the using thePTR_ERR()pattern adds unnecessary risk to the code because developers are used to seeing and writing this pattern:
tspi->rx_dma=tegra_dma_allocate_channel(TEGRA_DMA_MODE_ONESHOT); if (!tspi->rx_dma)
? ? ? ?return-ENOMEM;
Quoted 6 lines
The compiler won't catch it if the return value is an
PTR_ERR.(I know that some subsystems make heavy use of it, particularly filesystems and the block layer which do need to return specific error codes to userspace. ?My argument is that the specific error code is probably irrelevant when allocating DMA channels)
I don't have a strong opinion either way but that's the way Colin's code is written. I'll let him speak to that.
Quoted 6 lines
drivers/spi/spi_tegra.c line 630
Quoted 5 lines
drivers/spi/spi_tegra.c new file +5
-0+static int __init spi_tegra_init (void ) +{ + ? ? ? return platform_driver_probe(&spi_tegra_driver, spi_tegra_probe); +} +subsys_initcall (spi_tegra_init);
Quoted 7 lines
Unless there is a specific reason that this driver needs to be initialized earlier, it should use the
module_init()call. ?In which case the reason it is initialized early should be documented with a comment. ?(for reference: 4 of the mainlined spi bus drivers usesubsys_initcall()whereas 30 of them usemodule_init().. ?1 usesdevice_initcall()which is the same asmodule_init. ?None of the ones usingsubsys_initcall()give a reason why.)
heh.. I missed the module_inits because i grepped for initcall. changed to module_init.
patch incoming.
-Erik
PATCH v2 1/1 spi: add spi_tegra driver
Quoted 16 lines · Erik Gilling
Quoted 16 lines
As an aside, and commenting on the tegra DMA api... Is the
IS_ERR()pattern in thetegra_dma_allocate_channel()return really a good idea here? ?Personally I find that pattern isn't very useful, especially when the results is either "yes, allocated" or "no, not-alocated", and it is very easy to write code that tests the wrong thing on the return value because the compiler cannot catch it. ?In other words, if it isn't vital to return a specific error code, the using thePTR_ERR()pattern adds unnecessary risk to the code because developers are used to seeing and writing this pattern:
tspi->rx_dma=tegra_dma_allocate_channel(TEGRA_DMA_MODE_ONESHOT); if (!tspi->rx_dma)
? ? ? ?return-ENOMEM;The compiler won't catch it if the return value is an
PTR_ERR.
Quoted 7 lines
Quoted 4 lines
(I know that some subsystems make heavy use of it, particularly filesystems and the block layer which do need to return specific error codes to userspace. ?My argument is that the specific error code is probably irrelevant when allocating DMA channels)
I don't have a strong opinion either way but that's the way Colin's code is written. ?I'll let him speak to that.
I spoke with colin about this. He's fine getting rid of the PTR_ERR. I'll post a DMA and SPI patch when that's done.
-Erik
PATCH v2 1/1 spi: add spi_tegra driver
Quoted 22 lines · Erik Gilling
Quoted 21 lines · Erik Gilling
Quoted 20 lines
As an aside, and commenting on the tegra DMA api... Is the
IS_ERR()pattern in thetegra_dma_allocate_channel()return really a good idea here? ?Personally I find that pattern isn't very useful, especially when the results is either "yes, allocated" or "no, not-alocated", and it is very easy to write code that tests the wrong thing on the return value because the compiler cannot catch it. ?In other words, if it isn't vital to return a specific error code, the using thePTR_ERR()pattern adds unnecessary risk to the code because developers are used to seeing and writing this pattern:
tspi->rx_dma=tegra_dma_allocate_channel(TEGRA_DMA_MODE_ONESHOT); if (!tspi->rx_dma)
? ? ? ?return-ENOMEM;The compiler won't catch it if the return value is an
PTR_ERR.(I know that some subsystems make heavy use of it, particularly filesystems and the block layer which do need to return specific error codes to userspace. ?My argument is that the specific error code is probably irrelevant when allocating DMA channels)
Quoted 5 lines
Quoted 2 lines
I don't have a strong opinion either way but that's the way Colin's code is written. ?I'll let him speak to that.
I spoke with colin about this. ?He's fine getting rid of the
PTR_ERR. I'll post a DMA and SPI patch when that's done.
Okay, thanks.
g.
PATCH v3 1/1 spi: add spi_tegra driver
In reply to Erik Gillingv2 changes:
from Thierry Reding:
* add "select TEGRA_SYSTEM_DMA" to Kconfig
from Grant Likely:
* add oneline description to header
* inline references to DRIVER_NAME
* inline references to BUSY_TIMEOUT
* open coded bytes_per_word()
* spi_readl/writel -> spi_tegra_readl/writel
* move transfer validation to spi_tegra_transfer
* don't request_mem_region iomem as platform bus does that for us
* __exit -> __devexit
v3 changes:
from Russell King:
* put request_mem_region back int
from Grant Likely:
* remove #undef DEBUG
* add SLINK_ to register bit defines
* remove unused bytes_per_word
* make spi_tegra_readl/writel static linine
* various refactoring for clarity
* mark err if BSY bit is not cleared after 1000 retries
* move spinlock to protect setting of RDY bit
* subsys_initcall -> module_init
--- drivers/spi/Kconfig | 7 + drivers/spi/Makefile | 1 + drivers/spi/spi_tegra.c | 625 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 633 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/spi_tegra.c
drivers/spi/Kconfig +7 -0
@@ -298,6 +298,13 @@ config SPI_STMP3XXX help SPI driver for Freescale STMP37xx/378x SoC SSP interface +config SPI_TEGRA + tristate "Nvidia Tegra SPI controller" + depends on ARCH_TEGRA + select TEGRA_SYSTEM_DMA + help + SPI driver for NVidia Tegra SoCs + config SPI_TXX9 tristate "Toshiba TXx9 SPI controller" depends on GENERIC_GPIO && CPU_TX49XX
drivers/spi/Makefile +1 -0
@@ -39,6 +39,7 @@ obj-$(CONFIG_SPI_PPC4xx) += spi_ppc4xx.o obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx_hw.o obj-$(CONFIG_SPI_S3C64XX) += spi_s3c64xx.o +obj-$(CONFIG_SPI_TEGRA) += spi_tegra.o obj-$(CONFIG_SPI_TXX9) += spi_txx9.o obj-$(CONFIG_SPI_XILINX) += xilinx_spi.o obj-$(CONFIG_SPI_XILINX_OF) += xilinx_spi_of.o
drivers/spi/spi_tegra.c new file +625 -0
625 lines not on this page · raw message
Signature · 1 line
1.6.5.6
PATCH v4 1/1 spi: add spi_tegra driver
v2 changes:
from Thierry Reding:
* add "select TEGRA_SYSTEM_DMA" to Kconfig
from Grant Likely:
* add oneline description to header
* inline references to DRIVER_NAME
* inline references to BUSY_TIMEOUT
* open coded bytes_per_word()
* spi_readl/writel -> spi_tegra_readl/writel
* move transfer validation to spi_tegra_transfer
* don't request_mem_region iomem as platform bus does that for us
* __exit -> __devexit
v3 changes:
from Russell King:
* put request_mem_region back int
from Grant Likely:
* remove #undef DEBUG
* add SLINK_ to register bit defines
* remove unused bytes_per_word
* make spi_tegra_readl/writel static linine
* various refactoring for clarity
* mark err if BSY bit is not cleared after 1000 retries
* move spinlock to protect setting of RDY bit
* subsys_initcall -> module_init
v3 changes:
from Grant Likely:
* update spi_tegra to use PTR_ERRless dma API
--- drivers/spi/Kconfig | 7 + drivers/spi/Makefile | 1 + drivers/spi/spi_tegra.c | 625 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 633 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/spi_tegra.c
drivers/spi/Kconfig +7 -0
@@ -298,6 +298,13 @@ config SPI_STMP3XXX help SPI driver for Freescale STMP37xx/378x SoC SSP interface +config SPI_TEGRA + tristate "Nvidia Tegra SPI controller" + depends on ARCH_TEGRA + select TEGRA_SYSTEM_DMA + help + SPI driver for NVidia Tegra SoCs + config SPI_TXX9 tristate "Toshiba TXx9 SPI controller" depends on GENERIC_GPIO && CPU_TX49XX
drivers/spi/Makefile +1 -0
@@ -39,6 +39,7 @@ obj-$(CONFIG_SPI_PPC4xx) += spi_ppc4xx.o obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx_hw.o obj-$(CONFIG_SPI_S3C64XX) += spi_s3c64xx.o +obj-$(CONFIG_SPI_TEGRA) += spi_tegra.o obj-$(CONFIG_SPI_TXX9) += spi_txx9.o obj-$(CONFIG_SPI_XILINX) += xilinx_spi.o obj-$(CONFIG_SPI_XILINX_OF) += xilinx_spi_of.o
drivers/spi/spi_tegra.c new file +625 -0
625 lines not on this page · raw message
Signature · 1 line
1.6.5.6
PATCH v4 1/1 spi: add spi_tegra driver
Grant,
The DMA support is now in the tegra for-next tree. Please add
this driver to your spi-next tree.
Thanks,
Erik
Quoted 706 lines · Erik Gilling
v2 changes:
?from Thierry Reding:
? ?* add "selectTEGRA_SYSTEM_DMA" toKconfig
?from Grant Likely:
? ?* add oneline description to header
? ?* inline references toDRIVER_NAME
? ?* inline references toBUSY_TIMEOUT
? ?* open codedbytes_per_word()
? ?* spi_readl/writel -> spi_tegra_readl/writel
? ?* move transfer validation tospi_tegra_transfer
? ?* don'trequest_mem_regioniomem as platform bus does that for us ? ?*__exit->__devexitv3 changes:
?from Russell King:
? ?* putrequest_mem_regionback int
?from Grant Likely:
? ?* remove #undef DEBUG
? ?* addSLINK_to register bit defines
? ?* remove unusedbytes_per_word
? ?* make spi_tegra_readl/writel static linine
? ?* various refactoring for clarity
? ?* mark err if BSY bit is not cleared after 1000 retries ? ?* move spinlock to protect setting of RDY bit ? ?*subsys_initcall->module_initv3 changes:
?from Grant Likely:
? ?* updatespi_tegrato usePTR_ERRlessdma APISigned-off-by Erik Gilling <[email protected]>Cc Thierry Reding <[email protected]>Cc Grant Likely <[email protected]>Cc Russell King <[email protected]>---
?drivers/spi/Kconfig ? ? | ? ?7 + ?drivers/spi/Makefile ? ?| ? ?1 + ?drivers/spi/spi_tegra.c | ?625 +++++++++++++++++++++++++++++++++++++++++++++++?3 files changed, 633 insertions(+), 0 deletions(-) ?create mode 100644
drivers/spi/spi_tegra.cdrivers/spi/Kconfig +0
-0@@ -298,6 +298,13 @@ config SPI_STMP3XXX? ? ? ?help
? ? ? ? ?SPI driver for Freescale STMP37xx/378x SoC SSP interfacedrivers/spi/Kconfig +7
-0+config SPI_TEGRA + ? ? ? tristate "Nvidia Tegra SPI controller" + ? ? ? depends on ARCH_TEGRA + ? ? ? select TEGRA_SYSTEM_DMA + ? ? ? help + ? ? ? ? SPI driver for NVidia Tegra SoCs +?config
SPI_TXX9
? ? ? ?tristate "Toshiba TXx9 SPI controller"
? ? ? ?depends onGENERIC_GPIO&&CPU_TX49XXdrivers/spi/Makefile +0
-0@@ -39,6 +39,7 @@ obj-$(CONFIG_SPI_PPC4xx) ? ? ? ? ? ? ?+= spi_ppc4xx.o?obj-$(CONFIG_SPI_S3C24XX_GPIO) ? ? ? ? += spi_s3c24xx_gpio.o ?obj-$(CONFIG_SPI_S3C24XX) ? ? ? ? ? ? ?+= spi_s3c24xx_hw.o ?obj-$(CONFIG_SPI_S3C64XX) ? ? ? ? ? ? ?+= spi_s3c64xx.o +obj-$(CONFIG_SPI_TEGRA) ? ? ? ? ? ? ? ? ? ? ? ?+= spi_tegra.o ?obj-$(CONFIG_SPI_TXX9) ? ? ? ? ? ? ? ? += spi_txx9.o ?obj-$(CONFIG_SPI_XILINX) ? ? ? ? ? ? ? += xilinx_spi.o ?obj-$(CONFIG_SPI_XILINX_OF) ? ? ? ? ? ?+= xilinx_spi_of.o
drivers/spi/spi_tegra.c new file +625
-1@@ -0,0 +1,625 @@ +/* + * Driver for Nvidia TEGRA spi controller. + * + * Copyright (C) 2010 Google, Inc. + * + * Author: + * ? ? Erik Gilling <[email protected]> + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the + * GNU General Public License for more details. + * + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/dma-mapping.h> +#include <linux/dmapool.h> +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/delay.h> + +#include <linux/spi/spi.h> + +#include <mach/dma.h> + +#define SLINK_COMMAND ? ? ? ? ?0x000 +#define ? SLINK_BIT_LENGTH(x) ? ? ? ? ?(((x) & 0x1f) << 0) +#define ? SLINK_WORD_SIZE(x) ? ? ? ? ? (((x) & 0x1f) << 5) +#define ? SLINK_BOTH_EN ? ? ? ? ? ? ? ? ? ? ? ?(1 << 10) +#define ? SLINK_CS_SW ? ? ? ? ? ? ? ? ?(1 << 11) +#define ? SLINK_CS_VALUE ? ? ? ? ? ? ? (1 << 12) +#define ? SLINK_CS_POLARITY ? ? ? ? ? ?(1 << 13) +#define ? SLINK_IDLE_SDA_DRIVE_LOW ? ? (0 << 16) +#define ? SLINK_IDLE_SDA_DRIVE_HIGH ? ?(1 << 16) +#define ? SLINK_IDLE_SDA_PULL_LOW ? ? ?(2 << 16) +#define ? SLINK_IDLE_SDA_PULL_HIGH ? ? (3 << 16) +#define ? SLINK_IDLE_SDA_MASK ? ? ? ? ?(3 << 16) +#define ? SLINK_CS_POLARITY1 ? ? ? ? ? (1 << 20) +#define ? SLINK_CK_SDA ? ? ? ? ? ? ? ? (1 << 21) +#define ? SLINK_CS_POLARITY2 ? ? ? ? ? (1 << 22) +#define ? SLINK_CS_POLARITY3 ? ? ? ? ? (1 << 23) +#define ? SLINK_IDLE_SCLK_DRIVE_LOW ? ?(0 << 24) +#define ? SLINK_IDLE_SCLK_DRIVE_HIGH ? (1 << 24) +#define ? SLINK_IDLE_SCLK_PULL_LOW ? ? (2 << 24) +#define ? SLINK_IDLE_SCLK_PULL_HIGH ? ?(3 << 24) +#define ? SLINK_IDLE_SCLK_MASK ? ? ? ? (3 << 24) +#define ? SLINK_M_S ? ? ? ? ? ? ? ? ? ?(1 << 28) +#define ? SLINK_WAIT ? ? ? ? ? ? ? ? ? (1 << 29) +#define ? SLINK_GO ? ? ? ? ? ? ? ? ? ? (1 << 30) +#define ? SLINK_ENB ? ? ? ? ? ? ? ? ? ?(1 << 31) + +#define SLINK_COMMAND2 ? ? ? ? 0x004 +#define ? SLINK_LSBFE ? ? ? ? ? ? ? ? ?(1 << 0) +#define ? SLINK_SSOE ? ? ? ? ? ? ? ? ? (1 << 1) +#define ? SLINK_SPIE ? ? ? ? ? ? ? ? ? (1 << 4) +#define ? SLINK_BIDIROE ? ? ? ? ? ? ? ? ? ? ? ?(1 << 6) +#define ? SLINK_MODFEN ? ? ? ? ? ? ? ? (1 << 7) +#define ? SLINK_INT_SIZE(x) ? ? ? ? ? ?(((x) & 0x1f) << 8) +#define ? SLINK_CS_ACTIVE_BETWEEN ? ? ?(1 << 17) +#define ? SLINK_SS_EN_CS(x) ? ? ? ? ? ?(((x) & 0x3) << 18) +#define ? SLINK_SS_SETUP(x) ? ? ? ? ? ?(((x) & 0x3) << 20) +#define ? SLINK_FIFO_REFILLS_0 ? ? ? ? (0 << 22) +#define ? SLINK_FIFO_REFILLS_1 ? ? ? ? (1 << 22) +#define ? SLINK_FIFO_REFILLS_2 ? ? ? ? (2 << 22) +#define ? SLINK_FIFO_REFILLS_3 ? ? ? ? (3 << 22) +#define ? SLINK_FIFO_REFILLS_MASK ? ? ?(3 << 22) +#define ? SLINK_WAIT_PACK_INT(x) ? ? ? (((x) & 0x7) << 26) +#define ? SLINK_SPC0 ? ? ? ? ? ? ? ? ? (1 << 29) +#define ? SLINK_TXEN ? ? ? ? ? ? ? ? ? (1 << 30) +#define ? SLINK_RXEN ? ? ? ? ? ? ? ? ? (1 << 31) + +#define SLINK_STATUS ? ? ? ? ? 0x008 +#define ? SLINK_COUNT(val) ? ? ? ? ? ? (((val) >> 0) & 0x1f) +#define ? SLINK_WORD(val) ? ? ? ? ? ? ?(((val) >> 5) & 0x1f) +#define ? SLINK_BLK_CNT(val) ? ? ? ? ? (((val) >> 0) & 0xffff) +#define ? SLINK_MODF ? ? ? ? ? ? ? ? ? (1 << 16) +#define ? SLINK_RX_UNF ? ? ? ? ? ? ? ? (1 << 18) +#define ? SLINK_TX_OVF ? ? ? ? ? ? ? ? (1 << 19) +#define ? SLINK_TX_FULL ? ? ? ? ? ? ? ? ? ? ? ?(1 << 20) +#define ? SLINK_TX_EMPTY ? ? ? ? ? ? ? (1 << 21) +#define ? SLINK_RX_FULL ? ? ? ? ? ? ? ? ? ? ? ?(1 << 22) +#define ? SLINK_RX_EMPTY ? ? ? ? ? ? ? (1 << 23) +#define ? SLINK_TX_UNF ? ? ? ? ? ? ? ? (1 << 24) +#define ? SLINK_RX_OVF ? ? ? ? ? ? ? ? (1 << 25) +#define ? SLINK_TX_FLUSH ? ? ? ? ? ? ? (1 << 26) +#define ? SLINK_RX_FLUSH ? ? ? ? ? ? ? (1 << 27) +#define ? SLINK_SCLK ? ? ? ? ? ? ? ? ? (1 << 28) +#define ? SLINK_ERR ? ? ? ? ? ? ? ? ? ?(1 << 29) +#define ? SLINK_RDY ? ? ? ? ? ? ? ? ? ?(1 << 30) +#define ? SLINK_BSY ? ? ? ? ? ? ? ? ? ?(1 << 31) + +#define SLINK_MAS_DATA ? ? ? ? 0x010 +#define SLINK_SLAVE_DATA ? ? ? 0x014 + +#define SLINK_DMA_CTL ? ? ? ? ?0x018 +#define ? SLINK_DMA_BLOCK_SIZE(x) ? ? ?(((x) & 0xffff) << 0) +#define ? SLINK_TX_TRIG_1 ? ? ? ? ? ? ?(0 << 16) +#define ? SLINK_TX_TRIG_4 ? ? ? ? ? ? ?(1 << 16) +#define ? SLINK_TX_TRIG_8 ? ? ? ? ? ? ?(2 << 16) +#define ? SLINK_TX_TRIG_16 ? ? ? ? ? ? (3 << 16) +#define ? SLINK_TX_TRIG_MASK ? ? ? ? ? (3 << 16) +#define ? SLINK_RX_TRIG_1 ? ? ? ? ? ? ?(0 << 18) +#define ? SLINK_RX_TRIG_4 ? ? ? ? ? ? ?(1 << 18) +#define ? SLINK_RX_TRIG_8 ? ? ? ? ? ? ?(2 << 18) +#define ? SLINK_RX_TRIG_16 ? ? ? ? ? ? (3 << 18) +#define ? SLINK_RX_TRIG_MASK ? ? ? ? ? (3 << 18) +#define ? SLINK_PACKED ? ? ? ? ? ? ? ? (1 << 20) +#define ? SLINK_PACK_SIZE_4 ? ? ? ? ? ?(0 << 21) +#define ? SLINK_PACK_SIZE_8 ? ? ? ? ? ?(1 << 21) +#define ? SLINK_PACK_SIZE_16 ? ? ? ? ? (2 << 21) +#define ? SLINK_PACK_SIZE_32 ? ? ? ? ? (3 << 21) +#define ? SLINK_PACK_SIZE_MASK ? ? ? ? (3 << 21) +#define ? SLINK_IE_TXC ? ? ? ? ? ? ? ? (1 << 26) +#define ? SLINK_IE_RXC ? ? ? ? ? ? ? ? (1 << 27) +#define ? SLINK_DMA_EN ? ? ? ? ? ? ? ? (1 << 31) + +#define SLINK_STATUS2 ? ? ? ? ?0x01c +#define ? SLINK_TX_FIFO_EMPTY_COUNT(val) ? ? ? (((val) & 0x3f) >> 0) +#define ? SLINK_RX_FIFO_FULL_COUNT(val) ? ? ? ? ? ? ? ?(((val) & 0x3f) >> 16) + +#define SLINK_TX_FIFO ? ? ? ? ?0x100 +#define SLINK_RX_FIFO ? ? ? ? ?0x180 + +static const unsigned long spi_tegra_req_sels[] = { + ? ? ? TEGRA_DMA_REQ_SEL_SL2B1, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B2, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B3, + ? ? ? TEGRA_DMA_REQ_SEL_SL2B4, +}; + +#define BB_LEN ? ? ? ? ? ? ? ? 32 + +struct spi_tegra_data { + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct platform_device ?*pdev; + ? ? ? spinlock_t ? ? ? ? ? ? ?lock; + + ? ? ? struct clk ? ? ? ? ? ? ?*clk; + ? ? ? void __iomem ? ? ? ? ? ?*base; + ? ? ? unsigned long ? ? ? ? ? phys; + + ? ? ? u32 ? ? ? ? ? ? ? ? ? ? cur_speed; + + ? ? ? struct list_head ? ? ? ?queue; + ? ? ? struct spi_transfer ? ? *cur; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_pos; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_len; + ? ? ? unsigned ? ? ? ? ? ? ? ?cur_bytes_per_word; + + ? ? ? /* The tegra spi controller has a bug which causes the first word + ? ? ? ?* in PIO transactions to be garbage. ?Since packed DMA transactions + ? ? ? ?* require transfers to be 4 byte aligned we need a bounce buffer + ? ? ? ?* for the generic case. + ? ? ? ?*/ + ? ? ? struct tegra_dma_req ? ?rx_dma_req; + ? ? ? struct tegra_dma_channel *rx_dma; + ? ? ? u32 ? ? ? ? ? ? ? ? ? ? *rx_bb; + ? ? ? dma_addr_t ? ? ? ? ? ? ?rx_bb_phys; +}; + + +static inline unsigned long spi_tegra_readl(struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned long reg) +{ + ? ? ? return readl(tspi->base + reg); +} + +static inline void spi_tegra_writel(struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned long val, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned long reg) +{ + ? ? ? writel(val, tspi->base + reg); +} + +static void spi_tegra_go(struct spi_tegra_data *tspi) +{ + ? ? ? unsigned long val; + + ? ? ? wmb(); + + ? ? ? val = spi_tegra_readl(tspi, SLINK_DMA_CTL); + ? ? ? val &= ~SLINK_DMA_BLOCK_SIZE(~0) & ~SLINK_DMA_EN; + ? ? ? val |= SLINK_DMA_BLOCK_SIZE(tspi->rx_dma_req.size / 4 - 1); + ? ? ? spi_tegra_writel(tspi, val, SLINK_DMA_CTL); + + ? ? ? tegra_dma_enqueue_req(tspi->rx_dma, &tspi->rx_dma_req); + + ? ? ? val |= SLINK_DMA_EN; + ? ? ? spi_tegra_writel(tspi, val, SLINK_DMA_CTL); +} + +static unsigned spi_tegra_fill_tx_fifo(struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? unsigned len = min(t->len - tspi->cur_pos, BB_LEN * + ? ? ? ? ? ? ? ? ? ? ? ? ?tspi->cur_bytes_per_word); + ? ? ? u8 *tx_buf = (u8 *)t->tx_buf + tspi->cur_pos; + ? ? ? int i, j; + ? ? ? unsigned long val; + + ? ? ? val = spi_tegra_readl(tspi, SLINK_COMMAND); + ? ? ? val &= ~SLINK_WORD_SIZE(~0); + ? ? ? val |= SLINK_WORD_SIZE(len / tspi->cur_bytes_per_word - 1); + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND); + + ? ? ? for (i = 0; i < len; i += tspi->cur_bytes_per_word) { + ? ? ? ? ? ? ? val = 0; + ? ? ? ? ? ? ? for (j = 0; j < tspi->cur_bytes_per_word; j++) + ? ? ? ? ? ? ? ? ? ? ? val |= tx_buf[i + j] << j * 8; + + ? ? ? ? ? ? ? spi_tegra_writel(tspi, val, SLINK_TX_FIFO); + ? ? ? } + + ? ? ? tspi->rx_dma_req.size = len / tspi->cur_bytes_per_word * 4; + + ? ? ? return len; +} + +static unsigned spi_tegra_drain_rx_fifo(struct spi_tegra_data *tspi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? unsigned len = tspi->cur_len; + ? ? ? u8 *rx_buf = (u8 *)t->rx_buf + tspi->cur_pos; + ? ? ? int i, j; + ? ? ? unsigned long val; + + ? ? ? for (i = 0; i < len; i += tspi->cur_bytes_per_word) { + ? ? ? ? ? ? ? val = tspi->rx_bb[i / tspi->cur_bytes_per_word]; + ? ? ? ? ? ? ? for (j = 0; j < tspi->cur_bytes_per_word; j++) + ? ? ? ? ? ? ? ? ? ? ? rx_buf[i + j] = (val >> (j * 8)) & 0xff; + ? ? ? } + + ? ? ? return len; +} + +static void spi_tegra_start_transfer(struct spi_device *spi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer *t) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? u32 speed; + ? ? ? u8 bits_per_word; + ? ? ? unsigned long val; + + ? ? ? speed = t->speed_hz ? t->speed_hz : spi->max_speed_hz; + ? ? ? bits_per_word = t->bits_per_word ? t->bits_per_word ?: + ? ? ? ? ? ? ? spi->bits_per_word; + + ? ? ? tspi->cur_bytes_per_word = (bits_per_word - 1) / 8 + 1; + + ? ? ? if (speed != tspi->cur_speed) + ? ? ? ? ? ? ? clk_set_rate(tspi->clk, speed); + + ? ? ? if (tspi->cur_speed == 0) + ? ? ? ? ? ? ? clk_enable(tspi->clk); + + ? ? ? tspi->cur_speed = speed; + + ? ? ? val = spi_tegra_readl(tspi, SLINK_COMMAND2); + ? ? ? val &= ~SLINK_SS_EN_CS(~0) | SLINK_RXEN | SLINK_TXEN; + ? ? ? if (t->rx_buf) + ? ? ? ? ? ? ? val |= SLINK_RXEN; + ? ? ? if (t->tx_buf) + ? ? ? ? ? ? ? val |= SLINK_TXEN; + ? ? ? val |= SLINK_SS_EN_CS(spi->chip_select); + ? ? ? val |= SLINK_SPIE; + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND2); + + ? ? ? val = spi_tegra_readl(tspi, SLINK_COMMAND); + ? ? ? val &= ~SLINK_BIT_LENGTH(~0); + ? ? ? val |= SLINK_BIT_LENGTH(bits_per_word - 1); + + ? ? ? /* FIXME: should probably control CS manually so that we can be sure + ? ? ? ?* it does not go low between transfer and to support delay_usecs + ? ? ? ?* correctly. + ? ? ? ?*/ + ? ? ? val &= ~SLINK_IDLE_SCLK_MASK & ~SLINK_CK_SDA & ~SLINK_CS_SW; + + ? ? ? if (spi->mode & SPI_CPHA) + ? ? ? ? ? ? ? val |= SLINK_CK_SDA; + + ? ? ? if (spi->mode & SPI_CPOL) + ? ? ? ? ? ? ? val |= SLINK_IDLE_SCLK_DRIVE_HIGH; + ? ? ? else + ? ? ? ? ? ? ? val |= SLINK_IDLE_SCLK_DRIVE_LOW; + + ? ? ? val |= SLINK_M_S; + + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND); + + ? ? ? spi_tegra_writel(tspi, SLINK_RX_FLUSH | SLINK_TX_FLUSH, SLINK_STATUS); + + ? ? ? tspi->cur = t; + ? ? ? tspi->cur_pos = 0; + ? ? ? tspi->cur_len = spi_tegra_fill_tx_fifo(tspi, t); + + ? ? ? spi_tegra_go(tspi); +} + +static void spi_tegra_start_message(struct spi_device *spi, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_message *m) +{ + ? ? ? struct spi_transfer *t; + + ? ? ? m->actual_length = 0; + ? ? ? m->status = 0; + + ? ? ? t = list_first_entry(&m->transfers, struct spi_transfer, transfer_list); + ? ? ? spi_tegra_start_transfer(spi, t); +} + +static void tegra_spi_rx_dma_complete(struct tegra_dma_req *req) +{ + ? ? ? struct spi_tegra_data *tspi = req->dev; + ? ? ? unsigned long flags; + ? ? ? struct spi_message *m; + ? ? ? struct spi_device *spi; + ? ? ? int timeout = 0; + ? ? ? unsigned long val; + + ? ? ? /* the SPI controller may come back with both the BSY and RDY bits + ? ? ? ?* set. ?In this case we need to wait for the BSY bit to clear so + ? ? ? ?* that we are sure the DMA is finished. ?1000 reads was empirically + ? ? ? ?* determined to be long enough. + ? ? ? ?*/ + ? ? ? while (timeout++ < 1000) { + ? ? ? ? ? ? ? if (!(spi_tegra_readl(tspi, SLINK_STATUS) & SLINK_BSY)) + ? ? ? ? ? ? ? ? ? ? ? break; + ? ? ? } + + ? ? ? spin_lock_irqsave(&tspi->lock, flags); + + ? ? ? if (timeout >= 1000) + ? ? ? ? ? ? ? m->status = -EIO; + + ? ? ? val = spi_tegra_readl(tspi, SLINK_STATUS); + ? ? ? val |= SLINK_RDY; + ? ? ? spi_tegra_writel(tspi, val, SLINK_STATUS); + + + ? ? ? m = list_first_entry(&tspi->queue, struct spi_message, queue); + ? ? ? spi = m->state; + + ? ? ? tspi->cur_pos += spi_tegra_drain_rx_fifo(tspi, tspi->cur); + ? ? ? m->actual_length += tspi->cur_pos; + + ? ? ? if (tspi->cur_pos < tspi->cur->len) { + ? ? ? ? ? ? ? tspi->cur_len = spi_tegra_fill_tx_fifo(tspi, tspi->cur); + ? ? ? ? ? ? ? spi_tegra_go(tspi); + ? ? ? } else if (!list_is_last(&tspi->cur->transfer_list, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&m->transfers)) { + ? ? ? ? ? ? ? tspi->cur = ?list_first_entry(&tspi->cur->transfer_list, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct spi_transfer, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? transfer_list); + ? ? ? ? ? ? ? spi_tegra_start_transfer(spi, tspi->cur); + ? ? ? } else { + ? ? ? ? ? ? ? list_del(&m->queue); + + ? ? ? ? ? ? ? m->complete(m->context); + + ? ? ? ? ? ? ? if (!list_empty(&tspi->queue)) { + ? ? ? ? ? ? ? ? ? ? ? m = list_first_entry(&tspi->queue, struct spi_message, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?queue); + ? ? ? ? ? ? ? ? ? ? ? spi = m->state; + ? ? ? ? ? ? ? ? ? ? ? spi_tegra_start_message(spi, m); + ? ? ? ? ? ? ? } else { + ? ? ? ? ? ? ? ? ? ? ? clk_disable(tspi->clk); + ? ? ? ? ? ? ? ? ? ? ? tspi->cur_speed = 0; + ? ? ? ? ? ? ? } + ? ? ? } + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); +} + +static int spi_tegra_setup(struct spi_device *spi) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? unsigned long cs_bit; + ? ? ? unsigned long val; + ? ? ? unsigned long flags; + + ? ? ? dev_dbg(&spi->dev, "setup %d bpw, %scpol, %scpha, %dHz\n", + ? ? ? ? ? ? ? spi->bits_per_word, + ? ? ? ? ? ? ? spi->mode & SPI_CPOL ? "" : "~", + ? ? ? ? ? ? ? spi->mode & SPI_CPHA ? "" : "~", + ? ? ? ? ? ? ? spi->max_speed_hz); + + + ? ? ? switch (spi->chip_select) { + ? ? ? case 0: + ? ? ? ? ? ? ? cs_bit = SLINK_CS_POLARITY; + ? ? ? ? ? ? ? break; + + ? ? ? case 1: + ? ? ? ? ? ? ? cs_bit = SLINK_CS_POLARITY1; + ? ? ? ? ? ? ? break; + + ? ? ? case 2: + ? ? ? ? ? ? ? cs_bit = SLINK_CS_POLARITY2; + ? ? ? ? ? ? ? break; + + ? ? ? case 4: + ? ? ? ? ? ? ? cs_bit = SLINK_CS_POLARITY3; + ? ? ? ? ? ? ? break; + + ? ? ? default: + ? ? ? ? ? ? ? return -EINVAL; + ? ? ? } + + ? ? ? spin_lock_irqsave(&tspi->lock, flags); + + ? ? ? val = spi_tegra_readl(tspi, SLINK_COMMAND); + ? ? ? if (spi->mode & SPI_CS_HIGH) + ? ? ? ? ? ? ? val |= cs_bit; + ? ? ? else + ? ? ? ? ? ? ? val &= ~cs_bit; + ? ? ? spi_tegra_writel(tspi, val, SLINK_COMMAND); + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); + + ? ? ? return 0; +} + +static int spi_tegra_transfer(struct spi_device *spi, struct spi_message *m) +{ + ? ? ? struct spi_tegra_data *tspi = spi_master_get_devdata(spi->master); + ? ? ? struct spi_transfer *t; + ? ? ? unsigned long flags; + ? ? ? int was_empty; + + ? ? ? if (list_empty(&m->transfers) || !m->complete) + ? ? ? ? ? ? ? return -EINVAL; + + ? ? ? list_for_each_entry(t, &m->transfers, transfer_list) { + ? ? ? ? ? ? ? if (t->bits_per_word < 0 || t->bits_per_word > 32) + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL; + + ? ? ? ? ? ? ? if (t->len == 0) + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL; + + ? ? ? ? ? ? ? if (!t->rx_buf && !t->tx_buf) + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL; + ? ? ? } + + ? ? ? m->state = spi; + + ? ? ? spin_lock_irqsave(&tspi->lock, flags); + ? ? ? was_empty = list_empty(&tspi->queue); + ? ? ? list_add_tail(&m->queue, &tspi->queue); + + ? ? ? if (was_empty) + ? ? ? ? ? ? ? spi_tegra_start_message(spi, m); + + ? ? ? spin_unlock_irqrestore(&tspi->lock, flags); + + ? ? ? return 0; +} + +static void spi_tegra_cleanup(struct spi_device *spi) +{ + ? ? ? dev_dbg(&spi->dev, "cleanup\n"); +} + +static int __init spi_tegra_probe(struct platform_device *pdev) +{ + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct spi_tegra_data ? *tspi; + ? ? ? struct resource ? ? ? ? *r; + ? ? ? int ret; + + ? ? ? master = spi_alloc_master(&pdev->dev, sizeof *tspi); + ? ? ? if (master == NULL) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "master allocation failed\n"); + ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? } + + ? ? ? /* the spi->mode bits understood by this driver: */ + ? ? ? master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH; + + ? ? ? if (pdev->id != -1) + ? ? ? ? ? ? ? master->bus_num = pdev->id; + + ? ? ? master->setup = spi_tegra_setup; + ? ? ? master->transfer = spi_tegra_transfer; + ? ? ? master->cleanup = spi_tegra_cleanup; + ? ? ? master->num_chipselect = 4; + + ? ? ? dev_set_drvdata(&pdev->dev, master); + ? ? ? tspi = spi_master_get_devdata(master); + ? ? ? tspi->master = master; + ? ? ? tspi->pdev = pdev; + ? ? ? spin_lock_init(&tspi->lock); + + ? ? ? r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ? ? ? if (r == NULL) { + ? ? ? ? ? ? ? ret = -ENODEV; + ? ? ? ? ? ? ? goto err0; + ? ? ? } + + ? ? ? if (!request_mem_region(r->start, (r->end - r->start) + 1, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_name(&pdev->dev))) { + ? ? ? ? ? ? ? ret = -EBUSY; + ? ? ? ? ? ? ? goto err0; + ? ? ? } + + ? ? ? tspi->phys = r->start; + ? ? ? tspi->base = ioremap(r->start, r->end - r->start + 1); + ? ? ? if (!tspi->base) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can't ioremap iomem\n"); + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto err1; + ? ? ? } + + ? ? ? tspi->clk = clk_get(&pdev->dev, NULL); + ? ? ? if (IS_ERR_OR_NULL(tspi->clk)) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not get clock\n"); + ? ? ? ? ? ? ? ret = PTR_ERR(tspi->clk); + ? ? ? ? ? ? ? goto err2; + ? ? ? } + + ? ? ? INIT_LIST_HEAD(&tspi->queue); + + ? ? ? tspi->rx_dma = tegra_dma_allocate_channel(TEGRA_DMA_MODE_ONESHOT); + ? ? ? if (!tspi->rx_dma) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not allocate rx dma channel\n"); + ? ? ? ? ? ? ? ret = -ENODEV; + ? ? ? ? ? ? ? goto err3; + ? ? ? } + + ? ? ? tspi->rx_bb = dma_alloc_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&tspi->rx_bb_phys, GFP_KERNEL); + ? ? ? if (!tspi->rx_bb) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "can not allocate rx bounce buffer\n"); + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto err4; + ? ? ? } + + ? ? ? tspi->rx_dma_req.complete = tegra_spi_rx_dma_complete; + ? ? ? tspi->rx_dma_req.to_memory = 1; + ? ? ? tspi->rx_dma_req.dest_addr = tspi->rx_bb_phys; + ? ? ? tspi->rx_dma_req.dest_bus_width = 32; + ? ? ? tspi->rx_dma_req.source_addr = tspi->phys + SLINK_RX_FIFO; + ? ? ? tspi->rx_dma_req.source_bus_width = 32; + ? ? ? tspi->rx_dma_req.source_wrap = 4; + ? ? ? tspi->rx_dma_req.req_sel = spi_tegra_req_sels[pdev->id]; + ? ? ? tspi->rx_dma_req.dev = tspi; + + ? ? ? ret = spi_register_master(master); + + ? ? ? if (ret < 0) + ? ? ? ? ? ? ? goto err5; + + ? ? ? return ret; + +err5: + ? ? ? dma_free_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? tspi->rx_bb, tspi->rx_bb_phys); +err4: + ? ? ? tegra_dma_free_channel(tspi->rx_dma); +err3: + ? ? ? clk_put(tspi->clk); +err2: + ? ? ? iounmap(tspi->base); +err1: + ? ? ? release_mem_region(r->start, (r->end - r->start) + 1); +err0: + ? ? ? spi_master_put(master); + ? ? ? return ret; +} + +static int __devexit spi_tegra_remove(struct platform_device *pdev) +{ + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct spi_tegra_data ? *tspi; + ? ? ? struct resource ? ? ? ? *r; + + ? ? ? master = dev_get_drvdata(&pdev->dev); + ? ? ? tspi = spi_master_get_devdata(master); + + ? ? ? tegra_dma_free_channel(tspi->rx_dma); + + ? ? ? dma_free_coherent(&pdev->dev, sizeof(u32) * BB_LEN, + ? ? ? ? ? ? ? ? ? ? ? ? tspi->rx_bb, tspi->rx_bb_phys); + + ? ? ? clk_put(tspi->clk); + ? ? ? iounmap(tspi->base); + + ? ? ? spi_master_put(master); + ? ? ? r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ? ? ? release_mem_region(r->start, (r->end - r->start) + 1); + + ? ? ? return 0; +} + +MODULE_ALIAS("platform:spi_tegra"); + +static struct platform_driver spi_tegra_driver = { + ? ? ? .driver = { + ? ? ? ? ? ? ? .name = ? ? ? ? "spi_tegra", + ? ? ? ? ? ? ? .owner = ? ? ? ?THIS_MODULE, + ? ? ? }, + ? ? ? .remove = ? ? ? __devexit_p(spi_tegra_remove), +}; + +static int __init spi_tegra_init(void) +{ + ? ? ? return platform_driver_probe(&spi_tegra_driver, spi_tegra_probe); +} +module_init(spi_tegra_init); + +static void __exit spi_tegra_exit(void) +{ + ? ? ? platform_driver_unregister(&spi_tegra_driver); +} +module_exit(spi_tegra_exit); + +MODULE_LICENSE("GPL"); --1.6.5.6
PATCH v4 1/1 spi: add spi_tegra driver
In reply to Erik GillingQuoted 26 lines · Erik Gilling
v2 changes:
?from Thierry Reding:
? ?* add "selectTEGRA_SYSTEM_DMA" toKconfig
?from Grant Likely:
? ?* add oneline description to header
? ?* inline references toDRIVER_NAME
? ?* inline references toBUSY_TIMEOUT
? ?* open codedbytes_per_word()
? ?* spi_readl/writel -> spi_tegra_readl/writel
? ?* move transfer validation tospi_tegra_transfer
? ?* don'trequest_mem_regioniomem as platform bus does that for us ? ?*__exit->__devexitv3 changes:
?from Russell King:
? ?* putrequest_mem_regionback int
?from Grant Likely:
? ?* remove #undef DEBUG
? ?* addSLINK_to register bit defines
? ?* remove unusedbytes_per_word
? ?* make spi_tegra_readl/writel static linine
? ?* various refactoring for clarity
? ?* mark err if BSY bit is not cleared after 1000 retries ? ?* move spinlock to protect setting of RDY bit ? ?*subsys_initcall->module_init
Quoted 8 lines
v3 changes:
?from Grant Likely:
? ?* updatespi_tegrato usePTR_ERRlessdma APISigned-off-by Erik Gilling <[email protected]>Cc Thierry Reding <[email protected]>Cc Grant Likely <[email protected]>Cc Russell King <[email protected]>
Hi Erik,
Go ahead and add my Acked-by line and merge this via the tegra tree to avoid ordering issues. I also have a few more minor comments below.
g.
Quoted 4 lines
+ ? ? ? /* FIXME: should probably control CS manually so that we can be sure
+ ? ? ? ?* it does not go low between transfer and to supportdelay_usecs
+ ? ? ? ?* correctly.
+ ? ? ? ?*/
Yes, you'll probably want to revisit this. A lot of SPI hardware doesn't understand that the actual transfer may be longer that the data it immediately knows about. Also, it is common to use GPIOs as chip selects.
drivers/spi/spi_tegra.c line 470
Quoted 4 lines
drivers/spi/spi_tegra.c new file +4
-0+static void spi_tegra_cleanup (struct spi_device *spi) +{ + ? ? ? dev_dbg(&spi->dev, "cleanup\n"); +}
Remove the empty hook
Quoted 8 lines
(quoted diff) +8
-0+static int __init spi_tegra_probe(struct platform_device *pdev) +{ + ? ? ? struct spi_master ? ? ? *master; + ? ? ? struct spi_tegra_data ? *tspi; + ? ? ? struct resource ? ? ? ? *r; + ? ? ? int ret; + + ? ? ? master = spi_alloc_master(&pdev->dev, sizeof *tspi);
Review
Quoted 10 lines
(quoted diff) +10
-0+ ? ? ? if (master == NULL) { + ? ? ? ? ? ? ? dev_err(&pdev->dev, "master allocation failed\n"); + ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? } + + ? ? ? /* the spi->mode bits understood by this driver: */ + ? ? ? master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH; + + ? ? ? if (pdev->id != -1) + ? ? ? ? ? ? ? master->bus_num = pdev->id;
even if pdev->id is -1, you probably want to set master->bus_num to is so that a spi bus number can be dynamically assigned.
PATCH v4 1/1 spi: add spi_tegra driver
Quoted 36 lines · Grant Likely
Quoted 26 lines · Erik Gilling
v2 changes:
?from Thierry Reding:
? ?* add "selectTEGRA_SYSTEM_DMA" toKconfig
?from Grant Likely:
? ?* add oneline description to header
? ?* inline references toDRIVER_NAME
? ?* inline references toBUSY_TIMEOUT
? ?* open codedbytes_per_word()
? ?* spi_readl/writel -> spi_tegra_readl/writel
? ?* move transfer validation tospi_tegra_transfer
? ?* don'trequest_mem_regioniomem as platform bus does that for us ? ?*__exit->__devexitv3 changes:
?from Russell King:
? ?* putrequest_mem_regionback int
?from Grant Likely:
? ?* remove #undef DEBUG
? ?* addSLINK_to register bit defines
? ?* remove unusedbytes_per_word
? ?* make spi_tegra_readl/writel static linine
? ?* various refactoring for clarity
? ?* mark err if BSY bit is not cleared after 1000 retries ? ?* move spinlock to protect setting of RDY bit ? ?*subsys_initcall->module_initQuoted 8 lines
v3 changes:
?from Grant Likely:
? ?* updatespi_tegrato usePTR_ERRlessdma APISigned-off-by Erik Gilling <[email protected]>Cc Thierry Reding <[email protected]>Cc Grant Likely <[email protected]>Cc Russell King <[email protected]>
Quoted 4 lines
Hi Erik,
Go ahead and add my Acked-by line and merge this via the tegra tree to avoid ordering issues. ?I also have a few more minor comments below.
Hi Erik,
What's the status on this one? I've not seen it show up in the for-next branch of the android tegra tree.
g.
PATCH v4 1/1 spi: add spi_tegra driver
Grant,
Sorry. Fell off my radar. I'll push it this week.
Cheers,
Erik
Quoted 49 lines · Grant Likely
Quoted 36 lines · Grant Likely
Quoted 26 lines · Erik Gilling
v2 changes:
?from Thierry Reding:
? ?* add "selectTEGRA_SYSTEM_DMA" toKconfig
?from Grant Likely:
? ?* add oneline description to header
? ?* inline references toDRIVER_NAME
? ?* inline references toBUSY_TIMEOUT
? ?* open codedbytes_per_word()
? ?* spi_readl/writel -> spi_tegra_readl/writel
? ?* move transfer validation tospi_tegra_transfer
? ?* don'trequest_mem_regioniomem as platform bus does that for us ? ?*__exit->__devexitv3 changes:
?from Russell King:
? ?* putrequest_mem_regionback int
?from Grant Likely:
? ?* remove #undef DEBUG
? ?* addSLINK_to register bit defines
? ?* remove unusedbytes_per_word
? ?* make spi_tegra_readl/writel static linine
? ?* various refactoring for clarity
? ?* mark err if BSY bit is not cleared after 1000 retries ? ?* move spinlock to protect setting of RDY bit ? ?*subsys_initcall->module_initQuoted 8 lines
v3 changes:
?from Grant Likely:
? ?* updatespi_tegrato usePTR_ERRlessdma APISigned-off-by Erik Gilling <[email protected]>Cc Thierry Reding <[email protected]>Cc Grant Likely <[email protected]>Cc Russell King <[email protected]>Quoted 4 lines
Hi Erik,
Go ahead and add my Acked-by line and merge this via the tegra tree to avoid ordering issues. ?I also have a few more minor comments below.
Hi Erik,
What's the status on this one? ?I've not seen it show up in the for-next branch of the android tegra tree.
g.