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.