Hi,
On 18/12/2018 03:54, Vinod Koul wrote:
> On 17-12-18, 15:56, Gustavo Pimentel wrote:
>
>>>> +
>>>> +#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.
>
>> 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..
>
>>>> +
>>>> + 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
>
