Re: [PATCH 07/15] PM QoS: add a global notification mechanism for the device constraints

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

 



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