Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org C51FEC433D2
Subject: Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver
To:     Stephen Boyd <swboyd@chromium.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     mka@chromium.org, evgreen@chromium.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        agross@kernel.org, dianders@chromium.org, rnayak@codeaurora.org,
        ilina@codeaurora.org, lsrao@codeaurora.org,
        Mahesh Sivasubramanian <msivasub@codeaurora.org>
References: <1583479412-18320-1-git-send-email-mkshah@codeaurora.org>
 <1583479412-18320-3-git-send-email-mkshah@codeaurora.org>
 <20200307064231.GF1094083@builder>
 <e1133bcd-b1fe-98b3-3a28-c12d07ff8ebc@codeaurora.org>
 <158377799320.66766.16447517220100414599@swboyd.mtv.corp.google.com>
From:   Maulik Shah <mkshah@codeaurora.org>
Message-ID: <ad874421-f197-174f-3d30-2037c7fb57a0@codeaurora.org>
Date:   Tue, 10 Mar 2020 21:08:41 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <158377799320.66766.16447517220100414599@swboyd.mtv.corp.google.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-GB
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 3/9/2020 11:49 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-03-09 03:58:27)
>> On 3/7/2020 12:12 PM, Bjorn Andersson wrote:
>>> On Thu 05 Mar 23:23 PST 2020, Maulik Shah wrote:
>>>> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
>>>> new file mode 100644
>>>> index 00000000..79a14d2
>>>> --- /dev/null
>>>> +++ b/drivers/soc/qcom/soc_sleep_stats.c
>>>> @@ -0,0 +1,248 @@
> [...]
>>>> +    u32 pid;
>>>> +};
>>>> +
>>>> +static struct subsystem_data subsystems[] = {
>>>> +    { "modem", MODEM_ITEM_ID, PID_MODEM },
>>>> +    { "adsp", ADSP_ITEM_ID, PID_ADSP },
>>>> +    { "cdsp", CDSP_ITEM_ID, PID_CDSP },
>>>> +    { "slpi", SLPI_ITEM_ID, PID_SLPI },
>>>> +    { "gpu", GPU_ITEM_ID, PID_APSS },
>>>> +    { "display", DISPLAY_ITEM_ID, PID_APSS },
>>>> +};
>>>> +
>>>> +struct stats_config {
>>>> +    unsigned int offset_addr;
>>>> +    unsigned int num_records;
>>>> +    bool appended_stats_avail;
>>>> +};
>>>> +
>>>> +static const struct stats_config *config;
>>> Add this to soc_sleep_stats_data and pass that as s->private instead of
>>> just the reg, to avoid the global variable.
>> No, this is required to keep global. we are not passing just reg as s->private,
>> we are passing "reg + offset" which differs for each stat.
> Can you please stop sending these review comment replies and then
> immediately turning around and sending the next revision of the patch
> series. I doubt that the changes take less than an hour to write and it
> would be helpful for everyone involved to have constructive discussions
> about the code if there's ever a response besides "done" or "ok".
Sure, i be careful to wait for at least few days before spinning new one,
atleast wait till all discussion finish on current version.

Also will try to reply in more details for comments as much as possible.
>
> In this case it should be possible to get rid of the global 'config'.
> Make a new container struct to hold the config and offset or figure out
> what actually needs to be passed into the functions instead and do that
> when the files are created.

i will address this to get rid of global config.

Thanks,
Maulik

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation