On 13:53-20110527, MyungJoo Ham wrote: [..] > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index 56a6899..819c1b3 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -21,6 +21,7 @@ > #include <linux/rculist.h> > #include <linux/rcupdate.h> > #include <linux/opp.h> > +#include <linux/devfreq.h> > > /* > * Internal data structure organization with the OPP layer library is as > @@ -428,6 +429,11 @@ int opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) > list_add_rcu(&new_opp->node, head); > mutex_unlock(&dev_opp_list_lock); > > + /* > + * Notify generic dvfs for the change and ignore error > + * because the device may not have a devfreq entry > + */ > + devfreq_update(dev); I think I understand your thought about notifiers here - we had some initial thought that we may need notifiers, for e.g. clk change notifiers which could implement enable/disable on a dependent device, thermal management etc.. so far, we have'nt had a need to modify the lowest data store layer to handle this. In this case, I think you'd like a notifier mechanism in general for opp_add,enable or disable in the opp library. However, with this change, an opp_add now means: a) opp_add now depends on devfreq_update() to be part of it's integral operation - I think the devfreq should maintain it's own notifier mechanisms and not make OPP library do the notification trigger job of devfreq. b) By ignoring the error, how will the caller of opp_add now know if the failures were real - e.g. some device failed to act on the notification and the overall opp_add operation failed? And why should OPP library do recovery for on behalf of devfreq? c) How will you ensure the lock accuracy - given that you need to keep to the constraints of opp_add/opp_set_availability here? For example: devfreq_do is called from devfreq_monitor(workqueue) and devfreq_update in addition to having to call a bunch of function pointers whose implementation you do not know, having a function that ensures security of it's data, handles all lock conditions that is imposed differently by add,enable and disable is not visible in this implementation. c) How about considering the usage of OPP library with an SoC cpufreq implementation say for MPU AND using the same SoC using devfreq for controlling some devices like MALI in your example simultaneously? Lets say the thermal policy manager implementation for some reason did an opp_disable of a higher OPP of MPU - devfreq_update gets called with the device(mpu_dev).. devfreq_update needs to now invoke it's validation path and fails correctly - Vs devfreq_update being called by some module which actually uses devfreq and the update failed - How does one differentiate between the two? just my 2 cents, I think these kind of notifiers should probably stay out of opp library OR notifiers be introduced in a generic and safe fashion. > return 0; > } > > @@ -512,6 +518,9 @@ unlock: > mutex_unlock(&dev_opp_list_lock); > out: > kfree(new_opp); > + > + /* Notify generic dvfs for the change and ignore error */ > + devfreq_update(dev); Another place where I am confused - how does devfreq_update know in what context is it being called is the OPP getting enabled/disabled/added - devfreq_update has to do some additional work to make it's own decision - it is almost as if it uses devfreq_update as a trigger mechanism to reevalute the entire decision tree - without using information of the context opp_enable/disable/add has to maybe make better call. if this information is not needed, maybe some other mechanism implemented by devfreq layer might suffice.. > return r; > } [...] -- Regards, Nishanth Menon _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
