Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

[Sebastian Frias]

On 02/08/2016 02:37 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@xxxxxxxxxxx> writes:
>
> > On 02/05/2016 04:26 PM, Måns Rullgård wrote:
> > > Sebastian Frias <sf84@xxxxxxxxxxx> writes:
> > >
> > > > On 02/05/2016 04:08 PM, Måns Rullgård wrote:
> > > > > Sebastian Frias <sf84@xxxxxxxxxxx> writes:
> > > > >
> > > > > > On 02/05/2016 03:34 PM, Måns Rullgård wrote:
> > > > > > > Sebastian Frias <sf84@xxxxxxxxxxx> writes:
> > > > > > >
> > > > > > > > Signed-off-by: Sebastian Frias <sf84@xxxxxxxxxxx>
> > > > > > >
> > > > > > > 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.