Hi Rafael, Mark,
On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Saturday, August 13, 2011, mark gross wrote:
>> On Thu, Aug 11, 2011 at 05:06:42PM +0200, [email protected] wrote:
>> > From: Jean Pihet <[email protected]>
>> >
>> > 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 <[email protected]>
...
>> > +/* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
