On 1 February 2012 20:19, Matthew Garrett <mjg@xxxxxxxxxx> wrote:
>
> I'm not really a fan of this as it stands - the name isn't very
> intuitive and the code's pretty difficult to read. Would the following
> (incomplete and obviously untested) not have the effect you want? Then
> you register multiple trip points with the same cooling device but
> different private values, and the state set does whatever you want it
> to. Or am I misunderstanding the problem you're trying to solve?
Thanks for the detailed review of the patch. Actually i tried to merge
the benefits of trip type ACTIVE and PASSIVE into one so this name.
This new trip type is just like ACTIVE but instead of OFF(0)/ON(1)
all values greater then 0 is on. Anyways I looked at your
implementation below but this will not solve the purpose as
thermal_cooling_device_register() need to be be called only once to
register a cooling device such cpu frequency based. However the same
cooling device may be binded many times to different trips.
Say,
1) thermal_zone_bind_cooling_device(tz_dev, 0, cdev);
2) thermal_zone_bind_cooling_device(tz_dev, 1, cdev);
3) thermal_zone_bind_cooling_device(tz_dev, 2, cdev);
0,1, 2 are nothing but trip points so the set_cur_state should be called like
set_cur_state(cdev, 0)
set_cur_state(cdev, 1)
set_cur_state(cdev, 2) when the trip point threshold are crossed.
Yeah I agree that implementation logic looks complex but this to
prevent the lower temp trip points cooling handlers to be called. I
will surely make this better in next version.
>
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 220ce7e..817f2ba 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -50,6 +50,7 @@ struct thermal_cooling_device_instance {
> char attr_name[THERMAL_NAME_LENGTH];
> struct device_attribute attr;
> struct list_head node;
> + unsigned long private;
> };
>
> static DEFINE_IDR(thermal_tz_idr);
> @@ -909,7 +910,8 @@ static struct class thermal_class = {
> * @ops: standard thermal cooling devices callbacks.
> */
> struct thermal_cooling_device *thermal_cooling_device_register(
> - char *type, void *devdata, const struct thermal_cooling_device_ops *ops)
> + char *type, void *devdata, const struct thermal_cooling_device_ops *ops,
> + unsigned long private)
> {
> struct thermal_cooling_device *cdev;
> struct thermal_zone_device *pos;
> @@ -936,6 +938,7 @@ struct thermal_cooling_device *thermal_cooling_device_register(
> cdev->ops = ops;
> cdev->device.class = &thermal_class;
> cdev->devdata = devdata;
> + cdev->private = private;
> dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> result = device_register(&cdev->device);
> if (result) {
> @@ -1079,11 +1082,14 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
> continue;
>
> cdev = instance->cdev;
> -
> - if (temp >= trip_temp)
> - cdev->ops->set_cur_state(cdev, 1);
> - else
> - cdev->ops->set_cur_state(cdev, 0);
> + if (cdev->private) {
> + cdev->ops->set_cur_state(cdev, cdev->private);
> + } else {
> + if (temp >= trip_temp)
> + cdev->ops->set_cur_state(cdev, 1);
> + else
> + cdev->ops->set_cur_state(cdev, 0);
> + }
> }
> break;
> case THERMAL_TRIP_PASSIVE:
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 796f1ff..04aac09 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -148,7 +148,7 @@ int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
> struct thermal_cooling_device *);
> void thermal_zone_device_update(struct thermal_zone_device *);
> struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
> - const struct thermal_cooling_device_ops *);
> + const struct thermal_cooling_device_ops *, unsigned long private);
> void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>
> #ifdef CONFIG_NET
>
>
> --
> Matthew Garrett | mjg59@xxxxxxxxxxxxx
--
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
