[PATCH] staging: greybus: add comment for mutex

[PATCH] staging: greybus: add comment for mutex

[Ayush Tiwari]

This patch adds descriptive comment to mutex within the struct
gbaudio_codec_info to clarify its intended use and to address
checkpatch checks.

Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com>
---
 drivers/staging/greybus/audio_codec.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
index f3f7a7ec6be4..1f97d4fb16cd 100644
--- a/drivers/staging/greybus/audio_codec.h
+++ b/drivers/staging/greybus/audio_codec.h
@@ -71,6 +71,7 @@ struct gbaudio_codec_info {
 	/* to maintain runtime stream params for each DAI */
 	struct list_head dai_list;
 	struct mutex lock;
+	/* Lock to protect register access */
 	struct mutex register_mutex;
 };
 
-- 
2.40.1

Re: [PATCH] staging: greybus: add comment for mutex

[Alison Schofield]

On Wed, Mar 13, 2024 at 11:51:22PM +0530, Ayush Tiwari wrote:
> This patch adds descriptive comment to mutex within the struct
> gbaudio_codec_info to clarify its intended use and to address
> checkpatch checks.

Hi Ayush-

You may be right, but you need to convince your patch reviewers
why your comment accurately describes this mutex.

That's always the ask with this kind of patch.

BTW - Don't start your commit log with 'This patch...'.

Alison

> 
> Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com>
> ---
>  drivers/staging/greybus/audio_codec.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> index f3f7a7ec6be4..1f97d4fb16cd 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -71,6 +71,7 @@ struct gbaudio_codec_info {
>  	/* to maintain runtime stream params for each DAI */
>  	struct list_head dai_list;
>  	struct mutex lock;
> +	/* Lock to protect register access */
>  	struct mutex register_mutex;
>  };
>  
> -- 
> 2.40.1
> 
> 

Re: [PATCH] staging: greybus: add comment for mutex

[Dan Carpenter]

On Wed, Mar 13, 2024 at 07:32:20PM -0700, Alison Schofield wrote:
> On Wed, Mar 13, 2024 at 11:51:22PM +0530, Ayush Tiwari wrote:
> > This patch adds descriptive comment to mutex within the struct
> > gbaudio_codec_info to clarify its intended use and to address
> > checkpatch checks.
> 
> Hi Ayush-
> 
> You may be right, but you need to convince your patch reviewers
> why your comment accurately describes this mutex.
> 
> That's always the ask with this kind of patch.

Heh.  Yeah.  The comment wasn't right in this case.  The lock has
nothing to do with registers or register access.

> 
> BTW - Don't start your commit log with 'This patch...'.
> 

Outreachy folk are a more particular about some of this stuff than I am.
Which is fine.  Could you do me a favor though?  Could you ack patches
once you're happy with them?

regards,
dan carpenter

Re: [PATCH] staging: greybus: add comment for mutex

[Johan Hovold]

On Wed, Mar 13, 2024 at 11:51:22PM +0530, Ayush Tiwari wrote:
> This patch adds descriptive comment to mutex within the struct
> gbaudio_codec_info to clarify its intended use and to address
> checkpatch checks.
> 
> Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com>
> ---
>  drivers/staging/greybus/audio_codec.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> index f3f7a7ec6be4..1f97d4fb16cd 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -71,6 +71,7 @@ struct gbaudio_codec_info {
>  	/* to maintain runtime stream params for each DAI */
>  	struct list_head dai_list;
>  	struct mutex lock;
> +	/* Lock to protect register access */
>	struct mutex register_mutex;

No, please, don't send these kind of patches.

It's one thing thing to learn how to create and send a patch upstream by
removing or adding some white space here and there to drivers in
staging, but the above is not the kind of changes that people unfamiliar
with the code in question should be adding (and the comment in this case
adds no value at all).

Johan

Re: [PATCH] staging: greybus: add comment for mutex

[Alison Schofield]

On Thu, Mar 14, 2024 at 10:37:29AM +0300, Dan Carpenter wrote:
> On Wed, Mar 13, 2024 at 07:32:20PM -0700, Alison Schofield wrote:
> > On Wed, Mar 13, 2024 at 11:51:22PM +0530, Ayush Tiwari wrote:
> > > This patch adds descriptive comment to mutex within the struct
> > > gbaudio_codec_info to clarify its intended use and to address
> > > checkpatch checks.
> > 
> > Hi Ayush-
> > 
> > You may be right, but you need to convince your patch reviewers
> > why your comment accurately describes this mutex.
> > 
> > That's always the ask with this kind of patch.
> 
> Heh.  Yeah.  The comment wasn't right in this case.  The lock has
> nothing to do with registers or register access.
> 
> > 
> > BTW - Don't start your commit log with 'This patch...'.
> > 
> 
> Outreachy folk are a more particular about some of this stuff than I am.
> Which is fine.  Could you do me a favor though?  Could you ack patches
> once you're happy with them?

Dan -
Sure. In practice, the patches are often 'Reviewed-by' someone else
by the time I get back around to it (thanks to the different time
zones and plentiful reviewers) But I understand, I'll make it a 
habit of at Ack'ing after I've asked for something to change.
Alison


> 
> regards,
> dan carpenter
>