From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Tue, 25 Mar 2014 14:00:32 -0600
Subject: [U-Boot] [PATCH V4 09/13] ARM: tegra: use clrsetbits_le32 in
 pinmux driver
In-Reply-To: <1395774584-17305-1-git-send-email-wd@denx.de>
References: <1395764855-23377-1-git-send-email-swarren@wwwdotorg.org>
	<1395774584-17305-1-git-send-email-wd@denx.de>
Message-ID: <5331E060.1020201@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 03/25/2014 01:09 PM, Wolfgang Denk wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> This removes a bunch of open-coded register IO, masking, and shifting.
> I would have squashed this into "ARM: tegra: pinctrl: remove duplication"
> except that keeping it a separate commit allows easier bisection of any
> issues that are introduced by this patch. I also wrote this patch on top
> of the series, and pushing it any lower in the series results in some
> conflicts I didn't feel like fixing.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> ---
> V4: [wd] Drop update_reg_mask_shift_val() and use clrsetbits_le32()
>     directly.

I believe your own argument compels you to NAK this patch yourself.

In both V3 and V4:

1) Some very simple and obviously understandable open-coded bit
manipulation is replaced with a function call which hides the details of
the bit manipulation.

2) In both cases, the order of parameters to the function does actually
appear to be obvious from the function name: clrsetbits_le32(clr, set),
update_reg_mask_shift_val(reg, mask, shift, val)

3) I've seen plenty of examples where the function name doesn't
correctly describe what the function does, or the parameter order
doesn't match what would appear to be logical. Hence, in neither case is
point (2) relevant; one must /always/ check or remember the prototype of
a function in order to validate that the parameters at a call-site are
correct. Not checking means making an assumption, and assumptions can be
incorrect.

Now, I believe you argued that it was unsafe to hide the details of the
bit manipulation behind a function precisely because it was then harder
to see what the code was doing, and easier to pass the wrong values to
the function parameters, and this wouldn't be obvious at the call site.

I believe the /exact/ same argument applies no matter whether the
function is clrsetbits_le32() or update_reg_mask_shift_val().

Hence, if V3 which used update_reg_mask_shift_val() was unacceptable,
then so is this.

So NAK.