Comments inline.
..Remy
On 23/02/2016 02:06, Zhang, Helin wrote:
>
>> +static inline int
>> +i40e_read_regs(struct i40e_hw *hw, const struct reg_info *reg,
>> + uint32_t *reg_buf)
>> +{
>> + unsigned int i;
>> +
>> + for (i = 0; i < reg->count; i++)
>> + reg_buf[i] = I40E_READ_REG(hw,
>> + reg->base_addr + i * reg->stride);
>> + return reg->count;
>> +}
> From FVL5, some registers should be read by AQ commands, otherwise
it may fail to
> read without any warning.
> Please see my patches of which registers should be read by AQ commands.
> Please check i40e_osdep.h from below link. Thanks!
> http://www.dpdk.org/dev/patchwork/patch/10654/
Ok - will change for v2.
I noticed that other patches in the same patchset expose extra registers
- are these new or were they simply not exposed previously?
>> + /* Only support doing full dump */
>> + if (regs->offset != 0 && 0)
> '&& 0' means it will never be false, right?
> Anything wrong here?
Oops - some dead code that slipped through.. :)
>> + return -ENOTSUP;
> A message before this return to tell the uers what happened would be
better.
Will add these into v2.
>> +static int i40e_get_eeprom_length(__rte_unused struct rte_eth_dev *dev)
> Why needs __rte_unused?
Good point - surprised the compiler did not complain about them, as they
are not supposed to be there..
>> +static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
>> + struct ether_addr *mac_addr)
>> +{
>> + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
>>> dev_private);
>> +
>> + /* Flags: 0x3 updates port address */
>> + i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes,
>> NULL); }
> Checks are needed before writing the MAC address.
Will look into this.
>> +struct reg_info {
>> + uint32_t base_addr;
>> + uint32_t count;
>> + uint32_t stride;
>> + const char *name;
>> +} reg_info;
> I think array definition shouldn't be added into a header file,
otherwise any .c source
> file which includes that header file will define that.
Since it is quite a large table I think this approach, which is also
used in ixgbe, is the lesser of evils. i40e_ethdev.c itself is already
pretty big, and would prefer to avoid giving a driver-specific table
non-static visibility until it actually has to be used from other
compilation units.
