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(-)
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. *
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.
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,
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,
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,
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
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!
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
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
-
Alex Elder -
Fabio M. De Francesco -
Jaehee Park -
Julia Lawall