On Thu, Sep 9, 2010 at 11:51 PM, Grant Likely <grant.lik...@secretlab.ca> wrote: > On Thu, Sep 09, 2010 at 04:18:57PM +0900, Jassi Brar wrote: >> We can't do without setting channel and bus width to >> same size. Inorder to do that, use loop read/writes in >> polling mode and appropriate burst size in DMA mode. >> >> Signed-off-by: Jassi Brar <jassi.b...@samsung.com> > > Looks pretty good to me. A couple of minor comments below. > > g. > >> --- >> drivers/spi/spi_s3c64xx.c | 76 >> ++++++++++++++++++++++++++++++++++++--------- >> 1 files changed, 61 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c >> index 39816bb..3ff335d 100644 >> --- a/drivers/spi/spi_s3c64xx.c >> +++ b/drivers/spi/spi_s3c64xx.c >> @@ -255,15 +255,35 @@ static void enable_datapath(struct >> s3c64xx_spi_driver_data *sdd, >> chcfg |= S3C64XX_SPI_CH_TXCH_ON; >> if (dma_mode) { >> modecfg |= S3C64XX_SPI_MODE_TXDMA_ON; >> - s3c2410_dma_config(sdd->tx_dmach, 1); >> + switch (sdd->cur_bpw) { >> + case 32: >> + s3c2410_dma_config(sdd->tx_dmach, 4); >> + break; >> + case 16: >> + s3c2410_dma_config(sdd->tx_dmach, 2); >> + break; >> + default: >> + s3c2410_dma_config(sdd->tx_dmach, 1); >> + break; >> + } > > Nit: switch statements like this tend to obscure the fact that the same > function is called each time with a different value. How about the > following: > > width = 1; > if (sdd->cur_bpw == 32) > width = 4; > if (sdd->cur_bpw == 16) > width = 2; > s3c2410_dma_config(sdd->tx_dmach, width); > > It's also more concise.
yuck! why didn't i do s3c2410_dma_config(sdd->tx_dmach, sdd->cur_bpw / 8); ?? > >> s3c2410_dma_enqueue(sdd->tx_dmach, (void *)sdd, >> xfer->tx_dma, xfer->len); >> s3c2410_dma_ctrl(sdd->tx_dmach, S3C2410_DMAOP_START); >> } else { >> - unsigned char *buf = (unsigned char *) xfer->tx_buf; >> - int i = 0; >> - while (i < xfer->len) >> - writeb(buf[i++], regs + S3C64XX_SPI_TX_DATA); >> + switch (sdd->cur_bpw) { >> + case 32: >> + iowrite32_rep(regs + S3C64XX_SPI_TX_DATA, >> + xfer->tx_buf, xfer->len / 4); >> + break; >> + case 16: >> + iowrite16_rep(regs + S3C64XX_SPI_TX_DATA, >> + xfer->tx_buf, xfer->len / 2); >> + break; >> + default: >> + iowrite8_rep(regs + S3C64XX_SPI_TX_DATA, >> + xfer->tx_buf, xfer->len); >> + break; >> + } >> } >> } >> >> @@ -280,7 +300,17 @@ static void enable_datapath(struct >> s3c64xx_spi_driver_data *sdd, >> writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff) >> | S3C64XX_SPI_PACKET_CNT_EN, >> regs + S3C64XX_SPI_PACKET_CNT); >> - s3c2410_dma_config(sdd->rx_dmach, 1); >> + switch (sdd->cur_bpw) { >> + case 32: >> + s3c2410_dma_config(sdd->rx_dmach, 4); >> + break; >> + case 16: >> + s3c2410_dma_config(sdd->rx_dmach, 2); >> + break; >> + default: >> + s3c2410_dma_config(sdd->rx_dmach, 1); >> + break; >> + } > > Ditto here > >> s3c2410_dma_enqueue(sdd->rx_dmach, (void *)sdd, >> xfer->rx_dma, xfer->len); >> s3c2410_dma_ctrl(sdd->rx_dmach, S3C2410_DMAOP_START); >> @@ -360,20 +390,26 @@ static int wait_for_xfer(struct >> s3c64xx_spi_driver_data *sdd, >> return -EIO; >> } >> } else { >> - unsigned char *buf; >> - int i; >> - >> /* If it was only Tx */ >> if (xfer->rx_buf == NULL) { >> sdd->state &= ~TXBUSY; >> return 0; >> } >> >> - i = 0; >> - buf = xfer->rx_buf; >> - while (i < xfer->len) >> - buf[i++] = readb(regs + S3C64XX_SPI_RX_DATA); >> - >> + switch (sdd->cur_bpw) { >> + case 32: >> + ioread32_rep(regs + S3C64XX_SPI_RX_DATA, >> + xfer->rx_buf, xfer->len / 4); >> + break; >> + case 16: >> + ioread16_rep(regs + S3C64XX_SPI_RX_DATA, >> + xfer->rx_buf, xfer->len / 2); >> + break; >> + default: >> + ioread8_rep(regs + S3C64XX_SPI_RX_DATA, >> + xfer->rx_buf, xfer->len); >> + break; >> + } >> sdd->state &= ~RXBUSY; >> } >> >> @@ -423,15 +459,17 @@ static void s3c64xx_spi_config(struct >> s3c64xx_spi_driver_data *sdd) >> switch (sdd->cur_bpw) { >> case 32: >> val |= S3C64XX_SPI_MODE_BUS_TSZ_WORD; >> + val |= S3C64XX_SPI_MODE_CH_TSZ_WORD; >> break; >> case 16: >> val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD; >> + val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD; >> break; >> default: >> val |= S3C64XX_SPI_MODE_BUS_TSZ_BYTE; >> + val |= S3C64XX_SPI_MODE_CH_TSZ_BYTE; >> break; >> } >> - val |= S3C64XX_SPI_MODE_CH_TSZ_BYTE; /* Always 8bits wide */ >> >> writel(val, regs + S3C64XX_SPI_MODE_CFG); >> >> @@ -610,6 +648,14 @@ static void handle_msg(struct s3c64xx_spi_driver_data >> *sdd, >> bpw = xfer->bits_per_word ? : spi->bits_per_word; >> speed = xfer->speed_hz ? : spi->max_speed_hz; >> >> + if (bpw != 8 && xfer->len % (bpw / 8)) { > > The (bpw != 8) test is superfluous. xfer->len % (bpw / 8) always evaluate to true(error hence) when bpw==8 so we wanna avoid that case thanks ------------------------------------------------------------------------------ This SF.net Dev2Dev email is sponsored by: Show off your parallel programming skills. Enter the Intel(R) Threading Challenge 2010. http://p.sf.net/sfu/intel-thread-sfd _______________________________________________ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general