From: Sebastian Frias <sf84@laposte•net>
To: "Måns Rullgård" <mans@mansr•com>
Cc: "David S. Miller" <davem@davemloft•net>,
netdev@vger•kernel.org, LKML <linux-kernel@vger•kernel.org>,
mason <slash.tmp@free•fr>
Subject: Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Date: Mon, 08 Feb 2016 15:32:21 +0100 [thread overview]
Message-ID: <56B8A6F5.9060409@laposte.net> (raw)
In-Reply-To: <yw1xfux3fhmq.fsf@unicorn.mansr.com>
On 02/08/2016 02:37 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@laposte•net> writes:
>
>> On 02/05/2016 04:26 PM, Måns Rullgård wrote:
>>> Sebastian Frias <sf84@laposte•net> writes:
>>>
>>>> On 02/05/2016 04:08 PM, Måns Rullgård wrote:
>>>>> Sebastian Frias <sf84@laposte•net> writes:
>>>>>
>>>>>> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>>>>>>> Sebastian Frias <sf84@laposte•net> writes:
>>>>>>>
>>>>>>>> Signed-off-by: Sebastian Frias <sf84@laposte•net>
>>>>>>>
>>>>>>> Please change the subject to something like "net: ethernet: nb8800:
>>>>>>> support fixed-link DT node" and add a comment body.
>>>>>>
>>>>>> The subject is pretty explicit for such a simple patch, what else
>>>>>> could I add that wouldn't be unnecessary chat?
>>>>>
>>>>> It's customary to include a description body even if it's little more
>>>>> than a restatement of the subject. Also, while the subject usually only
>>>>> says _what_ the patch does, the body should additionally state _why_ it
>>>>> is needed.
>>>>
>>>> I understand, but _why_ it is needed is also obvious in this case; I
>>>> mean, without the patch "fixed-link" cannot be used.
>>>
>>> Then say so.
>>>
>>>> Other patches may not be as obvious/simple and thus justify and
>>>> require more details.
>>>>
>>>> Anyway, I added "Properly handles the case where the PHY is not connected
>>>> to the real MDIO bus" would that be ok?
>>>
>>> Have you read Documentation/SubmittingPatches? Do so (again) and pay
>>> special attention to section 2 "Describe your changes."
>>
>> I just sent v5.
>
> Thanks for your patience.
:-)
>
>> If for whatever reason, you or anybody else think that the comment is
>> not good, would you mind proposing a comment that would make everybody
>> happy so that the patch goes thru?
>> And if you or anybody else does not want the patch, could you please
>> say so as well?
>>
>> I have to admit this process (sending patches and getting it reviewed)
>> could benefit from more clarifications.
>> For example, the process could say that at least 2 reviewers must
>> agree on it (on the comments made to the patch and on the patch
>> itself).
>> I could also say that reviewers are to express not only their opinion
>> but to clearly and unequivocally accept or reject.
>>
>> For instance, right now, it is not clear to me if your comments are
>> "nice to have" or "blocking" the patch.
>> I don't know if the patch is welcome or not, etc.
>> So I submitted v5, but maybe it was not even necessary, it's hard to
>> know where in the submission process we are.
>
> In this case, it's ultimately up to Dave Miller. He'll take into
> account whatever comments others have made and decide whether he wants
> to accept it.
Ok, thanks.
>
>> By the way, I know some people like the command line, email, etc. but
>> there ought to be other tools better suited for patch review...
>
> Some kernel subsystems use http://patchwork.ozlabs.org/ to track status
> of various patches.
>
Thanks, I see that netdev is part of it, and that the patches are there:
https://patchwork.ozlabs.org/patch/580217/
seems like a slight layer over plain email and mailinglists; I was
thinking of something more in the line of https://www.gerritcodereview.com/
I believe Google uses Gerrit for Android.
I think Gerrit would probably be too big (and being written in Java,
using Prolog and other DSLs, implementing its own Git server in Java,
etc, may make some -or lots?- of kernel developers cry :-) )
However, in Gerrit it is easier to know where in the "review" process we
are, because people have to explicitly give a score "+/- X" when
commenting on a patch.
Also, the diff can operate between different versions of the patches
themselves to see if the inlined comments were addressed.
next prev parent reply other threads:[~2016-02-08 14:32 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-05 13:31 [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver Sebastian Frias
2016-02-05 13:39 ` Måns Rullgård
2016-02-05 13:49 ` [PATCH v2] net: ethernet: support "fixed-link" DT key/node " Sebastian Frias
2016-02-05 13:58 ` Måns Rullgård
2016-02-05 14:08 ` Sebastian Frias
2016-02-05 14:13 ` Måns Rullgård
2016-02-05 14:22 ` [PATCH v3] net: ethernet: support "fixed-link" DT node " Sebastian Frias
2016-02-05 14:34 ` Måns Rullgård
2016-02-05 14:56 ` Sebastian Frias
2016-02-05 15:08 ` Måns Rullgård
2016-02-05 15:20 ` Sebastian Frias
2016-02-05 15:26 ` Måns Rullgård
2016-02-08 10:23 ` [PATCH v5] net: ethernet: nb8800: support fixed-link DT node Sebastian Frias
2016-02-08 13:19 ` Måns Rullgård
2016-02-16 20:04 ` David Miller
2016-02-22 12:39 ` Mason
2016-02-08 10:34 ` [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver Sebastian Frias
2016-02-08 13:37 ` Måns Rullgård
2016-02-08 14:11 ` Mason
2016-02-08 14:38 ` Sebastian Frias
2016-02-08 14:44 ` Måns Rullgård
2016-02-08 14:32 ` Sebastian Frias [this message]
2016-02-08 14:50 ` Måns Rullgård
2016-02-05 14:56 ` [PATCH v4] net: ethernet: nb8800: support fixed-link DT node Sebastian Frias
2016-02-05 15:57 ` [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver Andy Shevchenko
2016-02-05 15:58 ` Måns Rullgård
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56B8A6F5.9060409@laposte.net \
--to=sf84@laposte$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=mans@mansr$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=slash.tmp@free$(echo .)fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox