Re: [PATCH] ARM: Add spi controller driver support for NUC900

[Wan ZongShun <mcuos.com@gmail.com>]

Dear Andrew,

Thanks a lot for your help, and I have a question below.

2009/11/19 Andrew Morton <akpm@linux-foundation.org>:
> 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?

Do you mean that I should put this 'free_irq()' in the front of
w90p910_spi_remove？

such as:
"
free_irq(hw->irq, hw);

platform_set_drvdata(dev, NULL);

spi_unregister_master(hw->master);

clk_disable(hw->clk);
clk_put(hw->clk);
"

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



-- 
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel