From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH v4 1/5] ethdev: add access to eeprom
Date: Wed, 25 Apr 2018 10:31:44 +0200
Message-ID: <35483134.QvJqGnanPf@xps>
References: <1521630379-9554-1-git-send-email-zijie.pan@6wind.com>
 <2623487.7NZ038SaYX@xps> <tencent_350B2AA47931B43446D17921@qq.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev <dev@dpdk.org>, "remy.horton" <remy.horton@intel.com>,
 "ferruh.yigit" <ferruh.yigit@intel.com>,
 "john.mcnamara" <john.mcnamara@intel.com>,
 "marko.kovacevic" <marko.kovacevic@intel.com>
To: Zijie Pan <zijie.pan@6wind.com>
Return-path: <dev-bounces@dpdk.org>
Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com
 [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 074B310BD
 for <dev@dpdk.org>; Wed, 25 Apr 2018 10:31:46 +0200 (CEST)
In-Reply-To: <tencent_350B2AA47931B43446D17921@qq.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://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: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

25/04/2018 10:21, Zijie Pan:
> Hi Thomas,
> 
> > > +/**
> > > + * Placeholder for accessing plugin module eeprom
> > > + */
> > > +struct rte_dev_module_info {
> > > + uint32_t type; /**< Type of plugin module eeprom */
> > > + uint32_t eeprom_len; /**< Length of plugin module eeprom */
> > > +};
> >
> > I am not sure "plugin module" is descriptive enough.
> 
> Here is the description when "man ethtool":
> -m --dump-module-eeprom --module-info
>     Retrieves  and if possible decodes the EEPROM from plugin modules, e.g SFP+, QSFP.
> Is there any suggestion how to describe it?

No better idea.

> > And I think the structure name should be rte_eth_dev_module_info
> > to make clear that we are talking about NIC modules.
> > Any better idea?
> 
> Yes, rte_eth_dev_module_info is better than rte_dev_module_info.
> But the name of other structures should be also changed. And the file name
> should be changed to rte_eth_dev_info.h.
> Do we need to make a patch first to change the name of the file and structures?

No you can just focus on your change.
If more clean-up is required in your opinion, you can do it later.


> > > +int __rte_experimental
> > > +rte_eth_dev_get_module_eeprom(uint16_t port_id,
> > > +       struct rte_dev_eeprom_info *info)
> > > +{
> > > + struct rte_eth_dev *dev;
> > > +
> > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > > +
> > > + dev = &rte_eth_devices[port_id];
> > > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_module_eeprom, -ENOTSUP);
> > > + return (*dev->dev_ops->get_module_eeprom)(dev, info);
> > > +}
> > > +
> > >  int
> > >  rte_eth_dev_get_dcb_info(uint16_t port_id,
> >
> > Please move this code after other EEPROM related functions.
> I put the functions after rte_eth_dev_set_eeprom(). Please apply the patch and check it.

OK

> > > --- a/lib/librte_ether/rte_ethdev_version.map
> > > +++ b/lib/librte_ether/rte_ethdev_version.map
> > > @@ -229,5 +229,7 @@ EXPERIMENTAL {
> > >  rte_mtr_policer_actions_update;
> > >  rte_mtr_stats_read;
> > >  rte_mtr_stats_update;
> > > + rte_eth_dev_get_module_info;
> > > + rte_eth_dev_get_module_eeprom;
> >
> > This must be inserted in alphabetical order.
> I will update it in v5 patchset.

Please send v5 ASAP, thanks.