From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 76D702BB0 for ; Wed, 24 Feb 2016 11:32:29 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 24 Feb 2016 02:32:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,493,1449561600"; d="scan'208";a="752301279" Received: from rhorton-mobl.ger.corp.intel.com (HELO [163.33.230.115]) ([163.33.230.115]) by orsmga003.jf.intel.com with ESMTP; 24 Feb 2016 02:32:27 -0800 Message-ID: <56CD86BA.7040702@intel.com> Date: Wed, 24 Feb 2016 10:32:26 +0000 From: Remy Horton Organization: Intel Shannon Limited User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: "Zhang, Helin" , "Xie, Huawei" References: <1453970895-2639-1-git-send-email-remy.horton@intel.com> <1453970895-2639-2-git-send-email-remy.horton@intel.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v1 1/3] drivers/net/i40e: Add ethdev functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Feb 2016 10:32:29 -0000 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.