Re: [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number

[Amit Kachhap <amit.kachhap@linaro.org>]

On 1 February 2012 20:19, Matthew Garrett <mjg@redhat.com> 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@srcf.ucam.org