On 1 February 2012 20:19, Matthew Garrett <[email protected]> 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 | [email protected]
>
_______________________________________________
linaro-dev mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/linaro-dev
