From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH 1/5] ethdev: add firmware version get
Date: Thu, 17 Nov 2016 14:36:39 +0100
Message-ID: <1520754.cvFYjLZGdK@xps13>
References: <1479375779-46629-1-git-send-email-qiming.yang@intel.com>
 <1479375779-46629-2-git-send-email-qiming.yang@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, remy.horton@intel.com, jingjing.wu@intel.com,
 jing.d.chen@intel.com
To: Qiming Yang <qiming.yang@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f49.google.com (mail-wm0-f49.google.com [74.125.82.49])
 by dpdk.org (Postfix) with ESMTP id 6AB9C36E
 for <dev@dpdk.org>; Thu, 17 Nov 2016 14:36:41 +0100 (CET)
Received: by mail-wm0-f49.google.com with SMTP id g23so311442747wme.1
 for <dev@dpdk.org>; Thu, 17 Nov 2016 05:36:41 -0800 (PST)
In-Reply-To: <1479375779-46629-2-git-send-email-qiming.yang@intel.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

2016-11-17 17:42, Qiming Yang:
> This patch added API for 'rte_eth_dev_fwver_get'
> 
> void rte_eth_dev_fwver_get(uint8_t port_id,
> char *fw_version, int fw_length);

Copying some code here doesn't help really help.
Could you describe what we can expect in this string?
How can we compare this version number across different
devices of the same driver?
How does it apply to FPGA devices?
What is the potential use?

[...]
>  /**
> + * Retrieve the firmware version of an Ethernet device.

We should stop talking about Ethernet device.
Networking device is more appropriate.
And in this case, "device" is enough.

> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param fw_version
> + *   A pointer the firmware version of an Ethernet device
> + * @param fw_length
> + *   The size of the firmware version, which should be large enough to store
> + *   the firmware version of the device.

How do we know that the length is too small?

> + */
> +void rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length);

Why not returning some errors?

You forgot to remove the deprecation notice in this patch.

PS: the series is broken as some patches are not numbered and this one is
not the first one. Please use a cover-letter to introduce such series.