> -----Original Message----- > From: Arend van Spriel [mailto:arend.vanspriel@xxxxxxxxxxxx] > Sent: Wednesday, January 10, 2018 10:08 PM > To: Pkshih; kvalo@xxxxxxxxxxxxxx > Cc: Larry.Finger@xxxxxxxxxxxx; 莊彥宣; linux-wireless@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxx] > >> Sent: Wednesday, January 10, 2018 4:13 PM > >> To: Pkshih; kvalo@xxxxxxxxxxxxxx > >> Cc: Larry.Finger@xxxxxxxxxxxx; 莊彥宣; linux-wireless@xxxxxxxxxxxxxxx > >> Subject: Re: [PATCH 02/10] rtlwifi: fix scan channel 1 fail after IPS > >> > >> On 1/10/2018 6:19 AM, pkshih@xxxxxxxxxxx wrote: > >>> From: Ping-Ke Shih <pkshih@xxxxxxxxxxx> > >>> > >>> 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@xxxxxxxxxxx> > >> > >> 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@xxxxxxxxxxx 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
