On 四, 2012-07-19 at 23:39 +0200, Rafael J. Wysocki wrote:
> On Thursday, July 19, 2012, Zhang Rui wrote:
> > This fixes the problem that a cooling device may be referenced by
> > by multiple trip points in multiple thermal zones.
> >
> > With this patch, we have two stages for updating a thermal zone,
> > 1. check if a thermal_instance needs to be updated or not
> > 2. update the cooling device, based on the target cooling state
> > of all its instances.
> >
> > Note that, currently, the cooling device is set to the deepest
> > cooling state required.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> > ---
> > drivers/thermal/thermal_sys.c | 41 ++++++++++++++++++++++++++++++++++++++---
> > include/linux/thermal.h | 1 +
> > 2 files changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 48b2668..803ce94 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -54,6 +54,7 @@ struct thermal_instance {
> > int trip;
> > unsigned long upper; /* Highest cooling state for this trip point */
> > unsigned long lower; /* Lowest cooling state for this trip point */
> > + unsigned long target; /* expected cooling state */
> > char attr_name[THERMAL_NAME_LENGTH];
> > struct device_attribute attr;
> > struct list_head tz_node; /* node in tz->instances */
> > @@ -853,6 +854,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
> > dev->trip = trip;
> > dev->upper = upper;
> > dev->lower = lower;
> > + dev->target = -1;
>
> Please define THERMAL_NO_TARGET and use that instead of plain -1.
>
agreed.
> >
> > result = get_idr(&tz->idr, &tz->lock, &dev->id);
> > if (result)
> > @@ -990,6 +992,7 @@ thermal_cooling_device_register(char *type, void *devdata,
> > strcpy(cdev->type, type);
> > INIT_LIST_HEAD(&cdev->instances);
> > cdev->ops = ops;
> > + cdev->updated = 1;
> > cdev->device.class = &thermal_class;
> > cdev->devdata = devdata;
> > dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> > @@ -1081,6 +1084,32 @@ void thermal_cooling_device_unregister(struct
> > }
> > EXPORT_SYMBOL(thermal_cooling_device_unregister);
> >
> > +static void thermal_zone_do_update(struct thermal_zone_device *tz)
> > +{
> > + struct thermal_instance *instance1, *instance2;
> > + struct thermal_cooling_device *cdev;
> > + int target;
>
> The 'target' field in the structure is unsigned long. It'd be better to use
> the same data type here.
>
> It would be good to use 'true' and 'false' as the values of 'updated' too.
>
agreed.
> > +
> > + list_for_each_entry(instance1, &tz->instances, tz_node) {
> > + cdev = instance1->cdev;
> > +
> > + /* cooling device has already been updated*/
> > + if (cdev->updated)
> > + continue;
> > +
> > + target = 0;
> > + /* Make sure cdev enters the deepest cooling state */
> > + list_for_each_entry(instance2, &cdev->instances, cdev_node) {
> > + if (instance2->target == -1)
> > + continue;
> > + if (instance2->target > target)
> > + target = instance2->target;
> > + }
> > + cdev->ops->set_cur_state(cdev, target);
> > + cdev->updated = 1;
> > + }
> > +}
>
> I would split the function above into two, one that would walk all
> instances of the thermal zone and the other, that would walk the
> instances for a given device. It would look much cleaner, then.
>
agreed.
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
