dma: Add Synopsys eDMA IP core driver
RFC 1/6 dma: Add Synopsys eDMA IP core driver
Hi,
Quoted 14 lines · Vinod Koul
Quoted 12 lines · Gustavo Pimentel
Quoted 11 lines
Review
Quoted 9 lines
(quoted diff) +9
-0+ +#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..
Quoted 5 lines
Quoted 3 lines
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 18 lines
Quoted 3 lines
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 11 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + 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?
Quoted 4 lines
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