Re: [PATCH 06/16] Thermal: set upper and lower limits

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

On 四, 2012-07-19 at 22:55 +0200, Rafael J. Wysocki wrote:
> On Thursday, July 19, 2012, Zhang Rui wrote:
> > set upper and lower limits when binding
> > a thermal cooling device to a thermal zone device.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  Documentation/thermal/sysfs-api.txt |    6 ++++-
> >  drivers/acpi/thermal.c              |   49 ++++++++++++++++++++++-------------
> >  drivers/platform/x86/acerhdf.c      |    2 +-
> >  drivers/thermal/thermal_sys.c       |   20 +++++++++-----
> >  include/linux/thermal.h             |    3 ++-
> >  5 files changed, 53 insertions(+), 27 deletions(-)
> > 
> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > index b7b3609..f169821 100644
> > --- a/Documentation/thermal/sysfs-api.txt
> > +++ b/Documentation/thermal/sysfs-api.txt
> > @@ -84,7 +84,7 @@ temperature) and throttle appropriate devices.
> >  
> >  1.3 interface for binding a thermal zone device with a thermal cooling device
> >  1.3.1 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
> > -		int trip, struct thermal_cooling_device *cdev);
> > +	int trip, struct thermal_cooling_device *cdev, long upper, long lower);
> >  
> >      This interface function bind a thermal cooling device to the certain trip
> >      point of a thermal zone device.
> > @@ -93,6 +93,10 @@ temperature) and throttle appropriate devices.
> >      cdev: thermal cooling device
> >      trip: indicates which trip point the cooling devices is associated with
> >  	  in this thermal zone.
> > +    upper:the Maximum cooling state for this trip point.
> > +          -1 means no upper limit, and the cooling device can be in max_state.
> > +    lower:the Minimum cooling state can be used for this trip point.
> > +          -1 means no lower limit, and the cooling device can be in cooling state 0.
> 
> If we have to use -1 (or all ones for unsigned long), I'd suggest to define a
> constant called CS_NO_LIMIT or something like that and use it instead of the
> plain numbers.
> 
agreed.

> And perhaps change the name of the above to
> thermal_zone_bind_cooling_device_limits() and define
> thermal_zone_bind_cooling_device() as a static inline that will not take the
> extra two arguments and make it call thermal_zone_bind_cooling_device_limits()
> with the CS_NO_LIMIT values in there.
> 
then you mean there are two APIs for binding?
users can invoke thermal_zone_bind_cooling_device if they do not care
the limits?
Hmm, I do not think we need to do this. And further more, as there are
and there will be more and more parameters for binding, say, maybe
weight/influence in the near future, I'm thinking of using a struct
thermal_bind_params to pass the parameters instead.

> >  1.3.2 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *tz,
> >  		int trip, struct thermal_cooling_device *cdev);
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > index 8275e7b..a7c97f5 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -727,11 +727,9 @@ static int thermal_notify(struct thermal_zone_device *thermal, int trip,
> >  	return 0;
> >  }
> >  
> > -typedef int (*cb)(struct thermal_zone_device *, int,
> > -		  struct thermal_cooling_device *);
> >  static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
> >  					struct thermal_cooling_device *cdev,
> > -					cb action)
> > +					int bind)
> 
> Should 'bind' be a bool?
> 
yep.

> >  {
> >  	struct acpi_device *device = cdev->devdata;
> >  	struct acpi_thermal *tz = thermal->devdata;
> > @@ -755,11 +753,18 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
> >  		    i++) {
> >  			handle = tz->trips.passive.devices.handles[i];
> >  			status = acpi_bus_get_device(handle, &dev);
> > -			if (ACPI_SUCCESS(status) && (dev == device)) {
> > -				result = action(thermal, trip, cdev);
> > -				if (result)
> > -					goto failed;
> > -			}
> > +			if (ACPI_FAILURE(status) || dev != device)
> > +				continue;
> > +			if (bind)
> > +				result =
> > +					thermal_zone_bind_cooling_device
> > +					(thermal, trip, cdev, -1, -1);
> > +			else
> > +				result =
> > +					thermal_zone_unbind_cooling_device
> > +					(thermal, trip, cdev);
> > +			if (result)
> > +				goto failed;
> >  		}
> >  	}
> >  
> > @@ -772,11 +777,16 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
> >  		    j++) {
> >  			handle = tz->trips.active[i].devices.handles[j];
> >  			status = acpi_bus_get_device(handle, &dev);
> > -			if (ACPI_SUCCESS(status) && (dev == device)) {
> > -				result = action(thermal, trip, cdev);
> > -				if (result)
> > -					goto failed;
> > -			}
> > +			if (ACPI_FAILURE(status) || dev != device)
> > +				continue;
> > +			if (bind)
> > +				result = thermal_zone_bind_cooling_device
> > +					(thermal, trip, cdev, -1, -1);
> > +			else
> > +				result = thermal_zone_unbind_cooling_device
> > +					(thermal, trip, cdev);
> > +			if (result)
> > +				goto failed;
> >  		}
> >  	}
> >  
> > @@ -784,7 +794,12 @@ static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
> >  		handle = tz->devices.handles[i];
> >  		status = acpi_bus_get_device(handle, &dev);
> >  		if (ACPI_SUCCESS(status) && (dev == device)) {
> > -			result = action(thermal, -1, cdev);
> > +			if (bind)
> > +				result = thermal_zone_bind_cooling_device
> > +						(thermal, -1, cdev, -1, -1);
> > +			else
> > +				result = thermal_zone_unbind_cooling_device
> > +						(thermal, -1, cdev);
> >  			if (result)
> >  				goto failed;
> >  		}
> > @@ -798,16 +813,14 @@ static int
> >  acpi_thermal_bind_cooling_device(struct thermal_zone_device *thermal,
> >  					struct thermal_cooling_device *cdev)
> >  {
> > -	return acpi_thermal_cooling_device_cb(thermal, cdev,
> > -				thermal_zone_bind_cooling_device);
> > +	return acpi_thermal_cooling_device_cb(thermal, cdev, 1);
> >  }
> >  
> >  static int
> >  acpi_thermal_unbind_cooling_device(struct thermal_zone_device *thermal,
> >  					struct thermal_cooling_device *cdev)
> >  {
> > -	return acpi_thermal_cooling_device_cb(thermal, cdev,
> > -				thermal_zone_unbind_cooling_device);
> > +	return acpi_thermal_cooling_device_cb(thermal, cdev, 0);
> >  }
> >  
> >  static const struct thermal_zone_device_ops acpi_thermal_zone_ops = {
> > diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> > index 39abb15..278aa0e 100644
> > --- a/drivers/platform/x86/acerhdf.c
> > +++ b/drivers/platform/x86/acerhdf.c
> > @@ -329,7 +329,7 @@ static int acerhdf_bind(struct thermal_zone_device *thermal,
> >  	if (cdev != cl_dev)
> >  		return 0;
> >  
> > -	if (thermal_zone_bind_cooling_device(thermal, 0, cdev)) {
> > +	if (thermal_zone_bind_cooling_device(thermal, 0, cdev, -1, -1)) {
> >  		pr_err("error binding cooling dev\n");
> >  		return -EINVAL;
> >  	}
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 4267a49..db35300 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -316,7 +316,7 @@ passive_store(struct device *dev, struct device_attribute *attr,
> >  				     sizeof("Processor")))
> >  				thermal_zone_bind_cooling_device(tz,
> >  								 THERMAL_TRIPS_NONE,
> > -								 cdev);
> > +								 cdev, -1, -1);
> >  		}
> >  		mutex_unlock(&thermal_list_lock);
> >  		if (!tz->passive_delay)
> > @@ -801,7 +801,8 @@ static void thermal_zone_device_check(struct work_struct *work)
> >   */
> >  int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
> >  				     int trip,
> > -				     struct thermal_cooling_device *cdev)
> > +				     struct thermal_cooling_device *cdev,
> > +				     long upper, long lower)
> 
> upper and lower were unsigned long in the previous patch.  Can we please
> use the same data type for them everywhere?
> 
yep.

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