[OPW,kernel] staging: silicom: fix space prohibited before semicolon
diff mbox

Message ID 20131002211919.GA21251@gmail.com
State Changes Requested
Headers show

Commit Message

Tugce Sirin Oct. 2, 2013, 9:19 p.m. UTC
---
 drivers/staging/silicom/bp_mod.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Josh Triplett Oct. 2, 2013, 9:24 p.m. UTC | #1
On Thu, Oct 03, 2013 at 12:19:22AM +0300, Tugce Sirin wrote:
> ---

You need a Signed-off-by for your patch; see the "Sign your work"
section of Documentation/SubmittingPatches for what that implies, and
then add it to the end of your commit message by always passing -s to
"git commit".

Other than that, this looks good; once you've made that change:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  drivers/staging/silicom/bp_mod.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/silicom/bp_mod.h b/drivers/staging/silicom/bp_mod.h
> index cfa1f43..8154a7b 100644
> --- a/drivers/staging/silicom/bp_mod.h
> +++ b/drivers/staging/silicom/bp_mod.h
> @@ -22,7 +22,7 @@ do {						\
>  	int  i;					\
>  	if (1) {				\
>  		for (i = 0; i < 1000; i++) {	\
> -			udelay(x) ;		\
> +			udelay(x);		\
>  		}				\
>  	} else {				\
>  		msleep(x);			\
> -- 
> 1.7.9.5
> 
> I hope this is the right way to send a patch.

You should actually put this kind of comment (that shouldn't be in the
commit message) below the "---" line and above the diffstat, as I did
with my reply above.

- Josh Triplett
Tugce Sirin Oct. 2, 2013, 9:28 p.m. UTC | #2
Yes, I needed to fix my commit message and using --amend wipe off my
signed-off line obviously. I'm correcting it. Thanks!


2013/10/3 Josh Triplett <josh@joshtriplett.org>

> On Thu, Oct 03, 2013 at 12:19:22AM +0300, Tugce Sirin wrote:
> > ---
>
> You need a Signed-off-by for your patch; see the "Sign your work"
> section of Documentation/SubmittingPatches for what that implies, and
> then add it to the end of your commit message by always passing -s to
> "git commit".
>
> Other than that, this looks good; once you've made that change:
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
>
> >  drivers/staging/silicom/bp_mod.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/silicom/bp_mod.h
> b/drivers/staging/silicom/bp_mod.h
> > index cfa1f43..8154a7b 100644
> > --- a/drivers/staging/silicom/bp_mod.h
> > +++ b/drivers/staging/silicom/bp_mod.h
> > @@ -22,7 +22,7 @@ do {                                                \
> >       int  i;                                 \
> >       if (1) {                                \
> >               for (i = 0; i < 1000; i++) {    \
> > -                     udelay(x) ;             \
> > +                     udelay(x);              \
> >               }                               \
> >       } else {                                \
> >               msleep(x);                      \
> > --
> > 1.7.9.5
> >
> > I hope this is the right way to send a patch.
>
> You should actually put this kind of comment (that shouldn't be in the
> commit message) below the "---" line and above the diffstat, as I did
> with my reply above.
>
> - Josh Triplett
>
Waskiewicz Jr, Peter P Oct. 2, 2013, 9:46 p.m. UTC | #3
On Thu, 2013-10-03 at 00:19 +0300, Tugce Sirin wrote:

This is Greg's call, but I personally usually like to see even a tiny
commit message.  It's obvious to me why the change is being made, but it
might be good to determine where the need arose.  I assume checkpatch.pl
caught this?  If so, I'd suggest a small commit message like this:

"Fix space before semicolon based on checkpatch.pl scan."

Also, one other suggestion when sending an updated patch, it's good to
include a version in the subject line.  That way if you end up sending
two or three (or more) revisions of a patch, it's easier for reviewers
to know which is the latest one to review.  Please look at the
Submitting a Patch section here:
http://kernelnewbies.org/OPWfirstpatch#head-2043a643d048c341b2a52044fd72d852aac87fef Specifically, look at the subsection titled "Versioning an updated patch based on feedback."

And please don't interpret all the feedback here that you didn't do this
mostly right, these are just suggestions so future patches flow even
smoother.  :-)

Cheers,
-PJ

> ---
>  drivers/staging/silicom/bp_mod.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/silicom/bp_mod.h b/drivers/staging/silicom/bp_mod.h
> index cfa1f43..8154a7b 100644
> --- a/drivers/staging/silicom/bp_mod.h
> +++ b/drivers/staging/silicom/bp_mod.h
> @@ -22,7 +22,7 @@ do {						\
>  	int  i;					\
>  	if (1) {				\
>  		for (i = 0; i < 1000; i++) {	\
> -			udelay(x) ;		\
> +			udelay(x);		\
>  		}				\
>  	} else {				\
>  		msleep(x);			\
> -- 
> 1.7.9.5
> 
> I hope this is the right way to send a patch.
>
Josh Triplett Oct. 2, 2013, 10:05 p.m. UTC | #4
On Wed, Oct 02, 2013 at 09:46:52PM +0000, Waskiewicz Jr, Peter P wrote:
> This is Greg's call, but I personally usually like to see even a tiny
> commit message.  It's obvious to me why the change is being made, but it
> might be good to determine where the need arose.  I assume checkpatch.pl
> caught this?  If so, I'd suggest a small commit message like this:
> 
> "Fix space before semicolon based on checkpatch.pl scan."

Depends on what you're fixing.  When the fix is extremely obvious, you
can sometimes get away with just a subject line and a signoff, but
erring on the side of verbosity is preferable. :)

Rather than duplicating most of the subject, you might consider quoting
the exact output of checkpatch.pl or whatever tool you used, for
reference.

- Josh Triplett
Waskiewicz Jr, Peter P Oct. 2, 2013, 10:12 p.m. UTC | #5
On Wed, 2013-10-02 at 15:05 -0700, Josh Triplett wrote:
> On Wed, Oct 02, 2013 at 09:46:52PM +0000, Waskiewicz Jr, Peter P wrote:
> > This is Greg's call, but I personally usually like to see even a tiny
> > commit message.  It's obvious to me why the change is being made, but it
> > might be good to determine where the need arose.  I assume checkpatch.pl
> > caught this?  If so, I'd suggest a small commit message like this:
> > 
> > "Fix space before semicolon based on checkpatch.pl scan."
> 
> Depends on what you're fixing.  When the fix is extremely obvious, you
> can sometimes get away with just a subject line and a signoff, but
> erring on the side of verbosity is preferable. :)

A verbose changelog helps those of us (aka me) who seem to leak more
information out of our heads than retain anymore.  Getting older
sucks.  :-)

> Rather than duplicating most of the subject, you might consider quoting
> the exact output of checkpatch.pl or whatever tool you used, for
> reference.

That's even better.  This way if/when updates are made to the tools, as
checkpatch.pl does get updated, the origin of a particular change can be
traced to pre-tool update if needed.

-PJ
Tugce Sirin Oct. 2, 2013, 10:47 p.m. UTC | #6
"Fix space before semicolon based on checkpatch.pl scan."

I did not add this only for this patch but I'll add it for next times.


    And please don't interpret all the feedback here that you didn't do this
    mostly right, these are just suggestions so future patches flow even
    smoother.  :-)

Feedback is awesome, I'm not easily offended by 'correcting' because if I'm
doing something it should be correct. :)


    So, can you send a v3 with Josh's Reviewed-by line below your
    Signed-off-by line?

I just did. I don't know why but it seems I just did not understand that
part.

Best regards,
Tugce Sirin



2013/10/3 Waskiewicz Jr, Peter P <peter.p.waskiewicz.jr@intel.com>

> On Wed, 2013-10-02 at 15:05 -0700, Josh Triplett wrote:
> > On Wed, Oct 02, 2013 at 09:46:52PM +0000, Waskiewicz Jr, Peter P wrote:
> > > This is Greg's call, but I personally usually like to see even a tiny
> > > commit message.  It's obvious to me why the change is being made, but
> it
> > > might be good to determine where the need arose.  I assume
> checkpatch.pl
> > > caught this?  If so, I'd suggest a small commit message like this:
> > >
> > > "Fix space before semicolon based on checkpatch.pl scan."
> >
> > Depends on what you're fixing.  When the fix is extremely obvious, you
> > can sometimes get away with just a subject line and a signoff, but
> > erring on the side of verbosity is preferable. :)
>
> A verbose changelog helps those of us (aka me) who seem to leak more
> information out of our heads than retain anymore.  Getting older
> sucks.  :-)
>
> > Rather than duplicating most of the subject, you might consider quoting
> > the exact output of checkpatch.pl or whatever tool you used, for
> > reference.
>
> That's even better.  This way if/when updates are made to the tools, as
> checkpatch.pl does get updated, the origin of a particular change can be
> traced to pre-tool update if needed.
>
> -PJ
>
> --
> You received this message because you are subscribed to the Google Groups
> "opw-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to opw-kernel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
Waskiewicz Jr, Peter P Oct. 2, 2013, 10:58 p.m. UTC | #7
On Thu, 2013-10-03 at 01:47 +0300, Tu?çe ?irin wrote:
>     "Fix space before semicolon based on checkpatch.pl scan."
> 
> I did not add this only for this patch but I'll add it for next times.

Not a problem.  Again, it's more preference for the maintainer taking
the patch, so if Greg doesn't care, then I don't care.  :-)

> 
>     And please don't interpret all the feedback here that you didn't
> do this
>     mostly right, these are just suggestions so future patches flow
> even
>     smoother.  :-)
> 
> 
> Feedback is awesome, I'm not easily offended by 'correcting' because
> if I'm doing something it should be correct. :)

Glad that is the case.  I just wanted to make sure the amount of
feedback for such a tiny patch didn't come across as "you're doing this
wrong!!"  Email can be such an emotionless medium of communication, and
I'm pretty sensitive to making sure my feedback doesn't come across as
negative or discouraging.

> 
>     So, can you send a v3 with Josh's Reviewed-by line below your
>     Signed-off-by line?
> 
> 
> I just did. I don't know why but it seems I just did not understand
> that part.

No problem.  The new patch looks like you now understand it.  :-)

-PJ

> 
> Best regards,
> 
> Tugce Sirin
> 
> 
> 
> 
> 2013/10/3 Waskiewicz Jr, Peter P <peter.p.waskiewicz.jr@intel.com>
>         On Wed, 2013-10-02 at 15:05 -0700, Josh Triplett wrote:
>         > On Wed, Oct 02, 2013 at 09:46:52PM +0000, Waskiewicz Jr,
>         Peter P wrote:
>         > > This is Greg's call, but I personally usually like to see
>         even a tiny
>         > > commit message.  It's obvious to me why the change is
>         being made, but it
>         > > might be good to determine where the need arose.  I assume
>         checkpatch.pl
>         > > caught this?  If so, I'd suggest a small commit message
>         like this:
>         > >
>         > > "Fix space before semicolon based on checkpatch.pl scan."
>         >
>         > Depends on what you're fixing.  When the fix is extremely
>         obvious, you
>         > can sometimes get away with just a subject line and a
>         signoff, but
>         > erring on the side of verbosity is preferable. :)
>         
>         
>         A verbose changelog helps those of us (aka me) who seem to
>         leak more
>         information out of our heads than retain anymore.  Getting
>         older
>         sucks.  :-)
>         
>         > Rather than duplicating most of the subject, you might
>         consider quoting
>         > the exact output of checkpatch.pl or whatever tool you used,
>         for
>         > reference.
>         
>         
>         That's even better.  This way if/when updates are made to the
>         tools, as
>         checkpatch.pl does get updated, the origin of a particular
>         change can be
>         traced to pre-tool update if needed.
>         
>         -PJ
>         
>         --
>         You received this message because you are subscribed to the
>         Google Groups "opw-kernel" group.
>         To unsubscribe from this group and stop receiving emails from
>         it, send an email to opw-kernel+unsubscribe@googlegroups.com.
>         For more options, visit
>         https://groups.google.com/groups/opt_out.
>         
> 
> 
> 
> -- 
> Tu?çe ?irin
Tugce Sirin Oct. 2, 2013, 11:09 p.m. UTC | #8
I'm glad about amount of feedbacks because if you would thing that "it's
okay for this tiny patch, let's not correct her" that would be a real pain
in next patches and especially in patchsets. Thank you all for your time
and patience.

Tugce Sirin


2013/10/3 Waskiewicz Jr, Peter P <peter.p.waskiewicz.jr@intel.com>

> On Thu, 2013-10-03 at 01:47 +0300, Tu?çe ?irin wrote:
> >     "Fix space before semicolon based on checkpatch.pl scan."
> >
> > I did not add this only for this patch but I'll add it for next times.
>
> Not a problem.  Again, it's more preference for the maintainer taking
> the patch, so if Greg doesn't care, then I don't care.  :-)
>
> >
> >     And please don't interpret all the feedback here that you didn't
> > do this
> >     mostly right, these are just suggestions so future patches flow
> > even
> >     smoother.  :-)
> >
> >
> > Feedback is awesome, I'm not easily offended by 'correcting' because
> > if I'm doing something it should be correct. :)
>
> Glad that is the case.  I just wanted to make sure the amount of
> feedback for such a tiny patch didn't come across as "you're doing this
> wrong!!"  Email can be such an emotionless medium of communication, and
> I'm pretty sensitive to making sure my feedback doesn't come across as
> negative or discouraging.
>
> >
> >     So, can you send a v3 with Josh's Reviewed-by line below your
> >     Signed-off-by line?
> >
> >
> > I just did. I don't know why but it seems I just did not understand
> > that part.
>
> No problem.  The new patch looks like you now understand it.  :-)
>
> -PJ
>
> >
> > Best regards,
> >
> > Tugce Sirin
> >
> >
> >
> >
> > 2013/10/3 Waskiewicz Jr, Peter P <peter.p.waskiewicz.jr@intel.com>
> >         On Wed, 2013-10-02 at 15:05 -0700, Josh Triplett wrote:
> >         > On Wed, Oct 02, 2013 at 09:46:52PM +0000, Waskiewicz Jr,
> >         Peter P wrote:
> >         > > This is Greg's call, but I personally usually like to see
> >         even a tiny
> >         > > commit message.  It's obvious to me why the change is
> >         being made, but it
> >         > > might be good to determine where the need arose.  I assume
> >         checkpatch.pl
> >         > > caught this?  If so, I'd suggest a small commit message
> >         like this:
> >         > >
> >         > > "Fix space before semicolon based on checkpatch.pl scan."
> >         >
> >         > Depends on what you're fixing.  When the fix is extremely
> >         obvious, you
> >         > can sometimes get away with just a subject line and a
> >         signoff, but
> >         > erring on the side of verbosity is preferable. :)
> >
> >
> >         A verbose changelog helps those of us (aka me) who seem to
> >         leak more
> >         information out of our heads than retain anymore.  Getting
> >         older
> >         sucks.  :-)
> >
> >         > Rather than duplicating most of the subject, you might
> >         consider quoting
> >         > the exact output of checkpatch.pl or whatever tool you used,
> >         for
> >         > reference.
> >
> >
> >         That's even better.  This way if/when updates are made to the
> >         tools, as
> >         checkpatch.pl does get updated, the origin of a particular
> >         change can be
> >         traced to pre-tool update if needed.
> >
> >         -PJ
> >
> >         --
> >         You received this message because you are subscribed to the
> >         Google Groups "opw-kernel" group.
> >         To unsubscribe from this group and stop receiving emails from
> >         it, send an email to opw-kernel+unsubscribe@googlegroups.com.
> >         For more options, visit
> >         https://groups.google.com/groups/opt_out.
> >
> >
> >
> >
> > --
> > Tu?çe ?irin
>
> --
> You received this message because you are subscribed to the Google Groups
> "opw-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to opw-kernel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

Patch
diff mbox

diff --git a/drivers/staging/silicom/bp_mod.h b/drivers/staging/silicom/bp_mod.h
index cfa1f43..8154a7b 100644
--- a/drivers/staging/silicom/bp_mod.h
+++ b/drivers/staging/silicom/bp_mod.h
@@ -22,7 +22,7 @@  do {						\
 	int  i;					\
 	if (1) {				\
 		for (i = 0; i < 1000; i++) {	\
-			udelay(x) ;		\
+			udelay(x);		\
 		}				\
 	} else {				\
 		msleep(x);			\