<div dir="ltr"><div dir="ltr">On Fri, 12 Jul 2019 at 00:42, Mark Menzynski <<a href="mailto:mmenzyns@redhat.com" target="_blank">mmenzyns@redhat.com</a>> wrote:</div><div dir="ltr"><br></div><div>Hello Mark,</div><div><br></div><div>Thank you for your first kernel patch series to nouveau!</div><div>This is a good starting point. However, I would suggest a couple of general improvements which apply to the whole series:</div><div><br></div><div>1. Invest a little bit more time in writing the commit messages.</div><div><br></div><div>There is a slightly subjective element to what makes a "good" commit message, but generally the principle is that it should concisely</div><div>convey the "what" of the patch, but more importantly the "why" of the patch.</div><div><br></div><div>A future reader (which might be yourself!) can understand *what* the patch does from the code, to a lesser or so amount of time.</div><div>The *why* of a patch really can only be communicated within the commit message, so a little extra time spent here is well spent.<br></div><div><br></div><div>A helpful approach I suggest is trying to follow the style of the folder, submodule or driver:</div><div><br></div><div> $ git log -n20 drivers/gpu/drm/nouveau/</div><div><br></div><div>There's also a web resource here: <a href="https://chris.beams.io/posts/git-commit/" target="_blank">https://chris.beams.io/posts/git-commit/</a> (although it's not Linux kernel specific)<br></div><div><br></div><div>2. Run ./scripts/<a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a> on the final patch series<br></div><div dir="ltr"><br></div><div>You may have already done this, as I noticed the current patches report no errors or warnings. If so, well done!<br></div><div><br></div><div>It's a good habit to re-run this script against the final v2 patch series again though.</div><div><br></div><div>You can also ask questions on freenode #nouveau where many of the nouveau graphics stack driver developers hang out.<br></div><div><br></div><div>Rhys<br></div><div dir="ltr"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">trello card:<br> <a href="https://trello.com/c/admzDRvd/50-reduce-performance-refuse-to-boot-if-not-all-the-power-connectors-are-plugged" rel="noreferrer" target="_blank">https://trello.com/c/admzDRvd/50-reduce-performance-refuse-to-boot-if-not-all-the-power-connectors-are-plugged</a><br> <br> Mark Menzynski (4):<br> moved gpio so it is sorted by values<br> gpio: checking if gpu power cable is connected<br> gpio: added power check for another GPIO<br> gpio: added power check for another GPIO<br></blockquote><div><br></div><div>I'd recommend adding to the first line of the commit message of these two patches something which distinguishes them</div><div>apart. e.g. it appears you've identified that there are different gpio pin functions for different nvidia gpu families, so perhaps</div><div>use the pre- / and post- family names to separate the two patches for a reader.</div><div><br></div><div>Also, take a look at how the prefix before the ":" is usually written in this area of the code:</div><div><br></div><div> $ git log -n10 --oneline drivers/gpu/drm/nouveau/<relevant subfolders><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> drm/nouveau/include/nvkm/subdev/bios/gpio.h | 5 ++++-<br> drm/nouveau/nvkm/subdev/gpio/base.c | 25 +++++++++++++++++++++<br> 2 files changed, 29 insertions(+), 1 deletion(-)<br> <br> -- <br> 2.21.0<br> <br> _______________________________________________<br> Nouveau mailing list<br> <a href="mailto:Nouveau@lists.freedesktop.org" target="_blank">Nouveau@lists.freedesktop.org</a><br> <a href="https://lists.freedesktop.org/mailman/listinfo/nouveau" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/nouveau</a></blockquote></div></div>