Re: [PATCH] [v9] wireless: Initial driver submission for pureLiFi STA devices

[Joe Perches <joe@xxxxxxxxxxx>]

On Thu, 2020-12-03 at 10:39 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.

The below is a trivial readability possible enhancement and should not
hinder this patch being applied.

> diff --git a/drivers/net/wireless/purelifi/dbgfs.c b/drivers/net/wireless/purelifi/dbgfs.c
[]
> +ssize_t purelifi_store_frequency(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf,
> +				 size_t len)
> +{
> +	int i, r, row, col, predivider, feedback_divider, output_div_0;
> +	int output_div_1, output_div_2, clkout3_phase;
> +	char values[6][4] = {{0}, {0} };
> +	char usr_input[8] = {0};
> +	bool valid = false;
> +	int rv = 0;
> +
> +	row = 0; col = 0;
> +	for (i = 0; i < min(len, sizeof(values)); i++) {
> +		values[row][col] = buf[i];
> +		col++;
> +		if (buf[i] == 0x20) {
> +			row++;
> +			col = 0;
> +		} else if (buf[i] == 0x0A) {
> +			if (row == 5 && col > 0) {
> +				rv = sscanf((char *)&values[0][0],
> +					    "%d", &predivider);

Aren't the casts to (char *) redundant ?

> +				if (rv != 1)
> +					valid = false;
> +				rv = sscanf((char *)&values[1][0],
> +					    "%d", &feedback_divider);
> +				if (rv != 1)
> +					valid = false;
> +				rv = sscanf((char *)&values[2][0],
> +					    "%d", &output_div_0);
> +				if (rv != 1)
> +					valid = false;
> +				rv = sscanf((char *)&values[3][0],
> +					    "%d", &output_div_1);
> +				if (rv != 1)
> +					valid = false;
> +				rv = sscanf((char *)&values[4][0],
> +					    "%d", &output_div_2);
> +				if (rv != 1)
> +					valid = false;
> +				rv = sscanf((char *)&values[5][0],
> +					    "%d", &clkout3_phase);
> +				if (rv != 1)
> +					valid = false;
> +				valid = true;

trivia: this might be more intelligible with an array and a loop

	int *index[] = {
		&predivider,
		&feedback_divider,
		&output_div_0,
		&output_div_1,
		&output_div_2,
		&clkout3_phase
	};

	valid = true;
	i = 0;
	while (valid && i < ARRAY_SIZE(index)) {
		if (sscanf(&values[i][0], "%d", index[i]) != 1)
			valid = false;
		i++;
	}
		
> +
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (valid) {
> +		usr_input[0] = (char)predivider;
> +		usr_input[1] = (char)feedback_divider;
> +		usr_input[2] = (char)output_div_0;
> +		usr_input[3] = (char)output_div_1;
> +		usr_input[4] = (char)output_div_2;
> +		usr_input[5] = (char)clkout3_phase;
> +
> +		dev_info(dev, "options IP_DIV: %d VCO_MULT: %d OP_DIV_PHY: %d",
> +			 usr_input[0], usr_input[1], usr_input[2]);
> +		dev_info(dev, "OP_DIV_CPU: %d OP_DIV_USB: %d CLK3_PH: %d\n",
> +			 usr_input[3], usr_input[4], usr_input[5]);
> +
> +		r = usb_write_req((const u8 *)&usr_input[0],
> +				  sizeof(usr_input), USB_REQ_SET_FREQ);
> +		if (r < 0)
> +			dev_err(dev, "SET_FREQ failed (%d)\n", r);
> +	} else {
> +		dev_err(dev, "Your options are invalid\n");
> +	}
> +
> +	return len;
> +}