From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 25 Mar 2014 16:04:20 -0400 Subject: [U-Boot] [PATCH V3 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver In-Reply-To: <20140325165410.8F1563814B8@gemini.denx.de> References: <1395764855-23377-1-git-send-email-swarren@wwwdotorg.org> <20140325165410.8F1563814B8@gemini.denx.de> Message-ID: <20140325200420.GU16360@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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: