lkml.org 
[lkml]   [2018]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2018-06-22 16:39    [W:0.070 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site