Messages in this thread | | | From | Mathieu Poirier <> | Date | Fri, 22 Jun 2018 08:38:28 -0600 | Subject | Re: [PATCH 5/6] coresight: catu: Add support for scatter gather tables |
| |
On Fri, 22 Jun 2018 at 03:01, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > > Hi Mathieu, > > On 21/06/18 18:13, Mathieu Poirier wrote: > > >> + > >> +/* > >> + * catu_populate_table : Populate the given CATU table. > >> + * The table is always populated as a circular table. > >> + * i.e, the "prev" link of the "first" table points to the "last" > >> + * table and the "next" link of the "last" table points to the > >> + * "first" table. The buffer should be made linear by calling > >> + * catu_set_table(). > >> + */ > >> +static void > >> +catu_populate_table(struct tmc_sg_table *catu_table) > >> +{ > >> + int i, dpidx, s_dpidx; > >> + unsigned long offset, buf_size, last_offset; > >> + dma_addr_t data_daddr; > >> + dma_addr_t prev_taddr, next_taddr, cur_taddr; > >> + cate_t *table_ptr, *next_table; > >> + > >> + buf_size = tmc_sg_table_buf_size(catu_table); > >> + dpidx = s_dpidx = 0; > > > > From the reading the code below variable s_dpidx stands for "small" data page > > index, which isn't obvious from the get go and could easily be mistaken for > > "system" data page index. Please add a comment to make your intentions clear. > > Sure, I will add comments. They are supposed to mean : > > dpidx => Data page index > s_dpidx => Sub-page index within the System page. > > May be I can rename the variables to => > > sys_pidx and catu_pidx
Perfect, this leaves no place for interpretation.
Thanks for being considerate.
> > > > > >> + offset = 0; > >> + > >> + table_ptr = catu_get_table(catu_table, 0, &cur_taddr); > >> + prev_taddr = 0; /* Prev link for the first table */ > >> + > >> + while (offset < buf_size) { > >> + /* > >> + * The @offset is always 1M aligned here and we have an > >> + * empty table @table_ptr to fill. Each table can address > >> + * upto 1MB data buffer. The last table may have fewer > >> + * entries if the buffer size is not aligned. > >> + */ > >> + last_offset = (offset + SZ_1M) < buf_size ? > >> + (offset + SZ_1M) : buf_size; > >> + for (i = 0; offset < last_offset; > >> + i++, offset += CATU_PAGE_SIZE) { > > > > I really like the choice of "table_end" in function catu_dump_table(). I think > > using the same denomination here would make it easier to understand the code. > > > > I wouldn't bother with such details if you weren't respinning this set. But now > > that you are and these are extremely simple I think it's worth it, and it will > > help slowing the prolifiration of gray hair around my head when I look back at > > this a year or two down the road. > > Sure, I can make those changes in the next revision. Thanks for the review. > > Cheers > Suzuki
| |