From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 07606A054F; Wed, 25 May 2022 11:40:39 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D67D740146; Wed, 25 May 2022 11:40:38 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id D543B400EF for ; Wed, 25 May 2022 11:40:37 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 599738F; Wed, 25 May 2022 12:40:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 599738F DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1653471637; bh=bQrkxhqTEHJj8uJnUC5XnFM6YkInYtDkYl1eZgeNtQ0=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=huwRwDL0tz+AE9FXZk+bkKZXXrztHNmBCUesiqFgqtQ7vTKxsQtFDakuIvwRu0AWx tkN5e5syIdfvfpzgIYIojtURH6OFNCNYMqUFTdraXCOfJ1CgTylG+tfhn1scqAz663 klK0uWy6T328VCEvmzr+uwULYP5J0Hy6be8TyeUs= Message-ID: Date: Wed, 25 May 2022 12:40:36 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH v8 2/5] ethdev: common utilities for different SFF specs Content-Language: en-US To: Robin Zhang , dev@dpdk.org Cc: thomas@monjalon.net, kevinx.liu@intel.com References: <20220524062442.194809-1-robinx.zhang@intel.com> <20220525031446.72578-1-robinx.zhang@intel.com> <20220525031446.72578-3-robinx.zhang@intel.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220525031446.72578-3-robinx.zhang@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Summary must not be a statement. Consider: "add common code for different SFF specs" or something like this. On 5/25/22 06:14, Robin Zhang wrote: > This patch implements SFF-8024 Rev 4.0 of pluggable I/O configuration "This patch implements" -> "Add support for" Basicaly "This patches" does not make sense. It the description of the patch in any case. > and some common utilities for SFF-8436/8636 and SFF-8472/8079. > > Signed-off-by: Robin Zhang > --- > lib/ethdev/meson.build | 1 + > lib/ethdev/sff_common.c | 326 ++++++++++++++++++++++++++++++++++++++++ > lib/ethdev/sff_common.h | 174 +++++++++++++++++++++ > 3 files changed, 501 insertions(+) > create mode 100644 lib/ethdev/sff_common.c > create mode 100644 lib/ethdev/sff_common.h > > diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build > index 49c77acb3f..8f39739e43 100644 > --- a/lib/ethdev/meson.build > +++ b/lib/ethdev/meson.build > @@ -12,6 +12,7 @@ sources = files( > 'rte_mtr.c', > 'rte_tm.c', > 'ethdev_sff_telemetry.c', > + 'sff_common.c', > ) > > headers = files( > diff --git a/lib/ethdev/sff_common.c b/lib/ethdev/sff_common.c > new file mode 100644 > index 0000000000..06d96fac72 > --- /dev/null > +++ b/lib/ethdev/sff_common.c > @@ -0,0 +1,326 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2022 Intel Corporation > + * > + * Implements SFF-8024 Rev 4.0 of pluggable I/O configuration and some > + * common utilities for SFF-8436/8636 and SFF-8472/8079 > + * Please, remove above extra empty line. > + */ > + > +#include > +#include Please, add empty line after system headers. > +#include Why do you need rte_mbuf.h? > +#include Do you really need it? Looks like no. > +#include Why do you need rte_flow.h? > +#include "sff_common.h" > +#include "ethdev_sff_telemetry.h" Library headers should go last and should be included using double quotes. [snip] > diff --git a/lib/ethdev/sff_common.h b/lib/ethdev/sff_common.h > new file mode 100644 > index 0000000000..264fb915cd > --- /dev/null > +++ b/lib/ethdev/sff_common.h > @@ -0,0 +1,174 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2022 Intel Corporation > + * > + * Implements SFF-8024 Rev 4.0 of pluggable I/O configuration and some > + * common utilities for SFF-8436/8636 and SFF-8472/8079 > + * > + */ > + > +#ifndef _SFF_COMMON_H_ > +#define _SFF_COMMON_H_ > + > +#include > +#include > +#include > +#include > +#include "ethdev_sff_telemetry.h" same comments about headers > +/* Most common case: 16-bit unsigned integer in a certain unit */ > +#define OFFSET_TO_U16(offset) \ > + (data[offset] << 8 | data[(offset) + 1]) Please, add SFF_ prefix to all these macros. It will match sff_ prefix of the header name and make it clear in the code which uses these macros. > + > +#define SPRINT_xX_PWR(str, var) \ SNPRINT_xX_PWR to highlight that it should be safe printing to a string > + snprintf(str, sizeof(str), "%.4f mW / %.2f dBm", \ > + (double)((var) / 10000.), \ > + convert_mw_to_dbm((double)((var) / 10000.))) > + > +#define SPRINT_BIAS(str, bias_cur) \ same here > + snprintf(str, sizeof(str), "%.3f mA", (double)(bias_cur / 500.)) > + > +#define SPRINT_TEMP(str, temp) \ same here > + snprintf(str, sizeof(str), "%.2f degrees C / %.2f degrees F", \ > + (double)(temp / 256.), \ > + (double)(temp / 256. * 1.8 + 32.)) > + > +#define SPRINT_VCC(str, sfp_voltage) \ same here > + snprintf(str, sizeof(str), "%.4f V", (double)(sfp_voltage / 10000.)) > + > +/* Channel Monitoring Fields */ > +struct sff_channel_diags { > + uint16_t bias_cur; /* Measured bias current in 2uA units */ > + uint16_t rx_power; /* Measured RX Power */ > + uint16_t tx_power; /* Measured TX Power */ > +}; > + > +/* Module Monitoring Fields */ > +struct sff_diags { > + > +#define MAX_CHANNEL_NUM 4 > +#define LWARN 0 > +#define HWARN 1 > +#define LALRM 2 > +#define HALRM 3 > +#define MCURR 4 Please, add prefix to these defines as well. > + > + /* Supports DOM */ > + uint8_t supports_dom; > + /* Supports alarm/warning thold */ > + uint8_t supports_alarms; > + /* RX Power: 0 = OMA, 1 = Average power */ > + uint8_t rx_power_type; > + /* TX Power: 0 = Not supported, 1 = Average power */ > + uint8_t tx_power_type; > + > + uint8_t calibrated_ext; /* Is externally calibrated */ > + /* [5] tables are low/high warn, low/high alarm, current */ > + /* SFP voltage in 0.1mV units */ > + uint16_t sfp_voltage[5]; > + /* SFP Temp in 16-bit signed 1/256 Celcius */ > + int16_t sfp_temp[5]; > + /* Measured bias current in 2uA units */ > + uint16_t bias_cur[5]; > + /* Measured TX Power */ > + uint16_t tx_power[5]; > + /* Measured RX Power */ > + uint16_t rx_power[5]; > + struct sff_channel_diags scd[MAX_CHANNEL_NUM]; > +}; > + > +double convert_mw_to_dbm(double mw); Please, add sff_ prefix > +void sff_show_value_with_unit(const uint8_t *data, unsigned int reg, > + const char *name, unsigned int mult, > + const char *unit, struct rte_tel_data *d); > +void sff_show_ascii(const uint8_t *data, unsigned int first_reg, > + unsigned int last_reg, const char *name, struct rte_tel_data *d); > +void sff_show_thresholds(struct sff_diags sd, struct rte_tel_data *d); > + > +void sff_8024_show_oui(const uint8_t *data, int id_offset, struct rte_tel_data *d); > +void sff_8024_show_identifier(const uint8_t *data, int id_offset, struct rte_tel_data *d); > +void sff_8024_show_connector(const uint8_t *data, int ctor_offset, struct rte_tel_data *d); > +void sff_8024_show_encoding(const uint8_t *data, int encoding_offset, > + int sff_type, struct rte_tel_data *d); > + > +#endif /* _SFF_COMMON_H_ */