From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753590AbbJGLPz (ORCPT ); Wed, 7 Oct 2015 07:15:55 -0400 Received: from mga02.intel.com ([134.134.136.20]:9774 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752182AbbJGLPw (ORCPT ); Wed, 7 Oct 2015 07:15:52 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,648,1437462000"; d="scan'208";a="659515678" Date: Wed, 7 Oct 2015 12:15:31 +0100 From: Vinod Koul To: Peter Griffin Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, srinivas.kandagatla@gmail.com, maxime.coquelin@st.com, patrice.chotard@st.com, lee.jones@linaro.org, robh+dt@kernel.org, dmaengine@vger.kernel.org, devicetree@vger.kernel.org, Ludovic Barre Subject: Re: [PATCH v2 4/9] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support Message-ID: <20151007111529.GA4810@vkoul-mobl.iind.intel.com> References: <1441980871-24475-1-git-send-email-peter.griffin@linaro.org> <1441980871-24475-5-git-send-email-peter.griffin@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1441980871-24475-5-git-send-email-peter.griffin@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 11, 2015 at 03:14:26PM +0100, Peter Griffin wrote: > +static char *fdma_clk_name[CLK_MAX_NUM] = { > + [CLK_SLIM] = "fdma_slim", > + [CLK_HI] = "fdma_hi", > + [CLK_LOW] = "fdma_low", > + [CLK_IC] = "fdma_ic", > +}; why do we want to have this sort of data in kernel, we should get the clocks from DT here > +static int config_reqctrl(struct st_fdma_chan *fchan) > +{ > + u32 maxburst = 0, addr = 0; > + enum dma_slave_buswidth width; > + int ch_id = fchan->vchan.chan.chan_id; > + struct st_fdma_dev *fdev = fchan->fdev; > + > + if (fchan->scfg.direction == DMA_DEV_TO_MEM) { > + fchan->cfg.req_ctrl &= ~REQ_CTRL_WNR; > + maxburst = fchan->scfg.src_maxburst; > + width = fchan->scfg.src_addr_width; > + addr = fchan->scfg.src_addr; > + } else if (fchan->scfg.direction == DMA_MEM_TO_DEV) { > + fchan->cfg.req_ctrl |= REQ_CTRL_WNR; > + maxburst = fchan->scfg.dst_maxburst; > + width = fchan->scfg.dst_addr_width; > + addr = fchan->scfg.dst_addr; This raises red flags, why do we have direction in channel, and where is this set. Direction belong to a descriptor and you are supposed to use direction from prep_ calls! > + if (!is_slave_direction(direction)) { > + dev_err(fchan->fdev->dev, "bad direction?\n"); > + return NULL; > + } > + > + if (!config_reqctrl(fchan)) { > + dev_err(fchan->fdev->dev, "bad width or direction\n"); would make sense to print proper error in called fn > +static enum dma_status st_fdma_tx_status(struct dma_chan *chan, > + dma_cookie_t cookie, > + struct dma_tx_state *txstate) > +{ > + struct st_fdma_chan *fchan = to_st_fdma_chan(chan); > + struct virt_dma_desc *vd; > + enum dma_status ret; > + unsigned long flags; > + > + ret = dma_cookie_status(chan, cookie, txstate); > + if (ret == DMA_COMPLETE) > + return ret; > + > + if (!txstate) > + return fchan->status; why channel status and not cookie status ? > +static const struct st_fdma_ram fdma_mpe31_mem[] = { > + { .name = "dmem", .offset = 0x10000, .size = 0x3000 }, > + { .name = "imem", .offset = 0x18000, .size = 0x8000 }, > +}; why aren't these coming from DT ? > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("STMicroelectronics FDMA engine driver"); > +MODULE_AUTHOR("Ludovic.barre "); No MODULE_ALIAS ? -- ~Vinod