Hi Rafael,
2011/8/14 Rafael J. Wysocki <rjw@xxxxxxx>:
> Hi,
>
> There is some code duplication in this patch that should better be
> avoided (details below).
>
> On Thursday, August 11, 2011, jean.pihet@xxxxxxxxxxxxxx wrote:
>> From: Jean Pihet <j-pihet@xxxxxx>
>>
>> Add a global notification chain that gets called upon changes to the
>> aggregated constraint value for any device.
>> The notification callbacks are passing the full constraint request data
>> in order for the callees to have access to it. The current use is for the
>> platform low-level code to access the target device of the constraint.
>>
...
>
> The following code:
>
>> + /*
>> + * Update constraints list and call the per-device callbacks if needed
>> + */
>> + ret = pm_qos_update_target(dev->power.constraints,
>> + &req->node, PM_QOS_ADD_REQ, value);
>> +
>> + if (ret) {
>> + /* Call the global callbacks if needed */
>> + curr_value = pm_qos_read_value(req->dev->power.constraints);
>> + blocking_notifier_call_chain(&dev_pm_notifiers,
>> + (unsigned long)curr_value,
>> + req);
>> + }
>
> is used in dev_pm_qos_update_request() and dev_pm_qos_remove_request()
> with the only difference being the command given to pm_qos_update_target().
> This asks for a common function, eg. dev_pm_qos_update_target(), containing
> that code that will be called by all of them (and, apparently, by
> dev_pm_qos_constraints_destroy()).
Ok that makes the code cleaner.
>
> ...
>> @@ -250,9 +329,18 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
>> * Update constraints list and call the per-device
>> * callbacks if needed
>> */
>> - pm_qos_update_target(req->dev->power.constraints,
>> - &req->node, PM_QOS_REMOVE_REQ,
>> - PM_QOS_DEFAULT_VALUE);
>> + ret |= pm_qos_update_target(req->dev->power.constraints,
>> + &req->node,
>> + PM_QOS_REMOVE_REQ,
>> + PM_QOS_DEFAULT_VALUE);
>
> I'm not sure why you're using the binary or operator here. Shouldn't that be
> a simple assignment?
>
>> +
>> + if (ret) {
>> + /* Call the global callbacks if needed */
>> + curr_value = dev->power.constraints->default_value;
>> + blocking_notifier_call_chain(&dev_pm_notifiers,
>> + (unsigned long)curr_value,
>> + req);
>> + }
In the sake of optimization I had a single return value that
aggregates the return values of the calls target_value and calls the
global notifier callbacks _only once_.
As you suggested it is better to use the common update code, which
makes the code cleaner but also removes this optimization.
>>
>> kfree(dev->power.constraints->notifiers);
>> kfree(dev->power.constraints);
>
> Thanks,
> Rafael
>
Regards,
Jean
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
