[PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Khadija Kamran]

In file drivers/staging/greybus/arche-platform.c,
- Length of line 181 exceeds 100 columns, fix by removing tabs from the
  line.
- If condition and spin_unlock_...() call is split into two lines, join
them to form a single line.

Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
---
Changes in v2:
 - Change the subject and log message
 - Merge if condition and spin_unlock...() from two lines to one 
 drivers/staging/greybus/arche-platform.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
index fcbd5f71eff2..00ed5dfd7915 100644
--- a/drivers/staging/greybus/arche-platform.c
+++ b/drivers/staging/greybus/arche-platform.c
@@ -176,12 +176,10 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 				 * Check we are not in middle of irq thread
 				 * already
 				 */
-				if (arche_pdata->wake_detect_state !=
-						WD_STATE_COLDBOOT_START) {
+				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);
+						WD_STATE_COLDBOOT_TRIG);
+					spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
 					return IRQ_WAKE_THREAD;
 				}
 			}
-- 
2.34.1

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Dan Carpenter]

On Sat, Mar 11, 2023 at 03:18:04AM +0500, Khadija Kamran wrote:
> In file drivers/staging/greybus/arche-platform.c,
> - Length of line 181 exceeds 100 columns, fix by removing tabs from the
>   line.
> - If condition and spin_unlock_...() call is split into two lines, join
> them to form a single line.
> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
> Changes in v2:
>  - Change the subject and log message
>  - Merge if condition and spin_unlock...() from two lines to one 

Apply your patch and then re-run checkpatch.pl -f on the file.  You will
see the problem.

regards,
dan carpenter

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Khadija Kamran]

On Sat, Mar 11, 2023 at 07:16:19AM +0300, Dan Carpenter wrote:
> On Sat, Mar 11, 2023 at 03:18:04AM +0500, Khadija Kamran wrote:
> > In file drivers/staging/greybus/arche-platform.c,
> > - Length of line 181 exceeds 100 columns, fix by removing tabs from the
> >   line.
> > - If condition and spin_unlock_...() call is split into two lines, join
> > them to form a single line.
> > 
> > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > ---
> > Changes in v2:
> >  - Change the subject and log message
> >  - Merge if condition and spin_unlock...() from two lines to one 
> 
> Apply your patch and then re-run checkpatch.pl -f on the file.  You will
> see the problem.

Hey Dan!
When I run checkpatch.pl on my file, I can see that my old CHECK no
longer exists instead a new CHECK is mentioned saying 'Alignment should
match open parenthesis'. I understand this from your previous email.
Should I stop working on this file and leave it as is?
Thank you!
>
> regards,
> dan carpenter

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Deepak R Varma]

On Sat, Mar 11, 2023 at 11:24:20AM +0500, Khadija Kamran wrote:
> On Sat, Mar 11, 2023 at 07:16:19AM +0300, Dan Carpenter wrote:
> > On Sat, Mar 11, 2023 at 03:18:04AM +0500, Khadija Kamran wrote:
> > > In file drivers/staging/greybus/arche-platform.c,
> > > - Length of line 181 exceeds 100 columns, fix by removing tabs from the
> > >   line.
> > > - If condition and spin_unlock_...() call is split into two lines, join
> > > them to form a single line.
> > > 
> > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > > ---
> > > Changes in v2:
> > >  - Change the subject and log message
> > >  - Merge if condition and spin_unlock...() from two lines to one 
> > 
> > Apply your patch and then re-run checkpatch.pl -f on the file.  You will
> > see the problem.
> 
> Hey Dan!
> When I run checkpatch.pl on my file, I can see that my old CHECK no
> longer exists instead a new CHECK is mentioned saying 'Alignment should
> match open parenthesis'. I understand this from your previous email.
> Should I stop working on this file and leave it as is?

Hi Dan,
Not trying to speak for you, so please override my message if this is
inaccurate.

Hi Khadija,
Yes. It is not useful to resolve one warning and introduce another. Tomorrow
someone else is going to try and revert it. So do not make the "remove tab"
change. I still like the merging of the split lines. It appears to improve code
readability. You can send in a v3 with just that merge change and wait for
feedback.

Also, remember to check your change with checkpatch. There is a section about
post-commit hooks on the tutorials page. This will allow you to integrate
checkpatch as part of your git commit step and do the job for you.

And also, always build your change locally on your machine. No new warnings or
errors should arise.

Hope that helps.
Deepak.


> Thank you!
> >
> > regards,
> > dan carpenter
> 

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Khadija Kamran]

On Sat, Mar 11, 2023 at 12:35:49PM +0530, Deepak R Varma wrote:
> On Sat, Mar 11, 2023 at 11:24:20AM +0500, Khadija Kamran wrote:
> > On Sat, Mar 11, 2023 at 07:16:19AM +0300, Dan Carpenter wrote:
> > > On Sat, Mar 11, 2023 at 03:18:04AM +0500, Khadija Kamran wrote:
> > > > In file drivers/staging/greybus/arche-platform.c,
> > > > - Length of line 181 exceeds 100 columns, fix by removing tabs from the
> > > >   line.
> > > > - If condition and spin_unlock_...() call is split into two lines, join
> > > > them to form a single line.
> > > > 
> > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > >  - Change the subject and log message
> > > >  - Merge if condition and spin_unlock...() from two lines to one 
> > > 
> > > Apply your patch and then re-run checkpatch.pl -f on the file.  You will
> > > see the problem.
> > 
> > Hey Dan!
> > When I run checkpatch.pl on my file, I can see that my old CHECK no
> > longer exists instead a new CHECK is mentioned saying 'Alignment should
> > match open parenthesis'. I understand this from your previous email.
> > Should I stop working on this file and leave it as is?
> 
> Hi Dan,
> Not trying to speak for you, so please override my message if this is
> inaccurate.
> 
> Hi Khadija,
> Yes. It is not useful to resolve one warning and introduce another. Tomorrow
> someone else is going to try and revert it. So do not make the "remove tab"
> change. I still like the merging of the split lines. It appears to improve code
> readability. You can send in a v3 with just that merge change and wait for
> feedback.
>
Hey Deepak, 
Thank you for the feedback. Before sending a patch v3, I think I should
wait for more feedback.
> Also, remember to check your change with checkpatch. There is a section about
> post-commit hooks on the tutorials page. This will allow you to integrate
> checkpatch as part of your git commit step and do the job for you.
> 
> And also, always build your change locally on your machine. No new warnings or
> errors should arise.
>
Yes I will keep that in mind for next patches. Thank you!
> Hope that helps.
> Deepak.
> 
> 
> > Thank you!
> > >
> > > regards,
> > > dan carpenter
> > 
> 
> 

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Julia Lawall]

On Sat, 11 Mar 2023, Khadija Kamran wrote:

> On Sat, Mar 11, 2023 at 12:35:49PM +0530, Deepak R Varma wrote:
> > On Sat, Mar 11, 2023 at 11:24:20AM +0500, Khadija Kamran wrote:
> > > On Sat, Mar 11, 2023 at 07:16:19AM +0300, Dan Carpenter wrote:
> > > > On Sat, Mar 11, 2023 at 03:18:04AM +0500, Khadija Kamran wrote:
> > > > > In file drivers/staging/greybus/arche-platform.c,
> > > > > - Length of line 181 exceeds 100 columns, fix by removing tabs from the
> > > > >   line.
> > > > > - If condition and spin_unlock_...() call is split into two lines, join
> > > > > them to form a single line.
> > > > >
> > > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > > > > ---
> > > > > Changes in v2:
> > > > >  - Change the subject and log message
> > > > >  - Merge if condition and spin_unlock...() from two lines to one
> > > >
> > > > Apply your patch and then re-run checkpatch.pl -f on the file.  You will
> > > > see the problem.
> > >
> > > Hey Dan!
> > > When I run checkpatch.pl on my file, I can see that my old CHECK no
> > > longer exists instead a new CHECK is mentioned saying 'Alignment should
> > > match open parenthesis'. I understand this from your previous email.
> > > Should I stop working on this file and leave it as is?
> >
> > Hi Dan,
> > Not trying to speak for you, so please override my message if this is
> > inaccurate.
> >
> > Hi Khadija,
> > Yes. It is not useful to resolve one warning and introduce another. Tomorrow
> > someone else is going to try and revert it. So do not make the "remove tab"
> > change. I still like the merging of the split lines. It appears to improve code
> > readability. You can send in a v3 with just that merge change and wait for
> > feedback.
> >
> Hey Deepak,
> Thank you for the feedback. Before sending a patch v3, I think I should
> wait for more feedback.

Khadija,

Please put some blank lines around your responses so they are easier to
find.

thanks,
julia

> > Also, remember to check your change with checkpatch. There is a section about
> > post-commit hooks on the tutorials page. This will allow you to integrate
> > checkpatch as part of your git commit step and do the job for you.
> >
> > And also, always build your change locally on your machine. No new warnings or
> > errors should arise.
> >
> Yes I will keep that in mind for next patches. Thank you!
> > Hope that helps.
> > Deepak.
> >
> >
> > > Thank you!
> > > >
> > > > regards,
> > > > dan carpenter
> > >
> >
> >
>
>

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Ira Weiny]

Khadija Kamran wrote:
> In file drivers/staging/greybus/arche-platform.c,
> - Length of line 181 exceeds 100 columns, fix by removing tabs from the
>   line.
> - If condition and spin_unlock_...() call is split into two lines, join
> them to form a single line.
> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>

Fundamentally the problem with arche_platform_wd_irq() is that the
indentation is too great.

"... if you need more than 3 levels of indentation, you’re screwed anyway,
and should fix your program."

	-- https://www.kernel.org/doc/html/v4.10/process/coding-style.html#indentation

I think a better solution would be to refactor the entire function.  This
would make the logic of the function more clear as well IMHO.

Here is another tip to help:

https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions

Do you think you could try that?
Ira

> ---
> Changes in v2:
>  - Change the subject and log message
>  - Merge if condition and spin_unlock...() from two lines to one 
>  drivers/staging/greybus/arche-platform.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..00ed5dfd7915 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -176,12 +176,10 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  				 * Check we are not in middle of irq thread
>  				 * already
>  				 */
> -				if (arche_pdata->wake_detect_state !=
> -						WD_STATE_COLDBOOT_START) {
> +				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);
> +						WD_STATE_COLDBOOT_TRIG);
> +					spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>  					return IRQ_WAKE_THREAD;
>  				}
>  			}
> -- 
> 2.34.1
> 
> 

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Khadija Kamran]

On Sun, Mar 12, 2023 at 06:01:03PM -0700, Ira Weiny wrote:
> Khadija Kamran wrote:
> > In file drivers/staging/greybus/arche-platform.c,
> > - Length of line 181 exceeds 100 columns, fix by removing tabs from the
> >   line.
> > - If condition and spin_unlock_...() call is split into two lines, join
> > them to form a single line.
> > 
> > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> 
> Fundamentally the problem with arche_platform_wd_irq() is that the
> indentation is too great.
> 
> "... if you need more than 3 levels of indentation, you’re screwed anyway,
> and should fix your program."
> 
> 	-- https://www.kernel.org/doc/html/v4.10/process/coding-style.html#indentation
> 
> I think a better solution would be to refactor the entire function.  This
> would make the logic of the function more clear as well IMHO.
> 
> Here is another tip to help:
> 
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions
> 
> Do you think you could try that?

Hey Ira!

Sorry about the late reply. Thank you for your feedback. I have looked
into the above link. Are you referring to the use of goto statements in
arche_platform_wd_irq() call?

Thank you!

Regards,
Khadija


> Ira
> 
> > ---
> > Changes in v2:
> >  - Change the subject and log message
> >  - Merge if condition and spin_unlock...() from two lines to one 
> >  drivers/staging/greybus/arche-platform.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > index fcbd5f71eff2..00ed5dfd7915 100644
> > --- a/drivers/staging/greybus/arche-platform.c
> > +++ b/drivers/staging/greybus/arche-platform.c
> > @@ -176,12 +176,10 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >  				 * Check we are not in middle of irq thread
> >  				 * already
> >  				 */
> > -				if (arche_pdata->wake_detect_state !=
> > -						WD_STATE_COLDBOOT_START) {
> > +				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);
> > +						WD_STATE_COLDBOOT_TRIG);
> > +					spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> >  					return IRQ_WAKE_THREAD;
> >  				}
> >  			}
> > -- 
> > 2.34.1
> > 
> > 
> 
> 

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Ira Weiny]

Khadija Kamran wrote:
> On Sun, Mar 12, 2023 at 06:01:03PM -0700, Ira Weiny wrote:
> > Khadija Kamran wrote:
> > > In file drivers/staging/greybus/arche-platform.c,
> > > - Length of line 181 exceeds 100 columns, fix by removing tabs from the
> > >   line.
> > > - If condition and spin_unlock_...() call is split into two lines, join
> > > them to form a single line.
> > > 
> > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > 
> > Fundamentally the problem with arche_platform_wd_irq() is that the
> > indentation is too great.
> > 
> > "... if you need more than 3 levels of indentation, you’re screwed anyway,
> > and should fix your program."
> > 
> > 	-- https://www.kernel.org/doc/html/v4.10/process/coding-style.html#indentation
> > 
> > I think a better solution would be to refactor the entire function.  This
> > would make the logic of the function more clear as well IMHO.
> > 
> > Here is another tip to help:
> > 
> > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions
> > 
> > Do you think you could try that?
> 
> Hey Ira!
> 
> Sorry about the late reply. Thank you for your feedback. I have looked
> into the above link. Are you referring to the use of goto statements in
> arche_platform_wd_irq() call?

I'm not quite sure I understand what you mean because currently
arche_platform_wd_irq() does not use gotos.

But I think you are asking if I think it should.  If that is what you mean
then 'yes' I think arche_platform_wd_irq() could use goto's to clean up
the flow.

Ira

Re: [PATCH v2] staging: greybus: remove tabs to fix line length and merge lines

[Khadija Kamran]

On Mon, Mar 20, 2023 at 05:33:44PM -0700, Ira Weiny wrote:
> Khadija Kamran wrote:
> > On Sun, Mar 12, 2023 at 06:01:03PM -0700, Ira Weiny wrote:
> > > Khadija Kamran wrote:
> > > > In file drivers/staging/greybus/arche-platform.c,
> > > > - Length of line 181 exceeds 100 columns, fix by removing tabs from the
> > > >   line.
> > > > - If condition and spin_unlock_...() call is split into two lines, join
> > > > them to form a single line.
> > > > 
> > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > > 
> > > Fundamentally the problem with arche_platform_wd_irq() is that the
> > > indentation is too great.
> > > 
> > > "... if you need more than 3 levels of indentation, you’re screwed anyway,
> > > and should fix your program."
> > > 
> > > 	-- https://www.kernel.org/doc/html/v4.10/process/coding-style.html#indentation
> > > 
> > > I think a better solution would be to refactor the entire function.  This
> > > would make the logic of the function more clear as well IMHO.
> > > 
> > > Here is another tip to help:
> > > 
> > > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions
> > > 
> > > Do you think you could try that?
> > 
> > Hey Ira!
> > 
> > Sorry about the late reply. Thank you for your feedback. I have looked
> > into the above link. Are you referring to the use of goto statements in
> > arche_platform_wd_irq() call?
> 
> I'm not quite sure I understand what you mean because currently
> arche_platform_wd_irq() does not use gotos.
> 
> But I think you are asking if I think it should.  If that is what you mean
> then 'yes' I think arche_platform_wd_irq() could use goto's to clean up
> the flow.
>

Hey Ira,

Yes, I am asking if I should use goto's. Thank you for the reply. Let me
get back to you after making the changes.

Regards,
Khadija

> Ira