Hi,

On 18/12/2018 03:54, Vinod Koul wrote:

On 17-12-18, 15:56, Gustavo Pimentel wrote:

quoted

quoted

quoted

+
+#define SET(reg, name, val)			\
+	reg.name = val
+
+#define SET_BOTH_CH(name, value)		\
+	do {					\
+		SET(dw->wr_edma, name, value);	\
+		SET(dw->rd_edma, name, value);	\
+	} while (0)

I am not sure how this helps, makes things not explicit..

Since this driver has 2 channels (write and read) I'd like to simplify all the
configurations that I've to make on both channels (avoiding any omission), that
why I created this macro.

So in order to configure a channel you need to write to both?

No. Let me show you in a different point of view. I think we're talking about
different things.

On dw_edma_probe() ~ line 755

I've this:

SET_BOTH_CH(src_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET_BOTH_CH(dst_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET_BOTH_CH(residue_granularity, DMA_RESIDUE_GRANULARITY_DESCRIPTOR);

SET_BOTH_CH(dev, chip->dev);

SET_BOTH_CH(device_alloc_chan_resources, dw_edma_alloc_chan_resources);
SET_BOTH_CH(device_free_chan_resources, dw_edma_free_chan_resources);

SET_BOTH_CH(device_config, dw_edma_device_config);
SET_BOTH_CH(device_pause, dw_edma_device_pause);
SET_BOTH_CH(device_resume, dw_edma_device_resume);
SET_BOTH_CH(device_terminate_all, dw_edma_device_terminate_all);
SET_BOTH_CH(device_issue_pending, dw_edma_device_issue_pending);
SET_BOTH_CH(device_tx_status, dw_edma_device_tx_status);
SET_BOTH_CH(device_prep_slave_sg, dw_edma_device_prep_slave_sg);

which is equivalent to do this:

SET(dw->wr_edma, src_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET(dw->rd_edma, src_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET(dw->wr_edma, dst_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET(dw->rd_edma, dst_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET(dw->wr_edma, residue_granularity, DMA_RESIDUE_GRANULARITY_DESCRIPTOR);
SET(dw->rd_edma, residue_granularity, DMA_RESIDUE_GRANULARITY_DESCRIPTOR);

SET(dw->wr_edma, dev, chip->dev);
SET(dw->rd_edma, dev, chip->dev);

SET(dw->wr_edma, device_alloc_chan_resources, dw_edma_alloc_chan_resources);
SET(dw->rd_edma, device_alloc_chan_resources, dw_edma_alloc_chan_resources);
SET(dw->wr_edma, device_free_chan_resources, dw_edma_free_chan_resources);
SET(dw->rd_edma, device_free_chan_resources, dw_edma_free_chan_resources);

SET(dw->wr_edma, device_config, dw_edma_device_config);
SET(dw->rd_edma, device_config, dw_edma_device_config);
SET(dw->wr_edma, device_pause, dw_edma_device_pause);
SET(dw->rd_edma, device_pause, dw_edma_device_pause);
SET(dw->wr_edma, device_resume, dw_edma_device_resume);
SET(dw->rd_edma, device_resume, dw_edma_device_resume);
SET(dw->wr_edma, device_terminate_all, dw_edma_device_terminate_all);
SET(dw->rd_edma, device_terminate_all, dw_edma_device_terminate_all);
SET(dw->wr_edma, device_issue_pending, dw_edma_device_issue_pending);
SET(dw->rd_edma, device_issue_pending, dw_edma_device_issue_pending);
SET(dw->wr_edma, device_tx_status, dw_edma_device_tx_status);
SET(dw->rd_edma, device_tx_status, dw_edma_device_tx_status);
SET(dw->wr_edma, device_prep_slave_sg, dw_edma_device_prep_slave_sg);
SET(dw->rd_edma, device_prep_slave_sg, dw_edma_device_prep_slave_sg);

What you type of coding style do prefer in your opinion?
I only created the macro to simplify the visual coding view of those operations.

quoted

Should I add some comment on top of this macro or do you think that is better to
replicate the code for each channel?

That will help to explain..

quoted

quoted

quoted

+
+	err = ops->device_config(dchan);

okay what does this callback do. You are already under and dmaengine fwk
so what is the need to add one more abstraction layer, can you explain
that in details please

This callback just configures the eDMA HW block interrupt address (abort and
done) and data for each channel. This callback could easily moved to the
dw_edma_probe() where each channel is created at first.
Should I do it in your opinion?

My question is about callbacks in this driver in general. You are
already under a dmaengine fwk, so this is adding one more layer on top
with callbacks implementing low level stiff. Why do we need another layer
is the question..

Feel free to ask anytime.

I created a second layer, because I've knowledge that will be necessary to
implement in the future several eDMA registers mapping versions, which may or
may not require a different handling, that's why I created that layer.

Gustavo