| Message ID | 20180110052002.9544-3-pkshih@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 1/10/2018 6:19 AM, pkshih@realtek.com wrote: > From: Ping-Ke Shih <pkshih@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 <timlee@realtek.com> 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) > &rtlmac->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(&hw->conf.chandef)); > + > /*<3> Enable Interrupt */ > rtlpriv->cfg->ops->enable_interrupt(hw); > >
> -----Original Message----- > From: Arend van Spriel [mailto:arend.vanspriel@broadcom.com] > Sent: Wednesday, January 10, 2018 4:13 PM > To: Pkshih; kvalo@codeaurora.org > Cc: Larry.Finger@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, pkshih@realtek.com wrote: > > From: Ping-Ke Shih <pkshih@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 <timlee@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) > > &rtlmac->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
On 1/10/2018 10:38 AM, Pkshih wrote: > > >> -----Original Message----- >> From: Arend van Spriel [mailto:arend.vanspriel@broadcom.com] >> Sent: Wednesday, January 10, 2018 4:13 PM >> To: Pkshih; kvalo@codeaurora.org >> Cc: Larry.Finger@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, pkshih@realtek.com wrote: >>> From: Ping-Ke Shih <pkshih@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 <timlee@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) >>> &rtlmac->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, pkshih@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(&hw->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
> -----Original Message----- > From: Arend van Spriel [mailto:arend.vanspriel@broadcom.com] > Sent: Wednesday, January 10, 2018 10:08 PM > To: Pkshih; kvalo@codeaurora.org > Cc: Larry.Finger@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.vanspriel@broadcom.com] > >> Sent: Wednesday, January 10, 2018 4:13 PM > >> To: Pkshih; kvalo@codeaurora.org > >> Cc: Larry.Finger@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, pkshih@realtek.com wrote: > >>> From: Ping-Ke Shih <pkshih@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 <timlee@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) > >>> &rtlmac->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, pkshih@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(&hw->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
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) &rtlmac->retry_long); RT_CLEAR_PS_LEVEL(ppsc, RT_RF_OFF_LEVL_HALT_NIC); + /*<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(&hw->conf.chandef)); + /*<3> Enable Interrupt */ rtlpriv->cfg->ops->enable_interrupt(hw);