Quoting Maulik Shah (2019-11-06 01:22:14)
>
> On 9/6/2019 12:07 AM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2019-09-05 02:17:07)
> >> +
> >> +static inline ssize_t subsystem_stats_print(char *prvbuf, ssize_t length,
> >> + struct subsystem_stats *record,
> >> + const char *name)
> >> +{
> >> + return scnprintf(prvbuf, length, "%s\n\tVersion:0x%x\n"
> >> + "\tSleep Count:0x%x\n"
> >> + "\tSleep Last Entered At:0x%llx\n"
> >> + "\tSleep Last Exited At:0x%llx\n"
> >> + "\tSleep Accumulated Duration:0x%llx\n\n",
> >> + name, record->version_id, record->count,
> >> + record->last_entered, record->last_exited,
> >> + record->accumulated_duration);
> > Information in sysfs is supposed to be one value per file. This is a
> > bunch of different values and it includes a version field. Looks almost
> > like something we would put into /proc, but of course that doesn't make
> > any sense to put in /proc either.
> >
> > Please rethink the whole approach here. Can this be placed under the
> > remoteproc nodes for each remote processor that's in the system? That
> > would make it more discoverable by userspace looking at the remoteproc
> > devices. I suppose GPU and DISPLAY aren't "remoteproc"s though so maybe
> > this should be a new 'class' for devices that have an RPMh RSC? Maybe
> > make a qcom_rpmh_rsc class and then have these be stats in there.
>
> since stats can be used by userspace for the purpose of computing
> battery utilization /sys/power seems to be good place to keep it to me.
>
> Adding it under class may require it to be device. we are using it only
> as module.
>
I believe /sys/power is for the power management subsystem, not
specifically battery utilization or remote processor power states.
Wouldn't battery be /sys/class/power_supply? Why not put this underneath
some /sys/class/remoteproc or so?
> >> + kobject_put(prvdata->kobj);
> >> + kfree(prvdata);
> >> + return ret;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void __exit subsystem_sleep_stats_exit(void)
> >> +{
> >> + sysfs_remove_file(prvdata->kobj, &prvdata->ka.attr);
> >> + kobject_put(prvdata->kobj);
> >> + kfree(prvdata);
> >> +}
> >> +
> >> +module_init(subsystem_sleep_stats_init);
> > So if this is compiled into an arm/arm64 image that doesn't include qcom
> > platform support it will create this directory? That's just nonsensical.
>
> Kconfig depends on QCOM_SMEM which inturn depends on ARCH_QCOM to get
> compiled into.
>
> It won't get compiled for other than qcom platforms.
Sure it won't get compiled for anything that doesn't have ARCH_QCOM
enabled, but it can run on a board or SoC that isn't qcom. That's the
concern.
