Hi Jingjing, After we have aligned this morning, I will reply your email again.
> -----Original Message----- > From: Wu, Jingjing > Sent: Thursday, May 10, 2018 17:21 > To: Xu, Rosen <[email protected]>; [email protected]; [email protected] > Cc: Xu, Rosen <[email protected]>; Zhang, Roy Fan > <[email protected]>; Doherty, Declan <[email protected]>; > Richardson, Bruce <[email protected]>; [email protected]; > Yigit, Ferruh <[email protected]>; Ananyev, Konstantin > <[email protected]>; Zhang, Tianfei <[email protected]>; > Liu, Song <[email protected]>; Wu, Hao <[email protected]>; > [email protected]; Wu, Yanglong <[email protected]> > Subject: RE: [dpdk-dev] [PATCH v10 3/3] iFPGA: Add Intel FPGA BUS Rawdev > Driver > > Hi, Rosen > > Few comments below. > > Thanks > Jingjing > > [...] > > > +static int > > +ifpga_rawdev_start(struct rte_rawdev *dev) { > > + int ret = 0; > > + struct opae_adapter *adapter; > > + > > + IFPGA_RAWDEV_PMD_FUNC_TRACE(); > > + > > + RTE_FUNC_PTR_OR_ERR_RET(dev, -EINVAL); > > + > > + adapter = ifpga_rawdev_get_priv(dev); > > + if (!adapter) > > + return -ENODEV; > > + > Set dev->started? > > + return ret; > > +} Fixed. > > [...] > > > + > > +static const struct rte_rawdev_ops ifpga_rawdev_ops = { > > + .dev_info_get = ifpga_rawdev_info_get, > > + .dev_configure = NULL, > If go the declaration of rte_rawdev_configure, you will see "This function > must be invoked first before any other function in the API." > So I think we need to function for it, even it does nothing. Yes, the rte_rawdev_configure function should not NULL. I have fill it. > > [...] > > > +static struct rte_pci_driver rte_ifpga_rawdev_pmd = { > > + .id_table = pci_ifpga_map, > > + .drv_flags = RTE_PCI_DRV_NEED_MAPPING | > RTE_PCI_DRV_INTR_LSC, > Is RTE_PCI_DRV_INTR_LSC supported? Not supported, and I removed it. > [...] > > > +static struct rte_vdev_driver ifpga_cfg_driver = { > > + .probe = ifpga_cfg_probe, > > + .remove = ifpga_cfg_remove, > > +}; > > + > > +RTE_PMD_REGISTER_VDEV(net_ifpga_cfg, ifpga_cfg_driver); > I think prefix net_ would mean the device is net device (eth_dev)? How about > to change the prefix to raw_? > > > +RTE_PMD_REGISTER_ALIAS(net_ifpga_cfg, ifpga_cfg); > > +RTE_PMD_REGISTER_PARAM_STRING(net_ifpga_cfg, > > + "bdf=<string> " > ifpga=<string>? Fixed. > > + "port=<int> " > > + "afu_bts=<path>"); > > +
