Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver

[Stephen Boyd]

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".

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.