[PATCH] staging: iio: cdc: add comment to mutex

[PATCH] staging: iio: cdc: add comment to mutex

[Anchal Jain]

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

Signed-off-by: Anchal Jain <anchalj109@gmail.com>
---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index f6b9a10..88c6ebe 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -78,7 +78,7 @@ struct ad7150_chip_info {
 	u8 mag_timeout[2][2];
 	int old_state;
 	char *conversion_mode;
-	struct mutex state_lock;
+	struct mutex state_lock; /*generic mutex for cdc and drivers*/
 };
 
 /*
-- 
1.9.1

Re: [PATCH] staging: iio: cdc: add comment to mutex

[Greg KH]

On Tue, Sep 27, 2016 at 04:15:12PM +0530, Anchal Jain wrote:
> Fix the checkpatch.pl issues:
> CHECK: struct mutes definition without comment
> 
> Signed-off-by: Anchal Jain <anchalj109@gmail.com>
> ---
>  drivers/staging/iio/cdc/ad7150.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index f6b9a10..88c6ebe 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -78,7 +78,7 @@ struct ad7150_chip_info {
>  	u8 mag_timeout[2][2];
>  	int old_state;
>  	char *conversion_mode;
> -	struct mutex state_lock;
> +	struct mutex state_lock; /*generic mutex for cdc and drivers*/

That's the best description for this?  I don't understand this, do you?

Why no ' ' after and before '*'?

thnks,

greg k-h

Re: [PATCH] staging: iio: cdc: add comment to mutex

[Anchal Jain]

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

>That's the best description for this?  I don't understand this, do you?
>
>Why no ' ' after and before '*'?

because the in C we prefer in the comment in like that
/*....*/
not like this
/'*'....'*'/
If I used this there is not problem the compiler should read as a comment
because in normal C program we can use comment in these pattern /..../
but according to  Linux kernel Coding style
<https://www.kernel.org/doc/Documentation/CodingStyle>

And I am not sure it is the best description may be it is not fully correct
if you want to do some change in comment than I can.

Thank you,
<https://www.kernel.org/doc/Documentation/CodingStyle>

On Tue, Sep 27, 2016 at 4:46 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Sep 27, 2016 at 04:15:12PM +0530, Anchal Jain wrote:
> > Fix the checkpatch.pl issues:
> > CHECK: struct mutes definition without comment
> >
> > Signed-off-by: Anchal Jain <anchalj109@gmail.com>
> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/
> ad7150.c
> > index f6b9a10..88c6ebe 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -78,7 +78,7 @@ struct ad7150_chip_info {
> >       u8 mag_timeout[2][2];
> >       int old_state;
> >       char *conversion_mode;
> > -     struct mutex state_lock;
> > +     struct mutex state_lock; /*generic mutex for cdc and drivers*/
>
> That's the best description for this?  I don't understand this, do you?
>
> Why no ' ' after and before '*'?
>
> thnks,
>
> greg k-h
>

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

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

[Julia Lawall]

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



On Tue, 27 Sep 2016, Anchal Jain wrote:

> >That's the best description for this?  I don't understand this, do you?
> >
> >Why no ' ' after and before '*'?
>
> because the in C we prefer in the comment in like that
> /*....*/
> not like this
> /'*'....'*'/

Normally comments /* look like this */, /*not like this*/.

> If I used this there is not problem the compiler should read as a comment
> because in normal C program we can use comment in these pattern /..../
> but according to  Linux kernel Coding style
>
> And I am not sure it is the best description may be it is not fully correct
> if you want to do some change in comment than I can.

I think it is a problem that can only be solved with a good understanding
of what the mutex is used for.  You may want to look at what comments are
associated with mutex declarations in other files.

julia

> Thank you,
>
> On Tue, Sep 27, 2016 at 4:46 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>       On Tue, Sep 27, 2016 at 04:15:12PM +0530, Anchal Jain wrote:
>       > Fix the checkpatch.pl issues:
>       > CHECK: struct mutes definition without comment
>       >
>       > Signed-off-by: Anchal Jain <anchalj109@gmail.com>
>       > ---
>       >  drivers/staging/iio/cdc/ad7150.c | 2 +-
>       >  1 file changed, 1 insertion(+), 1 deletion(-)
>       >
>       > diff --git a/drivers/staging/iio/cdc/ad7150.c
>       b/drivers/staging/iio/cdc/ad7150.c
>       > index f6b9a10..88c6ebe 100644
>       > --- a/drivers/staging/iio/cdc/ad7150.c
>       > +++ b/drivers/staging/iio/cdc/ad7150.c
>       > @@ -78,7 +78,7 @@ struct ad7150_chip_info {
>       >       u8 mag_timeout[2][2];
>       >       int old_state;
>       >       char *conversion_mode;
>       > -     struct mutex state_lock;
>       > +     struct mutex state_lock; /*generic mutex for cdc and
>       drivers*/
>
>       That's the best description for this?  I don't understand this,
>       do you?
>
>       Why no ' ' after and before '*'?
>
>       thnks,
>
>       greg k-h
>
>
> --
> 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/CAAbeOSuN_FAYa_PUDQRQBsy
> vuKriv5SQykZwsM8KM%3D9dsMJOGg%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

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

[Alison Schofield]

On Tue, Sep 27, 2016 at 04:15:12PM +0530, Anchal Jain wrote:
> Fix the checkpatch.pl issues:
> CHECK: struct mutes definition without comment
> 
> Signed-off-by: Anchal Jain <anchalj109@gmail.com>

Ahah ;) I've been wondering when someone would get around
to these.  There are a lot of these CHECK's in IIO.

Following on Julia & Greg's comments, you do need to 
explore what the lock is for and give a succinct comment.

Unfortunately, when you: git grep "struct mutex", even in 
upstream iio, you won't find too many examples...but you'll
find a few.  Usually, simply say /* protect <something> */

Fortunately, in this case, there is a comment for it, just
in the wrong place - scroll up. That makes your task easier,
although I still would suggest condensing it to a short phrase that
fit adjacent to the definition.

alisons

> ---
>  drivers/staging/iio/cdc/ad7150.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index f6b9a10..88c6ebe 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -78,7 +78,7 @@ struct ad7150_chip_info {
>  	u8 mag_timeout[2][2];
>  	int old_state;
>  	char *conversion_mode;
> -	struct mutex state_lock;
> +	struct mutex state_lock; /*generic mutex for cdc and drivers*/
>  };
>  
>  /*
> -- 
> 1.9.1
> 
> -- 
> 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 visit https://groups.google.com/d/msgid/outreachy-kernel/20160927104504.GA13155%40life-desktop.
> For more options, visit https://groups.google.com/d/optout.

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

[Alison Schofield]

On Tue, Sep 27, 2016 at 10:07:02AM -0700, Alison Schofield wrote:
> On Tue, Sep 27, 2016 at 04:15:12PM +0530, Anchal Jain wrote:
> > Fix the checkpatch.pl issues:
> > CHECK: struct mutes definition without comment
> > 
> > Signed-off-by: Anchal Jain <anchalj109@gmail.com>
> 
> Ahah ;) I've been wondering when someone would get around
> to these.  There are a lot of these CHECK's in IIO.
> 
> Following on Julia & Greg's comments, you do need to 
> explore what the lock is for and give a succinct comment.
> 
> Unfortunately, when you: git grep "struct mutex", even in 
> upstream iio, you won't find too many examples...but you'll
> find a few.  Usually, simply say /* protect <something> */
> 
> Fortunately, in this case, there is a comment for it, just
> in the wrong place - scroll up. That makes your task easier,
> although I still would suggest condensing it to a short phrase that
> fit adjacent to the definition.
> 
> alisons

Although, this change is 'just a comment', if you do a few of them
you'll begin developing an understanding of where and why things
are locked in IIO drivers.  That's where the value is - you are
exploring the code and figuring things out. 

Good patches for honing your patching skills.

alisons


> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > index f6b9a10..88c6ebe 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -78,7 +78,7 @@ struct ad7150_chip_info {
> >  	u8 mag_timeout[2][2];
> >  	int old_state;
> >  	char *conversion_mode;
> > -	struct mutex state_lock;
> > +	struct mutex state_lock; /*generic mutex for cdc and drivers*/
> >  };
> >  
> >  /*
> > -- 
> > 1.9.1
> > 
> > -- 
> > 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 visit https://groups.google.com/d/msgid/outreachy-kernel/20160927104504.GA13155%40life-desktop.
> > For more options, visit https://groups.google.com/d/optout.

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

[Anchal Jain]

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

>Ahah ;) I've been wondering when someone would get around
>to these.  There are a lot of these CHECK's in IIO.
>
>Following on Julia & Greg's comments, you do need to
>explore what the lock is for and give a succinct comment.
>
>Unfortunately, when you: git grep "struct mutex", even in
>upstream iio, you won't find too many examples...but you'll
>find a few.  Usually, simply say /* protect <something> */
>
>Fortunately, in this case, there is a comment for it, just
>in the wrong place - scroll up. That makes your task easier,
>although I still would suggest condensing it to a short phrase that
>fit adjacent to the definition.




I according to your suggestion I read the comment above the "struct
ad7150_chip_info"
And check the other files where they used the "struct mutex"

As you said to use just simply /* protect <something> */
then in the comment, Mention some hardware stuff what was that stuff ?
so I can make the comment.


On Wed, Sep 28, 2016 at 1:16 AM, Alison Schofield <amsfield22@gmail.com>
wrote:

> On Tue, Sep 27, 2016 at 10:07:02AM -0700, Alison Schofield wrote:
> > On Tue, Sep 27, 2016 at 04:15:12PM +0530, Anchal Jain wrote:
> > > Fix the checkpatch.pl issues:
> > > CHECK: struct mutes definition without comment
> > >
> > > Signed-off-by: Anchal Jain <anchalj109@gmail.com>
> >
> > Ahah ;) I've been wondering when someone would get around
> > to these.  There are a lot of these CHECK's in IIO.
> >
> > Following on Julia & Greg's comments, you do need to
> > explore what the lock is for and give a succinct comment.
> >
> > Unfortunately, when you: git grep "struct mutex", even in
> > upstream iio, you won't find too many examples...but you'll
> > find a few.  Usually, simply say /* protect <something> */
> >
> > Fortunately, in this case, there is a comment for it, just
> > in the wrong place - scroll up. That makes your task easier,
> > although I still would suggest condensing it to a short phrase that
> > fit adjacent to the definition.
> >
> > alisons
>
> Although, this change is 'just a comment', if you do a few of them
> you'll begin developing an understanding of where and why things
> are locked in IIO drivers.  That's where the value is - you are
> exploring the code and figuring things out.
>
> Good patches for honing your patching skills.
>
> alisons
>
>
> > > ---
> > >  drivers/staging/iio/cdc/ad7150.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7150.c
> b/drivers/staging/iio/cdc/ad7150.c
> > > index f6b9a10..88c6ebe 100644
> > > --- a/drivers/staging/iio/cdc/ad7150.c
> > > +++ b/drivers/staging/iio/cdc/ad7150.c
> > > @@ -78,7 +78,7 @@ struct ad7150_chip_info {
> > >     u8 mag_timeout[2][2];
> > >     int old_state;
> > >     char *conversion_mode;
> > > -   struct mutex state_lock;
> > > +   struct mutex state_lock; /*generic mutex for cdc and drivers*/
> > >  };
> > >
> > >  /*
> > > --
> > > 1.9.1
> > >
> > > --
> > > 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 visit https://groups.google.com/d/
> msgid/outreachy-kernel/20160927104504.GA13155%40life-desktop.
> > > For more options, visit https://groups.google.com/d/optout.
>

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

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

[Alison Schofield]

On Wed, Sep 28, 2016 at 10:30:59PM +0530, Anchal Jain wrote:
> >Ahah ;) I've been wondering when someone would get around
> >to these.  There are a lot of these CHECK's in IIO.
> >
> >Following on Julia & Greg's comments, you do need to
> >explore what the lock is for and give a succinct comment.
> >
> >Unfortunately, when you: git grep "struct mutex", even in
> >upstream iio, you won't find too many examples...but you'll
> >find a few.  Usually, simply say /* protect <something> */
> >
> >Fortunately, in this case, there is a comment for it, just
> >in the wrong place - scroll up. That makes your task easier,
> >although I still would suggest condensing it to a short phrase that
> >fit adjacent to the definition.
> 
> 
> 
> 
> I according to your suggestion I read the comment above the "struct
> ad7150_chip_info"
> And check the other files where they used the "struct mutex"
> 
> As you said to use just simply /* protect <something> */
> then in the comment, Mention some hardware stuff what was that stuff ?
> so I can make the comment.
> 
> 

That header comment says:
state_lock: ensure consistent state of this structure wrt the hardware.

At the risk of being overly pedantic...the lock will make sure that the state
data in this structure always accurately reflects the state of the
hardware.  When you look above at the structure fields...all those u8
items, I'm guessing they are settings in the hardware.  So, when parts
of this driver start changing those settings, we need to use a lock
so that simultaneous writes of that state data don't conflict

If there had not been that comment to tell us what the lock was for,
we would have had to go review all the places it was used to create
the comment.

So, back to your task of shrinking that comment.  I see it's the
last item in the struct, so we wouldn't mess up the beauty of the
struct by making a multiline comment.  Probably not needed though,
since the nice header comment is available a short scroll away.

Shortened version of that to append could be:
	struct mutex state_lock;	/* protect hardware state */

I'd suggest going with similar and sending it out for further review.
Your CC list looks off.  Seems it should have picked up iio maintainers,
reviewers, and the linux-iio list.

Need to put driver name in commit msg (iio: cdc: ad7150)

alisons

> On Wed, Sep 28, 2016 at 1:16 AM, Alison Schofield <amsfield22@gmail.com>
> wrote:
> 
> > On Tue, Sep 27, 2016 at 10:07:02AM -0700, Alison Schofield wrote:
> > > On Tue, Sep 27, 2016 at 04:15:12PM +0530, Anchal Jain wrote:
> > > > Fix the checkpatch.pl issues:
> > > > CHECK: struct mutes definition without comment
> > > >
> > > > Signed-off-by: Anchal Jain <anchalj109@gmail.com>
> > >
> > > Ahah ;) I've been wondering when someone would get around
> > > to these.  There are a lot of these CHECK's in IIO.
> > >
> > > Following on Julia & Greg's comments, you do need to
> > > explore what the lock is for and give a succinct comment.
> > >
> > > Unfortunately, when you: git grep "struct mutex", even in
> > > upstream iio, you won't find too many examples...but you'll
> > > find a few.  Usually, simply say /* protect <something> */
> > >
> > > Fortunately, in this case, there is a comment for it, just
> > > in the wrong place - scroll up. That makes your task easier,
> > > although I still would suggest condensing it to a short phrase that
> > > fit adjacent to the definition.
> > >
> > > alisons
> >
> > Although, this change is 'just a comment', if you do a few of them
> > you'll begin developing an understanding of where and why things
> > are locked in IIO drivers.  That's where the value is - you are
> > exploring the code and figuring things out.
> >
> > Good patches for honing your patching skills.
> >
> > alisons
> >
> >
> > > > ---
> > > >  drivers/staging/iio/cdc/ad7150.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > b/drivers/staging/iio/cdc/ad7150.c
> > > > index f6b9a10..88c6ebe 100644
> > > > --- a/drivers/staging/iio/cdc/ad7150.c
> > > > +++ b/drivers/staging/iio/cdc/ad7150.c
> > > > @@ -78,7 +78,7 @@ struct ad7150_chip_info {
> > > >     u8 mag_timeout[2][2];
> > > >     int old_state;
> > > >     char *conversion_mode;
> > > > -   struct mutex state_lock;
> > > > +   struct mutex state_lock; /*generic mutex for cdc and drivers*/
> > > >  };
> > > >
> > > >  /*
> > > > --
> > > > 1.9.1
> > > >
> > > > --
> > > > 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 visit https://groups.google.com/d/
> > msgid/outreachy-kernel/20160927104504.GA13155%40life-desktop.
> > > > For more options, visit https://groups.google.com/d/optout.
> >