Re: [dpdk-dev] [PATCH v2] app/test-pmd: add IFPGA AFU register read/write access for testpmd

["Pattan, Reshma" <reshma.pattan@intel.com>]

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rosen Xu
> Sent: Friday, December 14, 2018 1:14 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>;
> Xu, Rosen <rosen.xu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: [dpdk-dev] [PATCH v2] app/test-pmd: add IFPGA AFU register
> read/write access for testpmd
> 
> Currently register read/write of testpmd is only for PCI device, but more and
> more IFPGA based AFU devices need this feature to access registers, this patch
> will add support for it.
> 
> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> -	pci_len = pci_dev->mem_resource[0].len;
> -	if (reg_off >= pci_len) {
> +	if (reg_off >= len) {
>  		printf("Port %d: register offset %u (0x%X) out of port PCI "

Here log message mentions only PCI not ifpga device. Might need to edit the log.

> port_reg_bit_display(portid_t port_id, uint32_t reg_off, uint8_t bit_x)  {
>  	uint32_t reg_v;
> -
> +	const struct rte_bus *bus;
> 
>  	if (port_id_is_invalid(port_id, ENABLED_WARN))
>  		return;
> @@ -935,7 +940,16 @@ void print_valid_ports(void)
>  		return;
>  	if (reg_bit_pos_is_invalid(bit_x))
>  		return;
> -	reg_v = port_id_pci_reg_read(port_id, reg_off);
> +
> +	bus = rte_bus_find_by_device(ports[port_id].dev_info.device);
> +	if (bus && !strcmp(bus->name, "pci")) {
> +		reg_v = port_id_pci_reg_read(port_id, reg_off);
> +	} else if (bus && !strcmp(bus->name, "ifpga")) {
> +		reg_v = port_id_afu_reg_read(port_id, reg_off);
> +	} else {
> +		printf("Not a PCI or AFU device\n");
> +		return;
> +	}

Here and in other places for reg_read , we have similar code  i.e. finding the device , checking its type, if ifpga call ifpga function else call pci functions.
Can this common code be moved to new function say pci_read_reg like port_reg_set() which we have already.
Also , again inside respective pci/ifpga reg read/write we are checking for pci type. So can all this be simplified, to remove redundant code.

Thanks,
Reshma