diff mbox series

[2/4] thermal/core: Precompute the jiffies

Message ID 20201202120657.1969-2-daniel.lezcano@linaro.org
State Superseded
Headers show
Series [1/4] thermal/core: Rename passive_delay and polling_delay with units | expand

Commit Message

Daniel Lezcano Dec. 2, 2020, 12:06 p.m. UTC 
The delays are stored in ms units and when the polling function is
called this delay is converted into jiffies at each call.

Instead of doing the conversion again and again, compute the jiffies
at init time and use the value directly when setting the polling.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

---
 drivers/thermal/thermal_core.c  |  5 +++--
 drivers/thermal/thermal_core.h  | 18 ++++++++++++++++++
 drivers/thermal/thermal_sysfs.c |  4 ++--
 include/linux/thermal.h         |  7 +++++++
 4 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Thara Gopinath Dec. 15, 2020, 2:38 p.m. UTC | #1 
Hi Daniel,

On 12/2/20 7:06 AM, Daniel Lezcano wrote:
> The delays are stored in ms units and when the polling function is

> called this delay is converted into jiffies at each call.

> 

> Instead of doing the conversion again and again, compute the jiffies

> at init time and use the value directly when setting the polling.


A generic comment. You can avoid patch 1 of this series and directly
have patch 2 , right? There is no need to rename 
polling_delay/passive_delay to *_delay_ms and then remove it again?

> 

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---

>   drivers/thermal/thermal_core.c  |  5 +++--

>   drivers/thermal/thermal_core.h  | 18 ++++++++++++++++++

>   drivers/thermal/thermal_sysfs.c |  4 ++--

>   include/linux/thermal.h         |  7 +++++++

>   4 files changed, 30 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c

> index 53f55ceca220..3111ca2c87a1 100644

> --- a/drivers/thermal/thermal_core.c

> +++ b/drivers/thermal/thermal_core.c

> @@ -1340,8 +1340,9 @@ thermal_zone_device_register(const char *type, int trips, int mask,

>   	tz->device.class = &thermal_class;

>   	tz->devdata = devdata;

>   	tz->trips = trips;

> -	tz->passive_delay_ms = passive_delay;

> -	tz->polling_delay_ms = polling_delay;

> +

> +	thermal_zone_set_passive_delay(tz, passive_delay);

> +	thermal_zone_set_polling_delay(tz, polling_delay);

>   

>   	/* sys I/F */

>   	/* Add nodes that are always present via .groups */

> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h

> index 8df600fa7b79..2c9551ed5ef8 100644

> --- a/drivers/thermal/thermal_core.h

> +++ b/drivers/thermal/thermal_core.h

> @@ -128,6 +128,24 @@ int thermal_build_list_of_policies(char *buf);

>   /* Helpers */

>   void thermal_zone_set_trips(struct thermal_zone_device *tz);

>   

> +static inline void thermal_zone_set_passive_delay(

> +	struct thermal_zone_device *tz, int delay_ms)

> +{

> +	tz->passive_delay_ms = delay_ms;

> +	tz->passive_delay_jiffies = msecs_to_jiffies(delay_ms);

> +	if (delay_ms > 1000)

> +		tz->passive_delay_jiffies = round_jiffies(tz->passive_delay_jiffies);

> +}

> +

> +static inline void thermal_zone_set_polling_delay(

> +	struct thermal_zone_device *tz, int delay_ms)

> +{

> +	tz->polling_delay_ms = delay_ms;

> +	tz->polling_delay_jiffies = msecs_to_jiffies(delay_ms);

> +	if (delay_ms > 1000)

> +		tz->polling_delay_jiffies = round_jiffies(tz->polling_delay_jiffies);

> +}


How about one function instead?
static inline void thermal_zone_set_delay_jiffies(int *delay_jiffes, int 
delay_ms)
{
	*delay_jiffies = msecs_to_jiffies(delay_ms);
	if (delay_ms > 1000)
		*delay_jiffies = round_jiffies(*delay_jiffies);
}

And then calling 
thermal_zone_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay)..

Regards
Thara
> +

>   /* sysfs I/F */

>   int thermal_zone_create_device_groups(struct thermal_zone_device *, int);

>   void thermal_zone_destroy_device_groups(struct thermal_zone_device *);

> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c

> index f465462d8aa1..9598b288a0a1 100644

> --- a/drivers/thermal/thermal_sysfs.c

> +++ b/drivers/thermal/thermal_sysfs.c

> @@ -234,11 +234,11 @@ passive_store(struct device *dev, struct device_attribute *attr,

>   

>   	if (state && !tz->forced_passive) {

>   		if (!tz->passive_delay_ms)

> -			tz->passive_delay_ms = 1000;

> +			thermal_zone_set_passive_delay(tz, 1000);

>   		thermal_zone_device_rebind_exception(tz, "Processor",

>   						     sizeof("Processor"));

>   	} else if (!state && tz->forced_passive) {

> -		tz->passive_delay_ms = 0;

> +		thermal_zone_set_passive_delay(tz, 0);

>   		thermal_zone_device_unbind_exception(tz, "Processor",

>   						     sizeof("Processor"));

>   	}

> diff --git a/include/linux/thermal.h b/include/linux/thermal.h

> index 230d451bf335..5dd9bdb6c6ad 100644

> --- a/include/linux/thermal.h

> +++ b/include/linux/thermal.h

> @@ -118,9 +118,14 @@ struct thermal_cooling_device {

>    * @trips_disabled;	bitmap for disabled trips

>    * @passive_delay_ms:	number of milliseconds to wait between polls when

>    *			performing passive cooling.

> + * @passive_delay_jiffies: number of jiffies to wait between polls when

> + *			performing passive cooling.

>    * @polling_delay_ms:	number of milliseconds to wait between polls when

>    *			checking whether trip points have been crossed (0 for

>    *			interrupt driven systems)

> + * @polling_delay_jiffies: number of jiffies to wait between polls when

> + *			checking whether trip points have been crossed (0 for

> + *			interrupt driven systems)

>    * @temperature:	current temperature.  This is only for core code,

>    *			drivers should use thermal_zone_get_temp() to get the

>    *			current temperature

> @@ -161,6 +166,8 @@ struct thermal_zone_device {

>   	unsigned long trips_disabled;	/* bitmap for disabled trips */

>   	int passive_delay_ms;

>   	int polling_delay_ms;

> +	int passive_delay_jiffies;

> +	int polling_delay_jiffies;

>   	int temperature;

>   	int last_temperature;

>   	int emul_temperature;

> 


-- 
Warm Regards
Thara
Daniel Lezcano Dec. 15, 2020, 10:58 p.m. UTC | #2 
On Tue, Dec 15, 2020 at 09:38:06AM -0500, Thara Gopinath wrote:
> Hi Daniel,

> 

> On 12/2/20 7:06 AM, Daniel Lezcano wrote:

> > The delays are stored in ms units and when the polling function is

> > called this delay is converted into jiffies at each call.

> > 

> > Instead of doing the conversion again and again, compute the jiffies

> > at init time and use the value directly when setting the polling.

> 

> A generic comment. You can avoid patch 1 of this series and directly

> have patch 2 , right? There is no need to rename polling_delay/passive_delay

> to *_delay_ms and then remove it again?


Yes, I can simplify that.

> > 

> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> > ---


[ ... ]

> > +static inline void thermal_zone_set_polling_delay(

> > +	struct thermal_zone_device *tz, int delay_ms)

> > +{

> > +	tz->polling_delay_ms = delay_ms;

> > +	tz->polling_delay_jiffies = msecs_to_jiffies(delay_ms);

> > +	if (delay_ms > 1000)

> > +		tz->polling_delay_jiffies = round_jiffies(tz->polling_delay_jiffies);

> > +}

> 

> How about one function instead?

> static inline void thermal_zone_set_delay_jiffies(int *delay_jiffes, int

> delay_ms)

> {

> 	*delay_jiffies = msecs_to_jiffies(delay_ms);

> 	if (delay_ms > 1000)

> 		*delay_jiffies = round_jiffies(*delay_jiffies);

> }

> 

> And then calling thermal_zone_set_delay_jiffies(&tz->passive_delay_jiffies,

> passive_delay)..

 
Yes, agree. I'll do this change.

Thanks for the review

  -- Daniel

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
diff mbox series

Patch

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 53f55ceca220..3111ca2c87a1 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1340,8 +1340,9 @@  thermal_zone_device_register(const char *type, int trips, int mask,
 	tz->device.class = &thermal_class;
 	tz->devdata = devdata;
 	tz->trips = trips;
-	tz->passive_delay_ms = passive_delay;
-	tz->polling_delay_ms = polling_delay;
+
+	thermal_zone_set_passive_delay(tz, passive_delay);
+	thermal_zone_set_polling_delay(tz, polling_delay);
 
 	/* sys I/F */
 	/* Add nodes that are always present via .groups */
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 8df600fa7b79..2c9551ed5ef8 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -128,6 +128,24 @@  int thermal_build_list_of_policies(char *buf);
 /* Helpers */
 void thermal_zone_set_trips(struct thermal_zone_device *tz);
 
+static inline void thermal_zone_set_passive_delay(
+	struct thermal_zone_device *tz, int delay_ms)
+{
+	tz->passive_delay_ms = delay_ms;
+	tz->passive_delay_jiffies = msecs_to_jiffies(delay_ms);
+	if (delay_ms > 1000)
+		tz->passive_delay_jiffies = round_jiffies(tz->passive_delay_jiffies);
+}
+
+static inline void thermal_zone_set_polling_delay(
+	struct thermal_zone_device *tz, int delay_ms)
+{
+	tz->polling_delay_ms = delay_ms;
+	tz->polling_delay_jiffies = msecs_to_jiffies(delay_ms);
+	if (delay_ms > 1000)
+		tz->polling_delay_jiffies = round_jiffies(tz->polling_delay_jiffies);
+}
+
 /* sysfs I/F */
 int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
 void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index f465462d8aa1..9598b288a0a1 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -234,11 +234,11 @@  passive_store(struct device *dev, struct device_attribute *attr,
 
 	if (state && !tz->forced_passive) {
 		if (!tz->passive_delay_ms)
-			tz->passive_delay_ms = 1000;
+			thermal_zone_set_passive_delay(tz, 1000);
 		thermal_zone_device_rebind_exception(tz, "Processor",
 						     sizeof("Processor"));
 	} else if (!state && tz->forced_passive) {
-		tz->passive_delay_ms = 0;
+		thermal_zone_set_passive_delay(tz, 0);
 		thermal_zone_device_unbind_exception(tz, "Processor",
 						     sizeof("Processor"));
 	}
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 230d451bf335..5dd9bdb6c6ad 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -118,9 +118,14 @@  struct thermal_cooling_device {
  * @trips_disabled;	bitmap for disabled trips
  * @passive_delay_ms:	number of milliseconds to wait between polls when
  *			performing passive cooling.
+ * @passive_delay_jiffies: number of jiffies to wait between polls when
+ *			performing passive cooling.
  * @polling_delay_ms:	number of milliseconds to wait between polls when
  *			checking whether trip points have been crossed (0 for
  *			interrupt driven systems)
+ * @polling_delay_jiffies: number of jiffies to wait between polls when
+ *			checking whether trip points have been crossed (0 for
+ *			interrupt driven systems)
  * @temperature:	current temperature.  This is only for core code,
  *			drivers should use thermal_zone_get_temp() to get the
  *			current temperature
@@ -161,6 +166,8 @@  struct thermal_zone_device {
 	unsigned long trips_disabled;	/* bitmap for disabled trips */
 	int passive_delay_ms;
 	int polling_delay_ms;
+	int passive_delay_jiffies;
+	int polling_delay_jiffies;
 	int temperature;
 	int last_temperature;
 	int emul_temperature;