Re: [PATCH] ath6kl: Add provision to define suspend policy in disconnected state.

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

On 02/15/2012 02:22 PM, rmani@qca.qualcomm.com wrote:
> From: Raja Mani <rmani@qca.qualcomm.com>
> 
> It gives flexibility to the user to define suspend policy
> when the suspend mode is set to WOW and the device is in 
> disconnected state at the time of suspend.
> 
> Some bits (bit 4 to 7) of existing mod parameter suspend_mode
> is used for that purpose. There is no change in the existing
> way of defining suspend mode.
> 
> To force cut power:
>   suspend_mode=0x1
> 
> To force deep sleep:
>   suspend_mode=0x2
> 
> To force wow (deep sleep is the default mode
> in disconnected state):
>   suspend_mode=0x3
> 
> To force wow in connected state and cut power
> in disconnected state:
>   suspend_mode=0x13

What's the difference between values 0x3 and 0x13? It wasn't clear for
me from the description and neither from the code.

> To force wow in connected state and deep sleep
> in disconnected state:
>   suspend_mode=0x23

I'm not sure if adding bits to suspend_mode is the best way. To me
adding a separate parameter wow_mode looks easier from user's point of
view. What do you think?

And if you add a new module paratemer please use charp type so that can
use strings like 'deepsleep' and 'cutpower'. I wish that we had used
that already with suspend_mode but it's too late now :(

> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> @@ -2063,7 +2063,6 @@ int ath6kl_cfg80211_suspend(struct ath6kl *ar,
>  
>  		ret = ath6kl_wow_suspend(ar, wow);
>  		if (ret) {
> -			ath6kl_err("wow suspend failed: %d\n", ret);
>  			ar->state = prev_state;
>  			return ret;
>  		}

Please convert this to a debug message, maybe using the suspend level.
It might be useful when debugging something.

> diff --git a/drivers/net/wireless/ath/ath6kl/core.c b/drivers/net/wireless/ath/ath6kl/core.c
> index c4926cf..37c92a1 100644
> --- a/drivers/net/wireless/ath/ath6kl/core.c
> +++ b/drivers/net/wireless/ath/ath6kl/core.c
> @@ -41,6 +41,7 @@ int ath6kl_core_init(struct ath6kl *ar)
>  {
>  	struct ath6kl_bmi_target_info targ_info;
>  	struct net_device *ndev;
> +	u16 pri_suspend_mode, wow2_suspend_mode;

Why the number two? Why not just wow_suspend_mode?

Kalle