Hi Rafael, Mark, On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <r...@sisk.pl> wrote: > On Saturday, August 13, 2011, mark gross wrote: >> On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pi...@newoldbits.com wrote: >> > From: Jean Pihet <j-pi...@ti.com> >> > >> > In preparation for the per-device constratins support: >> > - rename update_target to pm_qos_update_target >> > - generalize and export pm_qos_update_target for usage by the upcoming >> > per-device latency constraints framework: >> > . operate on struct pm_qos_constraints for constraints management, >> > . introduce an 'action' parameter for constraints add/update/remove, >> > . the return value indicates if the aggregated constraint value has >> > changed, >> > - update the internal code to operate on struct pm_qos_constraints >> > - add a NULL pointer check in the API functions >> > >> > Signed-off-by: Jean Pihet <j-pi...@ti.com> ... >> > +/* Action requested to pm_qos_update_target */ >> > +enum pm_qos_req_action { >> > + PM_QOS_ADD_REQ, /* Add a new request */ >> > + PM_QOS_UPDATE_REQ, /* Update an existing request */ >> > + PM_QOS_REMOVE_REQ /* Remove an existing request */ >> > +}; >> > + >> >> What do you need this enum for? The function names *_update_*, *_add_*, >> and *_remove_* seem to be pretty redundant if you have to pass an enum >> that could possibly conflict with the function name. >> >> > #ifdef CONFIG_PM >> > +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node >> > *node, >> > + enum pm_qos_req_action action, int value); >> The action for update_target better damn well be "PM_QOS_UPDATE_REQ" or >> there is something strange going on.... BTW what shold this function do >> if the pm_qos_req_action was *not* the UPDATE one?
The meaning of pm_qos_update_target is 'update the PM QoS target constraints lists'. As described in the changelog the intention of this patch is to implement the constraints lists management logic in update_target and simplify the API functions (add/update/remove). It is also exported for the upcoming (patch 06/15]) to use it as well. ... >> > +int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node >> > *node, >> > + enum pm_qos_req_action action, int value) >> > { >> > unsigned long flags; >> > - int prev_value, curr_value; >> > + int prev_value, curr_value, new_value; >> > >> > spin_lock_irqsave(&pm_qos_lock, flags); >> > - prev_value = pm_qos_get_value(o); >> > - /* PM_QOS_DEFAULT_VALUE is a signal that the value is unchanged */ >> > - if (value != PM_QOS_DEFAULT_VALUE) { >> > + prev_value = pm_qos_get_value(c); >> > + if (value == PM_QOS_DEFAULT_VALUE) >> > + new_value = c->default_value; >> > + else >> > + new_value = value; >> > + >> > + switch (action) { >> > + case PM_QOS_REMOVE_REQ: >> We have a remove request API already. This overloading of this >> interface feels wrong to me. >> >> > + plist_del(node, &c->list); >> > + break; >> > + case PM_QOS_UPDATE_REQ: >> > /* >> > * to change the list, we atomically remove, reinit >> > * with new value and add, then see if the extremal >> > * changed >> > */ >> > - plist_del(node, &o->constraints->list); >> > - plist_node_init(node, value); >> > - plist_add(node, &o->constraints->list); >> > - } else if (del) { >> > - plist_del(node, &o->constraints->list); >> > - } else { >> > - plist_add(node, &o->constraints->list); >> > + plist_del(node, &c->list); >> > + case PM_QOS_ADD_REQ: >> Don't we have an API for adding a request? if you want to overload >> update like this then either we lose the add API or this shouldn't be >> here. >> ... >> > @@ -216,20 +242,16 @@ EXPORT_SYMBOL_GPL(pm_qos_request_active); >> > void pm_qos_add_request(struct pm_qos_request *req, >> > int pm_qos_class, s32 value) >> > { >> > - struct pm_qos_object *o = pm_qos_array[pm_qos_class]; >> > - int new_value; >> > + if (!req) /*guard against callers passing in null */ >> > + return; >> > >> > if (pm_qos_request_active(req)) { >> > WARN(1, KERN_ERR "pm_qos_add_request() called for already >> > added request\n"); >> > return; >> > } >> > - if (value == PM_QOS_DEFAULT_VALUE) >> > - new_value = o->constraints->default_value; >> > - else >> > - new_value = value; >> > - plist_node_init(&req->node, new_value); >> > req->pm_qos_class = pm_qos_class; >> > - update_target(o, &req->node, 0, PM_QOS_DEFAULT_VALUE); >> > + pm_qos_update_target(pm_qos_array[pm_qos_class]->constraints, >> > + &req->node, PM_QOS_ADD_REQ, value); >> >> Ok, using pm_qos_update_target to reduce the LOC is ok but I don't think >> this function and the enum should be exported outside of pm_qos.c > > They are used by the next patches adding the per-device QoS. Exactly > > Thanks, > Rafael > Jean -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html