On Fri, 22 Jun 2018 at 03:01, Suzuki K Poulose [off-list ref] wrote:

Hi Mathieu,

On 21/06/18 18:13, Mathieu Poirier wrote:

quoted

quoted

+
+/*
+ * 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.

quoted

quoted

+    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