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.