[PATCH v2] staging: iio: gyro: add comment to mutex

[PATCH v2] staging: iio: gyro: add comment to mutex

[Anchal Jain]

Fix the checkpatch.pl issues:
CHECK: struct mutex definition without comment


Signed-off-by: Anchal Jain <anchalj109@gmail.com>
---
 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;
 };
-- 
1.9.1

Re: [PATCH v2] staging: iio: gyro: add comment to mutex

[Anchal Jain]

[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]

>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 ob then
>that would to my mind be a worthwhile stepvious pair of spi device
structures)
>(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

On Sat, Oct 1, 2016 at 7:05 PM, Jonathan Cameron <jic23@kernel.org> 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 <anchalj109@gmail.com>
> 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;
> >  };
> >
>
>

[-- Attachment #2: Type: text/html, Size: 4225 bytes --]

Re: [PATCH v2] staging: iio: gyro: add comment to mutex

[Greg KH]

On Sat, Oct 01, 2016 at 02:35:05PM +0100, Jonathan Cameron 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 <anchalj109@gmail.com>
> 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.

I'm not going to apply it either for all of these same reasons, thanks
for the good review.

greg k-h

Re: [Outreachy kernel] Re: [PATCH v2] staging: iio: gyro: add comment to mutex

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4496 bytes --]



On Sun, 2 Oct 2016, Anchal Jain wrote:

> >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 ob then
> >that would to my mind be a worthwhile stepvious pair of spi device
> structures)
> >(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.

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

>
> Thank you,
>
> Anchal
>
> On Sat, Oct 1, 2016 at 7:05 PM, Jonathan Cameron <jic23@kernel.org> 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 <anchalj109@gmail.com>
>       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 outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> 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.
>
>