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;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <e1133bcd-b1fe-98b3-3a28-c12d07ff8ebc@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>
Subject: Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver
From:   Stephen Boyd <swboyd@chromium.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>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Maulik Shah <mkshah@codeaurora.org>
Date:   Mon, 09 Mar 2020 11:19:53 -0700
Message-ID: <158377799320.66766.16447517220100414599@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Maulik Shah (2020-03-09 03:58:27)
>=20
> 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[] =3D {
> >> +    { "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.
>=20
> No, this is required to keep global. we are not passing just reg as s->pr=
ivate,
> 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".

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.