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...
Attachment:
signature.asc
Description: PGP signature
