staging: iio: gyro: add comment to mutex
Subsystems (4)
PATCH v2 1/1 staging: iio: gyro: add comment to mutex
Fix the checkpatch.pl issues:
CHECK: struct mutex definition without comment
--- drivers/staging/iio/gyro/adis16060_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/iio/gyro/adis16060_core.c +1 -1
@@ -33,7 +33,7 @@struct adis16060_state {struct spi_device *us_w;struct spi_device *us_r; -struct mutex buf_lock; +struct mutex buf_lock;/* protect transfer buffers */ u8 buf[3 ]____cacheline_aligned ; };
Signature · 1 line
1.9.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 4 lines
If Greg has already picked this up I don't really care but it is kind of obvious that
buf_lockprotects buf, so I'm not going to bother applying it.
Quoted 8 lines
Dealing with check patch warnings is often an exercise in trying to find the right balance between 'fixing' them all and deciding that some of them are 'silly'.
Now, if you were to give kernel doc comments to the whole of that structure (which does contain the rather less ob then
that would to my mind be a worthwhile stepvious pair of spi device
structures)
Quoted 1 line
(and would get rid of this warning along the way!)
Thank you, For informing me...
But, Seriously I am too confused what should I do and what not because
Repeatedly send the same patchis not a good idea because I am not learning new things
and I don't know what is obvious for you guys.
I do not know about this code that's how it works in the kernel and why is here I am just a beginner. On some basic knowledge of C, I am trying to assume the things may be this code is working that thing or other
and according to check patch, I am trying to solve bugs.
So, if the check patch shows that kind of problem then what should I do because this not first time with me to waste my time on that silly things.
Thank you,
Anchal
Quoted 49 lines · Jonathan Cameron
Quoted 3 lines · Anchal Jain
Fix the checkpatch.pl issues:
CHECK: struct mutex definition without commentNitpick of the day, only one blank line here.
Quoted 2 lines
Signed-off-by Anchal Jain <[email protected]>Have to say this isn't one of the greatest warnings from checkpatch.
If Greg has already picked this up I don't really care but it is kind of obvious that
buf_lockprotects buf, so I'm not going to bother applying it.Dealing with check patch warnings is often an exercise in trying to find the right balance between 'fixing' them all and deciding that some of them are 'silly'.
Now, if you were to give kernel doc comments to the whole of that structure (which does contain the rather less obvious pair of spi device structures) then that would to my mind be a worthwhile step (and would get rid of this warning along the way!)
Thanks,
Jonathan
Review
Quoted 5 lines
--- drivers/staging/iio/gyro/adis16060_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/staging/iio/gyro/adis16060_core.cdrivers/staging/iio/gyro/adis16060_core.c line 36
Quoted 9 lines
index ab816a2..96230b6 100644
drivers/staging/iio/gyro/adis16060_core.c +1
-1@@ -33,7 +33,7 @@struct adis16060_state {struct spi_device *us_w;struct spi_device *us_r; -struct mutex buf_lock; +struct mutex buf_lock;/* protect transfer buffers */
Quoted 4 lines
drivers/staging/iio/gyro/adis16060_core.c +0
-0u8 buf[3 ]____cacheline_aligned ; };
Quoted 18 lines · Anchal Jain
Quoted 4 lines
If Greg has already picked this up I don't really care but it is kind of obvious that
buf_lockprotects buf, so I'm not going to bother applying it.Quoted 8 lines
Dealing with check patch warnings is often an exercise in trying to find the right balance between 'fixing' them all and deciding that some of them are 'silly'.
Now, if you were to give kernel doc comments to the whole of that structure (which does contain the rather less ob then
that would to my mind be a worthwhile stepvious pair of spi devicestructures)
Quoted 1 line
(and would get rid of this warning along the way!)
Thank you, For informing me...
But, Seriously I am too confused what should I do and what not because
Quoted 10 lines
Repeatedly send the same patchis not a good idea because
I am not learning new things
and I don't know what is obvious for you guys.
I do not know about this code that's how it works in the kernel and why is here I am just a beginner. On some basic knowledge of C, I am trying to assume the things may be this code is working that thing or other
and according to check patch, I am trying to solve bugs.
So, if the check patch shows that kind of problem then what should I do because this not first time with me to waste my time on that silly things.
You have to develop some intuition about what is useful and what is not. And even if you have some intuition, you may get it wrong sometimes. You can look at patches done by other people for the same issue. For example, in this case git log -G "struct mutex" could be helpful. Or, as needed, learn from the feedback of others.
Checkpatch is just a tool made with regular expressions. It doesn't know about the semantics of the code, and in particular it doesn't know about the intent of the developer. So it can suggest wrong or useless things sometimes.
julia
Quoted 66 lines
Thank you,
Anchal
On Sat, Oct 1, 2016 at 7:05 PM, Jonathan Cameron <[email protected]> wrote:
On 28/09/16 19:15, Anchal Jain wrote: > Fix the checkpatch.pl issues: > CHECK: struct mutex definition without comment > Nitpick of the day, only one blank line here. > > Signed-off-by: Anchal Jain <[email protected]> Have to say this isn't one of the greatest warnings from checkpatch. If Greg has already picked this up I don't really care but it is kind of obvious that buf_lock protects buf, so I'm not going to bother applying it. Dealing with check patch warnings is often an exercise in trying to find the right balance between 'fixing' them all and deciding that some of them are 'silly'. Now, if you were to give kernel doc comments to the whole of that structure (which does contain the rather less obvious pair of spi device structures) then that would to my mind be a worthwhile step (and would get rid of this warning along the way!) Thanks, Jonathan > --- > drivers/staging/iio/gyro/adis16060_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c > index ab816a2..96230b6 100644 > --- a/drivers/staging/iio/gyro/adis16060_core.c > +++ b/drivers/staging/iio/gyro/adis16060_core.c > @@ -33,7 +33,7 @@ > struct adis16060_state { > struct spi_device *us_w; > struct spi_device *us_r; > - struct mutex buf_lock; > + struct mutex buf_lock; /* protect transfer buffers */ > > u8 buf[3] ____cacheline_aligned; > }; >--
You received this message because you are subscribed to the Google Groups
"outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/CAAbeOSv9B7fTLx1%2BeJsn fTVbNm5%3D0e79W4O0eZQnXQ51u1Bjhg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Greg KH
maintainerQuoted 6 lines · Jonathan Cameron
Quoted 3 lines · Anchal Jain
Fix the checkpatch.pl issues:
CHECK: struct mutex definition without commentNitpick of the day, only one blank line here.
Quoted 1 line
Quoted 7 lines
Quoted 1 line
Signed-off-by Anchal Jain <[email protected]>Have to say this isn't one of the greatest warnings from checkpatch.
If Greg has already picked this up I don't really care but it is kind of obvious that
buf_lockprotects buf, so I'm not going to bother applying it.
I'm not going to apply it either for all of these same reasons, thanks for the good review.
greg k-h