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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
