Re: [dpdk-dev] [PATCH v1 03/42] net/txgbe: add device init and uninit

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/1/2020 12:50 PM, Jiawen Wu wrote:
> Add basic init and uninit function, registers and some macro definitions prepare for hardware infrastructure.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

<...>

> +static const struct eth_dev_ops txgbe_eth_dev_ops = {
> +	.dev_start                  = txgbe_dev_start,
> +	.dev_stop                   = txgbe_dev_stop,
> +	.dev_close                  = txgbe_dev_close,
> +	.stats_get                  = txgbe_dev_stats_get,
> +	.stats_reset                = txgbe_dev_stats_reset,

What do you think adding '.stats_get' & '.stats_reset' when
'txgbe_dev_stats_get()' & 'txgbe_dev_stats_reset()' implemented (patch 27/42).

Same is also for '.dev_start' & '.dev_stop'. (It will work if you drop
'txgbe_dev_stop()' from 'txgbe_dev_close()', and since the device did not start
at first place, it should be OK, it can be added where device start/stop support
added.)

I see you are using empty functions to constract the driver, I think better to
reduce the number of the empty functions as much as possible although sometimes
you may have to add them, you know it better.

<...>

> +++ b/drivers/net/txgbe/txgbe_pf.c
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2015-2020
> + */
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <stdarg.h>
> +#include <inttypes.h>
> +
> +#include <rte_interrupts.h>
> +#include <rte_log.h>
> +#include <rte_debug.h>
> +#include <rte_eal.h>
> +#include <rte_ether.h>
> +#include <rte_ethdev_driver.h>
> +#include <rte_memcpy.h>
> +#include <rte_malloc.h>
> +#include <rte_random.h>

Similar comment for all new files, if the include list really required or they
are from copy/paste. Can you only keep the includes needed, and add them as they
needed.

<...>