Messages in this thread | | | Date | Thu, 14 Apr 2016 06:55:46 +0100 | From | Mark Brown <> | Subject | Re: [PATCH v2 2/2] spi: pic32-sqi: add SPI driver for PIC32 SQI controller. |
| |
On Wed, Apr 13, 2016 at 06:52:58PM +0530, Purna Chandra Mandal wrote:
> + enable = readl(sqi->regs + PESQI_INT_ENABLE_REG); > + status = readl(sqi->regs + PESQI_INT_STAT_REG); > + if (!status) > + return IRQ_NONE; > +
For robustness the check should be if there was anything handled, not if there was anything set.
> +static dma_addr_t pic32_sqi_map_transfer(struct pic32_sqi *sqi, > + struct spi_transfer *transfer) > +{ > + struct device *dev = &sqi->master->dev;
Don't open code DMA mapping of the buffers, use the core support.
> + reg = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sqi->regs = devm_ioremap_resource(&pdev->dev, reg); > + if (!sqi->regs) { > + dev_err(&pdev->dev, "mem map failed\n");
devm_ioremap_resource() will log for you.
> + clk_prepare_enable(sqi->sys_clk); > + clk_prepare_enable(sqi->base_clk);
Check the return value please.
> + /* install irq handlers */ > + ret = devm_request_irq(&pdev->dev, sqi->irq, pic32_sqi_isr, > + 0, dev_name(&pdev->dev), sqi); > + if (ret < 0) { > + dev_err(&pdev->dev, "request-irq %d, failed ?\n", sqi->irq); > + goto err_free_ring; > + }
This will free before the clocks are disabled. Are you sure that's safe? It's generally not good to mix devm_ and non-devm operations especially things like these that aren't simple frees of data. It is safer to use a normal request_irq().
> +static int pic32_sqi_remove(struct platform_device *pdev) > +{ > + struct pic32_sqi *sqi = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(sqi->base_clk); > + clk_disable_unprepare(sqi->sys_clk); > + > + /* release memory */ > + ring_desc_ring_free(sqi);
This will free the descriptor ring before the interrupt... [unhandled content-type:application/pgp-signature] | |