staging: iio: cdc: add comment to mutex

PATCH In review Anchal Jain 4 participants 8 messages Sep 27, 2016 – Sep 28, 2016

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

Anchal Jain

author

2016-09-27 10:45 UTC #0 lore raw

PATCH 1/1 staging: iio: cdc: add comment to mutex

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

Signed-off-by Anchal Jain <[email protected]>

---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/iio/cdc/ad7150.c +1 -1

index f6b9a10..88c6ebe 100644
@@ -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*/
 };
 
 /*

Signature · 1 line

1.9.1

Greg KH

maintainer

2016-09-27 11:16 UTC #1 lore raw

Quoted 13 lines · Anchal Jain

Fix the checkpatch.pl issues:
CHECK: struct mutes definition without comment
Signed-off-by Anchal Jain <[email protected]>
---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/iio/cdc/ad7150.c +0 -0
index f6b9a10..88c6ebe 100644

drivers/staging/iio/cdc/ad7150.c struct ad7150_chip_info { · line 81

Quoted 5 lines

drivers/staging/iio/cdc/ad7150.c +1 -1

@@ -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

Anchal Jain

author

2016-09-27 12:16 UTC #2 lore raw

Quoted 3 lines

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>

Quoted 29 lines · Greg KH

drivers/staging/iio/cdc/
Quoted 9 lines · Anchal Jain
Fix the checkpatch.pl issues:
CHECK: struct mutes definition without comment
Signed-off-by Anchal Jain <[email protected]>
---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/iio/cdc/ renamed from drivers/staging/iio/cdc/ad7150.c +0 -0
ad7150.c
drivers/staging/iio/cdc/ad7150.c struct ad7150_chip_info { · line 81
Quoted 9 lines
index f6b9a10..88c6ebe 100644
drivers/staging/iio/cdc/ad7150.c +1 -1
@@ -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

Julia Lawall

2016-09-27 12:54 UTC #3 lore raw

Quoted 8 lines · Anchal Jain

Quoted 3 lines
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*/.

Quoted 6 lines

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

Quoted 46 lines

Thank you,
On Tue, Sep 27, 2016 at 4:46 PM, Greg KH <[email protected]> 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 <[email protected]>
      > ---
      >  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 [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/CAAbeOSuN_FAYa_PUDQRQBsy vuKriv5SQykZwsM8KM%3D9dsMJOGg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Alison Schofield

2016-09-27 17:07 UTC #4 lore raw

In reply to Anchal Jain

Quoted 4 lines · Anchal Jain

Fix the checkpatch.pl issues:
CHECK: struct mutes definition without comment
Signed-off-by Anchal Jain <[email protected]>

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

Quoted 26 lines

---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/iio/cdc/ad7150.c +1 -1

index f6b9a10..88c6ebe 100644
@@ -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*/
 };
 
 /*

Signature · 8 lines

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 [email protected].
To post to this group, send email to [email protected].
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.

Alison Schofield

2016-09-27 19:46 UTC #5 lore raw

Quoted 16 lines · Alison Schofield

Quoted 4 lines · Anchal Jain
Fix the checkpatch.pl issues:
CHECK: struct mutes definition without comment
Signed-off-by Anchal Jain <[email protected]>
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> */

Quoted 6 lines

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

Quoted 26 lines

---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/iio/cdc/ad7150.c +1 -1

index f6b9a10..88c6ebe 100644
@@ -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*/
 };
 
 /*

Signature · 8 lines

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 [email protected].
To post to this group, send email to [email protected].
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.

Anchal Jain

author

2016-09-28 17:01 UTC #6 lore raw

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 66 lines · Alison Schofield

Quoted 16 lines · Alison Schofield
Quoted 4 lines · Anchal Jain
Fix the checkpatch.pl issues:
CHECK: struct mutes definition without comment
Signed-off-by Anchal Jain <[email protected]>
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> */
Quoted 6 lines
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
Quoted 5 lines
Review
Quoted 5 lines
---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/staging/iio/cdc/ad7150.c
Quoted 11 lines
Quoted 11 lines
index f6b9a10..88c6ebe 100644
drivers/staging/iio/cdc/ad7150.c +1 -1
@@ -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*/
 };
 
Quoted 6 lines
Quoted 6 lines
drivers/staging/iio/cdc/ad7150.c +0 -1
 /*
--
1.9.1
--
You received this message because you are subscribed to the Google
Groups "outreachy-kernel" group.
Quoted 1 line
Quoted 1 line
To unsubscribe from this group and stop receiving emails from it, send
an email to [email protected].
Quoted 1 line
Quoted 1 line
To post to this group, send email to [email protected]
.
Quoted 1 line
Quoted 1 line
To view this discussion on the web visit https://groups.google.com/d/
msgid/outreachy-kernel/20160927104504.GA13155%40life-desktop.
Quoted 1 line
Quoted 1 line
For more options, visit https://groups.google.com/d/optout.

Alison Schofield

2016-09-28 17:34 UTC #7 lore raw

Quoted 18 lines · Anchal Jain

Quoted 6 lines
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.
Quoted 8 lines
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.

Quoted 9 lines

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

Quoted 69 lines

Quoted 66 lines · Alison Schofield
Quoted 16 lines · Alison Schofield
Quoted 4 lines · Anchal Jain
Fix the checkpatch.pl issues:
CHECK: struct mutes definition without comment
Signed-off-by Anchal Jain <[email protected]>
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> */
Quoted 6 lines
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
Quoted 5 lines
Review
Quoted 5 lines
---
 drivers/staging/iio/cdc/ad7150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/staging/iio/cdc/ad7150.c
Quoted 11 lines
Quoted 11 lines
index f6b9a10..88c6ebe 100644
drivers/staging/iio/cdc/ad7150.c +1 -1
@@ -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*/
 };
 
Quoted 6 lines
Quoted 6 lines
drivers/staging/iio/cdc/ad7150.c +0 -1
 /*
--
1.9.1
--
You received this message because you are subscribed to the Google
Groups "outreachy-kernel" group.
Quoted 1 line
Quoted 1 line
To unsubscribe from this group and stop receiving emails from it, send
an email to [email protected].
Quoted 1 line
Quoted 1 line
To post to this group, send email to [email protected]
.
Quoted 1 line
Quoted 1 line
To view this discussion on the web visit https://groups.google.com/d/
msgid/outreachy-kernel/20160927104504.GA13155%40life-desktop.
Quoted 1 line
Quoted 1 line
For more options, visit https://groups.google.com/d/optout.