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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 16, 2011 at 6:08 AM, mark gross <markgross@xxxxxxxxxxx> wrote:
> On Sun, Aug 14, 2011 at 03:37:43PM +0200, Rafael J. Wysocki wrote:
>> On Sunday, August 14, 2011, Jean Pihet wrote:
>> > Hi Rafael, Mark,
>> >
>> > On Sat, Aug 13, 2011 at 10:34 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
>> > > On Saturday, August 13, 2011, mark gross wrote:
>> > >> On Thu, Aug 11, 2011 at 05:06:42PM +0200, jean.pihet@xxxxxxxxxxxxxx wrote:
>> > >> > From: Jean Pihet <j-pihet@xxxxxx>
>> > >> >
>> > >> > 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-pihet@xxxxxx>
>> > ...
>> > >> > +/* 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.
>>
>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux