From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:40781 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751006AbbCMA0D (ORCPT ); Thu, 12 Mar 2015 20:26:03 -0400 From: Laurent Pinchart To: Sakari Ailus Cc: linux-media@vger.kernel.org, linux-omap@vger.kernel.org Subject: Re: [PATCH] media: omap3isp: hist: Move histogram DMA to DMA engine Date: Fri, 13 Mar 2015 02:26:04 +0200 Message-ID: <109710782.yRElq6GaBL@avalon> In-Reply-To: <20150312235632.GQ11954@valkosipuli.retiisi.org.uk> References: <1425850675-32266-1-git-send-email-laurent.pinchart@ideasonboard.com> <20150312235632.GQ11954@valkosipuli.retiisi.org.uk> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Sakari, Thank you for the review. On Friday 13 March 2015 01:56:32 Sakari Ailus wrote: > On Sun, Mar 08, 2015 at 11:37:55PM +0200, Laurent Pinchart wrote: > ... > > > @@ -198,24 +177,58 @@ static void hist_dma_cb(int lch, u16 ch_status, void > > *data) > > static int hist_buf_dma(struct ispstat *hist) > > { > > dma_addr_t dma_addr = hist->active_buf->dma_addr; > > + struct dma_async_tx_descriptor *tx; > > + struct dma_slave_config cfg; > > + dma_cookie_t cookie; > > + int ret; > > > > if (unlikely(!dma_addr)) { > > dev_dbg(hist->isp->dev, "hist: invalid DMA buffer address\n"); > > - hist_reset_mem(hist); > > - return STAT_NO_BUF; > > + goto error; > > } > > > > isp_reg_writel(hist->isp, 0, OMAP3_ISP_IOMEM_HIST, ISPHIST_ADDR); > > isp_reg_set(hist->isp, OMAP3_ISP_IOMEM_HIST, ISPHIST_CNT, > > ISPHIST_CNT_CLEAR); > > > > omap3isp_flush(hist->isp); > > > > - hist->dma_config.dst_start = dma_addr; > > - hist->dma_config.elem_count = hist->buf_size / sizeof(u32); > > - omap_set_dma_params(hist->dma_ch, &hist->dma_config); > > > > - omap_start_dma(hist->dma_ch); > > + memset(&cfg, 0, sizeof(cfg)); > > + cfg.src_addr = hist->isp->mmio_base_phys[OMAP3_ISP_IOMEM_HIST] > > + + ISPHIST_DATA; > > + cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > + cfg.src_maxburst = hist->buf_size / 4; > > How about initialising the struct when you declare it instead? That might be > a matter of opinion though, but I think I prefer that. Up to you. I sometimes agree with this argument, but in this case the separation between initialization and usage of the structure would in my opinion impede readability. I've also checked the code generated by the compiler, and except for memset being replaced by __memzero, initializing the structure at declaration time doesn't make a difference. > Acked-by: Sakari Ailus -- Regards, Laurent Pinchart