Sorry it's taken me so long to respond to this.
> > My implementation has two primary goals:
> >
> > 1.) To help device drivers track and control their device's power state
> >
> > 2.) To bubble power state dependency changes from a leaf node to the
> > last affected parent node in the chain.
> >
> > The basic unit of the tree is referred to as a "pm_node". Each
> > "pm_node" has a list of states it supports, a current state, and a
> > driver to handle state transitions. Any "pm_node" can have children and
> > therefore act as a power container.
I think it's a mistake to make this a tree. The natural structure is a
DAG, where nodes can have multiple parents. It might not be worthwhile
trying to store the parent/child/sibling pointers in the structures.
> > struct pm_state {
> > char *name;
> > unsigned long index;
> > unsigned long link_req;
> > void *link_data;
> > };
> >
> > "index" is a driver specific value that indicates the state. "link_req"
> > is a set of flags that indicate requirements from the parent "pm_node".
> > "link_data" provides additional requirement information.
Given that there can be multiple parents, the link_* members of this
structure won't be useful. Also, I don't see that index expresses any
information not already present in name.
> > struct pm_node {
> > struct semaphore sem;
> > struct list_head children;
> > struct list_head child_list;
> > struct pm_node *parent;
> > struct pm_node *tmp_path;
> > struct device *dev;
> >
> > struct pm_node_driver *drv;
> > unsigned int current_state;
> > unsigned int target_state;
>
> Would not it be cleaner to have struct pm_state *current_state, target state?
I don't see the point of having target_state at all.
> > "states" is a list of power states sorted from most functional to least
> > functional.
In general it's not possible to sort states in that way. Some states
might permit one subset of the complete set of functions while other
states permit a different, partially overlapping, subset.
> > "current_state" and "target_state" are index values for
> > that list.
> >
> > struct pm_node_driver {
> > int (*set_state) (struct pm_node *node);
> > int (*raise_event) (struct pm_node *domain,
> > struct pm_node *child,
> > unsigned long *new_state);
>
> struct pm_state here, too?
>
> > void (*lower_event) (struct pm_node *domain);
> > };
> >
> > "raise_event" notifies the power domain when a child has increased its
>
> "Increase" == went more power hungry?
There's no need to have separate raise_event and lower_event entry points.
In fact you might not want to have those entry points at all, since the
information they convey will be so bus-specific. It might be better to
rely on bus-specific entry points. I'm not sure about this.
Alan Stern
