* Jean Pihet <jean.pihet@xxxxxxxxxxxxxx> wrote:
> Ingo,
>
> On Tue, Oct 26, 2010 at 9:10 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
> >
> > * Thomas Renninger <trenn@xxxxxxx> wrote:
> >
> >> Changes in V2:
> >> - Introduce PWR_EVENT_EXIT instead of 0 to mark non-power state
> >> - Use u32 instead of u64 for cpuid, state which is by far enough
>
> ...
>
> >>
> >> +#define PWR_EVENT_EXIT 0xFFFFFFFF
> >
> > Shouldnt this be part of the POWER_ enum? (and you can write -1 there)
> >
> >> +#ifndef _TRACE_POWER_ENUM_
> >> +#define _TRACE_POWER_ENUM_
> >> +enum {
> >> + POWER_NONE = 0,
> >> + POWER_CSTATE = 1,
> >> + POWER_PSTATE = 2,
> >> +};
> >> +#endif
> >
> > Since we are cleaning up all these events, those enum definitions dont really look
> > logical. For example, what is 'POWER_NONE'? Can a CPU have 'no power'?
>
> The enum belongs to the deprecated API so I would rather not touch it.
> Keeping the deprecated code isolated will make it easier to remove
> later.
So what will replace it? We still have a state field.
Passing in platform specific codes is a step backwards.
> >> +TRACE_EVENT(machine_suspend,
> >> +
> >> + TP_PROTO(unsigned int state),
> >> +
> >> + TP_ARGS(state),
> >> +
> >> + TP_STRUCT__entry(
> >> + __field( u32, state )
> >> + ),
> >
> > Hm, this event is not used anywhere in the submitted patches. Where is the patch
> > that adds usage, and what are the possible values for 'state'?
>
> This will come as a separate patch, which fits all platforms. Cf.
> http://marc.info/?l=linux-omap&m=128620575300682&w=2.
> The state field is of type suspend_state_t, cf. include/linux/suspend.h
Ok, that's at least generic. Needs the review of Rafael, to determine whether this
state value is all we want to know when we enter suspend.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-trace-users" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|