All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
@ 2023-03-27 20:18 Khadija Kamran
  2023-03-27 20:34 ` Alex Elder
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Khadija Kamran @ 2023-03-27 20:18 UTC (permalink / raw)
  To: outreachy
  Cc: Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
	greybus-dev, linux-staging, linux-kernel

Refactor function by adding goto statement. This reduces the
indentation and fixes the issue reported by checkpatch.pl script.

"CHECK: line length of 101 exceeds 100 columns"

Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
---
 drivers/staging/greybus/arche-platform.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
index fcbd5f71eff2..c7d3b6f7368f 100644
--- a/drivers/staging/greybus/arche-platform.c
+++ b/drivers/staging/greybus/arche-platform.c
@@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 				 */
 				if (arche_pdata->wake_detect_state !=
 						WD_STATE_COLDBOOT_START) {
-					arche_platform_set_wake_detect_state(arche_pdata,
-									     WD_STATE_COLDBOOT_TRIG);
-					spin_unlock_irqrestore(&arche_pdata->wake_lock,
-							       flags);
-					return IRQ_WAKE_THREAD;
+					goto out;
 				}
 			}
 		}
@@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
 
 	return IRQ_HANDLED;
+
+out:
+	arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
+	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
+	return IRQ_WAKE_THREAD;
 }
 
 /*
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
  2023-03-27 20:18 [PATCH] staging: greybus: refactor arche_platform_wd_irq() function Khadija Kamran
@ 2023-03-27 20:34 ` Alex Elder
  2023-03-27 20:49 ` Alison Schofield
  2023-03-28  7:33 ` Dan Carpenter
  2 siblings, 0 replies; 6+ messages in thread
From: Alex Elder @ 2023-03-27 20:34 UTC (permalink / raw)
  To: Khadija Kamran, outreachy
  Cc: Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
	greybus-dev, linux-staging, linux-kernel

On 3/27/23 3:18 PM, Khadija Kamran wrote:
> Refactor function by adding goto statement. This reduces the
> indentation and fixes the issue reported by checkpatch.pl script.
> 
> "CHECK: line length of 101 exceeds 100 columns"

Looking at this entire function, it seems a great deal of it
has somewhat wide lines.  Part of the problem is that it
relies on arche_platform_set_wake_detect_state(), which is
36 characters long all by itself.

In any case, the line that is identified is the widest, of
course, by 10 or more characters.  But changing that one
line doesn't substantially improve things.

I'm reluctant to suggest this, because I don't want a lot
of "code churn" patches to follow based on this, but...

One could rename arche_platform_set_wake_detect_state()
to be just set_wake_detect_state().  It's private to
its source file (arche-platform.c) and therefore the
"arche_plaform_" prefix isn't really necessary.  And
perhaps the result would be code that is a little more
readable, because its lines aren't so long.

I'd be happy to hear others' thoughts on this.

					-Alex



> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>   drivers/staging/greybus/arche-platform.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..c7d3b6f7368f 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>   				 */
>   				if (arche_pdata->wake_detect_state !=
>   						WD_STATE_COLDBOOT_START) {
> -					arche_platform_set_wake_detect_state(arche_pdata,
> -									     WD_STATE_COLDBOOT_TRIG);
> -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> -							       flags);
> -					return IRQ_WAKE_THREAD;
> +					goto out;
>   				}
>   			}
>   		}
> @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>   	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>   
>   	return IRQ_HANDLED;
> +
> +out:
> +	arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> +	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> +	return IRQ_WAKE_THREAD;
>   }
>   
>   /*


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
  2023-03-27 20:18 [PATCH] staging: greybus: refactor arche_platform_wd_irq() function Khadija Kamran
  2023-03-27 20:34 ` Alex Elder
@ 2023-03-27 20:49 ` Alison Schofield
  2023-03-28  7:33 ` Dan Carpenter
  2 siblings, 0 replies; 6+ messages in thread
From: Alison Schofield @ 2023-03-27 20:49 UTC (permalink / raw)
  To: Khadija Kamran
  Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder,
	Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel

On Tue, Mar 28, 2023 at 01:18:53AM +0500, Khadija Kamran wrote:
> Refactor function by adding goto statement. This reduces the
> indentation and fixes the issue reported by checkpatch.pl script.

It seems our emails may have crossed on this issue.

See: https://lore.kernel.org/outreachy/ZBrFqtyY%2FHydnBdT@kroah.com/T/#m5ac406a1ebef0ee5c013b6db75ce1368e292c179

Alison

> 
> "CHECK: line length of 101 exceeds 100 columns"
> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/greybus/arche-platform.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..c7d3b6f7368f 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  				 */
>  				if (arche_pdata->wake_detect_state !=
>  						WD_STATE_COLDBOOT_START) {
> -					arche_platform_set_wake_detect_state(arche_pdata,
> -									     WD_STATE_COLDBOOT_TRIG);
> -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> -							       flags);
> -					return IRQ_WAKE_THREAD;
> +					goto out;
>  				}
>  			}
>  		}
> @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>  
>  	return IRQ_HANDLED;
> +
> +out:
> +	arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> +	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> +	return IRQ_WAKE_THREAD;
>  }
>  
>  /*
> -- 
> 2.34.1
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
  2023-03-27 20:18 [PATCH] staging: greybus: refactor arche_platform_wd_irq() function Khadija Kamran
  2023-03-27 20:34 ` Alex Elder
  2023-03-27 20:49 ` Alison Schofield
@ 2023-03-28  7:33 ` Dan Carpenter
  2023-03-28 23:04   ` Alison Schofield
  2 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-03-28  7:33 UTC (permalink / raw)
  To: Khadija Kamran
  Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder,
	Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel

On Tue, Mar 28, 2023 at 01:18:53AM +0500, Khadija Kamran wrote:
> Refactor function by adding goto statement. This reduces the
> indentation and fixes the issue reported by checkpatch.pl script.
> 
> "CHECK: line length of 101 exceeds 100 columns"
> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/greybus/arche-platform.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..c7d3b6f7368f 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  				 */
>  				if (arche_pdata->wake_detect_state !=
>  						WD_STATE_COLDBOOT_START) {
> -					arche_platform_set_wake_detect_state(arche_pdata,
> -									     WD_STATE_COLDBOOT_TRIG);
> -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> -							       flags);
> -					return IRQ_WAKE_THREAD;
> +					goto out;

I don't like this goto.

It suggests that calling arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
is a part of a shared exit path when that is not true so it is sort of
lying to the reader and making things harder to understand.

Unlocking is shared.  goto unlock is fine.  But that doesn't help with
the very long lines.  (I am not saying that goto unlock is a worthwhile
patch to send but I would definitely have found it tolerable).

regards,
dan carpenter

>  				}
>  			}
>  		}
> @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>  
>  	return IRQ_HANDLED;
> +
> +out:
> +	arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> +	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> +	return IRQ_WAKE_THREAD;
>  }
>  
>  /*
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
  2023-03-28  7:33 ` Dan Carpenter
@ 2023-03-28 23:04   ` Alison Schofield
  2023-03-30 15:05     ` Khadija Kamran
  0 siblings, 1 reply; 6+ messages in thread
From: Alison Schofield @ 2023-03-28 23:04 UTC (permalink / raw)
  To: Khadija Kamran, Dan Carpenter
  Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder,
	Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel

On Tue, Mar 28, 2023 at 10:33:23AM +0300, Dan Carpenter wrote:
> On Tue, Mar 28, 2023 at 01:18:53AM +0500, Khadija Kamran wrote:
> > Refactor function by adding goto statement. This reduces the
> > indentation and fixes the issue reported by checkpatch.pl script.
> > 
> > "CHECK: line length of 101 exceeds 100 columns"
> > 
> > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > ---
> >  drivers/staging/greybus/arche-platform.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > index fcbd5f71eff2..c7d3b6f7368f 100644
> > --- a/drivers/staging/greybus/arche-platform.c
> > +++ b/drivers/staging/greybus/arche-platform.c
> > @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >  				 */
> >  				if (arche_pdata->wake_detect_state !=
> >  						WD_STATE_COLDBOOT_START) {
> > -					arche_platform_set_wake_detect_state(arche_pdata,
> > -									     WD_STATE_COLDBOOT_TRIG);
> > -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> > -							       flags);
> > -					return IRQ_WAKE_THREAD;
> > +					goto out;
> 
> I don't like this goto.
> 
> It suggests that calling arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> is a part of a shared exit path when that is not true so it is sort of
> lying to the reader and making things harder to understand.
> 
> Unlocking is shared.  goto unlock is fine.  But that doesn't help with
> the very long lines.  (I am not saying that goto unlock is a worthwhile
> patch to send but I would definitely have found it tolerable).
> 
> regards,
> dan carpenter


Khadija,
While you are reworking this, just wanted to note that, once 'out:'
becomes the single exit path for the function, it won't be a lie
any more. So you will have addressed Dan's concern.
Alison


> 
> >  				}
> >  			}
> >  		}
> > @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> >  
> >  	return IRQ_HANDLED;
> > +
> > +out:
> > +	arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> > +	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> > +	return IRQ_WAKE_THREAD;
> >  }
> >  
> >  /*
> > -- 
> > 2.34.1
> > 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
  2023-03-28 23:04   ` Alison Schofield
@ 2023-03-30 15:05     ` Khadija Kamran
  0 siblings, 0 replies; 6+ messages in thread
From: Khadija Kamran @ 2023-03-30 15:05 UTC (permalink / raw)
  To: Alison Schofield
  Cc: Dan Carpenter, outreachy, Vaibhav Hiremath, Johan Hovold,
	Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-staging,
	linux-kernel

On Tue, Mar 28, 2023 at 04:04:33PM -0700, Alison Schofield wrote:
> On Tue, Mar 28, 2023 at 10:33:23AM +0300, Dan Carpenter wrote:
> > On Tue, Mar 28, 2023 at 01:18:53AM +0500, Khadija Kamran wrote:
> > > Refactor function by adding goto statement. This reduces the
> > > indentation and fixes the issue reported by checkpatch.pl script.
> > > 
> > > "CHECK: line length of 101 exceeds 100 columns"
> > > 
> > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > > ---
> > >  drivers/staging/greybus/arche-platform.c | 11 ++++++-----
> > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > > index fcbd5f71eff2..c7d3b6f7368f 100644
> > > --- a/drivers/staging/greybus/arche-platform.c
> > > +++ b/drivers/staging/greybus/arche-platform.c
> > > @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > >  				 */
> > >  				if (arche_pdata->wake_detect_state !=
> > >  						WD_STATE_COLDBOOT_START) {
> > > -					arche_platform_set_wake_detect_state(arche_pdata,
> > > -									     WD_STATE_COLDBOOT_TRIG);
> > > -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> > > -							       flags);
> > > -					return IRQ_WAKE_THREAD;
> > > +					goto out;
> > 
> > I don't like this goto.
> > 
> > It suggests that calling arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> > is a part of a shared exit path when that is not true so it is sort of
> > lying to the reader and making things harder to understand.
> > 
> > Unlocking is shared.  goto unlock is fine.  But that doesn't help with
> > the very long lines.  (I am not saying that goto unlock is a worthwhile
> > patch to send but I would definitely have found it tolerable).
> > 
> > regards,
> > dan carpenter
> 
> 
> Khadija,
> While you are reworking this, just wanted to note that, once 'out:'
> becomes the single exit path for the function, it won't be a lie
> any more. So you will have addressed Dan's concern.
> Alison
>

Hey Alison,

Yes, I understand. Thank you. :)

Regards,
Khadija

> 
> > 
> > >  				}
> > >  			}
> > >  		}
> > > @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > >  	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> > >  
> > >  	return IRQ_HANDLED;
> > > +
> > > +out:
> > > +	arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> > > +	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> > > +	return IRQ_WAKE_THREAD;
> > >  }
> > >  
> > >  /*
> > > -- 
> > > 2.34.1
> > > 
> > 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-03-30 15:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 20:18 [PATCH] staging: greybus: refactor arche_platform_wd_irq() function Khadija Kamran
2023-03-27 20:34 ` Alex Elder
2023-03-27 20:49 ` Alison Schofield
2023-03-28  7:33 ` Dan Carpenter
2023-03-28 23:04   ` Alison Schofield
2023-03-30 15:05     ` Khadija Kamran

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.