On 2018-05-22 12:38, Andy Shevchenko wrote:

On Tue, May 22, 2018 at 9:33 PM,  [off-list ref] wrote:

quoted

On 2018-05-18 14:01, Andy Shevchenko wrote:

quoted

On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
[off-list ref] wrote:

quoted

quoted

quoted

+#define ACTIVATE                      0x1
+#define DEACTIVATE                    0x2
+#define ACT_CTRL_OPCODE_ACTIVATE      0x1
+#define ACT_CTRL_OPCODE_DEACTIVATE    0x2
+#define ACT_CTRL_ACT_TRIG             0x1

Are these bits? Perhaps BIT() ?

isn't it just better to use fixed size as u suggest in the next 
comment?

If the are bits, use BIT() macro.

quoted

quoted

quoted

+struct llcc_slice_desc *llcc_slice_getd(u32 uid)
+{
+       const struct llcc_slice_config *cfg;
+       struct llcc_slice_desc *desc;
+       u32 sz, count = 0;
+
+       cfg = drv_data->cfg;
+       sz = drv_data->cfg_size;
+

quoted

+       while (cfg && count < sz) {
+               if (cfg->usecase_id == uid)
+                       break;
+               cfg++;
+               count++;
+       }
+       if (cfg == NULL || count == sz)
+               return ERR_PTR(-ENODEV);

if (!cfg)
          return ERR_PTR(-ENODEV);

while (cfg->... != uid) {
  cfg++;
  count++;
}

if (count == sz)
 return ...

Though I would rather put it to for () loop.

In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.

do {
  if (!cfg || count == sz)
   return ...(-ENODEV);
 ...
} while (...);

Though, as I said for-loop will look slightly better I think.

Is this fine?
for (count = 0; count < sz; count++) {
    if (!cfg)
	return ERR_PTR(-ENODEV);
    if (cfg->usecase_id == uid)
	break;
    cfg++;
}
if (count == sz)
    return ERR_PTR(-ENODEV);

quoted

quoted

quoted

+       ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+                                 DEACTIVATE);

Perhaps one line (~83 characters here is OK) ?

The checkpatch script complains about such lines.

So what if it just 3 characters out?

Other reviewers sometimes are not okay if the checkpatch complains.
Because I have gotten many reviews previously concerning about line
length. Not sure how to proceed here.

quoted

quoted

quoted

+       ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+                                 ACTIVATE);

quoted

quoted

Ditto.

quoted

quoted

quoted

+               attr1_cfg = bcast_off +
+
LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
+               attr0_cfg = bcast_off +
+
LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);

quoted

quoted

Ditto.

quoted

quoted

quoted

+               attr1_val |= llcc_table[i].probe_target_ways <<
+                               ATTR1_PROBE_TARGET_WAYS_SHIFT;
+               attr1_val |= llcc_table[i].fixed_size <<
+                               ATTR1_FIXED_SIZE_SHIFT;
+               attr1_val |= llcc_table[i].priority <<
ATTR1_PRIORITY_SHIFT;

quoted

quoted

foo |=
  bar << SHIFT;

would look slightly better.

Did you consider this option ?

Yes, forgot to mention.