RE: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS

[Pkshih]

> -Original Message-
> From: Arend van Spriel [mailto:arend.vanspr...@broadcom.com]
> Sent: Wednesday, January 10, 2018 10:08 PM
> To: Pkshih; kv...@codeaurora.org
> Cc: larry.fin...@lwfinger.net; 莊彥宣; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS
> 
> On 1/10/2018 10:38 AM, Pkshih wrote:
> >
> >
> >> -Original Message-
> >> From: Arend van Spriel [mailto:arend.vanspr...@broadcom.com]
> >> Sent: Wednesday, January 10, 2018 4:13 PM
> >> To: Pkshih; kv...@codeaurora.org
> >> Cc: larry.fin...@lwfinger.net; 莊彥宣; linux-wireless@vger.kernel.org
> >> Subject: Re: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS
> >>
> >> On 1/10/2018 6:19 AM, pks...@realtek.com wrote:
> >>> From: Ping-Ke Shih <pks...@realtek.com>
> >>>
> >>> If there is no connection, driver will enter IPS state. Meanwhile, it
> >>> fails to scan channel 1 by the command 'iw dev wlan0 scan freq 2412',
> >>> because hardware channel setting lose after IPS. Thus, restore channel
> >>> setting from hw->conf.channel set by last rtl_op_config().
> >>>
> >>> Signed-off-by: Tim Lee <tim...@realtek.com>
> >>
> >> You need to add your sob here as well as you are submitting them.
> >>
> >
> > I'll add it in v2.
> >
> >>> ---
> >>>drivers/net/wireless/realtek/rtlwifi/ps.c | 6 ++
> >>>1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c
> b/drivers/net/wireless/realtek/rtlwifi/ps.c
> >>> index 6a4008845f49..0ffe43772c9a 100644
> >>> --- a/drivers/net/wireless/realtek/rtlwifi/ps.c
> >>> +++ b/drivers/net/wireless/realtek/rtlwifi/ps.c
> >>> @@ -51,6 +51,12 @@ bool rtl_ps_enable_nic(struct ieee80211_hw *hw)
> >>>   >retry_long);
> >>>   RT_CLEAR_PS_LEVEL(ppsc, RT_RF_OFF_LEVL_HALT_NIC);
> >>>
> >>> + /*<2.1> Switch Channel & Bandwidth to last rtl_op_config setting*/
> >>
> >> Is this type of comment really helpful? To me it seems the callback
> >> names provide enough context.
> >>
> >
> > Do you mean the "<2.1>" isn't needed?
> > This is because "<1>, <2>, <3>..." exist in the function, so
> > we want to make it to be consistent.
> 
> That is not what I mean. I mean why have a comment describing what is
> obvious from reading the code itself. So in this example:
> 
> On 1/10/2018 6:19 AM, pks...@realtek.com wrote:
> > +   /*<2.1> Switch Channel & Bandwidth to last rtl_op_config setting*/
> > +   rtlpriv->cfg->ops->switch_channel(hw);
> > +   rtlpriv->cfg->ops->set_channel_access(hw);
> > +   rtlpriv->cfg->ops->set_bw_mode(hw,
> > +   cfg80211_get_chandef_type(>conf.chandef));
> > +
> > /*<3> Enable Interrupt */
> > rtlpriv->cfg->ops->enable_interrupt(hw);
> 
> the code after the <2.1> comment calls a switch_channel() callback and a
> set_bw_mode() callback. In my opinion those names are pretty
> self-explanatory for the reader making the comment preceding it only
> noise. The same applies to step <3>.
> 

Got it! I'll follow this coding convention.
Thanks

PK

Re: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS

[Arend van Spriel]

On 1/10/2018 10:38 AM, Pkshih wrote:
> 
> 
>> -Original Message-
>> From: Arend van Spriel [mailto:arend.vanspr...@broadcom.com]
>> Sent: Wednesday, January 10, 2018 4:13 PM
>> To: Pkshih; kv...@codeaurora.org
>> Cc: larry.fin...@lwfinger.net; 莊彥宣; linux-wireless@vger.kernel.org
>> Subject: Re: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS
>>
>> On 1/10/2018 6:19 AM, pks...@realtek.com wrote:
>>> From: Ping-Ke Shih <pks...@realtek.com>
>>>
>>> If there is no connection, driver will enter IPS state. Meanwhile, it
>>> fails to scan channel 1 by the command 'iw dev wlan0 scan freq 2412',
>>> because hardware channel setting lose after IPS. Thus, restore channel
>>> setting from hw->conf.channel set by last rtl_op_config().
>>>
>>> Signed-off-by: Tim Lee <tim...@realtek.com>
>>
>> You need to add your sob here as well as you are submitting them.
>>
> 
> I'll add it in v2.
> 
>>> ---
>>>drivers/net/wireless/realtek/rtlwifi/ps.c | 6 ++
>>>1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c 
>>> b/drivers/net/wireless/realtek/rtlwifi/ps.c
>>> index 6a4008845f49..0ffe43772c9a 100644
>>> --- a/drivers/net/wireless/realtek/rtlwifi/ps.c
>>> +++ b/drivers/net/wireless/realtek/rtlwifi/ps.c
>>> @@ -51,6 +51,12 @@ bool rtl_ps_enable_nic(struct ieee80211_hw *hw)
>>> >retry_long);
>>> RT_CLEAR_PS_LEVEL(ppsc, RT_RF_OFF_LEVL_HALT_NIC);
>>>
>>> +   /*<2.1> Switch Channel & Bandwidth to last rtl_op_config setting*/
>>
>> Is this type of comment really helpful? To me it seems the callback
>> names provide enough context.
>>
> 
> Do you mean the "<2.1>" isn't needed?
> This is because "<1>, <2>, <3>..." exist in the function, so
> we want to make it to be consistent.

That is not what I mean. I mean why have a comment describing what is
obvious from reading the code itself. So in this example:

On 1/10/2018 6:19 AM, pks...@realtek.com wrote:
> + /*<2.1> Switch Channel & Bandwidth to last rtl_op_config setting*/
> + rtlpriv->cfg->ops->switch_channel(hw);
> + rtlpriv->cfg->ops->set_channel_access(hw);
> + rtlpriv->cfg->ops->set_bw_mode(hw,
> + cfg80211_get_chandef_type(>conf.chandef));
> +
>   /*<3> Enable Interrupt */
>   rtlpriv->cfg->ops->enable_interrupt(hw);

the code after the <2.1> comment calls a switch_channel() callback and a
set_bw_mode() callback. In my opinion those names are pretty
self-explanatory for the reader making the comment preceding it only
noise. The same applies to step <3>.

Regards,
Arend

RE: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS

[Pkshih]

> -Original Message-
> From: Arend van Spriel [mailto:arend.vanspr...@broadcom.com]
> Sent: Wednesday, January 10, 2018 4:13 PM
> To: Pkshih; kv...@codeaurora.org
> Cc: larry.fin...@lwfinger.net; 莊彥宣; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS
> 
> On 1/10/2018 6:19 AM, pks...@realtek.com wrote:
> > From: Ping-Ke Shih <pks...@realtek.com>
> >
> > If there is no connection, driver will enter IPS state. Meanwhile, it
> > fails to scan channel 1 by the command 'iw dev wlan0 scan freq 2412',
> > because hardware channel setting lose after IPS. Thus, restore channel
> > setting from hw->conf.channel set by last rtl_op_config().
> >
> > Signed-off-by: Tim Lee <tim...@realtek.com>
> 
> You need to add your sob here as well as you are submitting them.
> 

I'll add it in v2.

> > ---
> >   drivers/net/wireless/realtek/rtlwifi/ps.c | 6 ++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c 
> > b/drivers/net/wireless/realtek/rtlwifi/ps.c
> > index 6a4008845f49..0ffe43772c9a 100644
> > --- a/drivers/net/wireless/realtek/rtlwifi/ps.c
> > +++ b/drivers/net/wireless/realtek/rtlwifi/ps.c
> > @@ -51,6 +51,12 @@ bool rtl_ps_enable_nic(struct ieee80211_hw *hw)
> > >retry_long);
> > RT_CLEAR_PS_LEVEL(ppsc, RT_RF_OFF_LEVL_HALT_NIC);
> >
> > +   /*<2.1> Switch Channel & Bandwidth to last rtl_op_config setting*/
> 
> Is this type of comment really helpful? To me it seems the callback
> names provide enough context.
> 

Do you mean the "<2.1>" isn't needed? 
This is because "<1>, <2>, <3>..." exist in the function, so
we want to make it to be consistent.

PK

Re: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS

[Arend van Spriel]

On 1/10/2018 6:19 AM, pks...@realtek.com wrote:

From: Ping-Ke Shih 

If there is no connection, driver will enter IPS state. Meanwhile, it
fails to scan channel 1 by the command 'iw dev wlan0 scan freq 2412',
because hardware channel setting lose after IPS. Thus, restore channel
setting from hw->conf.channel set by last rtl_op_config().

Signed-off-by: Tim Lee 


You need to add your sob here as well as you are submitting them.


---
  drivers/net/wireless/realtek/rtlwifi/ps.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c 
b/drivers/net/wireless/realtek/rtlwifi/ps.c
index 6a4008845f49..0ffe43772c9a 100644
--- a/drivers/net/wireless/realtek/rtlwifi/ps.c
+++ b/drivers/net/wireless/realtek/rtlwifi/ps.c
@@ -51,6 +51,12 @@ bool rtl_ps_enable_nic(struct ieee80211_hw *hw)
>retry_long);
RT_CLEAR_PS_LEVEL(ppsc, RT_RF_OFF_LEVL_HALT_NIC);

+   /*<2.1> Switch Channel & Bandwidth to last rtl_op_config setting*/


Is this type of comment really helpful? To me it seems the callback 
names provide enough context.


Regards,
Arend


+   rtlpriv->cfg->ops->switch_channel(hw);
+   rtlpriv->cfg->ops->set_channel_access(hw);
+   rtlpriv->cfg->ops->set_bw_mode(hw,
+   cfg80211_get_chandef_type(>conf.chandef));
+
/*<3> Enable Interrupt */
rtlpriv->cfg->ops->enable_interrupt(hw);