* [PATCH] staging: iio: cdc: add comment to mutex @ 2016-09-27 10:45 Anchal Jain 2016-09-27 11:16 ` Greg KH 2016-09-27 17:07 ` [Outreachy kernel] " Alison Schofield 0 siblings, 2 replies; 8+ messages in thread From: Anchal Jain @ 2016-09-27 10:45 UTC (permalink / raw) To: gregkh; +Cc: Michael.Hennerich, outreachy-kernel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: iio: cdc: add comment to mutex 2016-09-27 10:45 [PATCH] staging: iio: cdc: add comment to mutex Anchal Jain @ 2016-09-27 11:16 ` Greg KH 2016-09-27 12:16 ` Anchal Jain 2016-09-27 17:07 ` [Outreachy kernel] " Alison Schofield 1 sibling, 1 reply; 8+ messages in thread From: Greg KH @ 2016-09-27 11:16 UTC (permalink / raw) To: Anchal Jain; +Cc: Michael.Hennerich, outreachy-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: iio: cdc: add comment to mutex 2016-09-27 11:16 ` Greg KH @ 2016-09-27 12:16 ` Anchal Jain 2016-09-27 12:53 ` [Outreachy kernel] " Julia Lawall 0 siblings, 1 reply; 8+ messages in thread From: Anchal Jain @ 2016-09-27 12:16 UTC (permalink / raw) To: Greg KH; +Cc: Michael.Hennerich, outreachy-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] staging: iio: cdc: add comment to mutex 2016-09-27 12:16 ` Anchal Jain @ 2016-09-27 12:53 ` Julia Lawall 0 siblings, 0 replies; 8+ messages in thread From: Julia Lawall @ 2016-09-27 12:53 UTC (permalink / raw) To: Anchal Jain; +Cc: Greg KH, Michael.Hennerich, outreachy-kernel [-- 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. > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: cdc: add comment to mutex 2016-09-27 10:45 [PATCH] staging: iio: cdc: add comment to mutex Anchal Jain 2016-09-27 11:16 ` Greg KH @ 2016-09-27 17:07 ` Alison Schofield 2016-09-27 19:46 ` Alison Schofield 1 sibling, 1 reply; 8+ messages in thread From: Alison Schofield @ 2016-09-27 17:07 UTC (permalink / raw) To: Anchal Jain; +Cc: gregkh, Michael.Hennerich, outreachy-kernel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: cdc: add comment to mutex 2016-09-27 17:07 ` [Outreachy kernel] " Alison Schofield @ 2016-09-27 19:46 ` Alison Schofield 2016-09-28 17:00 ` Anchal Jain 0 siblings, 1 reply; 8+ messages in thread From: Alison Schofield @ 2016-09-27 19:46 UTC (permalink / raw) To: Anchal Jain; +Cc: gregkh, Michael.Hennerich, outreachy-kernel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: cdc: add comment to mutex 2016-09-27 19:46 ` Alison Schofield @ 2016-09-28 17:00 ` Anchal Jain 2016-09-28 17:34 ` Alison Schofield 0 siblings, 1 reply; 8+ messages in thread From: Anchal Jain @ 2016-09-28 17:00 UTC (permalink / raw) To: Alison Schofield; +Cc: Greg KH, Michael.Hennerich, outreachy-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: iio: cdc: add comment to mutex 2016-09-28 17:00 ` Anchal Jain @ 2016-09-28 17:34 ` Alison Schofield 0 siblings, 0 replies; 8+ messages in thread From: Alison Schofield @ 2016-09-28 17:34 UTC (permalink / raw) To: Anchal Jain; +Cc: Greg KH, Michael.Hennerich, outreachy-kernel 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. > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-09-28 17:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-27 10:45 [PATCH] staging: iio: cdc: add comment to mutex Anchal Jain 2016-09-27 11:16 ` Greg KH 2016-09-27 12:16 ` Anchal Jain 2016-09-27 12:53 ` [Outreachy kernel] " Julia Lawall 2016-09-27 17:07 ` [Outreachy kernel] " Alison Schofield 2016-09-27 19:46 ` Alison Schofield 2016-09-28 17:00 ` Anchal Jain 2016-09-28 17:34 ` Alison Schofield
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.