On Thu, Aug 04, 2016 at 10:36:32AM -0700, John Stultz wrote: > On Thu, Aug 4, 2016 at 6:08 AM, Vinod Koul <vinod.k...@intel.com> wrote: > > On Fri, Jul 29, 2016 at 03:40:46PM -0700, John Stultz wrote: > >> On Sun, Jul 24, 2016 at 12:25 AM, Vinod Koul <vinod.k...@intel.com> wrote: > >> > On Wed, Jul 20, 2016 at 08:53:03PM -0700, John Stultz wrote: > >> >> From: Andy Green <andy.gr...@linaro.org> > >> >> > >> >> Max burst len is a 4-bit field, but at the moment it's clipped with > >> >> a 5-bit constant... reduce it to that which can be expressed > >> > > >> > Maybe we should GENMASK() etc to avoid these errors.. > >> > >> Not sure I follow what you're thinking here... can you clarify a bit? > > > > I am assuming the 4bit field was a mistake by orignal author. Using GENMASK, > > BIT etc macro helps you to avoid those as one would look at datasheet and > > say this is specfied as bit 5 thru 9, so let me say GENMASK(5, 0) rather > > than a typo which missed 5th bit. > > > > But ofcourse if the error was due to some other reason then this one doesnt > > help. > > It was more that in taking the cfg->src/dst_maxburst value and setting > it as a value that can be represented in 4 bits for the hardware: > if ((maxburst == 0) || (maxburst > 16)) > val = 16; > else > val = maxburst - 1; > > However, the logic error is trying to set the value to 16 if maxburst > is larger then 16, when 16 is 5 bits, when 15 is the largest value we > can express in 4 bits. > > So I'm not sure how GENMASK() would be particularly useful (I guess > using val = GENMASK(4,0) instead of val = 15?). But I'm not sure that > makes the code any clearer.
Looks like this was latter case, so it doesnt seem to help here -- ~Vinod
