[PATCH 0/4] staging: wilc1000: modify functions by making use of GENMASK macro

[PATCH 0/4] staging: wilc1000: modify functions by making use of GENMASK macro

[Ajay Singh]

This patch series contains changes to refactor functions by making use of
GENMASK macro and also removed unnecessary variable from
wilc_spi_read_int().

Ajay Singh (4):
  staging: wilc1000: remove use of 'happened' variable in
    wilc_spi_read_int()
  staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro
  staging: wilc1000: refactor wilc_spi_clear_int_ext() by using GENMASK
    macro
  staging: wilc1000: refactor sdio_clear_int_ext() by using GENMASK
    macro

 drivers/staging/wilc1000/wilc_sdio.c | 74 +++++++++++++++---------------------
 drivers/staging/wilc1000/wilc_spi.c  | 57 +++++++++++----------------
 2 files changed, 53 insertions(+), 78 deletions(-)

-- 
2.7.4

[PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()

[Ajay Singh]

Modified wilc_spi_read_int() by removing unnecessary use of "happened"
variable.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_spi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
index 6b392c9..131d2b7 100644
--- a/drivers/staging/wilc1000/wilc_spi.c
+++ b/drivers/staging/wilc1000/wilc_spi.c
@@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 	int ret;
 	u32 tmp;
 	u32 byte_cnt;
-	int happened, j;
+	int j;
 	u32 unknown_mask;
 	u32 irq_flags;
 	int k = IRG_FLAGS_OFFSET + 5;
@@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 
 	j = 0;
 	do {
-		happened = 0;
-
 		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
 		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
 
@@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 			dev_err(&spi->dev,
 				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
 				j, tmp, unknown_mask);
-				happened = 1;
+				break;
 		}
 
 		j++;
-	} while (happened);
+	} while (1);
 
 	*int_status = tmp;
 
-- 
2.7.4

Re: [PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()

[Dan Carpenter]

On Wed, Feb 21, 2018 at 09:42:09PM +0530, Ajay Singh wrote:
> Modified wilc_spi_read_int() by removing unnecessary use of "happened"
> variable.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_spi.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
> index 6b392c9..131d2b7 100644
> --- a/drivers/staging/wilc1000/wilc_spi.c
> +++ b/drivers/staging/wilc1000/wilc_spi.c
> @@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  	int ret;
>  	u32 tmp;
>  	u32 byte_cnt;
> -	int happened, j;
> +	int j;
>  	u32 unknown_mask;
>  	u32 irq_flags;
>  	int k = IRG_FLAGS_OFFSET + 5;
> @@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  
>  	j = 0;
>  	do {
> -		happened = 0;
> -
>  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
>  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
>  
> @@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  			dev_err(&spi->dev,
>  				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
>  				j, tmp, unknown_mask);
> -				happened = 1;
> +				break;

This is flipped around.  happened means don't break, but you've changed
it to be the opposite.

regards,
dan carpenter

>  		}
>  
>  		j++;
> -	} while (happened);
> +	} while (1);
>  

Re: [PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()

[Ajay Singh]

Hi Dan,

On Thu, 22 Feb 2018 10:20:58 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Wed, Feb 21, 2018 at 09:42:09PM +0530, Ajay Singh wrote:
> > Modified wilc_spi_read_int() by removing unnecessary use of "happened"
> > variable.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/wilc_spi.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
> > index 6b392c9..131d2b7 100644
> > --- a/drivers/staging/wilc1000/wilc_spi.c
> > +++ b/drivers/staging/wilc1000/wilc_spi.c
> > @@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> >  	int ret;
> >  	u32 tmp;
> >  	u32 byte_cnt;
> > -	int happened, j;
> > +	int j;
> >  	u32 unknown_mask;
> >  	u32 irq_flags;
> >  	int k = IRG_FLAGS_OFFSET + 5;
> > @@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> >  
> >  	j = 0;
> >  	do {
> > -		happened = 0;
> > -
> >  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
> >  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
> >  
> > @@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> >  			dev_err(&spi->dev,
> >  				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
> >  				j, tmp, unknown_mask);
> > -				happened = 1;
> > +				break;  
> 
> This is flipped around.  happened means don't break, but you've changed
> it to be the opposite.

You are right. Thanks for pointing it out. It's was a mistake. I will
change 'break' to 'continue' and while(1) to while(0) and resubmit the
patch.


Regards,
Ajay

> 
> regards,
> dan carpenter
> 
> >  		}
> >  
> >  		j++;
> > -	} while (happened);
> > +	} while (1);
> >    
> 

Re: [PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()

[Dan Carpenter]

On Thu, Feb 22, 2018 at 02:09:01PM +0530, Ajay Singh wrote:
> Hi Dan,
> 
> On Thu, 22 Feb 2018 10:20:58 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Wed, Feb 21, 2018 at 09:42:09PM +0530, Ajay Singh wrote:
> > > Modified wilc_spi_read_int() by removing unnecessary use of "happened"
> > > variable.
> > > 
> > > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > > ---
> > >  drivers/staging/wilc1000/wilc_spi.c | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
> > > index 6b392c9..131d2b7 100644
> > > --- a/drivers/staging/wilc1000/wilc_spi.c
> > > +++ b/drivers/staging/wilc1000/wilc_spi.c
> > > @@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> > >  	int ret;
> > >  	u32 tmp;
> > >  	u32 byte_cnt;
> > > -	int happened, j;
> > > +	int j;
> > >  	u32 unknown_mask;
> > >  	u32 irq_flags;
> > >  	int k = IRG_FLAGS_OFFSET + 5;
> > > @@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> > >  
> > >  	j = 0;
> > >  	do {
> > > -		happened = 0;
> > > -
> > >  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
> > >  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
> > >  
> > > @@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> > >  			dev_err(&spi->dev,
> > >  				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
> > >  				j, tmp, unknown_mask);
> > > -				happened = 1;
> > > +				break;  
> > 
> > This is flipped around.  happened means don't break, but you've changed
> > it to be the opposite.
> 
> You are right. Thanks for pointing it out. It's was a mistake. I will
> change 'break' to 'continue' and while(1) to while(0) and resubmit the
> patch.
> 

Don't be in a hurry to resend.  I always wait over night before
resending so that I'm not stressed when I review it.  What you are
proposing still sounds wrong because the j++ is essential.  Anyway, I
can't really review your v2 patch until you send it.

regards,
dan carpenter

[PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro

[Ajay Singh]

Use existing macro GENMASK to get the bitmask value. Moved the code to
get the bitmask value outside the loop, as its only required one time.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_spi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
index 131d2b7..c63f534 100644
--- a/drivers/staging/wilc1000/wilc_spi.c
+++ b/drivers/staging/wilc1000/wilc_spi.c
@@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 	tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
 
 	j = 0;
+	unknown_mask = GENMASK(g_spi.nint - 1, 0);
 	do {
 		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
 		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
@@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 			tmp |= (((irq_flags >> 0) & 0x7) << k);
 		}
 
-		unknown_mask = ~((1ul << g_spi.nint) - 1);
-
 		if ((tmp >> IRG_FLAGS_OFFSET) & unknown_mask) {
 			dev_err(&spi->dev,
 				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
-- 
2.7.4

Re: [PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro

[Dan Carpenter]

On Wed, Feb 21, 2018 at 09:42:10PM +0530, Ajay Singh wrote:
> Use existing macro GENMASK to get the bitmask value. Moved the code to
> get the bitmask value outside the loop, as its only required one time.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_spi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
> index 131d2b7..c63f534 100644
> --- a/drivers/staging/wilc1000/wilc_spi.c
> +++ b/drivers/staging/wilc1000/wilc_spi.c
> @@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  	tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
>  
>  	j = 0;
> +	unknown_mask = GENMASK(g_spi.nint - 1, 0);
>  	do {
>  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
>  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
> @@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  			tmp |= (((irq_flags >> 0) & 0x7) << k);
>  		}
>  
> -		unknown_mask = ~((1ul << g_spi.nint) - 1);
> -

This isn't right at all...  Say g_spi.nint is zero, then we're doing
GENMASK(-1, 0) which seems like it should be undefined.  If g_spi.nint
is 1 then "GENMASK(1 - 1, 0)" is 0x1 but "~((1 < 1) - 1)" is ~0x1.

I'm done reviewing this patch series...  You need to be more careful.
Create a small test program to test your patches.  As a reviewer,
creating test programs is how I review your patches.

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include <string.h>
#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
#include "kernel.h"
#include <assert.h>
#include <fcntl.h>
#include <sys/time.h>
#include <sys/ioctl.h>
#include <linux/blktrace_api.h>
#include <linux/fs.h>

#define BITS_PER_LONG 64
#define GENMASK(h, l) \
        (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))

int main(void)
{
        int i;
        u32 mask1, mask2;

        for (i = 0; i < 32; i++) {
                mask1 = ~((1ul << i) - 1);
                mask2 = GENMASK(i - 1, 0);
                if (mask1 == mask2)
                        continue;
                printf("ONE 0x%x %d\n", mask1, i);
                printf("TWO 0x%x\n", mask2);
        }
        return 0;
}

regards,
dan carpenter

Re: [PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro

[Claudiu Beznea]

Hi Dan,

On 22.02.2018 09:37, Dan Carpenter wrote:
> On Wed, Feb 21, 2018 at 09:42:10PM +0530, Ajay Singh wrote:
>> Use existing macro GENMASK to get the bitmask value. Moved the code to
>> get the bitmask value outside the loop, as its only required one time.
>>
>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>> ---
>>  drivers/staging/wilc1000/wilc_spi.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
>> index 131d2b7..c63f534 100644
>> --- a/drivers/staging/wilc1000/wilc_spi.c
>> +++ b/drivers/staging/wilc1000/wilc_spi.c
>> @@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>>  	tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
>>  
>>  	j = 0;
>> +	unknown_mask = GENMASK(g_spi.nint - 1, 0);
>>  	do {
>>  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
>>  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
>> @@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>>  			tmp |= (((irq_flags >> 0) & 0x7) << k);
>>  		}
>>  
>> -		unknown_mask = ~((1ul << g_spi.nint) - 1);
>> -
> 
> This isn't right at all...  Say g_spi.nint is zero, then we're doing
> GENMASK(-1, 0) which seems like it should be undefined.  If g_spi.nint
> is 1 then "GENMASK(1 - 1, 0)" is 0x1 but "~((1 < 1) - 1)" is ~0x1.
> 

Even in this driver g_spi.nint is always constant and equal with NUM_INT_EXT
(which is 3), it seems that enabling interrupts before initializing g_spi.nint
may lead to the state where g_spi.nint = 0, as Dan pointed.

int wilc1000_wlan_init(struct net_device *dev, struct wilc_vif *vif)
{
		// ...
                if (wl->gpio >= 0 && init_irq(dev)) {
                        ret = -EIO;
                        goto _fail_locks_;
                }

		// ...
                ret = linux_wlan_start_firmware(dev); -> calls
linux_wlan_start_firmware which in turn calls wilc->hif_func->hif_sync_ext(wilc,
NUM_INT_EXT);

	
}

Thank you,
Claudiu Beznea

> I'm done reviewing this patch series...  You need to be more careful.
> Create a small test program to test your patches.  As a reviewer,
> creating test programs is how I review your patches.
> 
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <limits.h>
> #include <string.h>
> #include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
> #include "kernel.h"
> #include <assert.h>
> #include <fcntl.h>
> #include <sys/time.h>
> #include <sys/ioctl.h>
> #include <linux/blktrace_api.h>
> #include <linux/fs.h>
> 
> #define BITS_PER_LONG 64
> #define GENMASK(h, l) \
>         (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> 
> int main(void)
> {
>         int i;
>         u32 mask1, mask2;
> 
>         for (i = 0; i < 32; i++) {
>                 mask1 = ~((1ul << i) - 1);
>                 mask2 = GENMASK(i - 1, 0);
>                 if (mask1 == mask2)
>                         continue;
>                 printf("ONE 0x%x %d\n", mask1, i);
>                 printf("TWO 0x%x\n", mask2);
>         }
>         return 0;
> }
> 
> regards,
> dan carpenter
> 
> 
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
> 

Re: [PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro

[Claudiu Beznea]

On 22.02.2018 11:23, Claudiu Beznea wrote:
> Hi Dan,
Sorry, I intended to be address this to Ajay,
> 
> On 22.02.2018 09:37, Dan Carpenter wrote:
>> On Wed, Feb 21, 2018 at 09:42:10PM +0530, Ajay Singh wrote:
>>> Use existing macro GENMASK to get the bitmask value. Moved the code to
>>> get the bitmask value outside the loop, as its only required one time.
>>>
>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>> ---
>>>  drivers/staging/wilc1000/wilc_spi.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
>>> index 131d2b7..c63f534 100644
>>> --- a/drivers/staging/wilc1000/wilc_spi.c
>>> +++ b/drivers/staging/wilc1000/wilc_spi.c
>>> @@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>>>  	tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
>>>  
>>>  	j = 0;
>>> +	unknown_mask = GENMASK(g_spi.nint - 1, 0);
>>>  	do {
>>>  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
>>>  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
>>> @@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>>>  			tmp |= (((irq_flags >> 0) & 0x7) << k);
>>>  		}
>>>  
>>> -		unknown_mask = ~((1ul << g_spi.nint) - 1);
>>> -
>>
>> This isn't right at all...  Say g_spi.nint is zero, then we're doing
>> GENMASK(-1, 0) which seems like it should be undefined.  If g_spi.nint
>> is 1 then "GENMASK(1 - 1, 0)" is 0x1 but "~((1 < 1) - 1)" is ~0x1.
>>
> 
> Even in this driver g_spi.nint is always constant and equal with NUM_INT_EXT
> (which is 3), it seems that enabling interrupts before initializing g_spi.nint
> may lead to the state where g_spi.nint = 0, as Dan pointed.
> 
> int wilc1000_wlan_init(struct net_device *dev, struct wilc_vif *vif)
> {
> 		// ...
>                 if (wl->gpio >= 0 && init_irq(dev)) {
>                         ret = -EIO;
>                         goto _fail_locks_;
>                 }
> 
> 		// ...
>                 ret = linux_wlan_start_firmware(dev); -> calls
> linux_wlan_start_firmware which in turn calls wilc->hif_func->hif_sync_ext(wilc,
> NUM_INT_EXT);
> 
> 	
> }
> 
> Thank you,
> Claudiu Beznea
> 
>> I'm done reviewing this patch series...  You need to be more careful.
>> Create a small test program to test your patches.  As a reviewer,
>> creating test programs is how I review your patches.
>>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <limits.h>
>> #include <string.h>
>> #include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
>> #include "kernel.h"
>> #include <assert.h>
>> #include <fcntl.h>
>> #include <sys/time.h>
>> #include <sys/ioctl.h>
>> #include <linux/blktrace_api.h>
>> #include <linux/fs.h>
>>
>> #define BITS_PER_LONG 64
>> #define GENMASK(h, l) \
>>         (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>>
>> int main(void)
>> {
>>         int i;
>>         u32 mask1, mask2;
>>
>>         for (i = 0; i < 32; i++) {
>>                 mask1 = ~((1ul << i) - 1);
>>                 mask2 = GENMASK(i - 1, 0);
>>                 if (mask1 == mask2)
>>                         continue;
>>                 printf("ONE 0x%x %d\n", mask1, i);
>>                 printf("TWO 0x%x\n", mask2);
>>         }
>>         return 0;
>> }
>>
>> regards,
>> dan carpenter
>>
>>
>>
>> _______________________________________________
>> devel mailing list
>> devel@linuxdriverproject.org
>> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>>
> 

[PATCH 3/4] staging: wilc1000: refactor wilc_spi_clear_int_ext() by using GENMASK macro

[Ajay Singh]

Use available macro GENMASK to get the bitmask value of specific value.
Simplified the logic by adding expected_irqs & unexpected_irqs to check
the interrupt bits.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_spi.c | 46 +++++++++++++++----------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
index c63f534..12827c2 100644
--- a/drivers/staging/wilc1000/wilc_spi.c
+++ b/drivers/staging/wilc1000/wilc_spi.c
@@ -985,8 +985,9 @@ static int wilc_spi_clear_int_ext(struct wilc *wilc, u32 val)
 {
 	struct spi_device *spi = to_spi_device(wilc->dev);
 	int ret;
-	u32 flags;
 	u32 tbl_ctl;
+	u32 expected_irqs, unexpected_irqs;
+	int i;
 
 	if (g_spi.has_thrpt_enh) {
 		ret = spi_internal_write(wilc, 0xe844 - WILC_SPI_REG_BASE,
@@ -994,37 +995,26 @@ static int wilc_spi_clear_int_ext(struct wilc *wilc, u32 val)
 		return ret;
 	}
 
-	flags = val & (BIT(MAX_NUM_INT) - 1);
-	if (flags) {
-		int i;
+	expected_irqs = val & GENMASK(g_spi.nint - 1, 0);
+	unexpected_irqs = val & GENMASK(MAX_NUM_INT - 1, g_spi.nint);
 
-		ret = 1;
-		for (i = 0; i < g_spi.nint; i++) {
-			/*
-			 * No matter what you write 1 or 0,
-			 * it will clear interrupt.
-			 */
-			if (flags & 1)
-				ret = wilc_spi_write_reg(wilc,
-							 0x10c8 + i * 4, 1);
-			if (!ret)
-				break;
-			flags >>= 1;
-		}
-		if (!ret) {
-			dev_err(&spi->dev,
-				"Failed wilc_spi_write_reg, set reg %x ...\n",
-				0x10c8 + i * 4);
-			goto _fail_;
-		}
-		for (i = g_spi.nint; i < MAX_NUM_INT; i++) {
-			if (flags & 1)
+	for (i = 0; i < g_spi.nint && expected_irqs; i++) {
+		/* No matter what you write 1 or 0, it will clear interrupt. */
+		if (expected_irqs & BIT(i)) {
+			ret = wilc_spi_write_reg(wilc, 0x10c8 + i * 4, 1);
+			if (!ret) {
 				dev_err(&spi->dev,
-					"Unexpected interrupt cleared %d...\n",
-					i);
-			flags >>= 1;
+					"Failed wilc_spi_write_reg, set reg %x ...\n",
+					0x10c8 + i * 4);
+				goto _fail_;
+			}
 		}
 	}
+	for (i = g_spi.nint; i < MAX_NUM_INT && unexpected_irqs; i++) {
+		if (unexpected_irqs & BIT(i))
+			dev_err(&spi->dev,
+				"Unexpected interrupt cleared %d...\n", i);
+	}
 
 	tbl_ctl = 0;
 	/* select VMM table 0 */
-- 
2.7.4

Re: [PATCH 3/4] staging: wilc1000: refactor wilc_spi_clear_int_ext() by using GENMASK macro

[Dan Carpenter]

Please check all these again, right?  I've glanced at this and it seems
wrong, but I'm too stupid to sure immediately and too lazy to be
thourough.

regards,
dan caprenter

[PATCH 4/4] staging: wilc1000: refactor sdio_clear_int_ext() by using GENMASK macro

[Ajay Singh]

Use GENMASK macro in sdio_clear_int_ext function to get the required
bitmask value. Simplified the logic by making use of GENMASK
macro.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_sdio.c | 74 +++++++++++++++---------------------
 1 file changed, 31 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_sdio.c b/drivers/staging/wilc1000/wilc_sdio.c
index a088999..8218085 100644
--- a/drivers/staging/wilc1000/wilc_sdio.c
+++ b/drivers/staging/wilc1000/wilc_sdio.c
@@ -876,14 +876,9 @@ static int sdio_clear_int_ext(struct wilc *wilc, u32 val)
 	if (g_sdio.has_thrpt_enh3) {
 		u32 reg;
 
-		if (g_sdio.irq_gpio) {
-			u32 flags;
+		reg = g_sdio.irq_gpio ?
+			(val & GENMASK(MAX_NUN_INT_THRPT_ENH2 - 1, 0)) : 0;
 
-			flags = val & (BIT(MAX_NUN_INT_THRPT_ENH2) - 1);
-			reg = flags;
-		} else {
-			reg = 0;
-		}
 		/* select VMM table 0 */
 		if ((val & SEL_VMM_TBL0) == SEL_VMM_TBL0)
 			reg |= BIT(5);
@@ -918,45 +913,38 @@ static int sdio_clear_int_ext(struct wilc *wilc, u32 val)
 		 * Cannot clear multiple interrupts.
 		 * Must clear each interrupt individually.
 		 */
-		u32 flags;
-
-		flags = val & (BIT(MAX_NUM_INT) - 1);
-		if (flags) {
-			int i;
-
-			ret = 1;
-			for (i = 0; i < g_sdio.nint; i++) {
-				if (flags & 1) {
-					struct sdio_cmd52 cmd;
-
-					cmd.read_write = 1;
-					cmd.function = 0;
-					cmd.raw = 0;
-					cmd.address = 0xf8;
-					cmd.data = BIT(i);
-
-					ret = wilc_sdio_cmd52(wilc, &cmd);
-					if (ret) {
-						dev_err(&func->dev,
-							"Failed cmd52, set 0xf8 data (%d) ...\n",
-							__LINE__);
-						goto _fail_;
-					}
-				}
-				if (!ret)
-					break;
-				flags >>= 1;
-			}
-			if (!ret)
-				goto _fail_;
-			for (i = g_sdio.nint; i < MAX_NUM_INT; i++) {
-				if (flags & 1)
+		int i;
+		u32 expected_irqs, unexpected_irqs;
+
+		expected_irqs = val & GENMASK(g_sdio.nint - 1, 0);
+		unexpected_irqs = val & GENMASK(MAX_NUM_INT - 1, g_sdio.nint);
+
+		for (i = 0; i < g_sdio.nint && expected_irqs; i++) {
+			if (expected_irqs & BIT(i)) {
+				struct sdio_cmd52 cmd;
+
+				cmd.read_write = 1;
+				cmd.function = 0;
+				cmd.raw = 0;
+				cmd.address = 0xf8;
+				cmd.data = BIT(i);
+
+				ret = wilc_sdio_cmd52(wilc, &cmd);
+				if (ret) {
 					dev_err(&func->dev,
-						"Unexpected interrupt cleared %d...\n",
-						i);
-				flags >>= 1;
+						"Failed cmd52, set 0xf8 data (%d) ...\n",
+						__LINE__);
+					goto _fail_;
+				}
 			}
 		}
+
+		for (i = g_sdio.nint; i < MAX_NUM_INT && unexpected_irqs; i++) {
+			if (unexpected_irqs & BIT(i))
+				dev_err(&func->dev,
+					"Unexpected interrupt cleared %d...\n",
+					i);
+		}
 	}
 
 	vmm_ctl = 0;
-- 
2.7.4

Re: [PATCH 0/4] staging: wilc1000: modify functions by making use of GENMASK macro

[Ajay Singh]

Please ignore this patch series, I will work on review comments and submit
another version later.
 
Regards,
Ajay