RE: Re: [PATCH 1/3] mwifiex: remove unnecessary wakeup interrupt number sanity check

[Xinming Hu]

Hi Brain,

> -Original Message-
> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: 2017年4月4日 2:34
> To: Dmitry Torokhov
> Cc: Xinming Hu; Linux Wireless; Kalle Valo; Rajat Jain; Amitkumar Karwar; 
> Cathy
> Luo; Xinming Hu
> Subject: [EXT] Re: [PATCH 1/3] mwifiex: remove unnecessary wakeup interrupt
> number sanity check
> 
> External Email
> 
> --
> On Fri, Mar 31, 2017 at 03:37:01PM -0700, Dmitry Torokhov wrote:
> > On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu <huxinming...@gmail.com>
> wrote:
> > > From: Xinming Hu <h...@marvell.com>
> > >
> > > Sanity check of interrupt number in interrupt handler is unnecessary
> > > and confusion, remove it.
> >
> > I'd reworded the explanation a bit: "If wakeup interrupt handler is
> > called, we know that the wakeup interrupt number is valid, there is no
> > need to check it."
> 
> Either way works for me (modulo grammar), though Dmitry's is more
> informative. So +1.
> 
> > Otherwise:
> >
> > Reviewed-by: Dmitry Torokhov <d...@chromium.org>
> 
> Ooh, me too!
> 
> Reviewed-by: Brian Norris <briannor...@chromium.org>
> 
> I believe this same pattern occurs in btmrvl_sdio. Possibly others.
> 

Thanks for the review, the similar change have been applied to Bluetooth(commit 
733db4ccec)

Thanks
SImon
> > > Signed-off-by: Xinming Hu <h...@marvell.com>
> > > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > > ---
> > >  drivers/net/wireless/marvell/mwifiex/main.c | 8 +++-
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> > > b/drivers/net/wireless/marvell/mwifiex/main.c
> > > index 30f4994..5e82602 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > > @@ -1503,11 +1503,9 @@ static irqreturn_t
> > > mwifiex_irq_wakeup_handler(int irq, void *priv)  {
> > > struct mwifiex_adapter *adapter = priv;
> > >
> > > -   if (adapter->irq_wakeup >= 0) {
> > > -   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > > -   adapter->wake_by_wifi = true;
> > > -   disable_irq_nosync(irq);
> > > -   }
> > > +   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > > +   adapter->wake_by_wifi = true;
> > > +   disable_irq_nosync(irq);
> > >
> > > /* Notify PM core we are wakeup source */
> > > pm_wakeup_event(adapter->dev, 0);
> > > --
> > > 1.8.1.4
> > >

RE: [EXT] Re: [PATCH 1/3] mwifiex: remove unnecessary wakeup interrupt number sanity check

[Xinming Hu]

Hi Dmitry,

> -Original Message-
> From: Dmitry Torokhov [mailto:d...@chromium.org]
> Sent: 2017年4月1日 6:37
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Brian Norris; Rajat Jain; Amitkumar Karwar;
> Cathy Luo; Xinming Hu
> Subject: [EXT] Re: [PATCH 1/3] mwifiex: remove unnecessary wakeup interrupt
> number sanity check
> 
> External Email
> 
> --
> On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu <huxinming...@gmail.com>
> wrote:
> > From: Xinming Hu <h...@marvell.com>
> >
> > Sanity check of interrupt number in interrupt handler is unnecessary
> > and confusion, remove it.
> 
> I'd reworded the explanation a bit: "If wakeup interrupt handler is called, we
> know that the wakeup interrupt number is valid, there is no need to check it."
> 
> Otherwise:
> 
> Reviewed-by: Dmitry Torokhov <d...@chromium.org>
> 

Thanks for the review. Will reword in V2.

Thanks
Simon
> >
> > Signed-off-by: Xinming Hu <h...@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/main.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> > b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 30f4994..5e82602 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1503,11 +1503,9 @@ static irqreturn_t
> > mwifiex_irq_wakeup_handler(int irq, void *priv)  {
> > struct mwifiex_adapter *adapter = priv;
> >
> > -   if (adapter->irq_wakeup >= 0) {
> > -   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > -   adapter->wake_by_wifi = true;
> > -   disable_irq_nosync(irq);
> > -   }
> > +   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > +   adapter->wake_by_wifi = true;
> > +   disable_irq_nosync(irq);
> >
> > /* Notify PM core we are wakeup source */
> > pm_wakeup_event(adapter->dev, 0);
> > --
> > 1.8.1.4
> >

Re: [PATCH 1/3] mwifiex: remove unnecessary wakeup interrupt number sanity check

[Brian Norris]

On Fri, Mar 31, 2017 at 03:37:01PM -0700, Dmitry Torokhov wrote:
> On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu  wrote:
> > From: Xinming Hu 
> >
> > Sanity check of interrupt number in interrupt handler is unnecessary and
> > confusion, remove it.
> 
> I'd reworded the explanation a bit: "If wakeup interrupt handler is
> called, we know that the wakeup interrupt number is valid, there is no
> need to check it."

Either way works for me (modulo grammar), though Dmitry's is more
informative. So +1.

> Otherwise:
> 
> Reviewed-by: Dmitry Torokhov 

Ooh, me too!

Reviewed-by: Brian Norris 

I believe this same pattern occurs in btmrvl_sdio. Possibly others.

> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Amitkumar Karwar 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/main.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> > b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 30f4994..5e82602 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1503,11 +1503,9 @@ static irqreturn_t mwifiex_irq_wakeup_handler(int 
> > irq, void *priv)
> >  {
> > struct mwifiex_adapter *adapter = priv;
> >
> > -   if (adapter->irq_wakeup >= 0) {
> > -   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > -   adapter->wake_by_wifi = true;
> > -   disable_irq_nosync(irq);
> > -   }
> > +   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > +   adapter->wake_by_wifi = true;
> > +   disable_irq_nosync(irq);
> >
> > /* Notify PM core we are wakeup source */
> > pm_wakeup_event(adapter->dev, 0);
> > --
> > 1.8.1.4
> >

Re: [PATCH 1/3] mwifiex: remove unnecessary wakeup interrupt number sanity check

[Dmitry Torokhov]

On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu  wrote:
> From: Xinming Hu 
>
> Sanity check of interrupt number in interrupt handler is unnecessary and
> confusion, remove it.

I'd reworded the explanation a bit: "If wakeup interrupt handler is
called, we know that the wakeup interrupt number is valid, there is no
need to check it."

Otherwise:

Reviewed-by: Dmitry Torokhov 

>
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index 30f4994..5e82602 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1503,11 +1503,9 @@ static irqreturn_t mwifiex_irq_wakeup_handler(int irq, 
> void *priv)
>  {
> struct mwifiex_adapter *adapter = priv;
>
> -   if (adapter->irq_wakeup >= 0) {
> -   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> -   adapter->wake_by_wifi = true;
> -   disable_irq_nosync(irq);
> -   }
> +   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> +   adapter->wake_by_wifi = true;
> +   disable_irq_nosync(irq);
>
> /* Notify PM core we are wakeup source */
> pm_wakeup_event(adapter->dev, 0);
> --
> 1.8.1.4
>

[PATCH 1/3] mwifiex: remove unnecessary wakeup interrupt number sanity check

[Xinming Hu]

From: Xinming Hu 

Sanity check of interrupt number in interrupt handler is unnecessary and
confusion, remove it.

Signed-off-by: Xinming Hu 
Signed-off-by: Amitkumar Karwar 
---
 drivers/net/wireless/marvell/mwifiex/main.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 30f4994..5e82602 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1503,11 +1503,9 @@ static irqreturn_t mwifiex_irq_wakeup_handler(int irq, 
void *priv)
 {
struct mwifiex_adapter *adapter = priv;
 
-   if (adapter->irq_wakeup >= 0) {
-   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
-   adapter->wake_by_wifi = true;
-   disable_irq_nosync(irq);
-   }
+   dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
+   adapter->wake_by_wifi = true;
+   disable_irq_nosync(irq);
 
/* Notify PM core we are wakeup source */
pm_wakeup_event(adapter->dev, 0);
-- 
1.8.1.4