[U-Boot] [PATCH V3 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

[Tom Rini <trini@ti.com>]

On Tue, Mar 25, 2014 at 05:54:10PM +0100, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <1395764855-23377-1-git-send-email-swarren@wwwdotorg.org> you wrote:
> > 
> > +static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
> > +					     u32 val)
> > +{
> > +	clrsetbits_le32(reg, mask << shift, val << shift);
> > +}
> 
> No, please do not do that.  Please use plain clrsetbits_le32() as is.
> All these hidden shifts are (a) mostly unreadable and (b) sometimes
> dangerous.

No, this is why the lack of comments hurts things.  This isn't sr32 from
OMAP-land (which was on my todo list somewhere, thanks).  sr32 was an
incorrect generic function.  This is a specific-use function that should
say something like:
/*
 * Set the correct pinmux value for a given part.  We need to clear out
 * M bits worth of the field and then set possibly less than M bits
 * worth of value.
 */

With respect to danger / readability, no, either way is just as
dangerous (or not dangerous) and it's still fairly dense code either
way and fixing a problem with an incorrect shift value is the same
effort.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140325/95b3eb9b/attachment.pgp>