Re: [PATCH 05/15] PM QoS: generalize and export the constraints management code

On Tue, Aug 16, 2011 at 6:08 AM, mark gross [off-list ref] wrote:

On Sun, Aug 14, 2011 at 03:37:43PM +0200, Rafael J. Wysocki wrote:

quoted

On Sunday, August 14, 2011, Jean Pihet wrote:

quoted

Hi Rafael, Mark,

On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Saturday, August 13, 2011, mark gross wrote:

quoted

On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@newoldbits.com wrote:

quoted

From: Jean Pihet <redacted>

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 <redacted>

...

quoted

quoted

quoted

+/* 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.

quoted

 #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.

The enums are fine by me and they allow us to simplify the code
quite a bit.

Ok, but they look a bit sloppy to me as we now have an API that says
"add" we can actually pass in an enum that says "remove".

We have an API that says 'update target' that we pass in a parameter
that says 'add request', 'update request' or 'remove request'.
If it is required I could just rename the internal function
update_target, in a later patch.

--mark

Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html