From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Tue, 25 Mar 2014 10:28:28 -0600 Subject: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver In-Reply-To: References: <1395426541-18706-1-git-send-email-swarren@wwwdotorg.org> <1395426541-18706-10-git-send-email-swarren@wwwdotorg.org> <5331A6B6.8090805@wwwdotorg.org> Message-ID: <5331AEAC.7010905@wwwdotorg.org> List-Id: 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 10:04 AM, Simon Glass wrote: > On 25 March 2014 08:54, Stephen Warren wrote: >> On 03/24/2014 08:27 PM, Simon Glass wrote: >>> On 21 March 2014 11:28, Stephen Warren wrote: >>>> 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 >>> >>> Acked-by: Simon Glass >>> >>> But see comment below. >> >>>> diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c b/arch/arm/cpu/tegra-common/pinmux-common.c >> >>>> +static inline void update_field(u32 *reg, u32 mask, u32 shift, u32 val) >>>> +{ >>>> + clrsetbits_le32(reg, mask << shift, val << shift); >>> >>> I wonder if it would be better to write this out explicitly in each site. ... >>>> + update_field(reg, 3, MUX_SHIFT(pin), mux); >>> >>> Because here you are obscuring the shift - the parameter order is by >>> no means obvious. >> >> Well, for pretty much no function is it obvious from the function name >> what the parameter order is; it's just one of those things you memorize >> or look up. The exact same issue exists for clrsetbits_le32() itself IMHO. > > Well that at least indicates that the clr mask comes before set. > >> >>> Or perhaps update_reg_mask_shift_val()? >> >> Still, I can rename the function if you want; it certainly does make it >> obvious. It's rather a long name though, but I guess wrapping the >> parameters isn't too bad. > > I'll leave it to you, it's just a thought. Well, the wrapping didn't work out to badly, so I posted V3 including this rename.