Message-ID: <e71e6496-aecf-8dbc-632f-807cc686a567@linaro.org>
Date:   Mon, 20 Dec 2021 08:57:12 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [greybus-dev] [PATCH] Header line: Coding style fix
Content-Language: en-US
To:     Mark Greer <mgreer@animalcreek.com>,
        Jorge Eduardo Fermino Oliveia Silva 
        <jorge.ubermensch@gmail.com>
Cc:     greybus-dev@lists.linaro.org, linux-kernel@vger.kernel.org
References: <Jorgecommit> <20211217143408.10313-1-jorgeubermensch@gmail.com>
 <20211217233408.GA47062@animalcreek.com>
From:   Alex Elder <elder@linaro.org>
In-Reply-To: <20211217233408.GA47062@animalcreek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>