lkml.org 
[lkml]   [2009]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ARM: Add spi controller driver support for NUC900
On Tue, 17 Nov 2009 14:48:40 +0800
Wan ZongShun <mcuos.com@gmail.com> wrote:

> Dear David,
>
> Add winbond/nuvoton NUC900 spi controller driver support,
> on my evaluation board,there is a winbond w25x16 spi flash,
> so I test my spi controller driver with m25p80.c.
>
>
> ...
>
> +static inline struct w90p910_spi *to_hw(struct spi_device *sdev)
> +{
> + return spi_master_get_devdata(sdev->master);
> +}
> +
> +static void w90p910_slave_seclect(struct spi_device *spi, unsigned int ssr)

I think you meant "select" here?

> +{
> + struct w90p910_spi *hw = to_hw(spi);
> + unsigned int val;
> + unsigned int cs = spi->mode & SPI_CS_HIGH ? 1 : 0;
> + unsigned int cpol = spi->mode & SPI_CPOL ? 1 : 0;
> +
> + val = __raw_readl(hw->regs + USI_SSR);
> +
> + if (!cs)
> + val &= ~SELECTLEV;
> + else
> + val |= SELECTLEV;
> +
> + if (!ssr)
> + val &= ~SELECTSLAVE;
> + else
> + val |= SELECTSLAVE;
> +
> + __raw_writel(val, hw->regs + USI_SSR);
> +
> + val = __raw_readl(hw->regs + USI_CNT);
> +
> + if (!cpol)
> + val &= ~SELECTPOL;
> + else
> + val |= SELECTPOL;
> +
> + __raw_writel(val, hw->regs + USI_CNT);
> +}

That's a read-modify-write operation. What locking prevents two
threads of control from altering the USI_SSR and USI_CNT registers at
the same time, resulting in an indeterminate setting?

> +static void w90p910_spi_chipsel(struct spi_device *spi, int value)
> +{
> + switch (value) {
> + case BITBANG_CS_INACTIVE:
> + w90p910_slave_seclect(spi, 0);
> + break;
> +
> + case BITBANG_CS_ACTIVE:
> + w90p910_slave_seclect(spi, 1);
> + break;
> + }
> +}
> +
> +static void w90p910_spi_setup_txnum(struct w90p910_spi *hw,
> + unsigned int txnum)
> +{
> + unsigned int val;
> +
> + val = __raw_readl(hw->regs + USI_CNT);
> +
> + if (!txnum)
> + val &= ~TXNUM;
> + else
> + val |= txnum << 0x08;
> +
> + __raw_writel(val, hw->regs + USI_CNT);
> +
> +}
> +
> +static void w90p910_spi_setup_txbitlen(struct w90p910_spi *hw,
> + unsigned int txbitlen)
> +{
> + unsigned int val;
> +
> + val = __raw_readl(hw->regs + USI_CNT);
> +
> + val |= (txbitlen << 0x03);
> +
> + __raw_writel(val, hw->regs + USI_CNT);
> +}
> +
> +static void w90p910_spi_gobusy(struct w90p910_spi *hw)
> +{
> + unsigned int val;
> +
> + val = __raw_readl(hw->regs + USI_CNT);
> +
> + val |= GOBUSY;
> +
> + __raw_writel(val, hw->regs + USI_CNT);
> +}

ditto, ditto, ditto.

> +static int w90p910_spi_setupxfer(struct spi_device *spi,
> + struct spi_transfer *t)
> +{
> + return 0;
> +}
> +
> +static int w90p910_spi_setup(struct spi_device *spi)
> +{
> + return 0;
> +}
> +
> +static inline unsigned int hw_txbyte(struct w90p910_spi *hw, int count)
> +{
> + return hw->tx ? hw->tx[count] : 0;
> +}
> +
> +static int w90p910_spi_txrx(struct spi_device *spi, struct spi_transfer *t)
> +{
> + struct w90p910_spi *hw = to_hw(spi);
> +
> + hw->tx = t->tx_buf;
> + hw->rx = t->rx_buf;
> + hw->len = t->len;
> + hw->count = 0;
> +
> + init_completion(&hw->done);
> +
> + __raw_writel(hw_txbyte(hw, 0x0), hw->regs + USI_TX0);
> +
> + w90p910_spi_gobusy(hw);
> +
> + wait_for_completion(&hw->done);
> +
> + return hw->count;
> +}

The init_completion() should be unneeded? The structure was
initialised at setup time and will be left in a reusable state after a
complete()/wait_for_completion(). Reinitialising the structure all the
time like this adds risk that it will be scribbled on while in use.

>
> ...
>
> +static int __devexit w90p910_spi_remove(struct platform_device *dev)
> +{
> + struct w90p910_spi *hw = platform_get_drvdata(dev);
> +
> + platform_set_drvdata(dev, NULL);
> +
> + spi_unregister_master(hw->master);
> +
> + clk_disable(hw->clk);
> + clk_put(hw->clk);

As far as I can tell, a hardware interrupt could still be pending, or
be under service while the above code is executing?

If so, I expect bad things will happen?

> + free_irq(hw->irq, hw);
> + iounmap(hw->regs);
> +
> + release_resource(hw->ioarea);
> + kfree(hw->ioarea);
> +
> + spi_master_put(hw->master);
> + return 0;
> +}
> +
>
> ...
>



\
 
 \ /
  Last update: 2009-11-18 23:25    [W:0.071 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site