Re: [dpdk-dev] [PATCH v6 03/14] net/ipn3ke: add IPN3KE ethdev PMD driver

[Stephen Hemminger <stephen@networkplumber.org>]

Minor nits.

> +
> +	(*rd_data) = rte_le_to_cpu_32(read_data);
> +	return 0;
> +}

Paren around *rd_data are unnecessary

> +	indirect_addrs = (volatile void *)(hw->eth_group_bar[eth_group_sel] +
> +		0x10);

cast is to void * is unnecesary

+	return ipn3ke_indirect_read(hw,
+				rd_data,
+				addr,
+				dev_sel,
+				eth_group_sel);

One parameter per line is not necessary here.

+static int
+ipn3ke_hw_cap_init(struct ipn3ke_hw *hw)
+{

Always returns 0 make it void
> +		/* representor port net_bdf_port */
> +		snprintf(name, sizeof(name), "net_%s_representor_%d",
> +			afu_dev->device.name, i);

That seems like a longer than necessary eth_dev_name and doesn't seem
to follow the convention of using PCI address.

Why do only Intel drivers call rte_eth_dev_create, everyone else calls
rte_eth_dev_allocate()?

> +
> +	hw = (struct ipn3ke_hw *)afu_dev->shared.data;

Cast of void * is unnecessary in C.

> +	for (i = 0; i < hw->port_num; i++) {
> +		snprintf(name, sizeof(name), "net_%s_representor_%d",

You use this format twice, it should be a #define.

> +static inline uint32_t _ipn3ke_indrct_read(struct ipn3ke_hw *hw,
> +		uint32_t addr)
> +{
> +	uint64_t word_offset = 0;
> +	uint64_t read_data = 0;
> +	uint64_t indirect_value = 0;
> +	volatile void *indirect_addrs = 0;

You set all these values in next view lines so these initializations
are useless. Doing extra initialization is bad since it overrides the
ability of compiler and static checkers to find code paths where data
is used uninitialized.

> +static inline void ipn3ke_xmac_smac_ovd_dis(struct ipn3ke_hw *hw,
> +	uint32_t mac_num, uint32_t eth_group_sel)
> +{
> +#define IPN3KE_XMAC_SMAC_OVERRIDE_DISABLE (0 & \
> +	(IPN3KE_MAC_TX_SRC_ADDR_OVERRIDE_MASK))
> +
> +	(*hw->f_mac_write)(hw,
> +					IPN3KE_XMAC_SMAC_OVERRIDE_DISABLE,
> +					IPN3KE_MAC_TX_SRC_ADDR_OVERRIDE,
> +					mac_num,
> +					eth_group_sel);


Indentation issue here?


> +extern int ipn3ke_afu_logtype;
> +
> +#define IPN3KE_AFU_PMD_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, ipn3ke_afu_logtype, "ipn3ke_afu: " fmt, \
> +		##args)

The common practice in Intel drivers is to put the newline in this macro
(and remove it from the format strings). Also put the function name rather
than always ipn3ke_afu: in the log message?