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 <jiawe...@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. <...>