From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753011AbeEVTik (ORCPT ); Tue, 22 May 2018 15:38:40 -0400 Received: from mail-qk0-f194.google.com ([209.85.220.194]:43682 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752989AbeEVTih (ORCPT ); Tue, 22 May 2018 15:38:37 -0400 X-Google-Smtp-Source: AB8JxZr3ZTet8UF5DkW6I/qhUJeQKz+rB28IMTos7fWZV75RyrO4EPh0J6mVOvbbbAFOwDWKLKeomA8OeG9kVrGHNcg= MIME-Version: 1.0 In-Reply-To: References: <1526492623-20527-1-git-send-email-rishabhb@codeaurora.org> <1526492623-20527-3-git-send-email-rishabhb@codeaurora.org> From: Andy Shevchenko Date: Tue, 22 May 2018 22:38:36 +0300 Message-ID: Subject: Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver To: Rishabh Bhatnagar Cc: linux-arm Mailing List , linux-arm-msm@vger.kernel.org, devicetree , Linux Kernel Mailing List , linux-arm@lists.infradead.org, tsoni@codeaurora.org, ckadabi@codeaurora.org, Evan Green , Rob Herring Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 22, 2018 at 9:33 PM, wrote: > On 2018-05-18 14:01, Andy Shevchenko wrote: >> On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar >> 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