On Mon, 1 Jun 2009, Robert Jarzmik wrote: > Trent Piepho <xyzzy@xxxxxxxxxxxxx> writes: > > Please pull from http://linuxtv.org/hg/~tap/v4l-dvb > > > > This series adds a function for bounding and alignment image sizes and > > modifies a number of drivers to use it. It came up when the pxa patches to > > deal with the alignment issues for that driver were posted. I haven't > > tested these patches for pxa. > > Hi Trent, > > I didn't see the review of that serie, I'm curious what others said. > As for my comments, I'll inline your code, sorry about that. > > > 02/14: v4l2: Create helper function for bounding and aligning images > > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d > > > +static unsigned int clamp_align(unsigned int x, unsigned int min, > + unsigned int max, unsigned int align) > +{ > + /* Bits that must be zero to be aligned */ > + unsigned int mask = ~((1 << align) - 1); > + > + /* Round to nearest aligned value */ > + if (align) > + x = (x + (1 << (align - 1))) & mask; > > If I'm not mistaken, these lines are an equivalent of : > balign = 1 << align; > if (align) > x = ALIGN(x + 1 - balign/2, balign); > > Isn't that simpler to read ? I don't think so, it's not obvious that it will round to the nearest value. You have to look up ALIGN and then __ALIGN_MASK and see that it will in effect add balign - 1 and then reduce x + 1 - balign/2 + balign - 1 into x + balign/2. It also generates worse code. You'd think the compiled would be able to optimize it into the same code, but it doesn't. Unless there is some issue with how it will work with negative values that causes a subtle difference. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
