| Message ID | 1465232558-31678-3-git-send-email-javier@osg.samsung.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
2016-06-06 19:02 GMT+02:00 Javier Martinez Canillas <javier@osg.samsung.com>: > From: Shengzhen Li <szli@marvell.com> > > Most cfg80211 operations are just a wrappers to functions defined in the > sta_ioctl.c file, so for consistency move the .get_tx_power logic there. > > Signed-off-by: Shengzhen Li <szli@marvell.com> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > [javier: update the subject line and commit message] > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> > > --- > > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 14 +++----------- > drivers/net/wireless/marvell/mwifiex/main.h | 2 ++ > drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 18 ++++++++++++++++++ > 3 files changed, 23 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > index b17f3d09a2c7..ff3f63ed95e1 100644 > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy, > int *dbm) > { > struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); > - struct mwifiex_private *priv = mwifiex_get_priv(adapter, > - MWIFIEX_BSS_ROLE_ANY); > - int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, > - HostCmd_ACT_GEN_GET, 0, NULL, true); > - > - if (ret < 0) > - return ret; > - > - /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */ > - *dbm = priv->tx_power_level; > + struct mwifiex_private *priv; > > - return 0; > + priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); > + return mwifiex_get_tx_power(priv, dbm); > } > > /* > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h > index 0207af00be42..79c28cfb7780 100644 > --- a/drivers/net/wireless/marvell/mwifiex/main.h > +++ b/drivers/net/wireless/marvell/mwifiex/main.h > @@ -1464,6 +1464,8 @@ int mwifiex_drv_get_driver_version(struct mwifiex_adapter *adapter, > int mwifiex_set_tx_power(struct mwifiex_private *priv, > struct mwifiex_power_cfg *power_cfg); > > +int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm); > + > int mwifiex_main_process(struct mwifiex_adapter *); > > int mwifiex_queue_tx_pkt(struct mwifiex_private *priv, struct sk_buff *skb); > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c > index 8e0862657122..70ff9b805b5b 100644 > --- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c > +++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c > @@ -775,6 +775,24 @@ int mwifiex_set_tx_power(struct mwifiex_private *priv, > } > > /* > + * IOCTL request handler to get tx power configuration. > + * > + * This function prepares the correct firmware command and > + * issues it. > + */ > +int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm) > +{ > + int ret; > + > + ret = mwifiex_send_cmd(priv, HostCmd_CMD_TXPWR_CFG, > + HostCmd_ACT_GEN_GET, 0, NULL, true); > + > + *dbm = priv->tx_power_level; > + > + return ret; > +} > + > +/* > * IOCTL request handler to get power save mode. > * > * This function prepares the correct firmware command and > -- > 2.5.5 > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Javier Martinez Canillas <javier@osg.samsung.com> writes: > From: Shengzhen Li <szli@marvell.com> > > Most cfg80211 operations are just a wrappers to functions defined in the > sta_ioctl.c file, so for consistency move the .get_tx_power logic there. > > Signed-off-by: Shengzhen Li <szli@marvell.com> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > [javier: update the subject line and commit message] > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> [...] > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy, > int *dbm) > { > struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); > - struct mwifiex_private *priv = mwifiex_get_priv(adapter, > - MWIFIEX_BSS_ROLE_ANY); > - int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, > - HostCmd_ACT_GEN_GET, 0, NULL, true); > - > - if (ret < 0) > - return ret; > - > - /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */ > - *dbm = priv->tx_power_level; > + struct mwifiex_private *priv; > > - return 0; > + priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); > + return mwifiex_get_tx_power(priv, dbm); > } So in patch 1 you added the patch and in patch 2 you move it to a different location? That doesn't make any sense, can't you just fold the two patches into one so that the function is added only once.
Hello Kalle, On 06/10/2016 10:30 AM, Kalle Valo wrote: > Javier Martinez Canillas <javier@osg.samsung.com> writes: > >> From: Shengzhen Li <szli@marvell.com> >> >> Most cfg80211 operations are just a wrappers to functions defined in the >> sta_ioctl.c file, so for consistency move the .get_tx_power logic there. >> >> Signed-off-by: Shengzhen Li <szli@marvell.com> >> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> >> [javier: update the subject line and commit message] >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> > > [...] > >> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c >> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c >> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy, >> int *dbm) >> { >> struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); >> - struct mwifiex_private *priv = mwifiex_get_priv(adapter, >> - MWIFIEX_BSS_ROLE_ANY); >> - int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, >> - HostCmd_ACT_GEN_GET, 0, NULL, true); >> - >> - if (ret < 0) >> - return ret; >> - >> - /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */ >> - *dbm = priv->tx_power_level; >> + struct mwifiex_private *priv; >> >> - return 0; >> + priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); >> + return mwifiex_get_tx_power(priv, dbm); >> } > > So in patch 1 you added the patch and in patch 2 you move it to a > different location? That doesn't make any sense, can't you just fold the > two patches into one so that the function is added only once. > I posted this patch in v1 but then Amitkumar shared his patch with me that was very similar to mine, only that the logic was in a different location. So I included his delta as a separate patch to try keeping attribution as best as possible. Best regards,
Hi Kalle/Javier, > From: Javier Martinez Canillas [mailto:javier@osg.samsung.com] > Sent: Friday, June 10, 2016 8:07 PM > To: Kalle Valo > Cc: linux-kernel@vger.kernel.org; Julian Calaby; Shengzhen Li; Enric > Balletbo i Serra; Amitkumar Karwar; netdev@vger.kernel.org; linux- > wireless@vger.kernel.org; Nishant Sarmukadam > Subject: Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station > ioctl file > > Hello Kalle, > > On 06/10/2016 10:30 AM, Kalle Valo wrote: > > Javier Martinez Canillas <javier@osg.samsung.com> writes: > > > >> From: Shengzhen Li <szli@marvell.com> > >> > >> Most cfg80211 operations are just a wrappers to functions defined in > >> the sta_ioctl.c file, so for consistency move the .get_tx_power logic > there. > >> > >> Signed-off-by: Shengzhen Li <szli@marvell.com> > >> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > >> [javier: update the subject line and commit message] > >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> > > > > [...] > > > >> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c > >> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > >> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy > *wiphy, > >> int *dbm) > >> { > >> struct mwifiex_adapter *adapter = > mwifiex_cfg80211_get_adapter(wiphy); > >> - struct mwifiex_private *priv = mwifiex_get_priv(adapter, > >> - MWIFIEX_BSS_ROLE_ANY); > >> - int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, > >> - HostCmd_ACT_GEN_GET, 0, NULL, true); > >> - > >> - if (ret < 0) > >> - return ret; > >> - > >> - /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler > */ > >> - *dbm = priv->tx_power_level; > >> + struct mwifiex_private *priv; > >> > >> - return 0; > >> + priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); > >> + return mwifiex_get_tx_power(priv, dbm); > >> } > > > > So in patch 1 you added the patch and in patch 2 you move it to a > > different location? That doesn't make any sense, can't you just fold > > the two patches into one so that the function is added only once. > > > > I posted this patch in v1 but then Amitkumar shared his patch with me > that was very similar to mine, only that the logic was in a different > location. > > So I included his delta as a separate patch to try keeping attribution > as best as possible. > This patch (2/3) is only for code rearrangement and adds an unnecessary wrapper function. We can simply drop the patch. Regards, Amitkumar
Hello Amitkumar, On 06/10/2016 12:26 PM, Amitkumar Karwar wrote: > Hi Kalle/Javier, > >> From: Javier Martinez Canillas [mailto:javier@osg.samsung.com] >> Sent: Friday, June 10, 2016 8:07 PM >> To: Kalle Valo >> Cc: linux-kernel@vger.kernel.org; Julian Calaby; Shengzhen Li; Enric >> Balletbo i Serra; Amitkumar Karwar; netdev@vger.kernel.org; linux- >> wireless@vger.kernel.org; Nishant Sarmukadam >> Subject: Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station >> ioctl file >> >> Hello Kalle, >> >> On 06/10/2016 10:30 AM, Kalle Valo wrote: >>> Javier Martinez Canillas <javier@osg.samsung.com> writes: >>> >>>> From: Shengzhen Li <szli@marvell.com> >>>> >>>> Most cfg80211 operations are just a wrappers to functions defined in >>>> the sta_ioctl.c file, so for consistency move the .get_tx_power logic >> there. >>>> >>>> Signed-off-by: Shengzhen Li <szli@marvell.com> >>>> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> >>>> [javier: update the subject line and commit message] >>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> >>> >>> [...] >>> >>>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c >>>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c >>>> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy >> *wiphy, >>>> int *dbm) >>>> { >>>> struct mwifiex_adapter *adapter = >> mwifiex_cfg80211_get_adapter(wiphy); >>>> - struct mwifiex_private *priv = mwifiex_get_priv(adapter, >>>> - MWIFIEX_BSS_ROLE_ANY); >>>> - int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, >>>> - HostCmd_ACT_GEN_GET, 0, NULL, true); >>>> - >>>> - if (ret < 0) >>>> - return ret; >>>> - >>>> - /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler >> */ >>>> - *dbm = priv->tx_power_level; >>>> + struct mwifiex_private *priv; >>>> >>>> - return 0; >>>> + priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); >>>> + return mwifiex_get_tx_power(priv, dbm); >>>> } >>> >>> So in patch 1 you added the patch and in patch 2 you move it to a >>> different location? That doesn't make any sense, can't you just fold >>> the two patches into one so that the function is added only once. >>> >> >> I posted this patch in v1 but then Amitkumar shared his patch with me >> that was very similar to mine, only that the logic was in a different >> location. >> >> So I included his delta as a separate patch to try keeping attribution >> as best as possible. >> > > This patch (2/3) is only for code rearrangement and adds an unnecessary wrapper function. We can simply drop the patch. > Agreed. Kalle, Patch 3/3 applies cleanly even after dropping patch 2/3. Is that Ok for you or do you want me to re-resend a v3 with only patches 1/3 and 3/3? > Regards, > Amitkumar > Best regards,
Javier Martinez Canillas <javier@osg.samsung.com> writes: >> This patch (2/3) is only for code rearrangement and adds an >> unnecessary wrapper function. We can simply drop the patch. > > Agreed. > > Kalle, > > Patch 3/3 applies cleanly even after dropping patch 2/3. > Is that Ok for you or do you want me to re-resend a v3 > with only patches 1/3 and 3/3? I can drop patch 2, no need to resend. Thanks.
Hello Kalle, On 06/10/2016 03:54 PM, Kalle Valo wrote: > Javier Martinez Canillas <javier@osg.samsung.com> writes: > >>> This patch (2/3) is only for code rearrangement and adds an >>> unnecessary wrapper function. We can simply drop the patch. >> >> Agreed. >> >> Kalle, >> >> Patch 3/3 applies cleanly even after dropping patch 2/3. >> Is that Ok for you or do you want me to re-resend a v3 >> with only patches 1/3 and 3/3? > > I can drop patch 2, no need to resend. Thanks. > I saw that you sent your pull request for v4.8 but the patches from this series were not included: https://lkml.org/lkml/2016/6/21/400 Best regards,
Javier Martinez Canillas <javier@osg.samsung.com> writes: >>> Patch 3/3 applies cleanly even after dropping patch 2/3. >>> Is that Ok for you or do you want me to re-resend a v3 >>> with only patches 1/3 and 3/3? >> >> I can drop patch 2, no need to resend. Thanks. >> > > I saw that you sent your pull request for v4.8 but the > patches from this series were not included: > > https://lkml.org/lkml/2016/6/21/400 I had forgotten to change the state of patches 1 and 3 from 'Changes Requested' back to 'New' and that's why I missed them. I did that now so they are back in my queue: https://patchwork.kernel.org/patch/9158855/ https://patchwork.kernel.org/patch/9158851/ Thanks for checking.
Hello Kalle, On 06/22/2016 02:17 AM, Kalle Valo wrote: > Javier Martinez Canillas <javier@osg.samsung.com> writes: > >>>> Patch 3/3 applies cleanly even after dropping patch 2/3. >>>> Is that Ok for you or do you want me to re-resend a v3 >>>> with only patches 1/3 and 3/3? >>> >>> I can drop patch 2, no need to resend. Thanks. >>> >> >> I saw that you sent your pull request for v4.8 but the >> patches from this series were not included: >> >> https://lkml.org/lkml/2016/6/21/400 > > I had forgotten to change the state of patches 1 and 3 from 'Changes > Requested' back to 'New' and that's why I missed them. I did that now so > they are back in my queue: > > https://patchwork.kernel.org/patch/9158855/ > https://patchwork.kernel.org/patch/9158851/ > > Thanks for checking. > Ok, thanks for the information. Best regards,
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index b17f3d09a2c7..ff3f63ed95e1 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy, int *dbm) { struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); - struct mwifiex_private *priv = mwifiex_get_priv(adapter, - MWIFIEX_BSS_ROLE_ANY); - int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, - HostCmd_ACT_GEN_GET, 0, NULL, true); - - if (ret < 0) - return ret; - - /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */ - *dbm = priv->tx_power_level; + struct mwifiex_private *priv; - return 0; + priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); + return mwifiex_get_tx_power(priv, dbm); } /* diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 0207af00be42..79c28cfb7780 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -1464,6 +1464,8 @@ int mwifiex_drv_get_driver_version(struct mwifiex_adapter *adapter, int mwifiex_set_tx_power(struct mwifiex_private *priv, struct mwifiex_power_cfg *power_cfg); +int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm); + int mwifiex_main_process(struct mwifiex_adapter *); int mwifiex_queue_tx_pkt(struct mwifiex_private *priv, struct sk_buff *skb); diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c index 8e0862657122..70ff9b805b5b 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c @@ -775,6 +775,24 @@ int mwifiex_set_tx_power(struct mwifiex_private *priv, } /* + * IOCTL request handler to get tx power configuration. + * + * This function prepares the correct firmware command and + * issues it. + */ +int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm) +{ + int ret; + + ret = mwifiex_send_cmd(priv, HostCmd_CMD_TXPWR_CFG, + HostCmd_ACT_GEN_GET, 0, NULL, true); + + *dbm = priv->tx_power_level; + + return ret; +} + +/* * IOCTL request handler to get power save mode. * * This function prepares the correct firmware command and