From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756899AbbIXQdS (ORCPT ); Thu, 24 Sep 2015 12:33:18 -0400 Received: from mail.skyhub.de ([78.46.96.112]:56097 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752372AbbIXQdR (ORCPT ); Thu, 24 Sep 2015 12:33:17 -0400 Date: Thu, 24 Sep 2015 18:33:14 +0200 From: Borislav Petkov To: Aravind Gopalakrishnan Cc: mchehab@osg.samsung.com, dougthompson@xmission.com, linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3] EDAC, amd64_edac: Extend scrub rate programmability feature for F15hM60h Message-ID: <20150924163314.GE3774@pd.tnic> References: <1442436811-23382-1-git-send-email-Aravind.Gopalakrishnan@amd.com> <1442436811-23382-3-git-send-email-Aravind.Gopalakrishnan@amd.com> <20150924091841.GA3457@pd.tnic> <5604218C.7020905@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5604218C.7020905@amd.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 24, 2015 at 11:15:08AM -0500, Aravind Gopalakrishnan wrote: > I was thinking it's a little better from readability POV to separate out the > for loop which does the job of finding the scrub value to program; > And __set_scrub_rate() writes the value to the appropriate register. > > Maybe I am making it too modular? Yeah, you are. The function is perfectly readable the way it is, it even fits on the half of my screen :) Also, it didn't really make things better - it added that *scrub_bw argument which is a second output argument. AFAIR, it even used to be like that at some point in time... And that's ugly - I much prefer having input arguments being input only and return values being return values only. If it can be helped, that is. And in this case, it is not necessary. > I realize it's unrelated to the patch and it's not doing something useful; > But I was thinking I'll fix the above indentation issues to keep indent > rules consistent and since I was touching the file anyway. > Can remove those in a V2.. Yeah, I believe we've talked about this before: a patch should do one logical change and one logical change *only* - no other unrelated hunks belong in it. Otherwise, they make reviewing it harder by making me wonder why is that hunk there and what does it have to do with the scrub rate changes. Unrelated hunks can - further down the road - complicate bisection and cherry-picking for other kernels. So please try to restrain yourself to do solely the one logical change your patch addresses. If you feel that some more work needs to be done on the file or the whole driver, you can always send *separate* patches ontop which we can discuss. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.