Hi Stephen, On 25 March 2014 08:54, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 03/24/2014 08:27 PM, Simon Glass wrote: >> Hi Stephen, >> >> On 21 March 2014 11:28, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> From: Stephen Warren <swar...@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 <swar...@nvidia.com> >> >> Acked-by: Simon Glass <s...@chromium.org> >> >> 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. >> >>> +} >>> + >>> void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func) >>> { >>> u32 *reg = MUX_REG(pin); >>> int i, mux = -1; >>> - u32 val; >>> >>> /* Error check on pin and func */ >>> assert(pmux_pingrp_isvalid(pin)); >>> @@ -110,42 +114,29 @@ void pinmux_set_func(enum pmux_pingrp pin, enum >>> pmux_func func) >>> } >>> assert(mux != -1); >>> >>> - val = readl(reg); >>> - val &= ~(3 << MUX_SHIFT(pin)); >>> - val |= (mux << MUX_SHIFT(pin)); >>> - writel(val, reg); >>> + 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. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot