Hi, On Wed, Jul 31, 2013 at 11:17:52AM +0530, Sourav Poddar wrote: > diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c > new file mode 100644 > index 0000000..3d10b69 > --- /dev/null > +++ b/drivers/spi/spi-ti-qspi.c > @@ -0,0 +1,545 @@ <snip> > +/* Device Control */ > +#define QSPI_DD(m, n) (m << (3 + n*8)) > +#define QSPI_CKPHA(n) (1 << (2 + n*8)) > +#define QSPI_CSPOL(n) (1 << (1 + n*8)) > +#define QSPI_CKPOL(n) (1 << (n*8)) add spaces around the * operator > +#define QSPI_FRAME_MAX 0xfff Frame max is 4096, 0x1000, right ? > +static inline void ti_qspi_read_data(struct ti_qspi *qspi, > + unsigned long reg, int wlen, u8 **rxbuf) > +{ > + switch (wlen) { > + case 8: > + **rxbuf = readb(qspi->base + reg); > + dev_vdbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf)); > + *rxbuf += 1; > + break; > + case 16: > + **rxbuf = readw(qspi->base + reg); > + dev_vdbg(qspi->dev, "rx done, read %04x\n", *(*rxbuf)); > + *rxbuf += 2; > + break; > + case 32: > + **rxbuf = readl(qspi->base + reg); > + dev_vdbg(qspi->dev, "rx done, read %04x\n", *(*rxbuf)); %08x, this was commented before. > +static int ti_qspi_setup(struct spi_device *spi) > +{ > + struct ti_qspi *qspi = spi_master_get_devdata(spi->master); > + struct ti_qspi_regs *ctx_reg = &qspi->ctx_reg; > + int clk_div = 0, ret; > + u32 clk_ctrl_reg, clk_rate, clk_mask; > + > + clk_rate = clk_get_rate(qspi->fclk); > + > + if (!qspi->spi_max_frequency) { > + dev_err(qspi->dev, "spi max frequency not defined\n"); > + return -EINVAL; > + } > + > + clk_div = DIV_ROUND_UP(clk_rate, qspi->spi_max_frequency) - 1; > + > + dev_dbg(qspi->dev, "%s: hz: %d, clock divider %d\n", __func__, > + qspi->spi_max_frequency, clk_div); > + > + ret = pm_runtime_get_sync(qspi->dev); > + if (ret) { > + dev_err(qspi->dev, "pm_runtime_get_sync() failed\n"); > + return ret; > + } > + > + clk_ctrl_reg = ti_qspi_read(qspi, QSPI_SPI_CLOCK_CNTRL_REG); > + > + clk_ctrl_reg &= ~QSPI_CLK_EN; > + > + if (spi->master->busy) { > + dev_dbg(qspi->dev, "master busy doing other trasnfers\n"); > + return -EBUSY; > + } this check can be done before pm_runtime_get_sync(), you're also leaking pm_runtime reference here. > + /* disable SCLK */ > + ti_qspi_write(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG); > + > + if (clk_div < 0) { > + dev_dbg(qspi->dev, "%s: clock divider < 0, using /1 divider\n", > + __func__); > + pm_runtime_put_sync(qspi->dev); > + return -EINVAL; > + } > + > + if (clk_div > QSPI_CLK_DIV_MAX) { > + dev_dbg(qspi->dev, "%s: clock divider >%d , using /%d divider\n", > + __func__, QSPI_CLK_DIV_MAX, QSPI_CLK_DIV_MAX + 1); > + pm_runtime_put_sync(qspi->dev); > + return -EINVAL; > + } why don't you move all checks to clk_div before pm_runtime_get_sync() call ? > +static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t) > +{ > + const u8 *txbuf; > + int wlen, count, ret; > + > + count = t->len; > + txbuf = t->tx_buf; > + wlen = t->bits_per_word; > + > + while (count--) { you're decrementing count by one, but in some cases you write 4 bytes or 2 bytes... This will blow up very soon. I can already see overflows happening... > +static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t) > +{ > + u8 *rxbuf; > + int wlen, count, ret; > + > + count = t->len; > + rxbuf = t->rx_buf; > + wlen = t->bits_per_word; > + > + while (count--) { ditto > +static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t) > +{ > + int ret; > + > + if (t->tx_buf) { > + ret = qspi_write_msg(qspi, t); > + if (ret) { > + dev_dbg(qspi->dev, "Error while writing\n"); > + return -EINVAL; why do you change the return value from qspi_write_msg() ? > + } > + } > + > + if (t->rx_buf) { > + ret = qspi_read_msg(qspi, t); > + if (ret) { > + dev_dbg(qspi->dev, "Error while writing\n"); > + return -EINVAL; why do you change the return value from qspi_read_msg() ? > +static int ti_qspi_start_transfer_one(struct spi_master *master, > + struct spi_message *m) > +{ > + struct ti_qspi *qspi = spi_master_get_devdata(master); > + struct spi_device *spi = m->spi; > + struct spi_transfer *t; > + int status = 0, ret; > + int frame_length; > + > + /* setup device control reg */ > + qspi->dc = 0; > + > + if (spi->mode & SPI_CPHA) > + qspi->dc |= QSPI_CKPHA(spi->chip_select); > + if (spi->mode & SPI_CPOL) > + qspi->dc |= QSPI_CKPOL(spi->chip_select); > + if (spi->mode & SPI_CS_HIGH) > + qspi->dc |= QSPI_CSPOL(spi->chip_select); > + > + frame_length = (m->frame_length << 3) / spi->bits_per_word; > + > + frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX); > + > + /* setup command reg */ > + qspi->cmd = 0; > + qspi->cmd |= QSPI_EN_CS(spi->chip_select); > + qspi->cmd |= QSPI_FLEN(frame_length); > + qspi->cmd |= QSPI_WC_CMD_INT_EN; > + > + ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG); > + > + list_for_each_entry(t, &m->transfers, transfer_list) { no locking around list traversal ? > +static irqreturn_t ti_qspi_isr(int irq, void *dev_id) > +{ > + struct ti_qspi *qspi = dev_id; > + u16 stat; > + > + irqreturn_t ret = IRQ_HANDLED; > + > + spin_lock(&qspi->lock); > + > + stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR); > + > + if (!stat) { > + dev_dbg(qspi->dev, "No IRQ triggered\n"); > + return IRQ_NONE; leaving lock held. > +static irqreturn_t ti_qspi_threaded_isr(int this_irq, void *dev_id) > +{ > + struct ti_qspi *qspi = dev_id; > + unsigned long flags; > + > + spin_lock_irqsave(&qspi->lock, flags); > + > + complete(&qspi->transfer_complete); you need to check if your word completion is actually set. Don't assume it's set because we might want to change the code later. > +static int ti_qspi_probe(struct platform_device *pdev) > +{ > + struct ti_qspi *qspi; > + struct spi_master *master; > + struct resource *r; > + struct device_node *np = pdev->dev.of_node; > + u32 max_freq; > + int ret = 0, num_cs, irq; > + > + master = spi_alloc_master(&pdev->dev, sizeof(*qspi)); > + if (!master) > + return -ENOMEM; > + > + master->mode_bits = SPI_CPOL | SPI_CPHA; > + > + master->bus_num = -1; > + master->flags = SPI_MASTER_HALF_DUPLEX; > + master->setup = ti_qspi_setup; > + master->auto_runtime_pm = true; > + master->transfer_one_message = ti_qspi_start_transfer_one; > + master->dev.of_node = pdev->dev.of_node; > + master->bits_per_word_mask = BIT(32 - 1) | BIT(16 - 1) | BIT(8 - 1); > + > + if (!of_property_read_u32(np, "num-cs", &num_cs)) > + master->num_chipselect = num_cs; > + > + platform_set_drvdata(pdev, master); > + > + qspi = spi_master_get_devdata(master); > + qspi->master = master; > + qspi->dev = &pdev->dev; > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "no irq resource?\n"); > + return irq; > + } > + > + spin_lock_init(&qspi->lock); > + > + qspi->base = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(qspi->base)) { > + ret = PTR_ERR(qspi->base); > + goto free_master; > + } > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr, > + ti_qspi_threaded_isr, 0, > + dev_name(&pdev->dev), qspi); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n", > + irq); > + goto free_master; > + } > + > + qspi->fclk = devm_clk_get(&pdev->dev, "fck"); > + if (IS_ERR(qspi->fclk)) { > + ret = PTR_ERR(qspi->fclk); > + dev_err(&pdev->dev, "could not get clk: %d\n", ret); > + } > + > + init_completion(&qspi->transfer_complete); > + > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_set_autosuspend_delay(&pdev->dev, QSPI_AUTOSUSPEND_TIMEOUT); > + pm_runtime_enable(&pdev->dev); > + > + if (!of_property_read_u32(np, "spi-max-frequency", &max_freq)) > + qspi->spi_max_frequency = max_freq; > + > + ret = spi_register_master(master); > + if (ret) > + goto free_master; > + > + return ret; you only get here with success, so return 0 is alright. -- balbi
Attachment:
signature.asc
Description: Digital signature