Thanks Rui for taking a Quick Look. I am not replying to all your comments, because most of them look fine to me. [A big cut] > > 1. Introduces the get_trend callback > seems duplicate work of my patch set. :( Yes I agree. I was in the middle of testing yesterday, so could not change it before today. I will review your 12 patches, and pull in here the needed ones. So, my next patch set will have those changes, and they don’t conflict. > > > 2. Introduces the notify_thermal_framework() API > > > 3. Exposes sysfs to show/store the throttle_policy > > > yes, I think we need this. > > > Patch 2/4: Introduce fair_share governor > > This throttles the cooling_devices according to their > > weights (and hence the name; Suggestion are welcome :-). > > The weights in turn describe the effectiveness of a > > particular cooling device in cooling a thermal zone. > > > > > Patch 3/4: Introduce step_wise governor > > This throttles/de-throttles the cooling devices one > > step at a time. This is exactly similar to the code > > we have in thermal_zone_device_update function. The > > intention is to move all 'throttling logic' related > > code outside thermal_sys.c and keep them separate. > > totally agree. Oh that’s nice. Will incorporate this change in my next patch set. Thank you for this. > > * Add more protection and tidy up the existing ones > > * Expose the weights and cooling devices through sysfs (Read-Only) > > what do you mean "cooling devices"? I meant for a thermal zone, we should expose: one Sysfs: that will list the weights of all cooling devices associated with this other Sysfs: that will list the names of cooling devices (in the same order as weights) This is just to let user land know of the binding I should have put 'names of cooling devices'. > > Yep, I think this is handled in my arbitrator patch. :p ok...I have not looked at it yet. Will have a look and incorporate the change in next patch set. > > > (To do this, we have to loop through the thermal_tz_list and > > thermal_cdev_list inside fair_share.c. Need to see how good it is > > to make this lists public) > > IMO, these governors should just update the thermal_instance, and then > invokes thermal_zone_do_update(). They should not change the cooling > state directly. > Give me some time to think about this one, will come back to you :-) > > * Make all _throttle methods have same signature. This way we can make use > > of function pointers and make the code a bit simpler. > > what does signature mean? Oh I meant they are all same 'taking similar arguments and same return type'. This way we can use an arr[function ptrs] that will be populated in each governor's init. This way inside the notify_framework method we don’t so many case statements. We could just do arr[throttle_policy](...) Thanks, Durga ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f