[PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

[Grant Likely]

On Thu, Jun 24, 2010 at 6:12 AM, Jason Wang <jason77.wang at gmail.com> wrote:
> In current design, the SPI channel is always enable during the period
> of handling a SPI message, it is risky when more than one SPI transfer
> are included in a message. Current working route like that:
> [
>  SPI channel enable
>  configure channel
>  handle transfer #1
>  configure channel
>  handle transfer #2
>  ...
>  SPI channel disable
> ]
> If we can disable channel before configure it and reenable
> channel after it is configured, it will be more safe.
>
> The commit a330ce2 "omap2_mcspi: Flush posted writes" make this risk
> to a real problem for ads7846 driver on omap3530evm. The
> ads7846 driver will send a SPI messge which includes,
> [
>  TX_ONLY transfer (1 byte)
>  RX_ONLY transfer (2 bytes)
>  TX_ONLY transfer (1 byte)
>  RX_ONLY transfer (2 bytes)
> ]
> If we don't add disable/reenable channel between TX and RX transfers,
> the RX transfer will get wrong datas sent from slave.

I don't understand the problem description.  Are you talking about
manipulation of the SS line, or a problem with the internal state on
the mcspi controller device?  What is the cause of the wrong data (ie.
is there a protocol error on the wire, or is the SPI controller not
performing the read correctly?)

Thanks,
g.

>
> Signed-off-by: Jason Wang <jason77.wang at gmail.com>
> ---
>  drivers/spi/omap2_mcspi.c |   16 ++++++----------
>  1 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
> index b3a94ca..1e3cb7b 100644
> --- a/drivers/spi/omap2_mcspi.c
> +++ b/drivers/spi/omap2_mcspi.c
> @@ -408,7 +408,6 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
>                                count -= (word_len <= 8)  ? 2 :
>                                        (word_len <= 16) ? 4 :
>                                        /* word_len <= 32 */ 8;
> -                               omap2_mcspi_set_enable(spi, 1);
>                                return count;
>                        }
>                }
> @@ -430,8 +429,10 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
>                                 (word_len <= 16) ? 2 :
>                               /* word_len <= 32 */ 4;
>                }
> -               omap2_mcspi_set_enable(spi, 1);
>        }
> +
> +       omap2_mcspi_set_enable(spi, 0);
> +
>        return count;
>  }
>
> @@ -517,8 +518,6 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
>                                                goto out;
>                                        }
>                                        c = 0;
> -                               } else if (c == 0 && tx == NULL) {
> -                                       omap2_mcspi_set_enable(spi, 0);
>                                }
>
>                                *rx++ = __raw_readl(rx_reg);
> @@ -570,8 +569,6 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
>                                                goto out;
>                                        }
>                                        c = 0;
> -                               } else if (c == 0 && tx == NULL) {
> -                                       omap2_mcspi_set_enable(spi, 0);
>                                }
>
>                                *rx++ = __raw_readl(rx_reg);
> @@ -623,8 +620,6 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
>                                                goto out;
>                                        }
>                                        c = 0;
> -                               } else if (c == 0 && tx == NULL) {
> -                                       omap2_mcspi_set_enable(spi, 0);
>                                }
>
>                                *rx++ = __raw_readl(rx_reg);
> @@ -646,7 +641,7 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
>                        dev_err(&spi->dev, "EOT timed out\n");
>        }
>  out:
> -       omap2_mcspi_set_enable(spi, 1);
> +       omap2_mcspi_set_enable(spi, 0);
>        return count - c;
>  }
>
> @@ -894,7 +889,6 @@ static void omap2_mcspi_work(struct work_struct *work)
>                cs = spi->controller_state;
>                cd = spi->controller_data;
>
> -               omap2_mcspi_set_enable(spi, 1);
>                list_for_each_entry(t, &m->transfers, transfer_list) {
>                        if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {
>                                status = -EINVAL;
> @@ -931,6 +925,8 @@ static void omap2_mcspi_work(struct work_struct *work)
>
>                        mcspi_write_chconf0(spi, chconf);
>
> +                       omap2_mcspi_set_enable(spi, 1);
> +
>                        if (t->len) {
>                                unsigned        count;
>
> --
> 1.5.6.5
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.