Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752277AbcDOGbo (ORCPT ); Fri, 15 Apr 2016 02:31:44 -0400 Received: from exsmtp01.microchip.com ([198.175.253.37]:22235 "EHLO email.microchip.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752005AbcDOGbk (ORCPT ); Fri, 15 Apr 2016 02:31:40 -0400 Subject: Re: [PATCH v2 2/2] spi: pic32-sqi: add SPI driver for PIC32 SQI controller. To: Mark Brown References: <1460553778-1662-1-git-send-email-purna.mandal@microchip.com> <1460553778-1662-2-git-send-email-purna.mandal@microchip.com> <20160414055546.GA18024@sirena.org.uk> CC: , From: Purna Chandra Mandal Message-ID: <57108A63.50803@microchip.com> Date: Fri, 15 Apr 2016 11:59:55 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <20160414055546.GA18024@sirena.org.uk> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2354 Lines: 68 On 04/14/2016 11:25 AM, Mark Brown wrote: > 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. In PESQI controller INT_STAT_REG specifies interrupting reason(s) as usual, but mostly to confirm completion of on-going DMA operation. It is sticky, no way to acknowledge the interrupt source except masking the source. In short, whole interrupt logic is mere extension of status polling logic. And in driver isr logic is completely depended on the INT_STAT_REG; if status is zero no handling can be done and is unexpected! >> +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. Make sense. Will use 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. ack. >> + clk_prepare_enable(sqi->sys_clk); >> + clk_prepare_enable(sqi->base_clk); > Check the return value please. Will add. >> + /* 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(). Yes, will use request_irq() instead. >> +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... ack, Will correct disable/free order.