[PATCH] staging: greybus: fix exceeds line length

[PATCH] staging: greybus: fix exceeds line length

[Khadija Kamran]

Length of line 182 exceeds 100 columns in file
drivers/staging/grebus/arche-platform.c, fix by removing tabs from the
line.

Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
---
 drivers/staging/greybus/arche-platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
index fcbd5f71eff2..0f0fbc263f8a 100644
--- a/drivers/staging/greybus/arche-platform.c
+++ b/drivers/staging/greybus/arche-platform.c
@@ -179,7 +179,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);
+						WD_STATE_COLDBOOT_TRIG);
 					spin_unlock_irqrestore(&arche_pdata->wake_lock,
 							       flags);
 					return IRQ_WAKE_THREAD;
-- 
2.34.1

Re: [PATCH] staging: greybus: fix exceeds line length

[Deepak R Varma]

On Fri, Mar 10, 2023 at 10:09:47PM +0500, Khadija Kamran wrote:
> Length of line 182 exceeds 100 columns in file
> drivers/staging/grebus/arche-platform.c, fix by removing tabs from the
> line.

Hi Khadija,
I think if you also include merging the if condition and the call to
spin_unlock...() on single lines, it should make the code more human. This would
result in updating the patch subject and log message since you are doing more
than "remove tabs". Can you try that and include in the next revision?

Deepak.

> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/greybus/arche-platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..0f0fbc263f8a 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -179,7 +179,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);
> +						WD_STATE_COLDBOOT_TRIG);
>  					spin_unlock_irqrestore(&arche_pdata->wake_lock,
>  							       flags);
>  					return IRQ_WAKE_THREAD;
> -- 
> 2.34.1
> 
> 

Re: [PATCH] staging: greybus: fix exceeds line length

[Khadija Kamran]

On Sat, Mar 11, 2023 at 12:05:54AM +0530, Deepak R Varma wrote:
> On Fri, Mar 10, 2023 at 10:09:47PM +0500, Khadija Kamran wrote:
> > Length of line 182 exceeds 100 columns in file
> > drivers/staging/grebus/arche-platform.c, fix by removing tabs from the
> > line.
> 
> Hi Khadija,
> I think if you also include merging the if condition and the call to
> spin_unlock...() on single lines, it should make the code more human. 
Hi Deepak! Sorry I am unable to understand how to merge the if condition
and spin_unlock...() together. Can you please elaborate. 
Thank you

Re: [PATCH] staging: greybus: fix exceeds line length

[Deepak R Varma]

On Fri, Mar 10, 2023 at 11:56:59PM +0500, Khadija Kamran wrote:
> On Sat, Mar 11, 2023 at 12:05:54AM +0530, Deepak R Varma wrote:
> > On Fri, Mar 10, 2023 at 10:09:47PM +0500, Khadija Kamran wrote:
> > > Length of line 182 exceeds 100 columns in file
> > > drivers/staging/grebus/arche-platform.c, fix by removing tabs from the
> > > line.
> > 
> > Hi Khadija,
> > I think if you also include merging the if condition and the call to
> > spin_unlock...() on single lines, it should make the code more human. 
> Hi Deepak! Sorry I am unable to understand how to merge the if condition
> and spin_unlock...() together. Can you please elaborate. 
> Thank you

Hah.. my bad. The if condition is split on two lines. Join them to form a single
line if evaluation.
Similarly, join the spin_un..() call that is split on two lines into a single
line.

Hope that clarifies it.

Deepak.

> 

Re: [PATCH] staging: greybus: fix exceeds line length

[Khadija Kamran]

Hi Deepak!
If I join the if condition to one line, the line length becomes 97
columns. Would that be okay?

Re: [PATCH] staging: greybus: fix exceeds line length

[Deepak R Varma]

On Sat, Mar 11, 2023 at 12:15:54AM +0500, Khadija Kamran wrote:
> Hi Deepak!
> If I join the if condition to one line, the line length becomes 97
> columns. Would that be okay?

Yes, the line length limit is recently revised to 100 characters.

Re: [PATCH] staging: greybus: fix exceeds line length

[Dan Carpenter]

On Fri, Mar 10, 2023 at 10:09:47PM +0500, Khadija Kamran wrote:
> Length of line 182 exceeds 100 columns in file
> drivers/staging/grebus/arche-platform.c, fix by removing tabs from the
> line.
> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/greybus/arche-platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..0f0fbc263f8a 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -179,7 +179,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);
> +						WD_STATE_COLDBOOT_TRIG);

The original line was done deliberately so that it lines up.  If we
apply your patch and re-run checkpatch -f on the file then it has a new
warning:

CHECK: Alignment should match open parenthesis
#182: FILE: drivers/staging/greybus/arche-platform.c:182:
+                                       arche_platform_set_wake_detect_state(arche_pdata,
+                                               WD_STATE_COLDBOOT_TRIG);

Always try to think about the bigger picture.  Why did the original
author do it that way?  The change makes checkpatch happy, but does it
make the code more readable?  Is there a more important readability
improvement to be done here?

For example, you could re-arrange the if statements like this and pull
everything in a few tabs.  Don't necessarily do that.  Just think about
doing it.  I write quite a few cleanup patches that I don't send because
the next day I just decide it's not worth it.

When I look at this file, the style is not bad at all.  But at the start
of the file there is #if IS_ENABLED(CONFIG_USB_HSIC_USB3613).  What is
that?  The CONFIG doesn't exist and the header doesn't exits.  Probably
it can be deleted.

But that raises a new question.  Lukas Bulwahn is always looking for
CONFIG_ entries which don't exist.  I would have expected him to find
this already.

Anyway, we can write our own scripts to make a list of stuff inside
IS_ENABLED():

git grep IS_ENABLED | \
	perl -ne 'if (/IS_ENABLED\((.+?)\)/){ print "$1\n"}' | \
	sort -u | tee CONFIG_list

Then we can go through the CONFIG_list file and see which other stuff
doesn't exist.

for i in $(grep ^CONFIG CONFIG_list  | cut -d '_' -f 2-) ; do \
	grep -q -w "config $i$" $(find -name Kconfig) || echo $i ; \
done | tee CONFIG_not_found

I have never done this before so I don't know what you'll find.  But
everywhere you look if you just look closer then it raises questions
which raise more questions.  So it's interesting to explore.  Anyway,
look closely at each line in the file and follow the rabbit holes until
you find something interesting to work on.

regards,
dan carpenter

diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
index fcbd5f71eff2..2d9e0c41b5e3 100644
--- a/drivers/staging/greybus/arche-platform.c
+++ b/drivers/staging/greybus/arche-platform.c
@@ -165,43 +165,39 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 		 * 30msec, then standby boot sequence is initiated, which is not
 		 * supported/implemented as of now. So ignore it.
 		 */
-		if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
-			if (time_before(jiffies,
-					arche_pdata->wake_detect_start +
-					msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
-				arche_platform_set_wake_detect_state(arche_pdata,
-								     WD_STATE_IDLE);
-			} else {
-				/*
-				 * Check we are not in middle of irq thread
-				 * already
-				 */
-				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;
-				}
-			}
-		}
-	} else {
-		/* wake/detect falling */
-		if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
-			arche_pdata->wake_detect_start = jiffies;
+		if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT)
+			goto out_unlock;
+
+		if (time_before(jiffies,
+				arche_pdata->wake_detect_start +
+				msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
+			arche_platform_set_wake_detect_state(arche_pdata,
+							     WD_STATE_IDLE);
+		} else if (arche_pdata->wake_detect_state != WD_STATE_COLDBOOT_START) {
 			/*
-			 * In the beginning, when wake/detect goes low
-			 * (first time), we assume it is meant for coldboot
-			 * and set the flag. If wake/detect line stays low
-			 * beyond 30msec, then it is coldboot else fallback
-			 * to standby boot.
+			 * Check we are not in middle of irq thread
+			 * already
 			 */
 			arche_platform_set_wake_detect_state(arche_pdata,
-							     WD_STATE_BOOT_INIT);
+							     WD_STATE_COLDBOOT_TRIG);
+			spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
+			return IRQ_WAKE_THREAD;
 		}
+	} else if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
+		/* wake/detect falling */
+		arche_pdata->wake_detect_start = jiffies;
+		/*
+		 * In the beginning, when wake/detect goes low
+		 * (first time), we assume it is meant for coldboot
+		 * and set the flag. If wake/detect line stays low
+		 * beyond 30msec, then it is coldboot else fallback
+		 * to standby boot.
+		 */
+		arche_platform_set_wake_detect_state(arche_pdata,
+						     WD_STATE_BOOT_INIT);
 	}
 
+out_unlock:
 	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
 
 	return IRQ_HANDLED;

Re: [PATCH] staging: greybus: fix exceeds line length

[Julia Lawall]

On Fri, 10 Mar 2023, Khadija Kamran wrote:

> Length of line 182 exceeds 100 columns in file
> drivers/staging/grebus/arche-platform.c, fix by removing tabs from the
> line.

Try to rephrase without using "fix".  Fix doens't give any information
about what is actually done.

julia

>
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/greybus/arche-platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..0f0fbc263f8a 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -179,7 +179,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);
> +						WD_STATE_COLDBOOT_TRIG);
>  					spin_unlock_irqrestore(&arche_pdata->wake_lock,
>  							       flags);
>  					return IRQ_WAKE_THREAD;
> --
> 2.34.1
>
>
>

Re: [PATCH] staging: greybus: fix exceeds line length

[Khadija Kamran]

Hey! 
Sorry, I am unable to understand this enough. Since it is mentioned that
the patch raises another checkpatch warning, should I stop working on
this patch and look for some other files? Kindly let me know.

As for now, I have submitted another revision of this patch.
Thank you.

Re: [PATCH] staging: greybus: fix exceeds line length

[Lukas Bulwahn]

On Fri, Mar 10, 2023 at 8:40 PM Dan Carpenter <error27@gmail.com> wrote:
>
> On Fri, Mar 10, 2023 at 10:09:47PM +0500, Khadija Kamran wrote:
> > Length of line 182 exceeds 100 columns in file
> > drivers/staging/grebus/arche-platform.c, fix by removing tabs from the
> > line.
> >
> > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > ---
> >  drivers/staging/greybus/arche-platform.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > index fcbd5f71eff2..0f0fbc263f8a 100644
> > --- a/drivers/staging/greybus/arche-platform.c
> > +++ b/drivers/staging/greybus/arche-platform.c
> > @@ -179,7 +179,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);
> > +                                             WD_STATE_COLDBOOT_TRIG);
>
> The original line was done deliberately so that it lines up.  If we
> apply your patch and re-run checkpatch -f on the file then it has a new
> warning:
>
> CHECK: Alignment should match open parenthesis
> #182: FILE: drivers/staging/greybus/arche-platform.c:182:
> +                                       arche_platform_set_wake_detect_state(arche_pdata,
> +                                               WD_STATE_COLDBOOT_TRIG);
>
> Always try to think about the bigger picture.  Why did the original
> author do it that way?  The change makes checkpatch happy, but does it
> make the code more readable?  Is there a more important readability
> improvement to be done here?
>
> For example, you could re-arrange the if statements like this and pull
> everything in a few tabs.  Don't necessarily do that.  Just think about
> doing it.  I write quite a few cleanup patches that I don't send because
> the next day I just decide it's not worth it.
>
> When I look at this file, the style is not bad at all.  But at the start
> of the file there is #if IS_ENABLED(CONFIG_USB_HSIC_USB3613).  What is
> that?  The CONFIG doesn't exist and the header doesn't exits.  Probably
> it can be deleted.
>
> But that raises a new question.  Lukas Bulwahn is always looking for
> CONFIG_ entries which don't exist.  I would have expected him to find
> this already.
>

True, and my (private) random linux notes of the checkkconfigsymbols
effort on this config states:

USB_HSIC_USB3613: Reported and maintenance of dead code is fine for
staging maintainer

So, I did report it, and a quick search on lore.kernel.org
(https://lore.kernel.org/all/?q=USB_HSIC_USB3613) will give us some
more insight and refresh Greg's and my memory:

I reported the issue here:
https://lore.kernel.org/all/CAKXUXMym0M1UNuNGUVpFr2yUwOwjkZ_sQpCD0jC8YB+hs=j-bA@mail.gmail.com/

And Greg responded:

"... It's a good example driver for those wanting to create a greybus
host controller driver so it's nice to have in the tree..."

So, even though the code is dead, it is a nice example of some driver
code. So, we keep it.


> Anyway, we can write our own scripts to make a list of stuff inside
> IS_ENABLED():
>
> git grep IS_ENABLED | \
>         perl -ne 'if (/IS_ENABLED\((.+?)\)/){ print "$1\n"}' | \
>         sort -u | tee CONFIG_list
>
> Then we can go through the CONFIG_list file and see which other stuff
> doesn't exist.
>
> for i in $(grep ^CONFIG CONFIG_list  | cut -d '_' -f 2-) ; do \
>         grep -q -w "config $i$" $(find -name Kconfig) || echo $i ; \
> done | tee CONFIG_not_found
>
> I have never done this before so I don't know what you'll find.  But
> everywhere you look if you just look closer then it raises questions
> which raise more questions.  So it's interesting to explore.  Anyway,
> look closely at each line in the file and follow the rabbit holes until
> you find something interesting to work on.
>

@Dan, Thanks for pointing out my clean-up activity here!

Yes, I agree with Dan. That is certainly an interesting task to
explore. If you search the mailing list, you will find another bash
script of similar length that Joe Perches used a few years back. I
personally use the script ./scripts/checkkconfigsymbols.py. They may
differ a bit on what they report, but I fear at this point, most of
its reports are false positives. I have looked at all of them, I am
checking the new ones introduced, and sending out patches to clean up
stuff. There are a few on my todo list, like cleaning up the
definition of CONFIG_CAAM_QI. If you want to assist, please let me
know. I can certainly give you a few things that are at least worth a
deeper investigation and maybe also some clean up.

Lukas

> regards,
> dan carpenter
>
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..2d9e0c41b5e3 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -165,43 +165,39 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>                  * 30msec, then standby boot sequence is initiated, which is not
>                  * supported/implemented as of now. So ignore it.
>                  */
> -               if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> -                       if (time_before(jiffies,
> -                                       arche_pdata->wake_detect_start +
> -                                       msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> -                               arche_platform_set_wake_detect_state(arche_pdata,
> -                                                                    WD_STATE_IDLE);
> -                       } else {
> -                               /*
> -                                * Check we are not in middle of irq thread
> -                                * already
> -                                */
> -                               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;
> -                               }
> -                       }
> -               }
> -       } else {
> -               /* wake/detect falling */
> -               if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> -                       arche_pdata->wake_detect_start = jiffies;
> +               if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT)
> +                       goto out_unlock;
> +
> +               if (time_before(jiffies,
> +                               arche_pdata->wake_detect_start +
> +                               msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> +                       arche_platform_set_wake_detect_state(arche_pdata,
> +                                                            WD_STATE_IDLE);
> +               } else if (arche_pdata->wake_detect_state != WD_STATE_COLDBOOT_START) {
>                         /*
> -                        * In the beginning, when wake/detect goes low
> -                        * (first time), we assume it is meant for coldboot
> -                        * and set the flag. If wake/detect line stays low
> -                        * beyond 30msec, then it is coldboot else fallback
> -                        * to standby boot.
> +                        * Check we are not in middle of irq thread
> +                        * already
>                          */
>                         arche_platform_set_wake_detect_state(arche_pdata,
> -                                                            WD_STATE_BOOT_INIT);
> +                                                            WD_STATE_COLDBOOT_TRIG);
> +                       spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> +                       return IRQ_WAKE_THREAD;
>                 }
> +       } else if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> +               /* wake/detect falling */
> +               arche_pdata->wake_detect_start = jiffies;
> +               /*
> +                * In the beginning, when wake/detect goes low
> +                * (first time), we assume it is meant for coldboot
> +                * and set the flag. If wake/detect line stays low
> +                * beyond 30msec, then it is coldboot else fallback
> +                * to standby boot.
> +                */
> +               arche_platform_set_wake_detect_state(arche_pdata,
> +                                                    WD_STATE_BOOT_INIT);
>         }
>
> +out_unlock:
>         spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>
>         return IRQ_HANDLED;