Messages in this thread | | | Date | Wed, 18 Nov 2009 14:21:09 -0800 | From | Andrew Morton <> | Subject | Re: [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; > +} > + > > ... >
| |