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

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

[Anchal Jain]

Add a comment in mutex to explain what the mutex is work here.

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..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 */
 };
 
 /*
-- 
1.9.1

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

[Greg KH]

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

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

[Anchal Jain]

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

>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,
On Sun, Oct 2, 2016 at 9:10 PM, Greg KH <gregkh@linuxfoundation.org> 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 <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..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
>

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

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

[Julia Lawall]

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



On Sun, 2 Oct 2016, Anchal Jain wrote:

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

>
> Thank you,
> On Sun, Oct 2, 2016 at 9:10 PM, Greg KH <gregkh@linuxfoundation.org> 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 <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..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 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/CAAbeOSuPu599ySH5Mm%2BV
> B3xx-j0UX25%2BZtb2qVbqh8qnErFCKg%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

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

[Anchal Jain]

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

>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 :-)

On Sun, Oct 2, 2016 at 9:23 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:

>
>
> On Sun, 2 Oct 2016, Anchal Jain wrote:
>
> > >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
>
> >
> > Thank you,
> > On Sun, Oct 2, 2016 at 9:10 PM, Greg KH <gregkh@linuxfoundation.org>
> 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 <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..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 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/CAAbeOSuPu599ySH5Mm%2BV
> > B3xx-j0UX25%2BZtb2qVbqh8qnErFCKg%40mail.gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >
> >

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

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

[Alison Schofield]

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

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

[Alison Schofield]

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