[PATCH v8 2/5] ethdev: common utilities for different SFF specs
Andrew Rybchenko
andrew.rybchenko at oktetlabs.ru
Wed May 25 11:40:36 CEST 2022
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 <robinx.zhang at intel.com>
> ---
> 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 <stdio.h>
> +#include <math.h>
Please, add empty line after system headers.
> +#include <rte_mbuf.h>
Why do you need rte_mbuf.h?
> +#include <rte_ethdev.h>
Do you really need it? Looks like no.
> +#include <rte_flow.h>
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 <stdio.h>
> +#include <rte_mbuf.h>
> +#include <rte_ethdev.h>
> +#include <rte_flow.h>
> +#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_ */
More information about the dev
mailing list