[PATCH v2 0/2] staging: greybus: fix warnings reported by checkpatch

[PATCH v2 0/2] staging: greybus: fix warnings reported by checkpatch

[Jaehee Park]

The first patch corrects a typo in a comment. The second patch removes
an unneeded return.

Changes in version 2:
- edited the subject to be more concise.
- edited the log message to be more descriptive.


Jaehee Park (2):
  staging: greybus: correct typo in comment
  staging: greybus: remove unneeded return

 drivers/staging/greybus/arche-apb-ctrl.c | 2 +-
 drivers/staging/greybus/audio_codec.c    | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] staging: greybus: correct typo in comment

[Jaehee Park]

Correct a spelling typo from 'Atleast' to 'At least' in comment.
Issue found by checkpatch.

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
 drivers/staging/greybus/arche-apb-ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
index bbf3ba744fc4..45afa208d004 100644
--- a/drivers/staging/greybus/arche-apb-ctrl.c
+++ b/drivers/staging/greybus/arche-apb-ctrl.c
@@ -445,7 +445,7 @@ static int __maybe_unused arche_apb_ctrl_suspend(struct device *dev)
 static int __maybe_unused arche_apb_ctrl_resume(struct device *dev)
 {
 	/*
-	 * Atleast for ES2 we have to meet the delay requirement between
+	 * At least for ES2 we have to meet the delay requirement between
 	 * unipro switch and AP bridge init, depending on whether bridge is in
 	 * OFF state or standby state.
 	 *
-- 
2.25.1

[PATCH v2 2/2] staging: greybus: remove unneeded return

[Jaehee Park]

An empty function with void return type does not need an explicit
return. Issue found by checkpatch.

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---
 drivers/staging/greybus/audio_codec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index 0f50d1e51e2c..3e3a16568def 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component *comp)
 static void gbcodec_remove(struct snd_soc_component *comp)
 {
 	/* Empty function for now */
-	return;
 }
 
 static int gbcodec_write(struct snd_soc_component *comp, unsigned int reg,
-- 
2.25.1

Re: [PATCH v2 2/2] staging: greybus: remove unneeded return

[Alex Elder]

On 4/12/22 2:59 PM, Jaehee Park wrote:
> An empty function with void return type does not need an explicit
> return. Issue found by checkpatch.
> 
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>

Dan's suggestion here was to simply remove this function
entirely.  It is only used as the ->remove callback
for the soc_codec_dev_gbaudio structure.

You can see that soc_codec_dev_gbaudio is only used in the
call to devm_snd_soc_register_component() near the end of
"audio_codec.c".  When a sound component is registered
that way, the ->remove callback is optional.  You can see
that because the only place in "sound/soc/soc-component.c"
that it is referenced is snd_soc_component_remove() (as
Dan said), and it only calls the function if it the pointer
is non-null.  Allowing null function pointers in places
like this. to allow them to be optionally omitted is not
an uncommon pattern you'll see in the kernel.

Anyway, please don't just add another small patch to remove
the function.  Just replace *this* patch with one that
removes the function, and omits the assignment if its
address to soc_codec_dev_gbaudio->remove.

					-Alex

> ---
>   drivers/staging/greybus/audio_codec.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> index 0f50d1e51e2c..3e3a16568def 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component *comp)
>   static void gbcodec_remove(struct snd_soc_component *comp)
>   {
>   	/* Empty function for now */
> -	return;
>   }
>   
>   static int gbcodec_write(struct snd_soc_component *comp, unsigned int reg,

Re: [PATCH v2 2/2] staging: greybus: remove unneeded return

[Fabio M. De Francesco]

On martedì 12 aprile 2022 21:59:15 CEST Jaehee Park wrote:
> An empty function with void return type does not need an explicit
> return. Issue found by checkpatch.
> 
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> ---
>  drivers/staging/greybus/audio_codec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/
greybus/audio_codec.c
> index 0f50d1e51e2c..3e3a16568def 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component 
*comp)
>  static void gbcodec_remove(struct snd_soc_component *comp)
>  {
>  	/* Empty function for now */
> -	return;
>  }
>  
>  static int gbcodec_write(struct snd_soc_component *comp, unsigned int 
reg,
> -- 
> 2.25.1
> 
Hi Jaehee,

If I recall it correctly, Dan Carpenter suggested to remove this empty 
function. 

When developers remove lines of code from a function which becomes empty
after the removals, they also remove the resulting empty function and
delete all the calls (if there are any left) at the same time.

Thanks,

Fabio M. De Francesco 

Re: [PATCH v2 1/2] staging: greybus: correct typo in comment

[Alex Elder]

On 4/12/22 2:59 PM, Jaehee Park wrote:
> Correct a spelling typo from 'Atleast' to 'At least' in comment.
> Issue found by checkpatch.
> 
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>

Thanks for updating the subject and description.

Looks good to me.

Reviewed-by: Alex Elder <elder@linaro.org>

(When you send version 3 of these patches, please include the
above line above your "Signed-off-by" line, to indicate I've
reviewed it.)

> ---
>   drivers/staging/greybus/arche-apb-ctrl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
> index bbf3ba744fc4..45afa208d004 100644
> --- a/drivers/staging/greybus/arche-apb-ctrl.c
> +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> @@ -445,7 +445,7 @@ static int __maybe_unused arche_apb_ctrl_suspend(struct device *dev)
>   static int __maybe_unused arche_apb_ctrl_resume(struct device *dev)
>   {
>   	/*
> -	 * Atleast for ES2 we have to meet the delay requirement between
> +	 * At least for ES2 we have to meet the delay requirement between
>   	 * unipro switch and AP bridge init, depending on whether bridge is in
>   	 * OFF state or standby state.
>   	 *

Re: [PATCH v2 2/2] staging: greybus: remove unneeded return

[Jaehee Park]

On Tue, Apr 12, 2022 at 03:35:04PM -0500, Alex Elder wrote:
> On 4/12/22 2:59 PM, Jaehee Park wrote:
> > An empty function with void return type does not need an explicit
> > return. Issue found by checkpatch.
> > 
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> 
> Dan's suggestion here was to simply remove this function
> entirely.  It is only used as the ->remove callback
> for the soc_codec_dev_gbaudio structure.
> 
> You can see that soc_codec_dev_gbaudio is only used in the
> call to devm_snd_soc_register_component() near the end of
> "audio_codec.c".  When a sound component is registered
> that way, the ->remove callback is optional.  You can see
> that because the only place in "sound/soc/soc-component.c"
> that it is referenced is snd_soc_component_remove() (as
> Dan said), and it only calls the function if it the pointer
> is non-null.  Allowing null function pointers in places
> like this. to allow them to be optionally omitted is not
> an uncommon pattern you'll see in the kernel.
> 
> Anyway, please don't just add another small patch to remove
> the function.  Just replace *this* patch with one that
> removes the function, and omits the assignment if its
> address to soc_codec_dev_gbaudio->remove.
> 
> 					-Alex

Hi Alex, Thank you for explaining where the functions are called from!
It makes a lot more sense now. I've used your explanation for my patch
log. Please let me know if I'm misunderstanding things. I've sent patch 
v3 for your review. The first patch (typo patch) already has your 
"review-by" -- thank you for the advice.
 
Thanks,
Jaehee
> 
> > ---
> >   drivers/staging/greybus/audio_codec.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> > index 0f50d1e51e2c..3e3a16568def 100644
> > --- a/drivers/staging/greybus/audio_codec.c
> > +++ b/drivers/staging/greybus/audio_codec.c
> > @@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component *comp)
> >   static void gbcodec_remove(struct snd_soc_component *comp)
> >   {
> >   	/* Empty function for now */
> > -	return;
> >   }
> >   static int gbcodec_write(struct snd_soc_component *comp, unsigned int reg,
> 

Re: [PATCH v2 2/2] staging: greybus: remove unneeded return

[Jaehee Park]

On Tue, Apr 12, 2022 at 10:35:58PM +0200, Fabio M. De Francesco wrote:
> On martedì 12 aprile 2022 21:59:15 CEST Jaehee Park wrote:
> > An empty function with void return type does not need an explicit
> > return. Issue found by checkpatch.
> > 
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > ---
> >  drivers/staging/greybus/audio_codec.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/
> greybus/audio_codec.c
> > index 0f50d1e51e2c..3e3a16568def 100644
> > --- a/drivers/staging/greybus/audio_codec.c
> > +++ b/drivers/staging/greybus/audio_codec.c
> > @@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component 
> *comp)
> >  static void gbcodec_remove(struct snd_soc_component *comp)
> >  {
> >  	/* Empty function for now */
> > -	return;
> >  }
> >  
> >  static int gbcodec_write(struct snd_soc_component *comp, unsigned int 
> reg,
> > -- 
> > 2.25.1
> > 
> Hi Jaehee,
> 
> If I recall it correctly, Dan Carpenter suggested to remove this empty 
> function. 
> 
> When developers remove lines of code from a function which becomes empty
> after the removals, they also remove the resulting empty function and
> delete all the calls (if there are any left) at the same time.
> 
> Thanks,
> 
> Fabio M. De Francesco 
> 
> 
>

Thanks Fabio for pointing this out!

Re: [PATCH v2 2/2] staging: greybus: remove unneeded return

[Julia Lawall]

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



On Tue, 12 Apr 2022, Fabio M. De Francesco wrote:

> On martedì 12 aprile 2022 21:59:15 CEST Jaehee Park wrote:
> > An empty function with void return type does not need an explicit
> > return. Issue found by checkpatch.
> >
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > ---
> >  drivers/staging/greybus/audio_codec.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/
> greybus/audio_codec.c
> > index 0f50d1e51e2c..3e3a16568def 100644
> > --- a/drivers/staging/greybus/audio_codec.c
> > +++ b/drivers/staging/greybus/audio_codec.c
> > @@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component
> *comp)
> >  static void gbcodec_remove(struct snd_soc_component *comp)
> >  {
> >  	/* Empty function for now */
> > -	return;
> >  }
> >
> >  static int gbcodec_write(struct snd_soc_component *comp, unsigned int
> reg,
> > --
> > 2.25.1
> >
> Hi Jaehee,
>
> If I recall it correctly, Dan Carpenter suggested to remove this empty
> function.
>
> When developers remove lines of code from a function which becomes empty
> after the removals, they also remove the resulting empty function and
> delete all the calls (if there are any left) at the same time.

It's probably not relevant in this case, but the function could be needed
if it is a branch of an ifdef.  Also if it is stored in a structure field
and the user of the structure does not check for NULL.

julia

Re: [PATCH v2 2/2] staging: greybus: remove unneeded return

[Fabio M. De Francesco]

On mercoledì 13 aprile 2022 08:16:20 CEST Julia Lawall wrote:
> 
> On Tue, 12 Apr 2022, Fabio M. De Francesco wrote:
> 
> > On martedì 12 aprile 2022 21:59:15 CEST Jaehee Park wrote:
> > > An empty function with void return type does not need an explicit
> > > return. Issue found by checkpatch.
> > >
> > > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > > ---
> > >  drivers/staging/greybus/audio_codec.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/
> > greybus/audio_codec.c
> > > index 0f50d1e51e2c..3e3a16568def 100644
> > > --- a/drivers/staging/greybus/audio_codec.c
> > > +++ b/drivers/staging/greybus/audio_codec.c
> > > @@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct 
snd_soc_component
> > *comp)
> > >  static void gbcodec_remove(struct snd_soc_component *comp)
> > >  {
> > >  	/* Empty function for now */
> > > -	return;
> > >  }
> > >
> > >  static int gbcodec_write(struct snd_soc_component *comp, unsigned 
int
> > reg,
> > > --
> > > 2.25.1
> > >
> > Hi Jaehee,
> >
> > If I recall it correctly, Dan Carpenter suggested to remove this empty
> > function.
> >
> > When developers remove lines of code from a function which becomes 
empty
> > after the removals, they also remove the resulting empty function and
> > delete all the calls (if there are any left) at the same time.
> 
> It's probably not relevant in this case, 

No, it's relevant :)
I should have been more exhaustive :(

> but the function could be needed
> if it is a branch of an ifdef.  Also if it is stored in a structure field
> and the user of the structure does not check for NULL.

Here we have one of special cases you've mentioned. The pointer to the 
function is stored in a structure field _but_ we know that the user does 
check for NULL.

Thanks,

Fabio

> julia