lkml.org 
[lkml]   [2018]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v7 2/2] drivers: soc: Add LLCC driver
    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.

    >>> + 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?

    >>> + 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 ?

    --
    With Best Regards,
    Andy Shevchenko

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