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
