Re: [PATCH V4 05/13] Thermal: Remove tc1/tc2 in generic thermal layer.

From: Eduardo Valentin <eduardo.valentin@ti.com>
To: Zhang Rui <rui.zhang@intel.com>
Cc: eduardo.valentin@ti.com, "Rafael J. Wysocki" <rjw@sisk.pl>,
	Matthew Garrett <mjg@redhat.com>, Len Brown <lenb@kernel.org>,
	R Durgadoss <durgadoss.r@intel.com>,
	Amit Kachhap <amit.kachhap@linaro.org>, Wei Ni <wni@nvidia.com>,
	linux-acpi@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH V4 05/13] Thermal: Remove tc1/tc2 in generic thermal layer.
Date: Tue, 21 Aug 2012 17:20:14 +0300	[thread overview]
Message-ID: <20120821142014.GB5030@besouro> (raw)
In-Reply-To: <1345510182.1682.927.camel@rui.sh.intel.com>

Hello,

On Tue, Aug 21, 2012 at 08:49:42AM +0800, Zhang Rui wrote:
> On 一, 2012-08-20 at 15:18 +0300, Eduardo Valentin wrote:
> > Hello,
> > 
> > On Thu, Aug 09, 2012 at 06:10:04PM +0300, Valentin, Eduardo wrote:
> > > 
> > > (resending the mail so that it reaches the mailing lists)
> > > 
> > > Nice, I checked your tree and this patch is fine for me. Next time you
> > > send it out, you may want to add my reviewed-by:
> > 
> > Except that in your tree it is still introducing compilation issues under omap-thermal-common.c
> > 
> why, I think I have fixed that already.
> would you please send an incremental patch so that I can apply it?


FYI, I sent as reply to your patch 0/13 a plain diff of including the needed changes in order to keep the code
in one piece after this series.

But as the driver is under staging, maybe it makes sense to send this change separately, to Greg K.

Posting here anyway:

diff --git a/drivers/staging/omap-thermal/omap-thermal-common.c b/drivers/staging/omap-thermal/omap-thermal-common.c
index 0675a5e..b4cd6cc 100644
--- a/drivers/staging/omap-thermal/omap-thermal-common.c
+++ b/drivers/staging/omap-thermal/omap-thermal-common.c
@@ -120,7 +120,9 @@ static int omap_thermal_bind(struct thermal_zone_device *thermal,
 
        /* TODO: bind with min and max states */
        /* Simple thing, two trips, one passive another critical */
-       return thermal_zone_bind_cooling_device(thermal, 0, cdev);
+       return thermal_zone_bind_cooling_device(thermal, 0, cdev,
+                                               THERMAL_NO_LIMIT,
+                                               THERMAL_NO_LIMIT);
 }
 
 /* Unbind callback functions for thermal zone */
@@ -246,7 +248,7 @@ int omap_thermal_expose_sensor(struct omap_bandgap *bg_ptr, int id,
        /* Create thermal zone */
        data->omap_thermal = thermal_zone_device_register(domain,
                                OMAP_TRIP_NUMBER, 0, data, &omap_thermal_ops,
-                               0, FAST_TEMP_MONITORING_RATE, 0, 0);
+                               0, FAST_TEMP_MONITORING_RATE);
        if (IS_ERR_OR_NULL(data->omap_thermal)) {
                dev_err(bg_ptr->dev, "thermal zone device is NULL\n");
                return PTR_ERR(data->omap_thermal);



> 
> thanks,
> rui
> > > 
> > > Reviewed-by: Eduardo Valentin <eduardo.valentin@ti.com>
> > 
> > So, apart from the above comment, this patch is still fine for me.
> > 
> > > 
> > > 
> > > >
> > > > thanks,
> > > > rui
> > > > > Apart from the compilation issues and the needed patch refresh, I am
> > > > > good with the patch. And thanks for pushing this forward.
> > > > >
> > > > > >  6 files changed, 11 insertions(+), 29 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > > > > > index 0c49e42..0c755d2 100644
> > > > > > --- a/drivers/acpi/thermal.c
> > > > > > +++ b/drivers/acpi/thermal.c
> > > > > > @@ -895,15 +895,12 @@ static int
> > > > > > acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
> > > > > >                 tz->thermal_zone =
> > > > > >                         thermal_zone_device_register("acpitz", trips,
> > > > > > 0, tz,
> > > > > >
> > > > > > &acpi_thermal_zone_ops,
> > > > > > -
> > > > > > tz->trips.passive.tc1,
> > > > > > -
> > > > > > tz->trips.passive.tc2,
> > > > > >
> > > > > > tz->trips.passive.tsp*100,
> > > > > >
> > > > > > tz->polling_frequency*100);
> > > > > >         else
> > > > > >                 tz->thermal_zone =
> > > > > >                         thermal_zone_device_register("acpitz", trips,
> > > > > > 0, tz,
> > > > > > -
> > > > > > &acpi_thermal_zone_ops,
> > > > > > -                                                    0, 0, 0,
> > > > > > +
> > > > > > &acpi_thermal_zone_ops, 0,
> > > > > >
> > > > > > tz->polling_frequency*100);
> > > > > >         if (IS_ERR(tz->thermal_zone))
> > > > > >                 return -ENODEV;
> > > > > > diff --git a/drivers/platform/x86/acerhdf.c
> > > > > > b/drivers/platform/x86/acerhdf.c
> > > > > > index a207466..84c5688 100644
> > > > > > --- a/drivers/platform/x86/acerhdf.c
> > > > > > +++ b/drivers/platform/x86/acerhdf.c
> > > > > > @@ -662,7 +662,7 @@ static int acerhdf_register_thermal(void)
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > >         thz_dev = thermal_zone_device_register("acerhdf", 1, 0, NULL,
> > > > > > -                                             &acerhdf_dev_ops, 0, 0,
> > > > > > 0,
> > > > > > +                                             &acerhdf_dev_ops, 0,
> > > > > >                                               (kernelmode) ?
> > > > > > interval*1000 : 0);
> > > > > >         if (IS_ERR(thz_dev))
> > > > > >                 return -EINVAL;
> > > > > > diff --git a/drivers/platform/x86/intel_mid_thermal.c
> > > > > > b/drivers/platform/x86/intel_mid_thermal.c
> > > > > > index 2b2c212..6e309dd 100644
> > > > > > --- a/drivers/platform/x86/intel_mid_thermal.c
> > > > > > +++ b/drivers/platform/x86/intel_mid_thermal.c
> > > > > > @@ -499,7 +499,7 @@ static int mid_thermal_probe(struct
> > > > > > platform_device *pdev)
> > > > > >                         goto err;
> > > > > >                 }
> > > > > >                 pinfo->tzd[i] = thermal_zone_device_register(name[i],
> > > > > > -                               0, 0, td_info, &tzd_ops, 0, 0, 0, 0);
> > > > > > +                               0, 0, td_info, &tzd_ops, 0, 0);
> > > > > >                 if (IS_ERR(pinfo->tzd[i])) {
> > > > > >                         kfree(td_info);
> > > > > >                         ret = PTR_ERR(pinfo->tzd[i]);
> > > > > > diff --git a/drivers/thermal/spear_thermal.c
> > > > > > b/drivers/thermal/spear_thermal.c
> > > > > > index 69a55d4..b55840a 100644
> > > > > > --- a/drivers/thermal/spear_thermal.c
> > > > > > +++ b/drivers/thermal/spear_thermal.c
> > > > > > @@ -148,7 +148,7 @@ static int spear_thermal_probe(struct
> > > > > > platform_device *pdev)
> > > > > >         writel_relaxed(stdev->flags, stdev->thermal_base);
> > > > > >
> > > > > >         spear_thermal = thermal_zone_device_register("spear_thermal",
> > > > > > 0, 0,
> > > > > > -                               stdev, &ops, 0, 0, 0, 0);
> > > > > > +                               stdev, &ops, 0, 0);
> > > > > >         if (IS_ERR(spear_thermal)) {
> > > > > >                 dev_err(&pdev->dev, "thermal zone device is NULL\n");
> > > > > >                 ret = PTR_ERR(spear_thermal);
> > > > > > diff --git a/drivers/thermal/thermal_sys.c
> > > > > > b/drivers/thermal/thermal_sys.c
> > > > > > index 68c93d4..a70ed4c 100644
> > > > > > --- a/drivers/thermal/thermal_sys.c
> > > > > > +++ b/drivers/thermal/thermal_sys.c
> > > > > > @@ -335,9 +335,6 @@ passive_store(struct device *dev, struct
> > > > > > device_attribute *attr,
> > > > > >                 tz->passive_delay = 0;
> > > > > >         }
> > > > > >
> > > > > > -       tz->tc1 = 1;
> > > > > > -       tz->tc2 = 1;
> > > > > > -
> > > > > >         tz->forced_passive = state;
> > > > > >
> > > > > >         thermal_zone_device_update(tz);
> > > > > > @@ -718,13 +715,12 @@ static void
> > > > > > thermal_zone_device_set_polling(struct thermal_zone_device *tz,
> > > > > >  static void thermal_zone_device_passive(struct thermal_zone_device
> > > > > > *tz,
> > > > > >                                         int temp, int trip_temp, int
> > > > > > trip)
> > > > > >  {
> > > > > > -       int trend = 0;
> > > > > > +       enum thermal_trend trend;
> > > > > >         struct thermal_cooling_device_instance *instance;
> > > > > >         struct thermal_cooling_device *cdev;
> > > > > >         long state, max_state;
> > > > > >
> > > > > > -       if (!tz->ops->get_trend ||
> > > > > > -           tz->ops->get_trend(tz, trip, (enum thermal_trend
> > > > > > *)&trend)) {
> > > > > > +       if (!tz->ops->get_trend || tz->ops->get_trend(tz, trip,
> > > > > > &trend)) {
> > > > > >                 /*
> > > > > >                  * compare the current temperature and previous
> > > > > > temperature
> > > > > >                  * to get the thermal trend, if no special requirement
> > > > > > @@ -747,11 +743,8 @@ static void thermal_zone_device_passive(struct
> > > > > > thermal_zone_device *tz,
> > > > > >         if (temp >= trip_temp) {
> > > > > >                 tz->passive = true;
> > > > > >
> > > > > > -               trend = (tz->tc1 * (temp - tz->last_temperature)) +
> > > > > > -                       (tz->tc2 * (temp - trip_temp));
> > > > > > -
> > > > > >                 /* Heating up? */
> > > > > > -               if (trend > 0) {
> > > > > > +               if (trend == THERMAL_TREND_RAISING) {
> > > > > >                         list_for_each_entry(instance,
> > > > > > &tz->cooling_devices,
> > > > > >                                             node) {
> > > > > >                                 if (instance->trip != trip)
> > > > > > @@ -762,7 +755,7 @@ static void thermal_zone_device_passive(struct
> > > > > > thermal_zone_device *tz,
> > > > > >                                 if (state++ < max_state)
> > > > > >                                         cdev->ops->set_cur_state(cdev,
> > > > > > state);
> > > > > >                         }
> > > > > > -               } else if (trend < 0) { /* Cooling off? */
> > > > > > +               } else if (trend == THERMAL_TREND_DROPPING) { /*
> > > > > > Cooling off? */
> > > > > >                         list_for_each_entry(instance,
> > > > > > &tz->cooling_devices,
> > > > > >                                             node) {
> > > > > >                                 if (instance->trip != trip)
> > > > > > @@ -1288,8 +1281,6 @@ static void remove_trip_attrs(struct
> > > > > > thermal_zone_device *tz)
> > > > > >   * @mask:      a bit string indicating the writeablility of trip
> > > > > > points
> > > > > >   * @devdata:   private device data
> > > > > >   * @ops:       standard thermal zone device callbacks
> > > > > > - * @tc1:       thermal coefficient 1 for passive calculations
> > > > > > - * @tc2:       thermal coefficient 2 for passive calculations
> > > > > >   * @passive_delay: number of milliseconds to wait between polls when
> > > > > >   *                performing passive cooling
> > > > > >   * @polling_delay: number of milliseconds to wait between polls when
> > > > > > checking
> > > > > > @@ -1297,13 +1288,12 @@ static void remove_trip_attrs(struct
> > > > > > thermal_zone_device *tz)
> > > > > >   *                driven systems)
> > > > > >   *
> > > > > >   * thermal_zone_device_unregister() must be called when the device is
> > > > > > no
> > > > > > - * longer needed. The passive cooling formula uses tc1 and tc2 as
> > > > > > described in
> > > > > > - * section 11.1.5.1 of the ACPI specification 3.0.
> > > > > > + * longer needed. The passive cooling depends on the .get_trend()
> > > > > > return value.
> > > > > >   */
> > > > > >  struct thermal_zone_device *thermal_zone_device_register(char *type,
> > > > > >         int trips, int mask, void *devdata,
> > > > > >         const struct thermal_zone_device_ops *ops,
> > > > > > -       int tc1, int tc2, int passive_delay, int polling_delay)
> > > > > > +       int passive_delay, int polling_delay)
> > > > > >  {
> > > > > >         struct thermal_zone_device *tz;
> > > > > >         struct thermal_cooling_device *pos;
> > > > > > @@ -1339,8 +1329,6 @@ struct thermal_zone_device
> > > > > > *thermal_zone_device_register(char *type,
> > > > > >         tz->device.class = &thermal_class;
> > > > > >         tz->devdata = devdata;
> > > > > >         tz->trips = trips;
> > > > > > -       tz->tc1 = tc1;
> > > > > > -       tz->tc2 = tc2;
> > > > > >         tz->passive_delay = passive_delay;
> > > > > >         tz->polling_delay = polling_delay;
> > > > > >
> > > > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > > > > index a01e3e6..d18dcf6 100644
> > > > > > --- a/include/linux/thermal.h
> > > > > > +++ b/include/linux/thermal.h
> > > > > > @@ -115,8 +115,6 @@ struct thermal_zone_device {
> > > > > >         struct thermal_attr *trip_hyst_attrs;
> > > > > >         void *devdata;
> > > > > >         int trips;
> > > > > > -       int tc1;
> > > > > > -       int tc2;
> > > > > >         int passive_delay;
> > > > > >         int polling_delay;
> > > > > >         int temperature;
> > > > > > @@ -163,8 +161,7 @@ enum {
> > > > > >  #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> > > > > >
> > > > > >  struct thermal_zone_device *thermal_zone_device_register(char *, int,
> > > > > > int,
> > > > > > -               void *, const struct thermal_zone_device_ops *, int
> > > > > > tc1,
> > > > > > -               int tc2, int passive_freq, int polling_freq);
> > > > > > +               void *, const struct thermal_zone_device_ops *, int,
> > > > > > int);
> > > > > >  void thermal_zone_device_unregister(struct thermal_zone_device *);
> > > > > >
> > > > > >  int thermal_zone_bind_cooling_device(struct thermal_zone_device *,
> > > > > > int,
> > > > > > --
> > > > > > 1.7.9.5
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > 
> > > 
> > > 
> > > --
> > > 
> > > Eduardo Valentin
> 
> 
--
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
