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 <[email protected]>
---
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 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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
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
>
>
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 <[email protected]>
>
> 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
> >
> >
>
>
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 <[email protected]>
> >
> > 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
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 <[email protected]>
> > >
> > > 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