RE: [PATCH 0/4] Thermal Framework Enhancements

[Zhang Rui <rui.zhang@intel.com>]

On 二, 2012-06-12 at 02:59 -0600, R, Durgadoss wrote:
> 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'.
> 
so I have two questions,
1. should kernel thermal manager handle these weights?
2. if yes, IMO, we can add one field in thermal_instance to describe
this weight. and then, the problem to me is that, we should easily
introduce an attribute for each cooling device instance, say,
/sys/class/thermal/thermal_zone0/cdev0_trip_point
                                /cdev0_weight
...

> > 
> > 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](...)
> 
okay.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html