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

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

On 29/01/2025 05:15, 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 | 126 ++++++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 80 insertions(+), 46 deletions(-)
> 
> diff --git a/qsfp.c b/qsfp.c
> index d272dbf..994ad5f 100644
> --- a/qsfp.c
> +++ b/qsfp.c
> @@ -649,13 +649,85 @@ 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)
>   {
> -	struct sff_diags sd = {0};
> -	char *rx_power_string = NULL;
>   	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)
> +{
> +	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) {
> +		if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636)
> +			continue;
> +
> +		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,
> +		       (i % SFF8636_MAX_CHANNEL_NUM) + 1,

Looks like this way will only work when MODULE_TYPE_SFF8636 properties
starts at (offset % SFF8636_MAX_CHANNEL_NUM) == 0. Maybe we have to save
the offset of the first SFF8636 item in module_aw_chan_flags[] ?

> +		       value ? "On" : "Off");
> +	}
> +}
> +
> +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,51 +759,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) {
> -			if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636)
> -				continue;
> -
> -			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,
> -			       (i % SFF8636_MAX_CHANNEL_NUM) + 1,
> -			       value ? "On" : "Off");
> -		}
> -		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);
>   	}