From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031604AbbKECWx (ORCPT ); Wed, 4 Nov 2015 21:22:53 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:52672 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030830AbbKECWv (ORCPT ); Wed, 4 Nov 2015 21:22:51 -0500 Subject: Re: [PATCH V2 3/3] dma: add Qualcomm Technologies HIDMA channel driver To: Andy Shevchenko References: <1446444460-21600-1-git-send-email-okaya@codeaurora.org> <1446444460-21600-4-git-send-email-okaya@codeaurora.org> <56394C37.4060603@codeaurora.org> Cc: dmaengine , timur@codeaurora.org, cov@codeaurora.org, jcm@redhat.com, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Vinod Koul , Dan Williams , devicetree , "linux-kernel@vger.kernel.org" From: Sinan Kaya Message-ID: <563ABD75.5080608@codeaurora.org> Date: Wed, 4 Nov 2015 21:22:45 -0500 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >> /* >> * We are posting descriptors to the hardware as soon as >> * they are ready, so this function does nothing. >> */ > > So, the Freescale driver was written before change went effective. I > guess in 2011 DMA Engine drivers should use issue pending. > Please, refactor since this behaviour is expected. > done >>>> +/* >>>> + * Submit descriptor to hardware. >>>> + * Lock the PM for each descriptor we are sending. >>>> + */ >>>> +static dma_cookie_t hidma_tx_submit(struct dma_async_tx_descriptor *txd) >>>> +{ >>>> + struct hidma_chan *mchan = to_hidma_chan(txd->chan); >>>> + struct hidma_dev *dmadev = mchan->dmadev; >>>> + struct hidma_desc *mdesc; >>>> + unsigned long irqflags; >>>> + dma_cookie_t cookie; >>>> + >>>> + if (!hidma_ll_isenabled(dmadev->lldev)) >>>> + return -ENODEV; >>>> + >>>> + pm_runtime_get_sync(dmadev->ddev.dev); >>> >>> >>> No point to do it here. It should be done on the function that >>> actually starts the transfer (see issue pending). >>> >> comment above > > See above as well. done > >>>> +static int hidma_probe(struct platform_device *pdev) >>>> +{ >>>> + struct hidma_dev *dmadev; >>>> + int rc = 0; >>>> + struct resource *trca_resource; >>>> + struct resource *evca_resource; >>>> + int chirq; >>>> + int current_channel_index = atomic_read(&channel_ref_count); >>>> + > >>>> + /* Set DMA mask to 64 bits. */ >>>> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); >>>> + if (rc) { >>>> + dev_warn(&pdev->dev, "unable to set coherent mask to >>>> 64"); >>>> + rc = dma_set_mask_and_coherent(&pdev->dev, >>>> DMA_BIT_MASK(32)); >>>> + } >>>> + if (rc) >>>> + goto dmafree; > > Maybe move these two lines inside previous condition? ok > >>>> + >>>> + dmadev->lldev = hidma_ll_init(dmadev->ddev.dev, >>>> + dmadev->nr_descriptors, dmadev->dev_trca, >>>> + dmadev->dev_evca, dmadev->evridx); >>>> + if (!dmadev->lldev) { >>>> + rc = -EPROBE_DEFER; >>>> + goto dmafree; >>>> + } >>>> + >>>> + rc = devm_request_irq(&pdev->dev, chirq, hidma_chirq_handler, 0, >>>> + "qcom-hidma", &dmadev->lldev); >>> >>> >>> Better to use request_irq(). >>> >> >> why? I thought we favored managed functions over standalone functions in >> simplify the exit path. > > IRQ is slightly different in workflow. In most cases, unfortunately, > there is no achievement by devm_ variant. > At least I know two for now. One of them is DMA Engine slave drivers, > though I didn't notice if you are using tasklet's here. > Otherwise it's okay. > I'm keeping it as it is for maintenance reasons. -- Sinan Kaya Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project