On Wed, Aug 2, 2017 at 3:20 PM, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote: > Amitkumar Karwar <amitkarwar@xxxxxxxxx> writes: > >> From: Karun Eagalapati <karun256@xxxxxxxxx> >> >> This patch adds support for legacy power save. Necessary >> configuration frames are downloaded to firmware when power save >> is enabled/disabled >> >> Signed-off-by: Karun Eagalapati <karun256@xxxxxxxxx> >> Signed-off-by: Amitkumar Karwar <amit.karwar@xxxxxxxxxxxxxxxxxx> > > [...] > >> +/* This function sends power save request to firmware */ >> +int rsi_send_ps_request(struct rsi_hw *adapter, bool enable) > > The comment is quite useless, IMHO the function name already tells the > same as the comment. > >> --- /dev/null >> +++ b/drivers/net/wireless/rsi/rsi_91x_ps.c >> @@ -0,0 +1,149 @@ >> +/* >> + * Copyright (c) 2017 Redpine Signals Inc. All rights reserved. >> + * >> + * Redistribution and use in source and binary forms, with or without >> + * modification, are permitted provided that the following conditions are met: >> + * >> + * 1. Redistributions of source code must retain the above copyright >> + * notice, this list of conditions and the following disclaimer. >> + * >> + * 2. Redistributions in binary form must reproduce the above copyright >> + * notice, this list of conditions and the following disclaimer in the >> + * documentation and/or other materials provided with the distribution. >> + * >> + * 3. Neither the name of the copyright holder nor the names of its >> + * contributors may be used to endorse or promote products derived from >> + * this software without specific prior written permission. >> + * >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" >> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE >> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE >> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE >> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR >> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF >> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS >> + * INTERRUPTION). HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN >> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) >> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE >> + * POSSIBILITY OF SUCH DAMAGE. >> + */ > > The license is now different than in rest of the driver files which is a > bit weird. I would prefer to have the same license throughout the > driver. > >> +/* This function returns the ps state in string format */ >> +char *str_psstate(enum ps_state state) > > [...] > >> +/* This function modifies PS state to a new state */ >> +static inline void rsi_modify_ps_state(struct rsi_hw *adapter, > > [...] > >> +/* This function Initalises ps_info structure with default parameters */ >> +void rsi_default_ps_params(struct rsi_hw *adapter) > > [...] > >> +/* This function is used to enable power save */ >> +void rsi_enable_ps(struct rsi_hw *adapter) > > [...] > >> +/* This function is used to disable power save */ >> +void rsi_disable_ps(struct rsi_hw *adapter) > > [...] > >> +/* This function Processes powersave confirmation */ >> +int rsi_handle_ps_confirm(struct rsi_hw *adapter, u8 *msg) > > More useless comments. The code comments should provide more > information, not replicate what is already in the code. > Thanks for review. I have addressed these comments in V2 series. Regards, Amitkumar Karwar
