linux-kernel - Re: [RFC PATCH 7/7] DMA: tegra-adma: Add support for Tegra210 ADMA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150824092440.GJ13546@localhost>
Date:	Mon, 24 Aug 2015 14:54:40 +0530
From:	Vinod Koul <vinod.koul@...el.com>
To:	Jon Hunter <jonathanh@...dia.com>
Cc:	Laxman Dewangan <ldewangan@...dia.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Alexandre Courbot <gnurou@...il.com>,
	dmaengine@...r.kernel.org, linux-tegra@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [RFC PATCH 7/7] DMA: tegra-adma: Add support for Tegra210 ADMA

On Mon, Aug 24, 2015 at 09:55:03AM +0100, Jon Hunter wrote:
> >> +static int tegra_adma_get_xfer_params(struct tegra_dma_channel *tdc,
> >> +				      struct tegra_adma_chan_regs *ch_regs,
> >> +				      enum dma_transfer_direction direction)
> >> +{
> >> +	u32 burst_size, ctrl, ctrl_mask, slave_id, fifo_mask, fifo_shift;
> >> +
> >> +	ch_regs->ahub_fifo_ctrl = tdc_read(tdc, ADMA_CH_AHUB_FIFO_CTRL);
> >> +	ch_regs->config = tdc_read(tdc, ADMA_CH_CONFIG);
> >> +	ch_regs->ctrl = tdc_read(tdc, ADMA_CH_CTRL);
> >> +	slave_id = tdc->dma_sconfig.slave_id;
> >> +
> >> +	switch (direction) {
> >> +	case DMA_MEM_TO_DEV:
> >> +		burst_size = fls(tdc->dma_sconfig.dst_maxburst);
> >> +		ctrl_mask = ADMA_CH_CTRL_TX_REQUEST_SELECT_MASK;
> >> +		ctrl = MEMORY_TO_AHUB << ADMA_CH_CTRL_TRANSFER_DIRECTION_SHIFT;
> >> +		ctrl |= slave_id << ADMA_CH_CTRL_TX_REQUEST_SELECT_SHIFT;
> >> +		fifo_mask = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_MASK;
> >> +		fifo_shift = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_SHIFT;
> >> +		break;
> > Empty line here pls
> 
> Ok, any reason why? Other dma drivers don't appear to do this.

This improves readablity. It is a good idea to sprinkle empty lines between
logial blocks

> >> +static int tegra_adma_pm_suspend(struct device *dev)
> >> +{
> >> +	struct tegra_dma *tdma = dev_get_drvdata(dev);
> >> +	int i;
> >> +	int ret;
> >> +
> >> +	ret = pm_runtime_get_sync(dev);
> > why is this required :)
> 
> To ensure that the clocks are enabled before the registers are read.
> This function saves the dma context before suspend, in case the hardware
> state is lost.
I think same reason here too, so based on my other argument, this may not be
required!

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/