[dpdk-dev] [PATCH v3 2/4] net/e1000: add firmware version get

[Yang, Qiming]

See the reply below.

-----Original Message-----
From: Yigit, Ferruh 
Sent: Tuesday, January 3, 2017 11:03 PM
To: Yang, Qiming <qiming.yang at intel.com>; dev at dpdk.org; thomas.monjalon at 6wind.com
Cc: Horton, Remy <remy.horton at 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.yang at 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)  
> }
>  

<...>