Re: [PATCH ethtool-next 05/14] qsfp: Refactor sff8636_show_dom() by moving code into separate functions

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 26/01/2025 03:56, Danielle Ratson wrote:
> The sff8636_show_dom() function is quite lengthy, and with the planned
> addition of JSON support, it will become even longer and more complex.
> 
> To improve readability and maintainability, refactor the function by
> moving portions of the code into separate functions, following the
> approach used in the cmis.c module.
> 
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> ---
>   qsfp.c | 144 +++++++++++++++++++++++++++++++++++----------------------
>   1 file changed, 89 insertions(+), 55 deletions(-)
> 
> diff --git a/qsfp.c b/qsfp.c
> index 674242c..13d8fb7 100644
> --- a/qsfp.c
> +++ b/qsfp.c
> @@ -649,13 +649,94 @@ out:
>   	}
>   }
>   
> -static void sff8636_show_dom(const struct sff8636_memory_map *map)
> +static void sff8636_show_dom_chan_lvl_tx_bias(const struct sff_diags *sd)
> +{
> +	char power_string[MAX_DESC_SIZE];
> +	int i;
> +
> +	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
> +		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
> +			 "Laser tx bias current", i+1);
> +		PRINT_BIAS(power_string, sd->scd[i].bias_cur);
> +	}
> +}
> +
> +static void sff8636_show_dom_chan_lvl_tx_power(const struct sff_diags *sd)
> +{
> +	char power_string[MAX_DESC_SIZE];
> +	int i;
> +
> +	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
> +		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
> +			 "Transmit avg optical power", i+1);
> +		PRINT_xX_PWR(power_string, sd->scd[i].tx_power);
> +	}
> +}
> +
> +static void sff8636_show_dom_chan_lvl_rx_power(const struct sff_diags *sd)
>   {
> -	struct sff_diags sd = {0};
> -	char *rx_power_string = NULL;
>   	char power_string[MAX_DESC_SIZE];
> +	char *rx_power_string = NULL;
> +	int i;
> +
> +	if (!sd->rx_power_type)
> +		rx_power_string = "Receiver signal OMA";
> +	else
> +		rx_power_string = "Rcvr signal avg optical power";
> +
> +	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
> +		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
> +			 rx_power_string, i+1);
> +		PRINT_xX_PWR(power_string, sd->scd[i].rx_power);
> +	}
> +}
> +
> +static void
> +sff8636_show_dom_chan_lvl_flags(const struct sff8636_memory_map *map)
> +{
> +	bool value;
>   	int i;
>   
> +	for (i = 0; module_aw_chan_flags[i].fmt_str; ++i) {
> +		int j = 1;
> +
> +		if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636)
> +			continue;
> +
> +		do {
> +			value = map->lower_memory[module_aw_chan_flags[i].offset] &
> +				module_aw_chan_flags[i].adver_value;
> +			printf("\t%-41s (Chan %d) : %s\n",
> +			       module_aw_chan_flags[i].fmt_str, j,
> +			       ONOFF(value));
> +			j++;
> +			i++;
> +		}
> +		while (module_aw_chan_flags[i].fmt_str &&
> +		       strcmp(module_aw_chan_flags[i].fmt_str,
> +			      module_aw_chan_flags[i-1].fmt_str) == 0);

Why do we need this complex logic of comparing strings and moving
iterators forth and back? We do have SFF8636_MAX_CHANNEL_NUM in other
functions, and we know that module_aw_chan_flags has items for all
channels, we can simplify the loop here by providing channel number
as ((i % SFF8636_MAX_CHANNEL_NUM) + 1). That will make the code much
easier to read.

This comment actually applies to the previous patch, as current one
simply moves it to a dedicated function. But I think patch 4 and
patch 5 can be merged in one.


> +		i--;
> +	}
> +}
> +
> +static void
> +sff8636_show_dom_mod_lvl_flags(const struct sff8636_memory_map *map)
> +{
> +	int i;
> +
> +	for (i = 0; module_aw_mod_flags[i].str; ++i) {
> +		if (module_aw_mod_flags[i].type == MODULE_TYPE_SFF8636)
> +			printf("\t%-41s : %s\n",
> +			       module_aw_mod_flags[i].str,
> +			       ONOFF(map->lower_memory[module_aw_mod_flags[i].offset]
> +				     & module_aw_mod_flags[i].value));
> +	}
> +}
> +
> +static void sff8636_show_dom(const struct sff8636_memory_map *map)
> +{
> +	struct sff_diags sd = {0};
> +
>   	/*
>   	 * There is no clear identifier to signify the existence of
>   	 * optical diagnostics similar to SFF-8472. So checking existence
> @@ -687,60 +768,13 @@ static void sff8636_show_dom(const struct sff8636_memory_map *map)
>   	printf("\t%-41s : %s\n", "Alarm/warning flags implemented",
>   		(sd.supports_alarms ? "Yes" : "No"));
>   
> -	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
> -		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
> -					"Laser tx bias current", i+1);
> -		PRINT_BIAS(power_string, sd.scd[i].bias_cur);
> -	}
> -
> -	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
> -		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
> -					"Transmit avg optical power", i+1);
> -		PRINT_xX_PWR(power_string, sd.scd[i].tx_power);
> -	}
> -
> -	if (!sd.rx_power_type)
> -		rx_power_string = "Receiver signal OMA";
> -	else
> -		rx_power_string = "Rcvr signal avg optical power";
> -
> -	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
> -		snprintf(power_string, MAX_DESC_SIZE, "%s(Channel %d)",
> -					rx_power_string, i+1);
> -		PRINT_xX_PWR(power_string, sd.scd[i].rx_power);
> -	}
> +	sff8636_show_dom_chan_lvl_tx_bias(&sd);
> +	sff8636_show_dom_chan_lvl_tx_power(&sd);
> +	sff8636_show_dom_chan_lvl_rx_power(&sd);
>   
>   	if (sd.supports_alarms) {
> -		bool value;
> -
> -		for (i = 0; module_aw_chan_flags[i].fmt_str; ++i) {
> -			int j = 1;
> -
> -			if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636)
> -				continue;
> -
> -			do {
> -				value = map->lower_memory[module_aw_chan_flags[i].offset] &
> -					module_aw_chan_flags[i].adver_value;
> -				printf("\t%-41s (Chan %d) : %s\n",
> -				       module_aw_chan_flags[i].fmt_str, j,
> -				       value ? "On" : "Off");
> -				j++;
> -				i++;
> -			}
> -			while (module_aw_chan_flags[i].fmt_str &&
> -			       strcmp(module_aw_chan_flags[i].fmt_str,
> -				      module_aw_chan_flags[i-1].fmt_str) == 0);
> -			i--;
> -		}
> -		for (i = 0; module_aw_mod_flags[i].str; ++i) {
> -			if (module_aw_mod_flags[i].type == MODULE_TYPE_SFF8636)
> -				printf("\t%-41s : %s\n",
> -				       module_aw_mod_flags[i].str,
> -				       (map->lower_memory[module_aw_mod_flags[i].offset]
> -				       & module_aw_mod_flags[i].value) ?
> -				       "On" : "Off");
> -		}
> +		sff8636_show_dom_chan_lvl_flags(map);
> +		sff8636_show_dom_mod_lvl_flags(map);
>   
>   		sff_show_thresholds(sd);
>   	}