staging: iio: cdc: ad7150: add comment to mutex
Subsystems (4)
PATCH v4 1/1 staging: iio: cdc: ad7150: add comment to mutex
Add a comment in mutex to explain what the mutex is work here.
--- drivers/staging/iio/cdc/ad7150.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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;/* protect hardware state */ };/*
Signature · 1 line
1.9.1
Greg KH
maintainerQuoted 12 lines · Anchal Jain
Add a comment in mutex to explain what the mutex is work here.
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
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;/* protect hardware state */
I think I need to just not take any of these types of patches, as it's not exactly a good thing for someone who does not understand the driver to be able to fix up.
Just ignore these warnings for now please.
thanks,
greg k-h
Quoted 5 lines
I think I need to just not take any of these types of patches, as it's not exactly a good thing for someone who does not understand the driver to be able to fix up.
Just ignore these warnings for now please
I am justing trying to understand your drivers. It will helpful me learn how it works
So, please I am requesting to you. Let me try. If it is not correct then don't accept it.
Thank you,
Quoted 30 lines · Greg KH
drivers/staging/iio/cdc/
Quoted 8 lines · Anchal Jain
Add a comment in mutex to explain what the mutex is work here.
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.cdrivers/staging/iio/cdc/ad7150.c struct ad7150_chip_info { · line 81
Quoted 9 lines
index f6b9a10..13709bf 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;/* protect hardware state */ I think I need to just not take any of these types of patches, as it's not exactly a good thing for someone who does not understand the driver to be able to fix up.
Just ignore these warnings for now please.
thanks,
greg k-h
Quoted 10 lines · Anchal Jain
Quoted 5 lines
I think I need to just not take any of these types of patches, as it's not exactly a good thing for someone who does not understand the driver to be able to fix up.
Just ignore these warnings for now please
I am justing trying to understand your drivers. It will helpful me learn how it works
So, please I am requesting to you. Let me try. If it is not correct then don't accept it.
If you want to submit such patches, you need to look carefully at how the lock is used in the code. In this case, the code between the lock and the unlock is rather complex. In general, it is rather difficult to understand why a lock is needed in a certain place, which is why the comment is suggested in the first place. But it is more targeting the person who originally wrote the driver than somone who is doing code cleanups. Then you need also to look at other drivers that already have such a comment to see what kind of information should be provided.
julia
Quoted 48 lines
Thank you,
On Sun, Oct 2, 2016 at 9:10 PM, Greg KH <[email protected]> wrote:On Sun, Oct 02, 2016 at 09:04:43PM +0530, Anchal Jain wrote: > Add a comment in mutex to explain what the mutex is work here. > > 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..13709bf 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; /* protect hardware state */ I think I need to just not take any of these types of patches, as it's not exactly a good thing for someone who does not understand the driver to be able to fix up. Just ignore these warnings for now please. thanks, 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/CAAbeOSuPu599ySH5Mm%2BV B3xx-j0UX25%2BZtb2qVbqh8qnErFCKg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Quoted 10 lines
If you want to submit such patches, you need to look carefully at how the lock is used in the code. In this case, the code between the lock and the unlock is rather complex. In general, it is rather difficult to understand why a lock is needed in a certain place, which is why the comment is suggested in the first place. But it is more targeting the person who originally wrote the driver than somone who is doing code cleanups. Then you need also to look at other drivers that already have such a comment to see what kind of information should be provided.
julia
thank you :-)
Quoted 76 lines · Julia Lawall
Quoted 10 lines · Anchal Jain
Quoted 5 lines
I think I need to just not take any of these types of patches, as it's not exactly a good thing for someone who does not understand the driver to be able to fix up.
Just ignore these warnings for now please
I am justing trying to understand your drivers. It will helpful me learn how it works
So, please I am requesting to you. Let me try. If it is not correct then don't accept it.If you want to submit such patches, you need to look carefully at how the lock is used in the code. In this case, the code between the lock and the unlock is rather complex. In general, it is rather difficult to understand why a lock is needed in a certain place, which is why the comment is suggested in the first place. But it is more targeting the person who originally wrote the driver than somone who is doing code cleanups. Then you need also to look at other drivers that already have such a comment to see what kind of information should be provided.
julia
Quoted 3 lines
Thank you,
On Sun, Oct 2, 2016 at 9:10 PM, Greg KH <[email protected]>Quoted 34 lines · wrote:
On Sun, Oct 02, 2016 at 09:04:43PM +0530, Anchal Jain wrote: > Add a comment in mutex to explain what the mutex is work here. > > 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..13709bf 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; /* protect hardware state */ I think I need to just not take any of these types of patches, as it's not exactly a good thing for someone who does not understand the driver to be able to fix up. Just ignore these warnings for now please. thanks, greg k-hQuoted 7 lines
--
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/CAAbeOSuPu599ySH5Mm%2BV
Quoted 4 lines
B3xx-j0UX25%2BZtb2qVbqh8qnErFCKg%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
Anchal,
I'm seeing that Greg doesn't have confidence in the comment added to the mutex. So, let's build his confidence. That would mean elaborating in the changelog a 'why' that shows that you did your due diligence, studied where the lock is used and came up with this comment.
Having said all that, in this case, and in another one you did or attempted, a comment was already present, just not in the place that checkpatch wants it.
So, a 'cheat' on this one would be to simply state that in the changelog. ie. Copied the comment from the structure header to the structure body to appease checkpatch.
BUT - rather than taking the cheat alone, follow up with a study of the code so that you can make a meaningful comment in the changelog. Now, if your study matches the comment - you know you understand it. You do a few of those, then you move onto ones that don't have the comment to compare.
The comment is important, because someone changing the code is going to wonder if they need to take that lock and look at the comment. If there is no comment, there going to go off and figure it out. If there is a comment, they may rely on the comment. We don't want to lead them astray.
I think on this one, you did all the above and I probably led you astray by suggesting a simplification of your changelog.
If you want to try again, I'm game!
alisons
Anchal,
I got through the rest of the email and see that Jonathan, IIO maintainer, may have little/no interest in picking these kind of changes up. It's time to look for other cleanups.
alisons