[PATCH] i2c: i2c-sh_mobile irq rollback fix

[PATCH] i2c: i2c-sh_mobile irq rollback fix

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Update the i2c-sh_mobile driver to properly free
interrupts. The function sh_mobile_i2c_hook_irqs()
is fixed so module both unload and load are working
as expected.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/i2c/busses/i2c-sh_mobile.c |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

--- 0001/drivers/i2c/busses/i2c-sh_mobile.c
+++ work/drivers/i2c/busses/i2c-sh_mobile.c	2010-08-02 15:14:48.000000000 +0900
@@ -538,15 +538,17 @@ static int sh_mobile_i2c_hook_irqs(struc
 {
 	struct resource *res;
 	int ret = -ENXIO;
-	int q, m;
-	int k = 0;
-	int n = 0;
+	int n, k = 0;
 
 	while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
 		for (n = res->start; hook && n <= res->end; n++) {
 			if (request_irq(n, sh_mobile_i2c_isr, IRQF_DISABLED,
-					dev_name(&dev->dev), dev))
+					dev_name(&dev->dev), dev)) {
+				for (n--; n >= res->start; n--)
+					free_irq(n, dev);
+
 				goto rollback;
+			}
 		}
 		k++;
 	}
@@ -554,16 +556,17 @@ static int sh_mobile_i2c_hook_irqs(struc
 	if (hook)
 		return k > 0 ? 0 : -ENOENT;
 
-	k--;
 	ret = 0;
 
  rollback:
-	for (q = k; k >= 0; k--) {
-		for (m = n; m >= res->start; m--)
-			free_irq(m, dev);
+	k--;
+
+	while (k >= 0) {
+		res = platform_get_resource(dev, IORESOURCE_IRQ, k);
+		for (n = res->start; n <= res->end; n++)
+			free_irq(n, dev);
 
-		res = platform_get_resource(dev, IORESOURCE_IRQ, k - 1);
-		m = res->end;
+		k--;
 	}
 
 	return ret;

Re: [PATCH] i2c: i2c-sh_mobile irq rollback fix

[Paul Mundt]

On Mon, Aug 02, 2010 at 04:16:37PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Update the i2c-sh_mobile driver to properly free
> interrupts. The function sh_mobile_i2c_hook_irqs()
> is fixed so module both unload and load are working
> as expected.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Applied, thanks.

Re: [PATCH] i2c: i2c-sh_mobile irq rollback fix

[Jean Delvare]

On Mon, 02 Aug 2010 16:16:37 +0900, Magnus Damm wrote:
> From: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
> 
> Update the i2c-sh_mobile driver to properly free
> interrupts. The function sh_mobile_i2c_hook_irqs()
> is fixed so module both unload and load are working
> as expected.
> 
> Signed-off-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
> ---
> 
>  drivers/i2c/busses/i2c-sh_mobile.c |   23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
> +++ work/drivers/i2c/busses/i2c-sh_mobile.c	2010-08-02 15:14:48.000000000 +0900
> @@ -538,15 +538,17 @@ static int sh_mobile_i2c_hook_irqs(struc
>  {
>  	struct resource *res;
>  	int ret = -ENXIO;
> -	int q, m;
> -	int k = 0;
> -	int n = 0;
> +	int n, k = 0;

You can't really hope to get the code right with such variable names.

-- 
Jean Delvare

Re: [PATCH] i2c: i2c-sh_mobile irq rollback fix

[Magnus Damm]

On Tue, Aug 10, 2010 at 10:18 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, 02 Aug 2010 16:16:37 +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
>>
>> Update the i2c-sh_mobile driver to properly free
>> interrupts. The function sh_mobile_i2c_hook_irqs()
>> is fixed so module both unload and load are working
>> as expected.
>>
>> Signed-off-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
>> ---
>>
>>  drivers/i2c/busses/i2c-sh_mobile.c |   23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
>> +++ work/drivers/i2c/busses/i2c-sh_mobile.c   2010-08-02 15:14:48.000000000 +0900
>> @@ -538,15 +538,17 @@ static int sh_mobile_i2c_hook_irqs(struc
>>  {
>>       struct resource *res;
>>       int ret = -ENXIO;
>> -     int q, m;
>> -     int k = 0;
>> -     int n = 0;
>> +     int n, k = 0;
>
> You can't really hope to get the code right with such variable names.

And your suggestion is...? If you see something wrong with the code
please let me know.

Thanks,

/ magnus

Re: [PATCH] i2c: i2c-sh_mobile irq rollback fix

[Jean Delvare]

On Mon, 16 Aug 2010 10:59:06 +0900, Magnus Damm wrote:
> On Tue, Aug 10, 2010 at 10:18 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Mon, 02 Aug 2010 16:16:37 +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
> >>
> >> Update the i2c-sh_mobile driver to properly free
> >> interrupts. The function sh_mobile_i2c_hook_irqs()
> >> is fixed so module both unload and load are working
> >> as expected.
> >>
> >> Signed-off-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
> >> ---
> >>
> >>  drivers/i2c/busses/i2c-sh_mobile.c |   23 +++++++++++++----------
> >>  1 file changed, 13 insertions(+), 10 deletions(-)
> >>
> >> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
> >> +++ work/drivers/i2c/busses/i2c-sh_mobile.c   2010-08-02 15:14:48.000000000 +0900
> >> @@ -538,15 +538,17 @@ static int sh_mobile_i2c_hook_irqs(struc
> >>  {
> >>       struct resource *res;
> >>       int ret = -ENXIO;
> >> -     int q, m;
> >> -     int k = 0;
> >> -     int n = 0;
> >> +     int n, k = 0;
> >
> > You can't really hope to get the code right with such variable names.
> 
> And your suggestion is...? If you see something wrong with the code
> please let me know.

Nothing specific. I'm just trying to make you realize that one-letter
variable names don't carry any meaning and thus make the code harder to
write, read and debug.

-- 
Jean Delvare

Re: [PATCH] i2c: i2c-sh_mobile irq rollback fix

[Magnus Damm]

On Mon, Aug 16, 2010 at 3:24 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, 16 Aug 2010 10:59:06 +0900, Magnus Damm wrote:
>> On Tue, Aug 10, 2010 at 10:18 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>> > On Mon, 02 Aug 2010 16:16:37 +0900, Magnus Damm wrote:
>> >> From: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
>> >>
>> >> Update the i2c-sh_mobile driver to properly free
>> >> interrupts. The function sh_mobile_i2c_hook_irqs()
>> >> is fixed so module both unload and load are working
>> >> as expected.
>> >>
>> >> Signed-off-by: Magnus Damm <damm-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
>> >> ---
>> >>
>> >>  drivers/i2c/busses/i2c-sh_mobile.c |   23 +++++++++++++----------
>> >>  1 file changed, 13 insertions(+), 10 deletions(-)
>> >>
>> >> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
>> >> +++ work/drivers/i2c/busses/i2c-sh_mobile.c   2010-08-02 15:14:48.000000000 +0900
>> >> @@ -538,15 +538,17 @@ static int sh_mobile_i2c_hook_irqs(struc
>> >>  {
>> >>       struct resource *res;
>> >>       int ret = -ENXIO;
>> >> -     int q, m;
>> >> -     int k = 0;
>> >> -     int n = 0;
>> >> +     int n, k = 0;
>> >
>> > You can't really hope to get the code right with such variable names.
>>
>> And your suggestion is...? If you see something wrong with the code
>> please let me know.
>
> Nothing specific. I'm just trying to make you realize that one-letter
> variable names don't carry any meaning and thus make the code harder to
> write, read and debug.

Yes, I admit that four one-letter variables in a single function is
begging for trouble. Having one or two one-letter variable names is
not so bad IMO.

I guess one-letter variable names is my attempt to stick to pure
function and avoid going in the direction of endless coding style
discussions. =)

Cheers,

/ magnus

Re: [PATCH] i2c: i2c-sh_mobile irq rollback fix

[Ben Dooks]

On Tue, Aug 10, 2010 at 03:18:28PM +0200, Jean Delvare wrote:
> On Mon, 02 Aug 2010 16:16:37 +0900, Magnus Damm wrote:
> > From: Magnus Damm <damm@opensource.se>
> > 
> > Update the i2c-sh_mobile driver to properly free
> > interrupts. The function sh_mobile_i2c_hook_irqs()
> > is fixed so module both unload and load are working
> > as expected.
> > 
> > Signed-off-by: Magnus Damm <damm@opensource.se>
> > ---
> > 
> >  drivers/i2c/busses/i2c-sh_mobile.c |   23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ work/drivers/i2c/busses/i2c-sh_mobile.c	2010-08-02 15:14:48.000000000 +0900
> > @@ -538,15 +538,17 @@ static int sh_mobile_i2c_hook_irqs(struc
> >  {
> >  	struct resource *res;
> >  	int ret = -ENXIO;
> > -	int q, m;
> > -	int k = 0;
> > -	int n = 0;
> > +	int n, k = 0;
> 
> You can't really hope to get the code right with such variable names.

I agree, this isn't very readable. If possible, please have a rethink
of the code. You may also want to start using devm_ to track your
resources.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.