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>. Regards, Arend
