[dpdk-dev] [PATCH v21 1/7] dmadev: introduce DMA device library public APIs

[fengchengwen]

Hi Thomas,

Most fixed in V22, some please see inline comment

Thanks.

On 2021/9/9 18:33, Thomas Monjalon wrote:
> Hi,
> 
> I am having a surface look at the API.
> I hope we can do better than previous libs.
> 
> 07/09/2021 14:56, Chengwen Feng:
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -496,6 +496,10 @@ F: drivers/raw/skeleton/
>>  F: app/test/test_rawdev.c
>>  F: doc/guides/prog_guide/rawdev.rst
>>  
>> +DMA device API - EXPERIMENTAL
>> +M: Chengwen Feng <fengchengwen at huawei.com>
>> +F: lib/dmadev/

[snip]

>> +
>> +/* Enumerates DMA device capabilities. */
> 
> You should group them with a doxygen group syntax.
> See https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/

Because RTE_DMADEV_CAPA_* has multiple lines of comments, the effect is not good
when using group syntax.

Also consider using enum to define, but its value is uint64_t, and enumeration is
generally of the int type.

So it stays the same here.

> 
>> +#define RTE_DMADEV_CAPA_MEM_TO_MEM	(1ull << 0)
> 
> Please use RTE_BIT macro (32 or 64).
> 
>> +/**< DMA device support memory-to-memory transfer.
>> + *
>> + * @see struct rte_dmadev_info::dev_capa
>> + */
> 

[snip]

> 
> This series add one file per patch.
> Instead it would be better to have groups of features per patch,
> meaning the implementation and the driver interface should be
> in the same patch.
> You can split like this:
> 	1/ device allocation
> 	2/ configuration and start/stop
> 	3/ dataplane functions
> 
> I would suggest 2 more patches:
> 	4/ event notification
> see https://patches.dpdk.org/project/dpdk/patch/20210730135533.417611-3-thomas@monjalon.net/
> 	5/ multi-process
> see https://patches.dpdk.org/project/dpdk/patch/20210730135533.417611-5-thomas@monjalon.net/
> 

The multi-process have many modify for device allocation, because the coupling
between the two is deep, I combines them to one patch.

> 
> Thanks for the work
> 
> 
> .
>