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

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

On 02/29/2012 08:48 AM, Raja Mani wrote:
> On Monday 27 February 2012 11:42 PM, Kalle Valo wrote:
>
>> 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 :(
> 
> My initial idea was, to reuse existing suspend_mode module
> parameter itself to avoid new module parameter.
> 
> I am okay to have a new module parameter wow_mode for that.

I think having a new module parameter is easier for everyone.

> Having charp type for new module parameter wow_mode is fine.
> But still suspend_mode parameter accepts value in uint.
> 
> Either we should have charp type for both suspend_mode and wow_mode
> module parameters (or) keep uint type for both. Otherwise having charp
> type for wow_mode and having uint for suspend_mode may create confusion
> to the user.
> 
> I prefer uint type at this point. What do you say ?

You have a point. uint in both parameters is better for now as we can't
change suspend_mode without breaking existing setups.

>>> --- 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.
> 
> I moved this error statement to ath6kl_sdio_suspend func in the same
> patch.
> 
> 		if (ret && ret != -ENOTCONN)
> 			ath6kl_err("wow suspend failed: %d\n", ret);
> 
> I think it should error message, not a debug message. We want to
> display wow suspend failures irrespective of debug mask level.

Yes, that's a good aproach.

>>> 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?
> 
> Agree, can be renamed to wow_suspend_mode. I'll correct it V2.

Thanks.

Kalle