See the reply below.
-----Original Message-----
From: Yigit, Ferruh
Sent: Tuesday, January 3, 2017 11:03 PM
To: Yang, Qiming <qiming.y...@intel.com>; dev@dpdk.org;
thomas.monja...@6wind.com
Cc: Horton, Remy <remy.hor...@intel.com>
Subject: Re: [PATCH v3 2/4] net/e1000: add firmware version get
On 12/27/2016 12:30 PM, Qiming Yang wrote:
> This patch adds a new function eth_igb_fw_version_get.
>
> Signed-off-by: Qiming Yang <qiming.y...@intel.com>
> ---
> v3 changes:
> * use eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
> u32 *fw_minor, u32 *fw_minor, u32 *fw_patch, u32 *etrack_id) instead
> of eth_igb_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
> int fw_length). Add statusment in /doc/guides/nics/features/igb.ini.
> ---
> ---
> doc/guides/nics/features/igb.ini | 1 +
> drivers/net/e1000/igb_ethdev.c | 43
> ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/doc/guides/nics/features/igb.ini
> b/doc/guides/nics/features/igb.ini
> index 9fafe72..ffd87ba 100644
> --- a/doc/guides/nics/features/igb.ini
> +++ b/doc/guides/nics/features/igb.ini
> @@ -39,6 +39,7 @@ EEPROM dump = Y
> Registers dump = Y
> BSD nic_uio = Y
> Linux UIO = Y
> +FW version = Y
Please keep same location with default.ini file. Why you are putting this just
into middle of the uio and vfio?
Qiming: It's a clerical error, I want to add this line at the end of this file.
> Linux VFIO = Y
> x86-32 = Y
> x86-64 = Y
> diff --git a/drivers/net/e1000/igb_ethdev.c
> b/drivers/net/e1000/igb_ethdev.c index 4a15447..25344b7 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -120,6 +120,8 @@ static int eth_igb_xstats_get_names(struct rte_eth_dev
> *dev,
> unsigned limit);
> static void eth_igb_stats_reset(struct rte_eth_dev *dev); static
> void eth_igb_xstats_reset(struct rte_eth_dev *dev);
> +static void eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
> + u32 *fw_minor, u32 *fw_patch, u32 *etrack_id);
I think you can use a struct as parameter here. But beware, that struct should
NOT be a public struct.
Qiming: I think only add a private struct for igb is unnecessary. Keep the
arguments consistent with rte_eth_dev_fw_info_get is better.
What do you think?
> static void eth_igb_infos_get(struct rte_eth_dev *dev,
> struct rte_eth_dev_info *dev_info); static const
> uint32_t
> *eth_igb_supported_ptypes_get(struct rte_eth_dev *dev); @@ -389,6
> +391,7 @@ static const struct eth_dev_ops eth_igb_ops = {
> .xstats_get_names = eth_igb_xstats_get_names,
> .stats_reset = eth_igb_stats_reset,
> .xstats_reset = eth_igb_xstats_reset,
> + .fw_version_get = eth_igb_fw_version_get,
> .dev_infos_get = eth_igb_infos_get,
> .dev_supported_ptypes_get = eth_igb_supported_ptypes_get,
> .mtu_set = eth_igb_mtu_set,
> @@ -1981,6 +1984,46 @@ eth_igbvf_stats_reset(struct rte_eth_dev *dev)
> }
>
<...>
