On Wed, Nov 25, 2009 at 11:48 PM, Jassi Brar <jassi.brar@xxxxxxxxxxx> wrote:
> Added S3C64XX SPI controller driver.
>
> Each SPI controller has exactly one CS line and as such doesn't
> provide for multi-cs. We implement a workaround to support
> multi-cs by _not_ configuring the mux'ed CS pin for each SPI
> controller. The CS mechanism is assumed to be fully machine
> specific - the driver doesn't even assume some GPIO pin is used
> to control the CS.
>
> The driver selects between DMA and POLLING mode depending upon
> the xfer size - DMA mode for xfers bigger than FIFO size, POLLING
> mode otherwise.
>
> The driver has been designed to be capable of running SoCs since
> s3c64xx and till date, for that reason some of the register fields
> have been passed via, SoC specific, platform data.
>
> Signed-off-by: Jassi Brar <jassi.brar@xxxxxxxxxxx>
> ---
> drivers/spi/Kconfig | 7 +
> drivers/spi/Makefile | 1 +
> drivers/spi/spi_s3c64xx.c | 1033 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/spi/spi_s3c64xx.h | 189 +++++++++
This is in the drivers/spi tree, but depends on the arch/arm stuff
from the first 4 patches. Will need to decide which tree to merge
this driver though.
I won't review patches 1-4, but I've got a few comments for this
patch. Mostly minor stuff. I don't seen anything on quick review
that jumps out at me as bad.
> +
> +#include "spi_s3c64xx.h"
First, why the separate header file? It doesn't look like there are
any external users of the header file data. Please merge it into the
top of the .c file.
> +
> +static struct s3c2410_dma_client s3c64xx_spi_dma_client = {
> + .name = "s3c64xx-spi-dma",
> +};
> +
> +#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
> +
> +static void flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> +{
> + struct s3c64xx_spi_cntrlr_info *sci = sdd->cntrlr_info;
> + unsigned long loops;
> + u32 val;
> +
> + val = readl(sdd->regs + S3C64XX_SPI_CH_CFG);
> + val |= S3C64XX_SPI_CH_SW_RST;
> + val &= ~S3C64XX_SPI_CH_HS_EN;
> + writel(val, sdd->regs + S3C64XX_SPI_CH_CFG);
> +
> + /* Flush TxFIFO*/
> + loops = msecs_to_loops(1);
> + do {
> + val = readl(sdd->regs + S3C64XX_SPI_STATUS);
> + } while (TX_FIFO_LVL(val, sci) && loops--);
nasty spin. Can you sleep here instead? How much time do you project
is needed to flush the fifo. It is a worthwhile question to ask on
the other spin loops through this file.
> +static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
> + struct spi_device *spi)
> +{
> + struct s3c64xx_spi_csinfo *cs;
> +
> + if (sdd->tgl_spi != NULL) { /* If last device toggled after mssg */
> + if (sdd->tgl_spi != spi) { /* if last mssg on diff device */
> + /* Deselect the last toggled device */
> + cs = sdd->tgl_spi->controller_data;
> + if (sdd->tgl_spi->mode & SPI_CS_HIGH)
> + cs->set_level(0);
> + else
> + cs->set_level(1);
> + }
> + sdd->tgl_spi = NULL;
> + }
> +
> + cs = spi->controller_data;
> + if (spi->mode & SPI_CS_HIGH)
> + cs->set_level(1);
> + else
> + cs->set_level(0);
cs->set_level(spi->mode & SPI_CS_HIGH ? 1 : 0) perhaps?
> +
> + S3C64XX_SPI_ACT(sdd);
> +}
[...]
> +static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
> + struct spi_device *spi)
> +{
> + struct s3c64xx_spi_csinfo *cs;
> +
> + S3C64XX_SPI_DEACT(sdd);
> +
> + if (sdd->tgl_spi == spi)
> + sdd->tgl_spi = NULL;
> +
> + cs = spi->controller_data;
> + if (spi->mode & SPI_CS_HIGH)
> + cs->set_level(0);
> + else
> + cs->set_level(1);
ditto here.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
