On Thu, Sep 9, 2010 at 11:51 PM, Grant Likely <[email protected]> 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 <[email protected]>
>
> 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
