Hi, > -----Original Message----- > From: Chautru, Nicolas <nicolas.chau...@intel.com> > Sent: Tuesday, April 14, 2020 8:16 > To: Xu, Rosen <rosen...@intel.com>; dev@dpdk.org; akhil.go...@nxp.com > Cc: Richardson, Bruce <bruce.richard...@intel.com> > Subject: RE: [dpdk-dev] [PATCH v2 06/13] baseband/fpga_5gnr_fec: add > queue configuration > > Thanks Rosen for your thorough code review. Some comments in-line below. > > > From: Xu, Rosen <rosen...@intel.com> > > > > Hi, > > > > Could you prefix all functions name with the FPGA IP name? FPGA is a > > very common device name. > > > > I don't see such a guideline being used across all other PMDs. > Unsure it always help notably as this would create many long function names > with fpga_5gnr_fec_ added each time, and these names are not exposed > outside of this .c file. > Also some of the fpga_ prefixes would apply for either fpga_lte_fec or > fpga_5gnr_fec PMD. > If this becomes of a new guide lines we can rename every single function in > each existing baseband PMD in future release (not just this new PMD).
What I mentioned is that, let's take FVL for example, in our PMD, we name it's PMD functions with I40e_xxx, i40e is Intel NIC name, but for your design it named with fpga_5gnr_xxx, fpga is a common device, That means not Intel only provide FPGA, no sure if any other developer summit other FPGA based 5G acceleration IP, is it ok? > > > /* Read a register of FPGA 5GNR FEC device */ static uint32_t > > > fpga_reg_read_32(void *mmio_base, uint32_t offset) @@ -31,9 > +106,115 > > > @@ > > > return rte_le_to_cpu_32(ret); > > > } > > > > Why you didn't use rte_writeXXX() API of DPDK rte library? > > > > rte_write32 includes some memory barriers which are not required here. > Also we handle the byte endianness in these internal implementations. > > Thanks, > Nic
