staging: greybus: fix warnings reported by checkpatch

PATCH Superseded Jaehee Park 4 participants 10 messages Apr 12, 2022 – Apr 13, 2022

Subsystems (4)

Staging Greybus Greybus audio protocols drivers Greybus platform drivers

lore

Share & tools

lore

Jaehee Park

author

2022-04-12 19:59 UTC #0 lore raw

v2 cover staging: greybus: fix warnings reported by checkpatch

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

Signature · 1 line

2.25.1

Jaehee Park

author

2022-04-12 19:59 UTC #1 lore raw

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

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

Signed-off-by Jaehee Park <[email protected]>

---
 drivers/staging/greybus/arche-apb-ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/greybus/arche-apb-ctrl.c +1 -1

index bbf3ba744fc4..45afa208d004 100644
@@ -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.
 	 *

Signature · 1 line

2.25.1

Alex Elder

Reviewed-by on v2 1/2

2022-04-12 20:39 UTC #2 lore raw

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

Quoted 4 lines · Jaehee Park

Correct a spelling typo from 'Atleast' to 'At least' in comment. Issue found by checkpatch.
Signed-off-by Jaehee Park <[email protected]>

Thanks for updating the subject and description.

Looks good to me.

Reviewed-by Alex Elder <[email protected]>

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

Quoted 17 lines

---
  drivers/staging/greybus/arche-apb-ctrl.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/greybus/arche-apb-ctrl.c +1 -1

index bbf3ba744fc4..45afa208d004 100644
@@ -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.
  	 *

Jaehee Park

author

2022-04-12 19:59 UTC #3 lore raw

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

In reply to Jaehee Park

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

Signed-off-by Jaehee Park <[email protected]>

---
 drivers/staging/greybus/audio_codec.c | 1 -
 1 file changed, 1 deletion(-)

drivers/staging/greybus/audio_codec.c +0 -1

index 0f50d1e51e2c..3e3a16568def 100644
@@ -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,

Signature · 1 line

2.25.1

Alex Elder

2022-04-12 20:35 UTC #4 lore raw

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

Quoted 4 lines · Jaehee Park

An empty function with void return type does not need an explicit return. Issue found by checkpatch.
Signed-off-by Jaehee Park <[email protected]>

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

Quoted 16 lines

---
  drivers/staging/greybus/audio_codec.c | 1 -
  1 file changed, 1 deletion(-)

drivers/staging/greybus/audio_codec.c +0 -1

index 0f50d1e51e2c..3e3a16568def 100644
@@ -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,

Jaehee Park

author

2022-04-13 04:43 UTC #5 lore raw

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

Quoted 21 lines · Alex Elder

Quoted 4 lines · Jaehee Park
An empty function with void return type does not need an explicit return. Issue found by checkpatch.
Signed-off-by Jaehee Park <[email protected]>
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.

Quoted 6 lines

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

Quoted 17 lines

Quoted 16 lines

---
  drivers/staging/greybus/audio_codec.c | 1 -
  1 file changed, 1 deletion(-)

drivers/staging/greybus/audio_codec.c +0 -1

index 0f50d1e51e2c..3e3a16568def 100644
@@ -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,

Fabio M. De Francesco

2022-04-12 20:36 UTC #6 lore raw

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

In reply to Jaehee Park

drivers/staging/

Quoted 9 lines · 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 <[email protected]>
---
 drivers/staging/greybus/audio_codec.c | 1 -
 1 file changed, 1 deletion(-)
drivers/staging/ renamed from drivers/staging/greybus/audio_codec.c +0 -0

greybus/audio_codec.c

drivers/staging/greybus/audio_codec.c static int gbcodec_probe(struct snd_soc_component

Quoted 4 lines

index 0f50d1e51e2c..3e3a16568def 100644
drivers/staging/greybus/audio_codec.c +0 -0
@@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component 

*comp)

drivers/staging/greybus/audio_codec.c static int gbcodec_probe(struct snd_soc_component *comp)

Quoted 7 lines

drivers/staging/greybus/audio_codec.c +0 -1

 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,

Quoted 3 lines

Signature · 1 line
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

Jaehee Park

author

2022-04-13 04:44 UTC #7 lore raw

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

Quoted 32 lines · Fabio M. De Francesco

drivers/staging/
Quoted 9 lines · 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 <[email protected]>
---
 drivers/staging/greybus/audio_codec.c | 1 -
 1 file changed, 1 deletion(-)
drivers/staging/ renamed from drivers/staging/greybus/audio_codec.c +0 -0
greybus/audio_codec.c
drivers/staging/greybus/audio_codec.c static int gbcodec_probe(struct snd_soc_component
Quoted 4 lines
index 0f50d1e51e2c..3e3a16568def 100644
drivers/staging/greybus/audio_codec.c +0 -0
@@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component 
*comp)
drivers/staging/greybus/audio_codec.c static int gbcodec_probe(struct snd_soc_component *comp)
Quoted 7 lines
drivers/staging/greybus/audio_codec.c +0 -1
 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,
Quoted 3 lines
Signature · 1 line
2.25.1
Hi Jaehee,
If I recall it correctly, Dan Carpenter suggested to remove this empty function.

Quoted 10 lines

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!

Julia Lawall

2022-04-13 06:16 UTC #8 lore raw

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

In reply to Fabio M. De Francesco

Quoted 27 lines · Fabio M. De Francesco

drivers/staging/
Quoted 9 lines · 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 <[email protected]>
---
 drivers/staging/greybus/audio_codec.c | 1 -
 1 file changed, 1 deletion(-)
drivers/staging/ renamed from drivers/staging/greybus/audio_codec.c +0 -0
greybus/audio_codec.c
drivers/staging/greybus/audio_codec.c static int gbcodec_probe(struct snd_soc_component
Quoted 4 lines
index 0f50d1e51e2c..3e3a16568def 100644
drivers/staging/greybus/audio_codec.c +0 -0
@@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct snd_soc_component
*comp)
drivers/staging/greybus/audio_codec.c static int gbcodec_probe(struct snd_soc_component *comp)
Quoted 7 lines
drivers/staging/greybus/audio_codec.c +0 -1
 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,
Quoted 3 lines
--
2.25.1

Quoted 8 lines

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

Fabio M. De Francesco

2022-04-13 06:36 UTC #9 lore raw

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

Quoted 12 lines · On mercoledì 13 aprile 2022 08:16:20 CEST Julia Lawall wrote:

Quoted 9 lines · Fabio M. De Francesco
Quoted 8 lines · 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 <[email protected]>
---
 drivers/staging/greybus/audio_codec.c | 1 -
 1 file changed, 1 deletion(-)

Quoted 6 lines

Quoted 6 lines
drivers/staging/
Quoted 1 line
drivers/staging/ renamed from drivers/staging/greybus/audio_codec.c +0 -0
greybus/audio_codec.c
drivers/staging/greybus/audio_codec.c static int gbcodec_probe(struct
Quoted 4 lines
index 0f50d1e51e2c..3e3a16568def 100644
drivers/staging/greybus/audio_codec.c +0 -0
@@ -1032,7 +1032,6 @@ static int gbcodec_probe(struct 

snd_soc_component

Quoted 8 lines

Quoted 8 lines
*comp)
drivers/staging/greybus/audio_codec.c static int gbcodec_probe(struct snd_soc_component *comp)
Quoted 7 lines
drivers/staging/greybus/audio_codec.c +0 -1
 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

Quoted 10 lines

Quoted 10 lines
reg,
Quoted 3 lines
--
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

Quoted 4 lines

Quoted 2 lines
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 :(

Quoted 3 lines

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

Quoted 1 line

julia