Re: [PATCH v2] ath6kl: Implement support for listen interval from userspace

[Kalle Valo <kvalo@qca.qualcomm.com>]

On 10/21/2011 01:35 AM, Rishi Panjwani wrote:
> In order to allow user space based control of listen interval, we use
> available debugfs infrastructure. Listen interval implies how frequently
> we want the WLAN chip to wake up and synchronize the beacons in case it
> is in sleep mode. The feature has been added for testing purposes. The
> command requires two parameters in the following order:

An empty line here.

> 1) listen_interval_time
> 2) listen_interval_beacons
> 
> The user has to write the listen interval_time (in msecs) and
> listen_interval_beacons (in no. of beacons) to the listen_interval file in
> ath6kl debug directory.
> 
> Example:
> 
> echo "30 1" > listen_interval

[...]

> +	token = strsep(&sptr, " ");
> +	if (!token)
> +		return -EINVAL;
> +	if (kstrtou16(token, 0, &listen_int_t) ||
> +		kstrtou16(sptr, 0, &listen_int_b))
> +		return -EINVAL;

Just to improve readability separate these to two if clauses:

if (kstrtou16(token, 0, &listen_int_t))
	return -EINVAL;

if (kstrtou16(sptr, 0, &listen_int_b))
	return -EINVAL;

> +	if ((listen_int_t >= 15) && (listen_int_t <= 5000) &&
> +		(listen_int_b >= 1) && (listen_int_b <= 50)) {
> +		ar->listen_intvl_t = listen_int_t;
> +		ar->listen_intvl_b = listen_int_b;
> +		ath6kl_wmi_listeninterval_cmd(ar->wmi, ar->listen_intvl_t,
> +							ar->listen_intvl_b);
> +	} else {
> +		return -EINVAL;
> +	}

The style in kernel usually is that we handle the errors in the if
blocks and the normal code flow is without any indentation. Easier to
follow code path that way. So you could change this to:

if ((listen_int_t < 15) || (listen_int_t > 5000))
	return -EINVAL;

if ((listen_int_b < 1) > (listen_int_b > 50))
	return -EINVAL;

ar->listen_intvl_t = listen_int_t;
ar->listen_intvl_b = listen_int_b;

ath6kl_wmi_listeninterval_cmd(ar->wmi, ar->listen_intvl_t,
				ar->listen_intvl_b);

A lot easier to read that way.

Kalle