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; } }
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
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
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
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
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
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-...
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
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-...
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
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-...
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
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-...
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
-
Dan Carpenter -
Deepak R Varma -
Ira Weiny -
Julia Lawall -
Khadija Kamran