Messages in this thread | | | Date | Fri, 2 Mar 2018 13:43:26 +0530 | From | Vinod Koul <> | Subject | Re: [PATCH v2 1/2] dmaengine: Introduce DW AXI DMAC driver |
| |
On Mon, Feb 26, 2018 at 05:56:27PM +0300, Eugeniy Paltsev wrote:
> +/* > + * Synopsys DesignWare AXI DMA Controller driver. > + * > + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com) > + * Author: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> > + * > + * SPDX-License-Identifier: GPL-2.0 > + */
This needs to be in below style
// SPDX-License-Identifier: GPL-2.0 // Copyright (C) ....
/* * other things u want to add */
> +#define DRV_NAME "axi_dw_dmac"
how about use KBUILD_MODNAME here?
> +static enum dma_status > +dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie, > + struct dma_tx_state *txstate) > +{ > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan); > + enum dma_status ret; > + > + ret = dma_cookie_status(dchan, cookie, txstate); > + > + if (chan->is_paused && ret == DMA_IN_PROGRESS) > + return DMA_PAUSED; > + > + return ret;
nitpick, how about
if (chan->is_paused && ret == DMA_IN_PROGRESS) ret = DMA_PAUSED;
return ret;
that way we have single point of return.
> +static int dma_chan_alloc_chan_resources(struct dma_chan *dchan) > +{ > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan); > + > + /* ASSERT: channel is idle */ > + if (axi_chan_is_hw_enable(chan)) { > + dev_err(chan2dev(chan), "%s is non-idle!\n", > + axi_chan_name(chan)); > + return -EBUSY; > + } > + > + dev_vdbg(dchan2dev(dchan), "%s: allocating\n", axi_chan_name(chan)); > + > + dma_cookie_init(dchan);
This is already done as part of vchan_init(), so why is it called explicitly and why is that not invoked here?
> +static void dma_chan_free_chan_resources(struct dma_chan *dchan) > +{ > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan); > + > + /* ASSERT: channel is idle */ > + if (axi_chan_is_hw_enable(chan)) > + dev_err(dchan2dev(dchan), "%s is non-idle!\n", > + axi_chan_name(chan)); > + > + axi_chan_disable(chan); > + axi_chan_irq_disable(chan, DWAXIDMAC_IRQ_ALL); > + > + vchan_free_chan_resources(&chan->vc); > + > + dev_vdbg(dchan2dev(dchan), "%s: free resources, descriptor still allocated: %u\n",
making the string in subsequent line make it lesser than 80 chars, here and few other places can benefit..
> +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id) /s/intretupt/interrupt/ ?
> +static int axi_dma_resume(struct axi_dma_chip *chip) > +{ > + int ret = 0;
superfluous init
> +static int parse_device_properties(struct axi_dma_chip *chip) > +{ > + struct device *dev = chip->dev; > + u32 tmp, carr[DMAC_MAX_CHANNELS]; > + int ret; > + > + ret = device_property_read_u32(dev, "dma-channels", &tmp); > + if (ret) > + return ret; > + if (tmp == 0 || tmp > DMAC_MAX_CHANNELS) > + return -EINVAL; > + > + chip->dw->hdata->nr_channels = tmp; > + > + ret = device_property_read_u32(dev, "snps,dma-masters", &tmp);
why is it snps,... shouldn't it be only dma-masters?
> +enum { > + DWAXIDMAC_ARWLEN_1 = 0x0, > + DWAXIDMAC_ARWLEN_2 = 0x1, > + DWAXIDMAC_ARWLEN_4 = 0x3, > + DWAXIDMAC_ARWLEN_8 = 0x7, > + DWAXIDMAC_ARWLEN_16 = 0xF, > + DWAXIDMAC_ARWLEN_32 = 0x1F, > + DWAXIDMAC_ARWLEN_64 = 0x3F, > + DWAXIDMAC_ARWLEN_128 = 0x7F, > + DWAXIDMAC_ARWLEN_256 = 0xFF,
GENMASK() for these?
-- ~Vinod
| |