On 12/17/21 5:34 PM, Mark Greer wrote:
On Fri, Dec 17, 2021 at 11:34:08AM -0300, Jorge Eduardo Fermino Oliveia Silva wrote:
[Note: I am traveling for the next week so I won't be very responsive.]
Hi Jorge.
Before we get to the platch please remember that you should send all Greybus patches to greybus-dev@lists.linaro.org and linux-kernel@vger.kernel.org. I will add them in now and leave all of the context so other can see what you sent.
Thanks for copying the list, Mark. I concur with your response.
Jorge, this patch is not acceptable, but I have some suggestions. Your change is very minor (and not technically necessary) but if you want to try a version 2, we can reconsider it.
First: Your subject line is not proper. Patch subjects should begin with keywords that identify what the patch affects. If you run this command: git log --oneline drivers/staging/greybus/audio_manager_private.h you will see examples of commits that affect this file.
Based on that, the header for your patch should be something like: staging: greybus: audio: fix a checkpatch complaint But I don't actually know why you are suggesting this change, and that brings me to the second point.
Your patch description should be more complete. Your one line description says "Solve CHECK: ..." but it doesn't give much context about that. Maybe that shows up in a build? I don't know. Your description might be more like: When running "checkpatch.pl" we get this warning: Lines should not end with a '(' Fix this by re-formatting the line in question.
But again, I don't actually know where you are seeing this message. Ideally, your description should be sufficient for someone to be able to reproduce the problem you're fixing, and then verify that your fix makes the problem go away.
Third, what Mark points out is absolutely correct, which is that you are "fixing" one formatting problem but creating a new one. There is no great solution here, because some of the symbol/type names are very long. I have two possible suggestions though: - Leave it as-is, and accept that the line ends in '(' - Re-format this way, so the warning goes away, but the result at least has consistent indentation (even if it isn't aligned with the open parenthesis:
int gb_audio_manager_module_create(struct gb_audio_manager_module **module,
struct kset *manager_kset, int id,
struct gb_audio_manager_module_descriptor *desc);
Finally, if you submit version 2 of a patch, be sure your subject line is clear about that, with "[PATCH v2] staging: greybus: ...".
I'll leave it up to you to decide whether to send version 2. Note that someone else might reject your patch (even if you do what I suggest above). Some people dislike patches which make minor and unnecessary changes to the code, because of the "churn" effect they have.
-Alex
Solve CHECK: Lines should not end with a '('
Reported-by: Jorge Eduardo Fermino Oliveia Silva jorgeubermensch@gmail.com Signed-off-by: Jorge Eduardo Fermino Oliveia Silva jorgeubermensch@gmail.com
drivers/staging/greybus/audio_manager_private.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/audio_manager_private.h b/drivers/staging/greybus/audio_manager_private.h index 2b3a766c7de7..a17f09a19014 100644 --- a/drivers/staging/greybus/audio_manager_private.h +++ b/drivers/staging/greybus/audio_manager_private.h @@ -12,10 +12,10 @@ #include "audio_manager.h" -int gb_audio_manager_module_create(
- struct gb_audio_manager_module **module,
- struct kset *manager_kset,
- int id, struct gb_audio_manager_module_descriptor *desc);
+int gb_audio_manager_module_create(struct gb_audio_manager_module **module,
struct kset *manager_kset,int id,struct gb_audio_manager_module_descriptor *desc);/* module destroyed via kobject_put */
The part you're removing has all of the parameters at the same indentation level and what you adding has them at two different indentation levels so I'm not sure this is a step forward. Since the kernel coding style doesn't address this specific case, AFAICS, I would leave it as is despite the complaint. If others disagree then go ahead as I really don't care much either way.
Mark
greybus-dev mailing list greybus-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/greybus-dev