Re: [PATCH v2 2/2] spi: pic32-sqi: add SPI driver for PIC32 SQI controller.

[Mark Brown]

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