Messages in this thread | | | Date | Tue, 22 May 2018 13:40:49 -0700 | From | rishabhb@codeauro ... | Subject | Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver |
| |
On 2018-05-22 12:38, Andy Shevchenko wrote: > On Tue, May 22, 2018 at 9:33 PM, <rishabhb@codeaurora.org> wrote: >> On 2018-05-18 14:01, Andy Shevchenko wrote: >>> On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar >>> <rishabhb@codeaurora.org> wrote: > >>>> +#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. > >>>> +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; >>>> + >>> >>> >>>> + 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);
> >>>> + 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.
>>>> + ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val, >>>> + ACTIVATE); > >>> Ditto. > >>>> + 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); > >>> Ditto. > >>>> + 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; > >>> foo |= >>> bar << SHIFT; >>> >>> would look slightly better. > > Did you consider this option ? Yes, forgot to mention.
| |