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

[Alison Schofield <amsfield22@gmail.com>]

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