Re: [PATCH v8 2/5] ethdev: common utilities for different SFF specs

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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@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_ */