Re: [PATCH v2 4/9] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support

[Vinod Koul <vinod.koul@intel.com>]

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 <Ludovic.barre@st.com>");

No MODULE_ALIAS ?

-- 
~Vinod