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

[Wan ZongShun]

Dear Andrew,

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

2009/11/19 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>:
> On Tue, 17 Nov 2009 14:48:40 +0800
> Wan ZongShun <mcuos.com@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/