[PATCH v2 0/4] thermal: of: of-thermal.c API extensions

[PATCH v2 0/4] thermal: of: of-thermal.c API extensions

[Lukasz Majewski]

Provided patch series extend of-thermal.c file API. It is necessary 
for ongoing Exynos TMU rework.
First version of this code can be found at:
http://www.spinics.net/lists/linux-samsung-soc/msg37719.html

This code provides some methods to access some of-thermal local data.
Moreover it provides a read only copy of trip points.

Lukasz Majewski (4):
  thermal: of: Extend of-thermal.c to provide number of trip points
  thermal: of: Extend of-thermal.c to provide check if trip point is
    enabled
  thermal: of: Extend of-thermal to export table of trip points
  thermal: of: Extend current of-thermal.c code to allow setting
    emulated temp

 drivers/thermal/of-thermal.c   | 97 ++++++++++++++++++++++++++++++++++++++++++
 drivers/thermal/thermal_core.h | 17 ++++++++
 include/linux/thermal.h        | 15 +++++++
 3 files changed, 129 insertions(+)

-- 
2.0.0.rc2

[PATCH v2 1/4] thermal: of: Extend of-thermal.c to provide number of trip points

[Lukasz Majewski]

This patch extends the of-thermal.c to provide information about number of
available trip points.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v2:
- Provide detailed (doxygen like) description of the of_thermal_get_ntrips()
  method
- Check for data pointer not being NULL
---
 drivers/thermal/of-thermal.c   | 20 ++++++++++++++++++++
 drivers/thermal/thermal_core.h |  5 +++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b7982f0..7170822 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -112,6 +112,26 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
 	return data->ops->get_temp(data->sensor_data, temp);
 }
 
+/**
+ * of_thermal_get_ntrips - function to export number of available trip
+ *			   points.
+ * @tz: pointer to a thermal zone
+ *
+ * This function is a globally visible wrapper to get number of trip points
+ * stored in the local struct __thermal_zone
+ *
+ * Return: number of available trip points, -ENODEV when data not available
+ */
+int of_thermal_get_ntrips(struct thermal_zone_device *tz)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data || IS_ERR(data))
+		return -ENODEV;
+
+	return data->ntrips;
+}
+
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index d15d243..c3c7b82 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -89,9 +89,14 @@ static inline void thermal_gov_user_space_unregister(void) {}
 #ifdef CONFIG_THERMAL_OF
 int of_parse_thermal_zones(void);
 void of_thermal_destroy_zones(void);
+int of_thermal_get_ntrips(struct thermal_zone_device *);
 #else
 static inline int of_parse_thermal_zones(void) { return 0; }
 static inline void of_thermal_destroy_zones(void) { }
+static inline int of_thermal_get_ntrips(struct thermal_zone_device *)
+{
+	return 0;
+}
 #endif
 
 #endif /* __THERMAL_CORE_H__ */
-- 
2.0.0.rc2

[PATCH v2 2/4] thermal: of: Extend of-thermal.c to provide check if trip point is enabled

[Lukasz Majewski]

This patch extends the of-thermal.c to provide check if trip point is
enabled.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v2:
- Replace int with bool
- Replace 1/0 with true/false
- Add check if data pointer is not NULL
- Add missing doxygen style comment for the function
---
 drivers/thermal/of-thermal.c   | 20 ++++++++++++++++++++
 drivers/thermal/thermal_core.h |  5 +++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 7170822..336af7f 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -132,6 +132,26 @@ int of_thermal_get_ntrips(struct thermal_zone_device *tz)
 	return data->ntrips;
 }
 
+/**
+ * of_thermal_is_trip_en - function to check if trip point is enabled
+ *
+ * @tz:	pointer to a thermal zone
+ * @trip:	trip point to evaluate
+ *
+ * This function is responsible for checking if passed trip point is enabled
+ *
+ * Return: true if trip point is enabled, false otherwise
+ */
+bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data || trip >= data->ntrips || trip < 0)
+		return false;
+
+	return true;
+}
+
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index c3c7b82..466208c 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -90,6 +90,7 @@ static inline void thermal_gov_user_space_unregister(void) {}
 int of_parse_thermal_zones(void);
 void of_thermal_destroy_zones(void);
 int of_thermal_get_ntrips(struct thermal_zone_device *);
+bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
 #else
 static inline int of_parse_thermal_zones(void) { return 0; }
 static inline void of_thermal_destroy_zones(void) { }
@@ -97,6 +98,10 @@ static inline int of_thermal_get_ntrips(struct thermal_zone_device *)
 {
 	return 0;
 }
+static inline bool of_thermal_is_trip_en(struct thermal_zone_device *, int)
+{
+	return 0;
+}
 #endif
 
 #endif /* __THERMAL_CORE_H__ */
-- 
2.0.0.rc2

[PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Lukasz Majewski]

This patch extends the of-thermal.c to export copy of trip points for
a given thermal zone.

Thermal drivers should use of_thermal_get_trip_points() method to get
pointer to table of thermal trip points.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v2:
- New patch - as suggested by Eduardo Valentin
---
 drivers/thermal/of-thermal.c   | 33 +++++++++++++++++++++++++++++++++
 drivers/thermal/thermal_core.h |  7 +++++++
 include/linux/thermal.h        | 14 ++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 336af7f..33921c5 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -89,6 +89,7 @@ struct __thermal_zone {
 	/* trip data */
 	int ntrips;
 	struct __thermal_trip *trips;
+	struct thermal_trip *gtrips;
 
 	/* cooling binding data */
 	int num_tbps;
@@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip)
 	return true;
 }
 
+/**
+ * of_thermal_get_trip_points - function to get access to a globally exported
+ *				trip points
+ *
+ * @tz:	pointer to a thermal zone
+ *
+ * This function provides a pointer to the copy of trip points table
+ *
+ * Return: pointer to trip points table, NULL otherwise
+ */
+const struct thermal_trip * const
+of_thermal_get_trip_points(struct thermal_zone_device *tz)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data)
+		return NULL;
+
+	return data->gtrips;
+}
+
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
@@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct device_node *np)
 			goto free_tbps;
 	}
 
+	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), GFP_KERNEL);
+	if (!tz->gtrips) {
+		ret = -ENOMEM;
+		goto free_tbps;
+	}
+
+	for (i = 0; i < tz->ntrips; i++)
+		memcpy(&(tz->gtrips[i]), &(tz->trips[i].temperature),
+		       sizeof(*tz->gtrips));
+
 finish:
 	of_node_put(child);
 	tz->mode = THERMAL_DEVICE_DISABLED;
@@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct __thermal_zone *tz)
 {
 	int i;
 
+	kfree(tz->gtrips);
 	for (i = 0; i < tz->num_tbps; i++)
 		of_node_put(tz->tbps[i].cooling_device);
 	kfree(tz->tbps);
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 466208c..a9580ca 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
 void of_thermal_destroy_zones(void);
 int of_thermal_get_ntrips(struct thermal_zone_device *);
 bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
+const struct thermal_trip * const
+of_thermal_get_trip_points(struct thermal_zone_device *);
 #else
 static inline int of_parse_thermal_zones(void) { return 0; }
 static inline void of_thermal_destroy_zones(void) { }
@@ -102,6 +104,11 @@ static inline bool of_thermal_is_trip_en(struct thermal_zone_device *, int)
 {
 	return 0;
 }
+static inline const struct thermal_trip * const
+of_thermal_get_trip_points(struct thermal_zone_device *)
+{
+	return NULL;
+}
 #endif
 
 #endif /* __THERMAL_CORE_H__ */
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 5bc28a7..88d7249 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
 	int (*get_trend)(void *, long *);
 };
 
+/**
+ * struct thermal_trip - Structure representing themal trip points exported from
+ *                       of-thermal
+ *
+ * @temperature:	trip point temperature
+ * @hysteresis:		trip point hysteresis
+ * @type:		trip point type
+ */
+struct thermal_trip {
+	unsigned long int temperature;
+	unsigned long int hysteresis;
+	enum thermal_trip_type type;
+};
+
 /* Function declarations */
 #ifdef CONFIG_THERMAL_OF
 struct thermal_zone_device *
-- 
2.0.0.rc2

[PATCH v2 4/4] thermal: of: Extend current of-thermal.c code to allow setting emulated temp

[Lukasz Majewski]

Before this change it was only possible to set get_temp() and get_trend()
methods to be used in the common code handling passing parameters via
device tree to "cpu-thermal" CPU thermal zone device.

Now it is possible to also set emulated value of temperature for debug
purposes.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v2:
- Rework the emulated temperature setting code to use of_thermal_sensor_ops
structure
---
 drivers/thermal/of-thermal.c | 24 ++++++++++++++++++++++++
 include/linux/thermal.h      |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 33921c5..ad7dc2b 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -174,6 +174,28 @@ of_thermal_get_trip_points(struct thermal_zone_device *tz)
 	return data->gtrips;
 }
 
+/**
+ * of_thermal_set_emul_temp - function to set emulated temperature
+ *
+ * @tz:	pointer to a thermal zone
+ * @temp:	temperature to set
+ *
+ * This function gives the ability to set emulated value of temperature,
+ * which is handy for debugging
+ *
+ * Return: zero on success, error code otherwise
+ */
+static int of_thermal_set_emul_temp(struct thermal_zone_device *tz,
+				    unsigned long temp)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data->ops || !data->ops->set_emul_temp)
+		return -EINVAL;
+
+	return data->ops->set_emul_temp(data->sensor_data, temp);
+}
+
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
@@ -405,6 +427,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 
 	tzd->ops->get_temp = of_thermal_get_temp;
 	tzd->ops->get_trend = of_thermal_get_trend;
+	tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
 	mutex_unlock(&tzd->lock);
 
 	return tzd;
@@ -533,6 +556,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
 	mutex_lock(&tzd->lock);
 	tzd->ops->get_temp = NULL;
 	tzd->ops->get_trend = NULL;
+	tzd->ops->set_emul_temp = NULL;
 
 	tz->ops = NULL;
 	tz->sensor_data = NULL;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 88d7249..5eb9d44 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -301,6 +301,7 @@ struct thermal_genl_event {
 struct thermal_zone_of_device_ops {
 	int (*get_temp)(void *, long *);
 	int (*get_trend)(void *, long *);
+	int (*set_emul_temp)(void *, unsigned long);
 };
 
 /**
-- 
2.0.0.rc2

Re: [PATCH v2 1/4] thermal: of: Extend of-thermal.c to provide number of trip points

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 2851 bytes --]


Lukasz,

On Thu, Nov 20, 2014 at 05:21:25PM +0100, Lukasz Majewski wrote:
> This patch extends the of-thermal.c to provide information about number of
> available trip points.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
> Changes for v2:
> - Provide detailed (doxygen like) description of the of_thermal_get_ntrips()
>   method
> - Check for data pointer not being NULL
> ---
>  drivers/thermal/of-thermal.c   | 20 ++++++++++++++++++++
>  drivers/thermal/thermal_core.h |  5 +++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b7982f0..7170822 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -112,6 +112,26 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>  	return data->ops->get_temp(data->sensor_data, temp);
>  }
>  
> +/**
> + * of_thermal_get_ntrips - function to export number of available trip
> + *			   points.
> + * @tz: pointer to a thermal zone
> + *
> + * This function is a globally visible wrapper to get number of trip points
> + * stored in the local struct __thermal_zone
> + *
> + * Return: number of available trip points, -ENODEV when data not available
> + */
> +int of_thermal_get_ntrips(struct thermal_zone_device *tz)
> +{
> +	struct __thermal_zone *data = tz->devdata;
> +
> +	if (!data || IS_ERR(data))
> +		return -ENODEV;
> +
> +	return data->ntrips;
> +}
> +

Missing
EXPORT_SYMBOL_GPL(of_thermal_get_ntrips);

>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>  				enum thermal_trend *trend)
>  {
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index d15d243..c3c7b82 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -89,9 +89,14 @@ static inline void thermal_gov_user_space_unregister(void) {}
>  #ifdef CONFIG_THERMAL_OF
>  int of_parse_thermal_zones(void);
>  void of_thermal_destroy_zones(void);
> +int of_thermal_get_ntrips(struct thermal_zone_device *);
>  #else
>  static inline int of_parse_thermal_zones(void) { return 0; }
>  static inline void of_thermal_destroy_zones(void) { }
> +static inline int of_thermal_get_ntrips(struct thermal_zone_device *)

You need to declare the parameter name with a name     ---------------^ .

> +{

This produces a compilation error if CONFIG_THERMAL_OF is not set:
In file included from drivers/thermal/step_wise.c:28:0:
drivers/thermal/thermal_core.h: In function ‘of_thermal_get_ntrips’:
drivers/thermal/thermal_core.h:96:48: error: parameter name omitted
 static inline int of_thermal_get_ntrips(struct thermal_zone_device *)



> +	return 0;
> +}
>  #endif
>  
>  #endif /* __THERMAL_CORE_H__ */
> -- 
> 2.0.0.rc2
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 2/4] thermal: of: Extend of-thermal.c to provide check if trip point is enabled

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]


Lukasz,

Same stuff here.

On Thu, Nov 20, 2014 at 05:21:26PM +0100, Lukasz Majewski wrote:
> This patch extends the of-thermal.c to provide check if trip point is
> enabled.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
> Changes for v2:
> - Replace int with bool
> - Replace 1/0 with true/false
> - Add check if data pointer is not NULL
> - Add missing doxygen style comment for the function
> ---
>  drivers/thermal/of-thermal.c   | 20 ++++++++++++++++++++
>  drivers/thermal/thermal_core.h |  5 +++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 7170822..336af7f 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -132,6 +132,26 @@ int of_thermal_get_ntrips(struct thermal_zone_device *tz)
>  	return data->ntrips;
>  }
>  
> +/**
> + * of_thermal_is_trip_en - function to check if trip point is enabled
> + *
> + * @tz:	pointer to a thermal zone
> + * @trip:	trip point to evaluate
> + *
> + * This function is responsible for checking if passed trip point is enabled
> + *
> + * Return: true if trip point is enabled, false otherwise
> + */
> +bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip)

This one looks more like "is this trip point valid?" than "is this trip
point enabled?", isn't it?

By having such a function to check if a trip is enabled, one would
expect to have functions to enable / disable trips points.

What do you think of naming it:
of_thermal_is_trip_valid()
?

> +{
> +	struct __thermal_zone *data = tz->devdata;
> +
> +	if (!data || trip >= data->ntrips || trip < 0)

Even your check is about looking if the trip value is within a range,
telling the caller if the trip is valid or not, right?

> +		return false;
> +
> +	return true;
> +}
> +

Missing:
EXPORT_SYMBOL_GPL(of_thermal_is_trip_en);

or

EXPORT_SYMBOL_GPL(of_thermal_is_trip_valid);

>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>  				enum thermal_trend *trend)
>  {
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index c3c7b82..466208c 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -90,6 +90,7 @@ static inline void thermal_gov_user_space_unregister(void) {}
>  int of_parse_thermal_zones(void);
>  void of_thermal_destroy_zones(void);
>  int of_thermal_get_ntrips(struct thermal_zone_device *);
> +bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
>  #else
>  static inline int of_parse_thermal_zones(void) { return 0; }
>  static inline void of_thermal_destroy_zones(void) { }
> @@ -97,6 +98,10 @@ static inline int of_thermal_get_ntrips(struct thermal_zone_device *)
>  {
>  	return 0;
>  }
> +static inline bool of_thermal_is_trip_en(struct thermal_zone_device *, int)

Same as in patch 01, this produces compilation error. Please, name your
parameters.

> +{
> +	return 0;
> +}
>  #endif
>  
>  #endif /* __THERMAL_CORE_H__ */
> -- 
> 2.0.0.rc2
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4711 bytes --]

Hello Lukasz,

On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
> This patch extends the of-thermal.c to export copy of trip points for
> a given thermal zone.
> 
> Thermal drivers should use of_thermal_get_trip_points() method to get
> pointer to table of thermal trip points.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
> Changes for v2:
> - New patch - as suggested by Eduardo Valentin
> ---
>  drivers/thermal/of-thermal.c   | 33 +++++++++++++++++++++++++++++++++
>  drivers/thermal/thermal_core.h |  7 +++++++
>  include/linux/thermal.h        | 14 ++++++++++++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 336af7f..33921c5 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -89,6 +89,7 @@ struct __thermal_zone {
>  	/* trip data */
>  	int ntrips;
>  	struct __thermal_trip *trips;
> +	struct thermal_trip *gtrips;
>  
>  	/* cooling binding data */
>  	int num_tbps;
> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip)
>  	return true;
>  }
>  
> +/**
> + * of_thermal_get_trip_points - function to get access to a globally exported
> + *				trip points
> + *
> + * @tz:	pointer to a thermal zone
> + *
> + * This function provides a pointer to the copy of trip points table
> + *
> + * Return: pointer to trip points table, NULL otherwise
> + */
> +const struct thermal_trip * const
> +of_thermal_get_trip_points(struct thermal_zone_device *tz)
> +{
> +	struct __thermal_zone *data = tz->devdata;
> +
> +	if (!data)
> +		return NULL;
> +
> +	return data->gtrips;
> +}
> +

EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);

>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>  				enum thermal_trend *trend)
>  {
> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct device_node *np)
>  			goto free_tbps;
>  	}
>  
> +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), GFP_KERNEL);
> +	if (!tz->gtrips) {
> +		ret = -ENOMEM;
> +		goto free_tbps;
> +	}
> +
> +	for (i = 0; i < tz->ntrips; i++)
> +		memcpy(&(tz->gtrips[i]), &(tz->trips[i].temperature),
> +		       sizeof(*tz->gtrips));
> +
>  finish:
>  	of_node_put(child);
>  	tz->mode = THERMAL_DEVICE_DISABLED;
> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct __thermal_zone *tz)
>  {
>  	int i;
>  
> +	kfree(tz->gtrips);
>  	for (i = 0; i < tz->num_tbps; i++)
>  		of_node_put(tz->tbps[i].cooling_device);
>  	kfree(tz->tbps);
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 466208c..a9580ca 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
>  void of_thermal_destroy_zones(void);
>  int of_thermal_get_ntrips(struct thermal_zone_device *);
>  bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
> +const struct thermal_trip * const
> +of_thermal_get_trip_points(struct thermal_zone_device *);
>  #else
>  static inline int of_parse_thermal_zones(void) { return 0; }
>  static inline void of_thermal_destroy_zones(void) { }
> @@ -102,6 +104,11 @@ static inline bool of_thermal_is_trip_en(struct thermal_zone_device *, int)
>  {

This produces compilation error when CONFIG_THERMAL_OF is not set. Name
the parameters to fix.

>  	return 0;
>  }
> +static inline const struct thermal_trip * const
> +of_thermal_get_trip_points(struct thermal_zone_device *)
> +{
> +	return NULL;
> +}
>  #endif
>  
>  #endif /* __THERMAL_CORE_H__ */
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 5bc28a7..88d7249 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
>  	int (*get_trend)(void *, long *);
>  };
>  
> +/**
> + * struct thermal_trip - Structure representing themal trip points exported from
> + *                       of-thermal
> + *

The only problem I have with this name is that would look like it is in
use in all thermal framework, which is not really the case. But I do
think having a type here is a good thing. So, not sure :-)

> + * @temperature:	trip point temperature
> + * @hysteresis:		trip point hysteresis
> + * @type:		trip point type
> + */
> +struct thermal_trip {
> +	unsigned long int temperature;
> +	unsigned long int hysteresis;
> +	enum thermal_trip_type type;
> +};
> +
>  /* Function declarations */
>  #ifdef CONFIG_THERMAL_OF
>  struct thermal_zone_device *
> -- 
> 2.0.0.rc2
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 4630 bytes --]

On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
> This patch extends the of-thermal.c to export copy of trip points for
> a given thermal zone.
> 
> Thermal drivers should use of_thermal_get_trip_points() method to get
> pointer to table of thermal trip points.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
> Changes for v2:
> - New patch - as suggested by Eduardo Valentin
> ---
>  drivers/thermal/of-thermal.c   | 33 +++++++++++++++++++++++++++++++++
>  drivers/thermal/thermal_core.h |  7 +++++++
>  include/linux/thermal.h        | 14 ++++++++++++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 336af7f..33921c5 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -89,6 +89,7 @@ struct __thermal_zone {
>  	/* trip data */
>  	int ntrips;
>  	struct __thermal_trip *trips;
> +	struct thermal_trip *gtrips;
>  
>  	/* cooling binding data */
>  	int num_tbps;
> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip)
>  	return true;
>  }
>  
> +/**
> + * of_thermal_get_trip_points - function to get access to a globally exported
> + *				trip points
> + *
> + * @tz:	pointer to a thermal zone
> + *
> + * This function provides a pointer to the copy of trip points table
> + *
> + * Return: pointer to trip points table, NULL otherwise
> + */
> +const struct thermal_trip * const
> +of_thermal_get_trip_points(struct thermal_zone_device *tz)

Another thing, can you please check why scripts/kernel-doc does not like
this prototype? It throws an warn:
Warning(drivers/thermal/of-thermal.c:168): cannot understand function
prototype: 'const struct thermal_trip * const
of_thermal_get_trip_points(struct thermal_zone_device *tz) '


> +{
> +	struct __thermal_zone *data = tz->devdata;
> +
> +	if (!data)
> +		return NULL;
> +
> +	return data->gtrips;
> +}
> +
>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>  				enum thermal_trend *trend)
>  {
> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct device_node *np)
>  			goto free_tbps;
>  	}
>  
> +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), GFP_KERNEL);
> +	if (!tz->gtrips) {
> +		ret = -ENOMEM;
> +		goto free_tbps;
> +	}
> +
> +	for (i = 0; i < tz->ntrips; i++)
> +		memcpy(&(tz->gtrips[i]), &(tz->trips[i].temperature),
> +		       sizeof(*tz->gtrips));
> +
>  finish:
>  	of_node_put(child);
>  	tz->mode = THERMAL_DEVICE_DISABLED;
> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct __thermal_zone *tz)
>  {
>  	int i;
>  
> +	kfree(tz->gtrips);
>  	for (i = 0; i < tz->num_tbps; i++)
>  		of_node_put(tz->tbps[i].cooling_device);
>  	kfree(tz->tbps);
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 466208c..a9580ca 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
>  void of_thermal_destroy_zones(void);
>  int of_thermal_get_ntrips(struct thermal_zone_device *);
>  bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
> +const struct thermal_trip * const
> +of_thermal_get_trip_points(struct thermal_zone_device *);
>  #else
>  static inline int of_parse_thermal_zones(void) { return 0; }
>  static inline void of_thermal_destroy_zones(void) { }
> @@ -102,6 +104,11 @@ static inline bool of_thermal_is_trip_en(struct thermal_zone_device *, int)
>  {
>  	return 0;
>  }
> +static inline const struct thermal_trip * const
> +of_thermal_get_trip_points(struct thermal_zone_device *)
> +{
> +	return NULL;
> +}
>  #endif
>  
>  #endif /* __THERMAL_CORE_H__ */
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 5bc28a7..88d7249 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
>  	int (*get_trend)(void *, long *);
>  };
>  
> +/**
> + * struct thermal_trip - Structure representing themal trip points exported from
> + *                       of-thermal
> + *
> + * @temperature:	trip point temperature
> + * @hysteresis:		trip point hysteresis
> + * @type:		trip point type
> + */
> +struct thermal_trip {
> +	unsigned long int temperature;
> +	unsigned long int hysteresis;
> +	enum thermal_trip_type type;
> +};
> +
>  /* Function declarations */
>  #ifdef CONFIG_THERMAL_OF
>  struct thermal_zone_device *
> -- 
> 2.0.0.rc2
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 4/4] thermal: of: Extend current of-thermal.c code to allow setting emulated temp

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 2926 bytes --]

On Thu, Nov 20, 2014 at 05:21:28PM +0100, Lukasz Majewski wrote:
> Before this change it was only possible to set get_temp() and get_trend()
> methods to be used in the common code handling passing parameters via
> device tree to "cpu-thermal" CPU thermal zone device.
> 
> Now it is possible to also set emulated value of temperature for debug
> purposes.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
> Changes for v2:
> - Rework the emulated temperature setting code to use of_thermal_sensor_ops
> structure
> ---
>  drivers/thermal/of-thermal.c | 24 ++++++++++++++++++++++++
>  include/linux/thermal.h      |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 33921c5..ad7dc2b 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -174,6 +174,28 @@ of_thermal_get_trip_points(struct thermal_zone_device *tz)
>  	return data->gtrips;
>  }
>  
> +/**
> + * of_thermal_set_emul_temp - function to set emulated temperature
> + *
> + * @tz:	pointer to a thermal zone
> + * @temp:	temperature to set
> + *
> + * This function gives the ability to set emulated value of temperature,
> + * which is handy for debugging
> + *
> + * Return: zero on success, error code otherwise
> + */
> +static int of_thermal_set_emul_temp(struct thermal_zone_device *tz,
> +				    unsigned long temp)
> +{
> +	struct __thermal_zone *data = tz->devdata;
> +
> +	if (!data->ops || !data->ops->set_emul_temp)
> +		return -EINVAL;
> +
> +	return data->ops->set_emul_temp(data->sensor_data, temp);
> +}
> +
>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>  				enum thermal_trend *trend)
>  {
> @@ -405,6 +427,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  
>  	tzd->ops->get_temp = of_thermal_get_temp;
>  	tzd->ops->get_trend = of_thermal_get_trend;
> +	tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
>  	mutex_unlock(&tzd->lock);
>  
>  	return tzd;
> @@ -533,6 +556,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>  	mutex_lock(&tzd->lock);
>  	tzd->ops->get_temp = NULL;
>  	tzd->ops->get_trend = NULL;
> +	tzd->ops->set_emul_temp = NULL;
>  
>  	tz->ops = NULL;
>  	tz->sensor_data = NULL;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 88d7249..5eb9d44 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -301,6 +301,7 @@ struct thermal_genl_event {
>  struct thermal_zone_of_device_ops {
>  	int (*get_temp)(void *, long *);
>  	int (*get_trend)(void *, long *);
> +	int (*set_emul_temp)(void *, unsigned long);

Please add it in the list of Optional functions in the comment above
this struct.

Apart from that:


Acked-by: Eduardo Valentin <edubezval@gmail.com>


>  };
>  
>  /**
> -- 
> 2.0.0.rc2
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 1/4] thermal: of: Extend of-thermal.c to provide number of trip points

[Lukasz Majewski]

Hi Eduardo,

> 
> Lukasz,
> 
> On Thu, Nov 20, 2014 at 05:21:25PM +0100, Lukasz Majewski wrote:
> > This patch extends the of-thermal.c to provide information about
> > number of available trip points.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> > Changes for v2:
> > - Provide detailed (doxygen like) description of the
> > of_thermal_get_ntrips() method
> > - Check for data pointer not being NULL
> > ---
> >  drivers/thermal/of-thermal.c   | 20 ++++++++++++++++++++
> >  drivers/thermal/thermal_core.h |  5 +++++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/thermal/of-thermal.c
> > b/drivers/thermal/of-thermal.c index b7982f0..7170822 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -112,6 +112,26 @@ static int of_thermal_get_temp(struct
> > thermal_zone_device *tz, return
> > data->ops->get_temp(data->sensor_data, temp); }
> >  
> > +/**
> > + * of_thermal_get_ntrips - function to export number of available
> > trip
> > + *			   points.
> > + * @tz: pointer to a thermal zone
> > + *
> > + * This function is a globally visible wrapper to get number of
> > trip points
> > + * stored in the local struct __thermal_zone
> > + *
> > + * Return: number of available trip points, -ENODEV when data not
> > available
> > + */
> > +int of_thermal_get_ntrips(struct thermal_zone_device *tz)
> > +{
> > +	struct __thermal_zone *data = tz->devdata;
> > +
> > +	if (!data || IS_ERR(data))
> > +		return -ENODEV;
> > +
> > +	return data->ntrips;
> > +}
> > +
> 
> Missing
> EXPORT_SYMBOL_GPL(of_thermal_get_ntrips);

OK

> 
> >  static int of_thermal_get_trend(struct thermal_zone_device *tz,
> > int trip, enum thermal_trend *trend)
> >  {
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h index d15d243..c3c7b82 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -89,9 +89,14 @@ static inline void
> > thermal_gov_user_space_unregister(void) {} #ifdef CONFIG_THERMAL_OF
> >  int of_parse_thermal_zones(void);
> >  void of_thermal_destroy_zones(void);
> > +int of_thermal_get_ntrips(struct thermal_zone_device *);
> >  #else
> >  static inline int of_parse_thermal_zones(void) { return 0; }
> >  static inline void of_thermal_destroy_zones(void) { }
> > +static inline int of_thermal_get_ntrips(struct thermal_zone_device
> > *)
> 
> You need to declare the parameter name with a name
> ---------------^ .
> 
> > +{
> 
> This produces a compilation error if CONFIG_THERMAL_OF is not set:
> In file included from drivers/thermal/step_wise.c:28:0:
> drivers/thermal/thermal_core.h: In function ‘of_thermal_get_ntrips’:
> drivers/thermal/thermal_core.h:96:48: error: parameter name omitted
>  static inline int of_thermal_get_ntrips(struct thermal_zone_device *)
> 

Good point. Thanks for check.

> 
> 
> > +	return 0;
> > +}
> >  #endif
> >  
> >  #endif /* __THERMAL_CORE_H__ */
> > -- 
> > 2.0.0.rc2
> > 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH v2 2/4] thermal: of: Extend of-thermal.c to provide check if trip point is enabled

[Lukasz Majewski]

Hi Eduardo,

> 
> Lukasz,
> 
> Same stuff here.
> 
> On Thu, Nov 20, 2014 at 05:21:26PM +0100, Lukasz Majewski wrote:
> > This patch extends the of-thermal.c to provide check if trip point
> > is enabled.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> > Changes for v2:
> > - Replace int with bool
> > - Replace 1/0 with true/false
> > - Add check if data pointer is not NULL
> > - Add missing doxygen style comment for the function
> > ---
> >  drivers/thermal/of-thermal.c   | 20 ++++++++++++++++++++
> >  drivers/thermal/thermal_core.h |  5 +++++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/thermal/of-thermal.c
> > b/drivers/thermal/of-thermal.c index 7170822..336af7f 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -132,6 +132,26 @@ int of_thermal_get_ntrips(struct
> > thermal_zone_device *tz) return data->ntrips;
> >  }
> >  
> > +/**
> > + * of_thermal_is_trip_en - function to check if trip point is
> > enabled
> > + *
> > + * @tz:	pointer to a thermal zone
> > + * @trip:	trip point to evaluate
> > + *
> > + * This function is responsible for checking if passed trip point
> > is enabled
> > + *
> > + * Return: true if trip point is enabled, false otherwise
> > + */
> > +bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int
> > trip)
> 
> This one looks more like "is this trip point valid?" than "is this
> trip point enabled?", isn't it?
> 
> By having such a function to check if a trip is enabled, one would
> expect to have functions to enable / disable trips points.
> 
> What do you think of naming it:
> of_thermal_is_trip_valid()
> ?
> 

Good point. I think that of_thermal_is_trip_valid() is a better name.

> > +{
> > +	struct __thermal_zone *data = tz->devdata;
> > +
> > +	if (!data || trip >= data->ntrips || trip < 0)
> 
> Even your check is about looking if the trip value is within a range,
> telling the caller if the trip is valid or not, right?

Yes. Correct. it justifies function rename to of_thermal_is_trip_valid()

> 
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> 
> Missing:
> EXPORT_SYMBOL_GPL(of_thermal_is_trip_en);
> 
> or
> 
> EXPORT_SYMBOL_GPL(of_thermal_is_trip_valid);

I will add that, no problem.

> 
> >  static int of_thermal_get_trend(struct thermal_zone_device *tz,
> > int trip, enum thermal_trend *trend)
> >  {
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h index c3c7b82..466208c 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -90,6 +90,7 @@ static inline void
> > thermal_gov_user_space_unregister(void) {} int
> > of_parse_thermal_zones(void); void of_thermal_destroy_zones(void);
> >  int of_thermal_get_ntrips(struct thermal_zone_device *);
> > +bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
> >  #else
> >  static inline int of_parse_thermal_zones(void) { return 0; }
> >  static inline void of_thermal_destroy_zones(void) { }
> > @@ -97,6 +98,10 @@ static inline int of_thermal_get_ntrips(struct
> > thermal_zone_device *) {
> >  	return 0;
> >  }
> > +static inline bool of_thermal_is_trip_en(struct
> > thermal_zone_device *, int)
> 
> Same as in patch 01, this produces compilation error. Please, name
> your parameters.

Ok.

> 
> > +{
> > +	return 0;
> > +}
> >  #endif
> >  
> >  #endif /* __THERMAL_CORE_H__ */
> > -- 
> > 2.0.0.rc2
> > 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Lukasz Majewski]

Hi Eduardo,

> Hello Lukasz,
> 
> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
> > This patch extends the of-thermal.c to export copy of trip points
> > for a given thermal zone.
> > 
> > Thermal drivers should use of_thermal_get_trip_points() method to
> > get pointer to table of thermal trip points.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> > Changes for v2:
> > - New patch - as suggested by Eduardo Valentin
> > ---
> >  drivers/thermal/of-thermal.c   | 33
> > +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h |
> > 7 +++++++ include/linux/thermal.h        | 14 ++++++++++++++
> >  3 files changed, 54 insertions(+)
> > 
> > diff --git a/drivers/thermal/of-thermal.c
> > b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -89,6 +89,7 @@ struct __thermal_zone {
> >  	/* trip data */
> >  	int ntrips;
> >  	struct __thermal_trip *trips;
> > +	struct thermal_trip *gtrips;
> >  
> >  	/* cooling binding data */
> >  	int num_tbps;
> > @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct
> > thermal_zone_device *tz, int trip) return true;
> >  }
> >  
> > +/**
> > + * of_thermal_get_trip_points - function to get access to a
> > globally exported
> > + *				trip points
> > + *
> > + * @tz:	pointer to a thermal zone
> > + *
> > + * This function provides a pointer to the copy of trip points
> > table
> > + *
> > + * Return: pointer to trip points table, NULL otherwise
> > + */
> > +const struct thermal_trip * const
> > +of_thermal_get_trip_points(struct thermal_zone_device *tz)
> > +{
> > +	struct __thermal_zone *data = tz->devdata;
> > +
> > +	if (!data)
> > +		return NULL;
> > +
> > +	return data->gtrips;
> > +}
> > +
> 
> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);

Ok.

> 
> >  static int of_thermal_get_trend(struct thermal_zone_device *tz,
> > int trip, enum thermal_trend *trend)
> >  {
> > @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct
> > device_node *np) goto free_tbps;
> >  	}
> >  
> > +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips),
> > GFP_KERNEL);
> > +	if (!tz->gtrips) {
> > +		ret = -ENOMEM;
> > +		goto free_tbps;
> > +	}
> > +
> > +	for (i = 0; i < tz->ntrips; i++)
> > +		memcpy(&(tz->gtrips[i]),
> > &(tz->trips[i].temperature),
> > +		       sizeof(*tz->gtrips));
> > +
> >  finish:
> >  	of_node_put(child);
> >  	tz->mode = THERMAL_DEVICE_DISABLED;
> > @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct
> > __thermal_zone *tz) {
> >  	int i;
> >  
> > +	kfree(tz->gtrips);
> >  	for (i = 0; i < tz->num_tbps; i++)
> >  		of_node_put(tz->tbps[i].cooling_device);
> >  	kfree(tz->tbps);
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
> >  void of_thermal_destroy_zones(void);
> >  int of_thermal_get_ntrips(struct thermal_zone_device *);
> >  bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
> > +const struct thermal_trip * const
> > +of_thermal_get_trip_points(struct thermal_zone_device *);
> >  #else
> >  static inline int of_parse_thermal_zones(void) { return 0; }
> >  static inline void of_thermal_destroy_zones(void) { }
> > @@ -102,6 +104,11 @@ static inline bool
> > of_thermal_is_trip_en(struct thermal_zone_device *, int) {
> 
> This produces compilation error when CONFIG_THERMAL_OF is not set.
> Name the parameters to fix.

As all the other cases, I will fix that.

> 
> >  	return 0;
> >  }
> > +static inline const struct thermal_trip * const
> > +of_thermal_get_trip_points(struct thermal_zone_device *)
> > +{
> > +	return NULL;
> > +}
> >  #endif
> >  
> >  #endif /* __THERMAL_CORE_H__ */
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 5bc28a7..88d7249 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
> >  	int (*get_trend)(void *, long *);
> >  };
> >  
> > +/**
> > + * struct thermal_trip - Structure representing themal trip points
> > exported from
> > + *                       of-thermal
> > + *
> 
> The only problem I have with this name is that would look like it is
> in use in all thermal framework, which is not really the case. But I
> do think having a type here is a good thing. So, not sure :-)

It can stay to be struct thermal_trip or we can rename it to
struct of_thermal_trip.

I'm fine with both names.

> 
> > + * @temperature:	trip point temperature
> > + * @hysteresis:		trip point hysteresis
> > + * @type:		trip point type
> > + */
> > +struct thermal_trip {
> > +	unsigned long int temperature;
> > +	unsigned long int hysteresis;
> > +	enum thermal_trip_type type;
> > +};
> > +
> >  /* Function declarations */
> >  #ifdef CONFIG_THERMAL_OF
> >  struct thermal_zone_device *
> > -- 
> > 2.0.0.rc2
> > 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Lukasz Majewski]

Hi Eduardo,

> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
> > This patch extends the of-thermal.c to export copy of trip points
> > for a given thermal zone.
> > 
> > Thermal drivers should use of_thermal_get_trip_points() method to
> > get pointer to table of thermal trip points.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> > Changes for v2:
> > - New patch - as suggested by Eduardo Valentin
> > ---
> >  drivers/thermal/of-thermal.c   | 33
> > +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h |
> > 7 +++++++ include/linux/thermal.h        | 14 ++++++++++++++
> >  3 files changed, 54 insertions(+)
> > 
> > diff --git a/drivers/thermal/of-thermal.c
> > b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -89,6 +89,7 @@ struct __thermal_zone {
> >  	/* trip data */
> >  	int ntrips;
> >  	struct __thermal_trip *trips;
> > +	struct thermal_trip *gtrips;
> >  
> >  	/* cooling binding data */
> >  	int num_tbps;
> > @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct
> > thermal_zone_device *tz, int trip) return true;
> >  }
> >  
> > +/**
> > + * of_thermal_get_trip_points - function to get access to a
> > globally exported
> > + *				trip points
> > + *
> > + * @tz:	pointer to a thermal zone
> > + *
> > + * This function provides a pointer to the copy of trip points
> > table
> > + *
> > + * Return: pointer to trip points table, NULL otherwise
> > + */
> > +const struct thermal_trip * const
> > +of_thermal_get_trip_points(struct thermal_zone_device *tz)
> 
> Another thing, can you please check why scripts/kernel-doc does not
> like this prototype? It throws an warn:
> Warning(drivers/thermal/of-thermal.c:168): cannot understand function
> prototype: 'const struct thermal_trip * const
> of_thermal_get_trip_points(struct thermal_zone_device *tz) '
> 

I will check that. However the warning looks a bit strange.

> 
> > +{
> > +	struct __thermal_zone *data = tz->devdata;
> > +
> > +	if (!data)
> > +		return NULL;
> > +
> > +	return data->gtrips;
> > +}
> > +
> >  static int of_thermal_get_trend(struct thermal_zone_device *tz,
> > int trip, enum thermal_trend *trend)
> >  {
> > @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct
> > device_node *np) goto free_tbps;
> >  	}
> >  
> > +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips),
> > GFP_KERNEL);
> > +	if (!tz->gtrips) {
> > +		ret = -ENOMEM;
> > +		goto free_tbps;
> > +	}
> > +
> > +	for (i = 0; i < tz->ntrips; i++)
> > +		memcpy(&(tz->gtrips[i]),
> > &(tz->trips[i].temperature),
> > +		       sizeof(*tz->gtrips));
> > +
> >  finish:
> >  	of_node_put(child);
> >  	tz->mode = THERMAL_DEVICE_DISABLED;
> > @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct
> > __thermal_zone *tz) {
> >  	int i;
> >  
> > +	kfree(tz->gtrips);
> >  	for (i = 0; i < tz->num_tbps; i++)
> >  		of_node_put(tz->tbps[i].cooling_device);
> >  	kfree(tz->tbps);
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
> >  void of_thermal_destroy_zones(void);
> >  int of_thermal_get_ntrips(struct thermal_zone_device *);
> >  bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
> > +const struct thermal_trip * const
> > +of_thermal_get_trip_points(struct thermal_zone_device *);
> >  #else
> >  static inline int of_parse_thermal_zones(void) { return 0; }
> >  static inline void of_thermal_destroy_zones(void) { }
> > @@ -102,6 +104,11 @@ static inline bool
> > of_thermal_is_trip_en(struct thermal_zone_device *, int) {
> >  	return 0;
> >  }
> > +static inline const struct thermal_trip * const
> > +of_thermal_get_trip_points(struct thermal_zone_device *)
> > +{
> > +	return NULL;
> > +}
> >  #endif
> >  
> >  #endif /* __THERMAL_CORE_H__ */
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 5bc28a7..88d7249 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
> >  	int (*get_trend)(void *, long *);
> >  };
> >  
> > +/**
> > + * struct thermal_trip - Structure representing themal trip points
> > exported from
> > + *                       of-thermal
> > + *
> > + * @temperature:	trip point temperature
> > + * @hysteresis:		trip point hysteresis
> > + * @type:		trip point type
> > + */
> > +struct thermal_trip {
> > +	unsigned long int temperature;
> > +	unsigned long int hysteresis;
> > +	enum thermal_trip_type type;
> > +};
> > +
> >  /* Function declarations */
> >  #ifdef CONFIG_THERMAL_OF
> >  struct thermal_zone_device *
> > -- 
> > 2.0.0.rc2
> > 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH v2 4/4] thermal: of: Extend current of-thermal.c code to allow setting emulated temp

[Lukasz Majewski]

Hi Eduardo,

> On Thu, Nov 20, 2014 at 05:21:28PM +0100, Lukasz Majewski wrote:
> > Before this change it was only possible to set get_temp() and
> > get_trend() methods to be used in the common code handling passing
> > parameters via device tree to "cpu-thermal" CPU thermal zone device.
> > 
> > Now it is possible to also set emulated value of temperature for
> > debug purposes.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> > Changes for v2:
> > - Rework the emulated temperature setting code to use
> > of_thermal_sensor_ops structure
> > ---
> >  drivers/thermal/of-thermal.c | 24 ++++++++++++++++++++++++
> >  include/linux/thermal.h      |  1 +
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/thermal/of-thermal.c
> > b/drivers/thermal/of-thermal.c index 33921c5..ad7dc2b 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -174,6 +174,28 @@ of_thermal_get_trip_points(struct
> > thermal_zone_device *tz) return data->gtrips;
> >  }
> >  
> > +/**
> > + * of_thermal_set_emul_temp - function to set emulated temperature
> > + *
> > + * @tz:	pointer to a thermal zone
> > + * @temp:	temperature to set
> > + *
> > + * This function gives the ability to set emulated value of
> > temperature,
> > + * which is handy for debugging
> > + *
> > + * Return: zero on success, error code otherwise
> > + */
> > +static int of_thermal_set_emul_temp(struct thermal_zone_device *tz,
> > +				    unsigned long temp)
> > +{
> > +	struct __thermal_zone *data = tz->devdata;
> > +
> > +	if (!data->ops || !data->ops->set_emul_temp)
> > +		return -EINVAL;
> > +
> > +	return data->ops->set_emul_temp(data->sensor_data, temp);
> > +}
> > +
> >  static int of_thermal_get_trend(struct thermal_zone_device *tz,
> > int trip, enum thermal_trend *trend)
> >  {
> > @@ -405,6 +427,7 @@ thermal_zone_of_add_sensor(struct device_node
> > *zone, 
> >  	tzd->ops->get_temp = of_thermal_get_temp;
> >  	tzd->ops->get_trend = of_thermal_get_trend;
> > +	tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
> >  	mutex_unlock(&tzd->lock);
> >  
> >  	return tzd;
> > @@ -533,6 +556,7 @@ void thermal_zone_of_sensor_unregister(struct
> > device *dev, mutex_lock(&tzd->lock);
> >  	tzd->ops->get_temp = NULL;
> >  	tzd->ops->get_trend = NULL;
> > +	tzd->ops->set_emul_temp = NULL;
> >  
> >  	tz->ops = NULL;
> >  	tz->sensor_data = NULL;
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 88d7249..5eb9d44 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -301,6 +301,7 @@ struct thermal_genl_event {
> >  struct thermal_zone_of_device_ops {
> >  	int (*get_temp)(void *, long *);
> >  	int (*get_trend)(void *, long *);
> > +	int (*set_emul_temp)(void *, unsigned long);
> 
> Please add it in the list of Optional functions in the comment above
> this struct.

I will add proper comment to struct thermal_zone_of_device_ops comment.

> 
> Apart from that:
> 
> 
> Acked-by: Eduardo Valentin <edubezval@gmail.com>
> 
> 
> >  };
> >  
> >  /**
> > -- 
> > 2.0.0.rc2
> > 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 5779 bytes --]

Hello,

On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote:
> Hi Eduardo,
> 
> > Hello Lukasz,
> > 
> > On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
> > > This patch extends the of-thermal.c to export copy of trip points
> > > for a given thermal zone.
> > > 
> > > Thermal drivers should use of_thermal_get_trip_points() method to
> > > get pointer to table of thermal trip points.
> > > 
> > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > ---
> > > Changes for v2:
> > > - New patch - as suggested by Eduardo Valentin
> > > ---
> > >  drivers/thermal/of-thermal.c   | 33
> > > +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h |
> > > 7 +++++++ include/linux/thermal.h        | 14 ++++++++++++++
> > >  3 files changed, 54 insertions(+)
> > > 
> > > diff --git a/drivers/thermal/of-thermal.c
> > > b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644
> > > --- a/drivers/thermal/of-thermal.c
> > > +++ b/drivers/thermal/of-thermal.c
> > > @@ -89,6 +89,7 @@ struct __thermal_zone {
> > >  	/* trip data */
> > >  	int ntrips;
> > >  	struct __thermal_trip *trips;
> > > +	struct thermal_trip *gtrips;
> > >  
> > >  	/* cooling binding data */
> > >  	int num_tbps;
> > > @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct
> > > thermal_zone_device *tz, int trip) return true;
> > >  }
> > >  
> > > +/**
> > > + * of_thermal_get_trip_points - function to get access to a
> > > globally exported
> > > + *				trip points
> > > + *
> > > + * @tz:	pointer to a thermal zone
> > > + *
> > > + * This function provides a pointer to the copy of trip points
> > > table
> > > + *
> > > + * Return: pointer to trip points table, NULL otherwise
> > > + */
> > > +const struct thermal_trip * const
> > > +of_thermal_get_trip_points(struct thermal_zone_device *tz)
> > > +{
> > > +	struct __thermal_zone *data = tz->devdata;
> > > +
> > > +	if (!data)
> > > +		return NULL;
> > > +
> > > +	return data->gtrips;
> > > +}
> > > +
> > 
> > EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
> 
> Ok.
> 
> > 
> > >  static int of_thermal_get_trend(struct thermal_zone_device *tz,
> > > int trip, enum thermal_trend *trend)
> > >  {
> > > @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct
> > > device_node *np) goto free_tbps;
> > >  	}
> > >  
> > > +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips),
> > > GFP_KERNEL);
> > > +	if (!tz->gtrips) {
> > > +		ret = -ENOMEM;
> > > +		goto free_tbps;
> > > +	}
> > > +
> > > +	for (i = 0; i < tz->ntrips; i++)
> > > +		memcpy(&(tz->gtrips[i]),
> > > &(tz->trips[i].temperature),
> > > +		       sizeof(*tz->gtrips));
> > > +
> > >  finish:
> > >  	of_node_put(child);
> > >  	tz->mode = THERMAL_DEVICE_DISABLED;
> > > @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct
> > > __thermal_zone *tz) {
> > >  	int i;
> > >  
> > > +	kfree(tz->gtrips);
> > >  	for (i = 0; i < tz->num_tbps; i++)
> > >  		of_node_put(tz->tbps[i].cooling_device);
> > >  	kfree(tz->tbps);
> > > diff --git a/drivers/thermal/thermal_core.h
> > > b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644
> > > --- a/drivers/thermal/thermal_core.h
> > > +++ b/drivers/thermal/thermal_core.h
> > > @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
> > >  void of_thermal_destroy_zones(void);
> > >  int of_thermal_get_ntrips(struct thermal_zone_device *);
> > >  bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
> > > +const struct thermal_trip * const
> > > +of_thermal_get_trip_points(struct thermal_zone_device *);
> > >  #else
> > >  static inline int of_parse_thermal_zones(void) { return 0; }
> > >  static inline void of_thermal_destroy_zones(void) { }
> > > @@ -102,6 +104,11 @@ static inline bool
> > > of_thermal_is_trip_en(struct thermal_zone_device *, int) {
> > 
> > This produces compilation error when CONFIG_THERMAL_OF is not set.
> > Name the parameters to fix.
> 
> As all the other cases, I will fix that.
> 
> > 
> > >  	return 0;
> > >  }
> > > +static inline const struct thermal_trip * const
> > > +of_thermal_get_trip_points(struct thermal_zone_device *)
> > > +{
> > > +	return NULL;
> > > +}
> > >  #endif
> > >  
> > >  #endif /* __THERMAL_CORE_H__ */
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index 5bc28a7..88d7249 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
> > >  	int (*get_trend)(void *, long *);
> > >  };
> > >  
> > > +/**
> > > + * struct thermal_trip - Structure representing themal trip points
> > > exported from
> > > + *                       of-thermal
> > > + *
> > 
> > The only problem I have with this name is that would look like it is
> > in use in all thermal framework, which is not really the case. But I
> > do think having a type here is a good thing. So, not sure :-)
> 
> It can stay to be struct thermal_trip or we can rename it to
> struct of_thermal_trip.
> 
> I'm fine with both names.

Leave it as 'thermal_trip'.

> 
> > 
> > > + * @temperature:	trip point temperature
> > > + * @hysteresis:		trip point hysteresis
> > > + * @type:		trip point type
> > > + */
> > > +struct thermal_trip {
> > > +	unsigned long int temperature;
> > > +	unsigned long int hysteresis;
> > > +	enum thermal_trip_type type;
> > > +};
> > > +
> > >  /* Function declarations */
> > >  #ifdef CONFIG_THERMAL_OF
> > >  struct thermal_zone_device *
> > > -- 
> > > 2.0.0.rc2
> > > 
> 
> 
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[navneet kumar]

Hi Eduardo, Lukasz,

[Combining my concerns as a single text blob here]

I. Firstly, with the current patch
	1. is it really needed to duplicate the struct thermal_trip? Why don’t
	we get rid of the __thermal_trip and stay with the 'thermal_trip' ? it
	is not a big change.

	2. gtrips is not updated on "set_trip_temp" etc. actions via sysfs. (am
	I missing something?).

II. The other concern is more of a design question
	1. Do we intend to keep the of-thermal as a middle layer between the
	thermal_core and the sensor device? OR, is the role of of-thermal just
	to parse the DT and opt out ? currently of-thermal is somewhat a hybrid
	of these as, in addition to parsing the dt, it holds on to the data
	related to trip points etc. etc.

	2. assuming latter is true (OF is just a dt parser helper): we should
	not be adding more intelligence and dependencies linked to the OF.

	3. assuming former is true (OF is a well-defined middle layer): All is
	good till the point OF maintains the trip points (which is a good thing
	since, OF caches on to the data); BUT, if we expose this information to
	the sensor device too (as this patch is doing),

		3a. we violate the layering principle :-)

		3b. more importantly, this is all just excessive logic that we
		put in which *could be useful* only if we intend to extend the
		role of OF as a trip point management layer that does more than
		just holding on to the data. This may include -

			-> The sensor devices to know nothing about the
			trip_points, instead the sensor devices would work on
			"temperature thresholds" and OF would map sensor
			thresholds to the actual trip points as needed
			(configured from DT); while the sensor devices stick to
			using "thresholds".

			-> Queuing from above, sensors, most of the time, only
			need to know a high and a low temp threshold; which
			essentially is a subset of the active/passive etc. trip
			points. Calculation of that based on the current temp,
			as of today is replicated across all the sensor drivers
			and can be hoisted up to the of-thermal.

Seems like this is the opportune time to make a call about the role of of-thermal?

On 11/26/2014 07:18 AM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> Hello,
> 
> On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote:
>> Hi Eduardo,
>>
>>> Hello Lukasz,
>>>
>>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
>>>> This patch extends the of-thermal.c to export copy of trip points
>>>> for a given thermal zone.
>>>>
>>>> Thermal drivers should use of_thermal_get_trip_points() method to
>>>> get pointer to table of thermal trip points.
>>>>
>>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>>>> ---
>>>> Changes for v2:
>>>> - New patch - as suggested by Eduardo Valentin
>>>> ---
>>>>  drivers/thermal/of-thermal.c   | 33
>>>> +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h |
>>>> 7 +++++++ include/linux/thermal.h        | 14 ++++++++++++++
>>>>  3 files changed, 54 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/of-thermal.c
>>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644
>>>> --- a/drivers/thermal/of-thermal.c
>>>> +++ b/drivers/thermal/of-thermal.c
>>>> @@ -89,6 +89,7 @@ struct __thermal_zone {
>>>>  	/* trip data */
>>>>  	int ntrips;
>>>>  	struct __thermal_trip *trips;
>>>> +	struct thermal_trip *gtrips;
Do we really need this duplication ?
>>>>  
>>>>  	/* cooling binding data */
>>>>  	int num_tbps;
>>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct
>>>> thermal_zone_device *tz, int trip) return true;
>>>>  }
>>>>  
>>>> +/**
>>>> + * of_thermal_get_trip_points - function to get access to a
>>>> globally exported
>>>> + *				trip points
>>>> + *
>>>> + * @tz:	pointer to a thermal zone
>>>> + *
>>>> + * This function provides a pointer to the copy of trip points
>>>> table
>>>> + *
>>>> + * Return: pointer to trip points table, NULL otherwise
>>>> + */
>>>> +const struct thermal_trip * const
>>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz)
>>>> +{
>>>> +	struct __thermal_zone *data = tz->devdata;
>>>> +
>>>> +	if (!data)
>>>> +		return NULL;
>>>> +
>>>> +	return data->gtrips;
>>>> +}
>>>> +
>>>
>>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
>>
>> Ok.
>>
>>>
>>>>  static int of_thermal_get_trend(struct thermal_zone_device *tz,
>>>> int trip, enum thermal_trend *trend)
>>>>  {
>>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct
>>>> device_node *np) goto free_tbps;
>>>>  	}
>>>>  
>>>> +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips),
>>>> GFP_KERNEL);
>>>> +	if (!tz->gtrips) {
>>>> +		ret = -ENOMEM;
>>>> +		goto free_tbps;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < tz->ntrips; i++)
>>>> +		memcpy(&(tz->gtrips[i]),
>>>> &(tz->trips[i].temperature),
>>>> +		       sizeof(*tz->gtrips));
>>>> +
>>>>  finish:
>>>>  	of_node_put(child);
>>>>  	tz->mode = THERMAL_DEVICE_DISABLED;
>>>> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct
>>>> __thermal_zone *tz) {
>>>>  	int i;
>>>>  
>>>> +	kfree(tz->gtrips);
>>>>  	for (i = 0; i < tz->num_tbps; i++)
>>>>  		of_node_put(tz->tbps[i].cooling_device);
>>>>  	kfree(tz->tbps);
Couldn't find the code that updates *gtrips as a result of set_trip_temp via
sysfs.

>>>> diff --git a/drivers/thermal/thermal_core.h
>>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644
>>>> --- a/drivers/thermal/thermal_core.h
>>>> +++ b/drivers/thermal/thermal_core.h
>>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
>>>>  void of_thermal_destroy_zones(void);
>>>>  int of_thermal_get_ntrips(struct thermal_zone_device *);
>>>>  bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
>>>> +const struct thermal_trip * const
>>>> +of_thermal_get_trip_points(struct thermal_zone_device *);
>>>>  #else
>>>>  static inline int of_parse_thermal_zones(void) { return 0; }
>>>>  static inline void of_thermal_destroy_zones(void) { }
>>>> @@ -102,6 +104,11 @@ static inline bool
>>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) {
>>>
>>> This produces compilation error when CONFIG_THERMAL_OF is not set.
>>> Name the parameters to fix.
>>
>> As all the other cases, I will fix that.
>>
>>>
>>>>  	return 0;
>>>>  }
>>>> +static inline const struct thermal_trip * const
>>>> +of_thermal_get_trip_points(struct thermal_zone_device *)
>>>> +{
>>>> +	return NULL;
>>>> +}
>>>>  #endif
>>>>  
>>>>  #endif /* __THERMAL_CORE_H__ */
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index 5bc28a7..88d7249 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
>>>>  	int (*get_trend)(void *, long *);
>>>>  };
>>>>  
>>>> +/**
>>>> + * struct thermal_trip - Structure representing themal trip points
>>>> exported from
>>>> + *                       of-thermal
>>>> + *
>>>
>>> The only problem I have with this name is that would look like it is
>>> in use in all thermal framework, which is not really the case. But I
>>> do think having a type here is a good thing. So, not sure :-)
>>
>> It can stay to be struct thermal_trip or we can rename it to
>> struct of_thermal_trip.
>>
>> I'm fine with both names.
> 
> Leave it as 'thermal_trip'.
> 
>>
>>>
>>>> + * @temperature:	trip point temperature
>>>> + * @hysteresis:		trip point hysteresis
>>>> + * @type:		trip point type
>>>> + */
>>>> +struct thermal_trip {
>>>> +	unsigned long int temperature;
>>>> +	unsigned long int hysteresis;
>>>> +	enum thermal_trip_type type;
>>>> +};
>>>> +
>>>>  /* Function declarations */
>>>>  #ifdef CONFIG_THERMAL_OF
>>>>  struct thermal_zone_device *
>>>> -- 
>>>> 2.0.0.rc2
>>>>
>>
>>
>>
>> -- 
>> Best regards,
>>
>> Lukasz Majewski
>>
>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> 
> * Unknown Key
> * 0x7DA4E256
> 

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Guenter Roeck]

On 11/26/2014 12:43 PM, navneet kumar wrote:
>
> Hi Eduardo, Lukasz,
>
> [Combining my concerns as a single text blob here]
>
> I. Firstly, with the current patch
> 	1. is it really needed to duplicate the struct thermal_trip? Why don’t
> 	we get rid of the __thermal_trip and stay with the 'thermal_trip' ? it
> 	is not a big change.
>
> 	2. gtrips is not updated on "set_trip_temp" etc. actions via sysfs. (am
> 	I missing something?).
>
> II. The other concern is more of a design question
> 	1. Do we intend to keep the of-thermal as a middle layer between the
> 	thermal_core and the sensor device? OR, is the role of of-thermal just
> 	to parse the DT and opt out ? currently of-thermal is somewhat a hybrid
> 	of these as, in addition to parsing the dt, it holds on to the data
> 	related to trip points etc. etc.
>
> 	2. assuming latter is true (OF is just a dt parser helper): we should
> 	not be adding more intelligence and dependencies linked to the OF.
>
> 	3. assuming former is true (OF is a well-defined middle layer): All is
> 	good till the point OF maintains the trip points (which is a good thing
> 	since, OF caches on to the data); BUT, if we expose this information to
> 	the sensor device too (as this patch is doing),
>
> 		3a. we violate the layering principle :-)
>
> 		3b. more importantly, this is all just excessive logic that we
> 		put in which *could be useful* only if we intend to extend the
> 		role of OF as a trip point management layer that does more than
> 		just holding on to the data. This may include -
>
> 			-> The sensor devices to know nothing about the
> 			trip_points, instead the sensor devices would work on
> 			"temperature thresholds" and OF would map sensor
> 			thresholds to the actual trip points as needed
> 			(configured from DT); while the sensor devices stick to
> 			using "thresholds".
>
> 			-> Queuing from above, sensors, most of the time, only
> 			need to know a high and a low temp threshold; which
> 			essentially is a subset of the active/passive etc. trip
> 			points. Calculation of that based on the current temp,
> 			as of today is replicated across all the sensor drivers
> 			and can be hoisted up to the of-thermal.
>

Sorry, lost you here. What replicated calculation do you refer to ?

Thanks,
Guenter

> Seems like this is the opportune time to make a call about the role of of-thermal?
>
> On 11/26/2014 07:18 AM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> Hello,
>>
>> On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote:
>>> Hi Eduardo,
>>>
>>>> Hello Lukasz,
>>>>
>>>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
>>>>> This patch extends the of-thermal.c to export copy of trip points
>>>>> for a given thermal zone.
>>>>>
>>>>> Thermal drivers should use of_thermal_get_trip_points() method to
>>>>> get pointer to table of thermal trip points.
>>>>>
>>>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>> ---
>>>>> Changes for v2:
>>>>> - New patch - as suggested by Eduardo Valentin
>>>>> ---
>>>>>   drivers/thermal/of-thermal.c   | 33
>>>>> +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h |
>>>>> 7 +++++++ include/linux/thermal.h        | 14 ++++++++++++++
>>>>>   3 files changed, 54 insertions(+)
>>>>>
>>>>> diff --git a/drivers/thermal/of-thermal.c
>>>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644
>>>>> --- a/drivers/thermal/of-thermal.c
>>>>> +++ b/drivers/thermal/of-thermal.c
>>>>> @@ -89,6 +89,7 @@ struct __thermal_zone {
>>>>>   	/* trip data */
>>>>>   	int ntrips;
>>>>>   	struct __thermal_trip *trips;
>>>>> +	struct thermal_trip *gtrips;
> Do we really need this duplication ?
>>>>>
>>>>>   	/* cooling binding data */
>>>>>   	int num_tbps;
>>>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct
>>>>> thermal_zone_device *tz, int trip) return true;
>>>>>   }
>>>>>
>>>>> +/**
>>>>> + * of_thermal_get_trip_points - function to get access to a
>>>>> globally exported
>>>>> + *				trip points
>>>>> + *
>>>>> + * @tz:	pointer to a thermal zone
>>>>> + *
>>>>> + * This function provides a pointer to the copy of trip points
>>>>> table
>>>>> + *
>>>>> + * Return: pointer to trip points table, NULL otherwise
>>>>> + */
>>>>> +const struct thermal_trip * const
>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz)
>>>>> +{
>>>>> +	struct __thermal_zone *data = tz->devdata;
>>>>> +
>>>>> +	if (!data)
>>>>> +		return NULL;
>>>>> +
>>>>> +	return data->gtrips;
>>>>> +}
>>>>> +
>>>>
>>>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
>>>
>>> Ok.
>>>
>>>>
>>>>>   static int of_thermal_get_trend(struct thermal_zone_device *tz,
>>>>> int trip, enum thermal_trend *trend)
>>>>>   {
>>>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct
>>>>> device_node *np) goto free_tbps;
>>>>>   	}
>>>>>
>>>>> +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips),
>>>>> GFP_KERNEL);
>>>>> +	if (!tz->gtrips) {
>>>>> +		ret = -ENOMEM;
>>>>> +		goto free_tbps;
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < tz->ntrips; i++)
>>>>> +		memcpy(&(tz->gtrips[i]),
>>>>> &(tz->trips[i].temperature),
>>>>> +		       sizeof(*tz->gtrips));
>>>>> +
>>>>>   finish:
>>>>>   	of_node_put(child);
>>>>>   	tz->mode = THERMAL_DEVICE_DISABLED;
>>>>> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct
>>>>> __thermal_zone *tz) {
>>>>>   	int i;
>>>>>
>>>>> +	kfree(tz->gtrips);
>>>>>   	for (i = 0; i < tz->num_tbps; i++)
>>>>>   		of_node_put(tz->tbps[i].cooling_device);
>>>>>   	kfree(tz->tbps);
> Couldn't find the code that updates *gtrips as a result of set_trip_temp via
> sysfs.
>
>>>>> diff --git a/drivers/thermal/thermal_core.h
>>>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644
>>>>> --- a/drivers/thermal/thermal_core.h
>>>>> +++ b/drivers/thermal/thermal_core.h
>>>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
>>>>>   void of_thermal_destroy_zones(void);
>>>>>   int of_thermal_get_ntrips(struct thermal_zone_device *);
>>>>>   bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
>>>>> +const struct thermal_trip * const
>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *);
>>>>>   #else
>>>>>   static inline int of_parse_thermal_zones(void) { return 0; }
>>>>>   static inline void of_thermal_destroy_zones(void) { }
>>>>> @@ -102,6 +104,11 @@ static inline bool
>>>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) {
>>>>
>>>> This produces compilation error when CONFIG_THERMAL_OF is not set.
>>>> Name the parameters to fix.
>>>
>>> As all the other cases, I will fix that.
>>>
>>>>
>>>>>   	return 0;
>>>>>   }
>>>>> +static inline const struct thermal_trip * const
>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *)
>>>>> +{
>>>>> +	return NULL;
>>>>> +}
>>>>>   #endif
>>>>>
>>>>>   #endif /* __THERMAL_CORE_H__ */
>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>>> index 5bc28a7..88d7249 100644
>>>>> --- a/include/linux/thermal.h
>>>>> +++ b/include/linux/thermal.h
>>>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
>>>>>   	int (*get_trend)(void *, long *);
>>>>>   };
>>>>>
>>>>> +/**
>>>>> + * struct thermal_trip - Structure representing themal trip points
>>>>> exported from
>>>>> + *                       of-thermal
>>>>> + *
>>>>
>>>> The only problem I have with this name is that would look like it is
>>>> in use in all thermal framework, which is not really the case. But I
>>>> do think having a type here is a good thing. So, not sure :-)
>>>
>>> It can stay to be struct thermal_trip or we can rename it to
>>> struct of_thermal_trip.
>>>
>>> I'm fine with both names.
>>
>> Leave it as 'thermal_trip'.
>>
>>>
>>>>
>>>>> + * @temperature:	trip point temperature
>>>>> + * @hysteresis:		trip point hysteresis
>>>>> + * @type:		trip point type
>>>>> + */
>>>>> +struct thermal_trip {
>>>>> +	unsigned long int temperature;
>>>>> +	unsigned long int hysteresis;
>>>>> +	enum thermal_trip_type type;
>>>>> +};
>>>>> +
>>>>>   /* Function declarations */
>>>>>   #ifdef CONFIG_THERMAL_OF
>>>>>   struct thermal_zone_device *
>>>>> --
>>>>> 2.0.0.rc2
>>>>>
>>>
>>>
>>>
>>> --
>>> Best regards,
>>>
>>> Lukasz Majewski
>>>
>>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>>
>> * Unknown Key
>> * 0x7DA4E256
>>
>

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 11604 bytes --]


Hello Navneet,

On Wed, Nov 26, 2014 at 12:43:18PM -0800, navneet kumar wrote:
> 
> Hi Eduardo, Lukasz,
> 
> [Combining my concerns as a single text blob here]
> 
> I. Firstly, with the current patch
> 	1. is it really needed to duplicate the struct thermal_trip? Why don’t
> 	we get rid of the __thermal_trip and stay with the 'thermal_trip' ? it
> 	is not a big change.
> 
> 	2. gtrips is not updated on "set_trip_temp" etc. actions via sysfs. (am
> 	I missing something?).
> 

Good! Comments are always welcome, that's how we make patches :-).

Both above make sense to me.

> II. The other concern is more of a design question
> 	1. Do we intend to keep the of-thermal as a middle layer between the
> 	thermal_core and the sensor device? OR, is the role of of-thermal just
> 	to parse the DT and opt out ? currently of-thermal is somewhat a hybrid
> 	of these as, in addition to parsing the dt, it holds on to the data
> 	related to trip points etc. etc.
> 

of-thermal has always been as your latter statement, and I intend to keep it as it should
be, just a of parser for thermal data.

> 	2. assuming latter is true (OF is just a dt parser helper): we should
> 	not be adding more intelligence and dependencies linked to the OF.
> 

Yes this is right. We should never be adding intelligence to it, except
for parsing the thermal data out of device tree and adding the proper
structures inside the kernel.

> 	3. assuming former is true (OF is a well-defined middle layer): All is
> 	good till the point OF maintains the trip points (which is a good thing
> 	since, OF caches on to the data); BUT, if we expose this information to
> 	the sensor device too (as this patch is doing),
> 

the former is not true.

> 		3a. we violate the layering principle :-)
> 

well, even if it was, I disagree here. :-) The split between data
coming from DT and the driver is still in place. Besides, there is other
layers that expose some of their data, and that doesn't violate their
layering principles. CPUfreq, for one closer example, exposes its freq table.

It would be different if by exposing this data, the users would be
affecting the behavior of the layer. And that is not the intention of
cpufreq table. In the same way, that is not the intention of this patch.

> 		3b. more importantly, this is all just excessive logic that we
> 		put in which *could be useful* only if we intend to extend the
> 		role of OF as a trip point management layer that does more than
> 		just holding on to the data. This may include -
> 
> 			-> The sensor devices to know nothing about the
> 			trip_points, instead the sensor devices would work on
> 			"temperature thresholds" and OF would map sensor
> 			thresholds to the actual trip points as needed
> 			(configured from DT); while the sensor devices stick to
> 			using "thresholds".
> 
> 			-> Queuing from above, sensors, most of the time, only
> 			need to know a high and a low temp threshold; which
> 			essentially is a subset of the active/passive etc. trip
> 			points. Calculation of that based on the current temp,
> 			as of today is replicated across all the sensor drivers
> 			and can be hoisted up to the of-thermal.
> 

There is no intention to add such logic to of thermal. The main reason
is of-thermal should never be competing with thermal core. Any extension
in of-thermal needs to be supported by thermal core.

I believe all the above is left currently to thermal zone device
drivers. The problem with of-thermal is that it had to be born as a
thermal zone device driver. And that is because..

> Seems like this is the opportune time to make a call about the role of of-thermal?
> 

.. the point one may be missing here is the fact that with current
thermal subsystem implementation, handling thermal devices is somewhat
different from other devices within the kernel.

The real design issue (or not an issue) we have is the fact that thermal
drivers adds both the driver and the device. Changing that is somewhat
disruptive. Not impossible, but requires considering the existing driver's code.

If we had a better split from who adds the device and who provides the
driver, then of-thermal would never be a "glue layer" or born as thermal
zone device driver. It would simply, parse DT, add the devices, done.

Then, drivers would register themselves as handlers for specific thermal
devices. That is the correct design, based on common design found in other
parts of the kernel. Another little missing end would be the "compatible"
string for thermal zone devices in DT. But as I said, it should not be a
blocker.

So, given the current situation, we have essentially two options: (a)
stick to the same design we have for now, having of-thermal as dummy as
possible, and grow in small steps towards the correct design; or (b)
redesign thermal core to have a better split between devices and
drivers, then adjust of-thermal.


For now, the path we are taking is (a). In fact, to fit the needs of
coming drivers, specially considering they are based on DT booting
platforms, it is always tempting to add intelligence to of-thermal. But,
as I mentioned, I want to avoid growing intelligence in it, for obvious
reasons.

> On 11/26/2014 07:18 AM, Eduardo Valentin wrote:
> > * PGP Signed by an unknown key
> > 
> > Hello,
> > 
> > On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote:
> >> Hi Eduardo,
> >>
> >>> Hello Lukasz,
> >>>
> >>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
> >>>> This patch extends the of-thermal.c to export copy of trip points
> >>>> for a given thermal zone.
> >>>>
> >>>> Thermal drivers should use of_thermal_get_trip_points() method to
> >>>> get pointer to table of thermal trip points.
> >>>>
> >>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> >>>> ---
> >>>> Changes for v2:
> >>>> - New patch - as suggested by Eduardo Valentin
> >>>> ---
> >>>>  drivers/thermal/of-thermal.c   | 33
> >>>> +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h |
> >>>> 7 +++++++ include/linux/thermal.h        | 14 ++++++++++++++
> >>>>  3 files changed, 54 insertions(+)
> >>>>
> >>>> diff --git a/drivers/thermal/of-thermal.c
> >>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644
> >>>> --- a/drivers/thermal/of-thermal.c
> >>>> +++ b/drivers/thermal/of-thermal.c
> >>>> @@ -89,6 +89,7 @@ struct __thermal_zone {
> >>>>  	/* trip data */
> >>>>  	int ntrips;
> >>>>  	struct __thermal_trip *trips;
> >>>> +	struct thermal_trip *gtrips;
> Do we really need this duplication ?
> >>>>  
> >>>>  	/* cooling binding data */
> >>>>  	int num_tbps;
> >>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct
> >>>> thermal_zone_device *tz, int trip) return true;
> >>>>  }
> >>>>  
> >>>> +/**
> >>>> + * of_thermal_get_trip_points - function to get access to a
> >>>> globally exported
> >>>> + *				trip points
> >>>> + *
> >>>> + * @tz:	pointer to a thermal zone
> >>>> + *
> >>>> + * This function provides a pointer to the copy of trip points
> >>>> table
> >>>> + *
> >>>> + * Return: pointer to trip points table, NULL otherwise
> >>>> + */
> >>>> +const struct thermal_trip * const
> >>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz)
> >>>> +{
> >>>> +	struct __thermal_zone *data = tz->devdata;
> >>>> +
> >>>> +	if (!data)
> >>>> +		return NULL;
> >>>> +
> >>>> +	return data->gtrips;
> >>>> +}
> >>>> +
> >>>
> >>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
> >>
> >> Ok.
> >>
> >>>
> >>>>  static int of_thermal_get_trend(struct thermal_zone_device *tz,
> >>>> int trip, enum thermal_trend *trend)
> >>>>  {
> >>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct
> >>>> device_node *np) goto free_tbps;
> >>>>  	}
> >>>>  
> >>>> +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips),
> >>>> GFP_KERNEL);
> >>>> +	if (!tz->gtrips) {
> >>>> +		ret = -ENOMEM;
> >>>> +		goto free_tbps;
> >>>> +	}
> >>>> +
> >>>> +	for (i = 0; i < tz->ntrips; i++)
> >>>> +		memcpy(&(tz->gtrips[i]),
> >>>> &(tz->trips[i].temperature),
> >>>> +		       sizeof(*tz->gtrips));
> >>>> +
> >>>>  finish:
> >>>>  	of_node_put(child);
> >>>>  	tz->mode = THERMAL_DEVICE_DISABLED;
> >>>> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct
> >>>> __thermal_zone *tz) {
> >>>>  	int i;
> >>>>  
> >>>> +	kfree(tz->gtrips);
> >>>>  	for (i = 0; i < tz->num_tbps; i++)
> >>>>  		of_node_put(tz->tbps[i].cooling_device);
> >>>>  	kfree(tz->tbps);
> Couldn't find the code that updates *gtrips as a result of set_trip_temp via
> sysfs.
> 
> >>>> diff --git a/drivers/thermal/thermal_core.h
> >>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644
> >>>> --- a/drivers/thermal/thermal_core.h
> >>>> +++ b/drivers/thermal/thermal_core.h
> >>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
> >>>>  void of_thermal_destroy_zones(void);
> >>>>  int of_thermal_get_ntrips(struct thermal_zone_device *);
> >>>>  bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
> >>>> +const struct thermal_trip * const
> >>>> +of_thermal_get_trip_points(struct thermal_zone_device *);
> >>>>  #else
> >>>>  static inline int of_parse_thermal_zones(void) { return 0; }
> >>>>  static inline void of_thermal_destroy_zones(void) { }
> >>>> @@ -102,6 +104,11 @@ static inline bool
> >>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) {
> >>>
> >>> This produces compilation error when CONFIG_THERMAL_OF is not set.
> >>> Name the parameters to fix.
> >>
> >> As all the other cases, I will fix that.
> >>
> >>>
> >>>>  	return 0;
> >>>>  }
> >>>> +static inline const struct thermal_trip * const
> >>>> +of_thermal_get_trip_points(struct thermal_zone_device *)
> >>>> +{
> >>>> +	return NULL;
> >>>> +}
> >>>>  #endif
> >>>>  
> >>>>  #endif /* __THERMAL_CORE_H__ */
> >>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >>>> index 5bc28a7..88d7249 100644
> >>>> --- a/include/linux/thermal.h
> >>>> +++ b/include/linux/thermal.h
> >>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
> >>>>  	int (*get_trend)(void *, long *);
> >>>>  };
> >>>>  
> >>>> +/**
> >>>> + * struct thermal_trip - Structure representing themal trip points
> >>>> exported from
> >>>> + *                       of-thermal
> >>>> + *
> >>>
> >>> The only problem I have with this name is that would look like it is
> >>> in use in all thermal framework, which is not really the case. But I
> >>> do think having a type here is a good thing. So, not sure :-)
> >>
> >> It can stay to be struct thermal_trip or we can rename it to
> >> struct of_thermal_trip.
> >>
> >> I'm fine with both names.
> > 
> > Leave it as 'thermal_trip'.
> > 
> >>
> >>>
> >>>> + * @temperature:	trip point temperature
> >>>> + * @hysteresis:		trip point hysteresis
> >>>> + * @type:		trip point type
> >>>> + */
> >>>> +struct thermal_trip {
> >>>> +	unsigned long int temperature;
> >>>> +	unsigned long int hysteresis;
> >>>> +	enum thermal_trip_type type;
> >>>> +};
> >>>> +
> >>>>  /* Function declarations */
> >>>>  #ifdef CONFIG_THERMAL_OF
> >>>>  struct thermal_zone_device *
> >>>> -- 
> >>>> 2.0.0.rc2
> >>>>
> >>
> >>
> >>
> >> -- 
> >> Best regards,
> >>
> >> Lukasz Majewski
> >>
> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> > 
> > * Unknown Key
> > * 0x7DA4E256
> > 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[navneet kumar]

On 11/26/2014 01:12 PM, Guenter Roeck wrote:
> On 11/26/2014 12:43 PM, navneet kumar wrote:
>>
>> Hi Eduardo, Lukasz,
>>
>> [Combining my concerns as a single text blob here]
>>
>> I. Firstly, with the current patch
>>     1. is it really needed to duplicate the struct thermal_trip? Why
>> don’t
>>     we get rid of the __thermal_trip and stay with the 'thermal_trip'
>> ? it
>>     is not a big change.
>>
>>     2. gtrips is not updated on "set_trip_temp" etc. actions via
>> sysfs. (am
>>     I missing something?).
>>
>> II. The other concern is more of a design question
>>     1. Do we intend to keep the of-thermal as a middle layer between the
>>     thermal_core and the sensor device? OR, is the role of of-thermal
>> just
>>     to parse the DT and opt out ? currently of-thermal is somewhat a
>> hybrid
>>     of these as, in addition to parsing the dt, it holds on to the data
>>     related to trip points etc. etc.
>>
>>     2. assuming latter is true (OF is just a dt parser helper): we should
>>     not be adding more intelligence and dependencies linked to the OF.
>>
>>     3. assuming former is true (OF is a well-defined middle layer):
>> All is
>>     good till the point OF maintains the trip points (which is a good
>> thing
>>     since, OF caches on to the data); BUT, if we expose this
>> information to
>>     the sensor device too (as this patch is doing),
>>
>>         3a. we violate the layering principle :-)
>>
>>         3b. more importantly, this is all just excessive logic that we
>>         put in which *could be useful* only if we intend to extend the
>>         role of OF as a trip point management layer that does more than
>>         just holding on to the data. This may include -
>>
>>             -> The sensor devices to know nothing about the
>>             trip_points, instead the sensor devices would work on
>>             "temperature thresholds" and OF would map sensor
>>             thresholds to the actual trip points as needed
>>             (configured from DT); while the sensor devices stick to
>>             using "thresholds".
>>
>>             -> Queuing from above, sensors, most of the time, only
>>             need to know a high and a low temp threshold; which
>>             essentially is a subset of the active/passive etc. trip
>>             points. Calculation of that based on the current temp,
>>             as of today is replicated across all the sensor drivers
>>             and can be hoisted up to the of-thermal.
>>
> 
> Sorry, lost you here. What replicated calculation do you refer to ?
> 
> Thanks,
> Guenter
> 
Some sensors have an ability to generate interrupts based upon a configured high
and low temp thresholds/values. Once any of these limits is crossed, the sensor
hardware has to be reconfigured with a new set of such values.

I'll try to explain with an example -
consider trip points TP1, TP2, TP3, TP4 sorted as low to high; and the current 
temp T1 such that -
TP2<T1<TP3

With the conditions as is, and assuming the interrupts have been configured, an
interrupt will trigger if T1 goes below TP2 or exceeds TP3.

Now, say, T1 crosses TP3, the sensor hardware generates an interrupt (avoids polling 
on the zone temp). Furthermore, we require that the new limits for the interrupts
be TP3 and TP4 for 'low' and 'high' thresholds interrupts
respectively, such that an interrupt will be fired when temp goes below TP3 or
exceeds TP4.

Above case is when the temp crosses a set of High-low. The same situation may
occur when a trip point temp is changed from sysfs, in which case, the thresholds
for configuring the interrupts may have to be reconfigured.

Today, the sensors find the high-low thresholds by -
1. querying the temp from the thermal_zone
2. traversing the trip points (which may have been cached locally by the sensor),
   and figuring out the high/low temperature thresholds. 

These above steps aka 'replicated calculation'.

Whatever the case may be, the real question is the role and scope of of-thermal.


>> Seems like this is the opportune time to make a call about the role of
>> of-thermal?
>>
>> On 11/26/2014 07:18 AM, Eduardo Valentin wrote:
>>> * PGP Signed by an unknown key
>>>
>>> Hello,
>>>
>>> On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote:
>>>> Hi Eduardo,
>>>>
>>>>> Hello Lukasz,
>>>>>
>>>>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
>>>>>> This patch extends the of-thermal.c to export copy of trip points
>>>>>> for a given thermal zone.
>>>>>>
>>>>>> Thermal drivers should use of_thermal_get_trip_points() method to
>>>>>> get pointer to table of thermal trip points.
>>>>>>
>>>>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>>> ---
>>>>>> Changes for v2:
>>>>>> - New patch - as suggested by Eduardo Valentin
>>>>>> ---
>>>>>>   drivers/thermal/of-thermal.c   | 33
>>>>>> +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h |
>>>>>> 7 +++++++ include/linux/thermal.h        | 14 ++++++++++++++
>>>>>>   3 files changed, 54 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/thermal/of-thermal.c
>>>>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644
>>>>>> --- a/drivers/thermal/of-thermal.c
>>>>>> +++ b/drivers/thermal/of-thermal.c
>>>>>> @@ -89,6 +89,7 @@ struct __thermal_zone {
>>>>>>       /* trip data */
>>>>>>       int ntrips;
>>>>>>       struct __thermal_trip *trips;
>>>>>> +    struct thermal_trip *gtrips;
>> Do we really need this duplication ?
>>>>>>
>>>>>>       /* cooling binding data */
>>>>>>       int num_tbps;
>>>>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct
>>>>>> thermal_zone_device *tz, int trip) return true;
>>>>>>   }
>>>>>>
>>>>>> +/**
>>>>>> + * of_thermal_get_trip_points - function to get access to a
>>>>>> globally exported
>>>>>> + *                trip points
>>>>>> + *
>>>>>> + * @tz:    pointer to a thermal zone
>>>>>> + *
>>>>>> + * This function provides a pointer to the copy of trip points
>>>>>> table
>>>>>> + *
>>>>>> + * Return: pointer to trip points table, NULL otherwise
>>>>>> + */
>>>>>> +const struct thermal_trip * const
>>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz)
>>>>>> +{
>>>>>> +    struct __thermal_zone *data = tz->devdata;
>>>>>> +
>>>>>> +    if (!data)
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    return data->gtrips;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>>>   static int of_thermal_get_trend(struct thermal_zone_device *tz,
>>>>>> int trip, enum thermal_trend *trend)
>>>>>>   {
>>>>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct
>>>>>> device_node *np) goto free_tbps;
>>>>>>       }
>>>>>>
>>>>>> +    tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips),
>>>>>> GFP_KERNEL);
>>>>>> +    if (!tz->gtrips) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        goto free_tbps;
>>>>>> +    }
>>>>>> +
>>>>>> +    for (i = 0; i < tz->ntrips; i++)
>>>>>> +        memcpy(&(tz->gtrips[i]),
>>>>>> &(tz->trips[i].temperature),
>>>>>> +               sizeof(*tz->gtrips));
>>>>>> +
>>>>>>   finish:
>>>>>>       of_node_put(child);
>>>>>>       tz->mode = THERMAL_DEVICE_DISABLED;
>>>>>> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct
>>>>>> __thermal_zone *tz) {
>>>>>>       int i;
>>>>>>
>>>>>> +    kfree(tz->gtrips);
>>>>>>       for (i = 0; i < tz->num_tbps; i++)
>>>>>>           of_node_put(tz->tbps[i].cooling_device);
>>>>>>       kfree(tz->tbps);
>> Couldn't find the code that updates *gtrips as a result of
>> set_trip_temp via
>> sysfs.
>>
>>>>>> diff --git a/drivers/thermal/thermal_core.h
>>>>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644
>>>>>> --- a/drivers/thermal/thermal_core.h
>>>>>> +++ b/drivers/thermal/thermal_core.h
>>>>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
>>>>>>   void of_thermal_destroy_zones(void);
>>>>>>   int of_thermal_get_ntrips(struct thermal_zone_device *);
>>>>>>   bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
>>>>>> +const struct thermal_trip * const
>>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *);
>>>>>>   #else
>>>>>>   static inline int of_parse_thermal_zones(void) { return 0; }
>>>>>>   static inline void of_thermal_destroy_zones(void) { }
>>>>>> @@ -102,6 +104,11 @@ static inline bool
>>>>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) {
>>>>>
>>>>> This produces compilation error when CONFIG_THERMAL_OF is not set.
>>>>> Name the parameters to fix.
>>>>
>>>> As all the other cases, I will fix that.
>>>>
>>>>>
>>>>>>       return 0;
>>>>>>   }
>>>>>> +static inline const struct thermal_trip * const
>>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *)
>>>>>> +{
>>>>>> +    return NULL;
>>>>>> +}
>>>>>>   #endif
>>>>>>
>>>>>>   #endif /* __THERMAL_CORE_H__ */
>>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>>>> index 5bc28a7..88d7249 100644
>>>>>> --- a/include/linux/thermal.h
>>>>>> +++ b/include/linux/thermal.h
>>>>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
>>>>>>       int (*get_trend)(void *, long *);
>>>>>>   };
>>>>>>
>>>>>> +/**
>>>>>> + * struct thermal_trip - Structure representing themal trip points
>>>>>> exported from
>>>>>> + *                       of-thermal
>>>>>> + *
>>>>>
>>>>> The only problem I have with this name is that would look like it is
>>>>> in use in all thermal framework, which is not really the case. But I
>>>>> do think having a type here is a good thing. So, not sure :-)
>>>>
>>>> It can stay to be struct thermal_trip or we can rename it to
>>>> struct of_thermal_trip.
>>>>
>>>> I'm fine with both names.
>>>
>>> Leave it as 'thermal_trip'.
>>>
>>>>
>>>>>
>>>>>> + * @temperature:    trip point temperature
>>>>>> + * @hysteresis:        trip point hysteresis
>>>>>> + * @type:        trip point type
>>>>>> + */
>>>>>> +struct thermal_trip {
>>>>>> +    unsigned long int temperature;
>>>>>> +    unsigned long int hysteresis;
>>>>>> +    enum thermal_trip_type type;
>>>>>> +};
>>>>>> +
>>>>>>   /* Function declarations */
>>>>>>   #ifdef CONFIG_THERMAL_OF
>>>>>>   struct thermal_zone_device *
>>>>>> -- 
>>>>>> 2.0.0.rc2
>>>>>>
>>>>
>>>>
>>>>
>>>> -- 
>>>> Best regards,
>>>>
>>>> Lukasz Majewski
>>>>
>>>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>>>
>>> * Unknown Key
>>> * 0x7DA4E256
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

[Lukasz Majewski]

Hi Eduardo,

> 
> Hello Navneet,
> 
> On Wed, Nov 26, 2014 at 12:43:18PM -0800, navneet kumar wrote:
> > 
> > Hi Eduardo, Lukasz,
> > 
> > [Combining my concerns as a single text blob here]
> > 
> > I. Firstly, with the current patch
> > 	1. is it really needed to duplicate the struct
> > thermal_trip? Why don’t we get rid of the __thermal_trip and stay
> > with the 'thermal_trip' ? it is not a big change.

The __thermal_trip seems to be somehow "private" structure (to
of-thermal) which holds not only data which could be exposed to sensors.

> > 
> > 	2. gtrips is not updated on "set_trip_temp" etc. actions
> > via sysfs. (am I missing something?).

True. This is a bug. Thanks for spotting.

> > 
> 
> Good! Comments are always welcome, that's how we make patches :-).
> 
> Both above make sense to me.
> 
> > II. The other concern is more of a design question
> > 	1. Do we intend to keep the of-thermal as a middle layer
> > between the thermal_core and the sensor device? OR, is the role of
> > of-thermal just to parse the DT and opt out ? currently of-thermal
> > is somewhat a hybrid of these as, in addition to parsing the dt, it
> > holds on to the data related to trip points etc. etc.
> > 
> 
> of-thermal has always been as your latter statement, and I intend to
> keep it as it should be, just a of parser for thermal data.

It seems to me that now of-thermal is doing more than parsing
thermal data. 

For example it is an abstraction layer for
calling .get_temp(), .get_trend().

It has its own set of "private" trip points which aren't exposed to
sensors.

Also, it registers thermal_zone_device.

A lot of stuff is done by the of-thermal. Frankly, I don't
mind, since this is a common code for many sensors.

For example with of-thermal.c usage, we are able to cut down LOC number
by half for Exynos TMU when moving to OF.

> 
> > 	2. assuming latter is true (OF is just a dt parser helper):
> > we should not be adding more intelligence and dependencies linked
> > to the OF.
> > 
> 
> Yes this is right. We should never be adding intelligence to it,

But a lot of stuff is already added and to be worse it is well adopted
API by sensors.

> except for parsing the thermal data out of device tree and adding the
> proper structures inside the kernel.

In my understanding the of-thermal code to expose the above features
need to:
- parse device tree
- export trip points in a table to sensors
- export cpu frequencies (OPPs?) with information about corresponding
  temperature

And that is all. The sensor is then responsible for initializing HW and
register the thermal zone with thermal_core.


> 
> > 	3. assuming former is true (OF is a well-defined middle
> > layer): All is good till the point OF maintains the trip points
> > (which is a good thing since, OF caches on to the data); BUT, if we
> > expose this information to the sensor device too (as this patch is
> > doing),
> > 
> 
> the former is not true.
> 
> > 		3a. we violate the layering principle :-)
> > 

Are we? of-thermal.c is parsing DT and it exposes read only information
about trip points. Also it gives you information about e.g. number of
available trip points.

> 
> well, even if it was, I disagree here. :-) The split between data
> coming from DT and the driver is still in place. Besides, there is
> other layers that expose some of their data, and that doesn't violate
> their layering principles. CPUfreq, for one closer example, exposes
> its freq table.
> 
> It would be different if by exposing this data, the users would be
> affecting the behavior of the layer. And that is not the intention of
> cpufreq table. In the same way, that is not the intention of this
> patch.
> 
> > 		3b. more importantly, this is all just excessive
> > logic that we put in which *could be useful* only if we intend to
> > extend the role of OF as a trip point management layer that does
> > more than just holding on to the data. This may include -
> > 
> > 			-> The sensor devices to know nothing about
> > the trip_points, instead the sensor devices would work on
> > 			"temperature thresholds" and OF would map
> > sensor thresholds to the actual trip points as needed
> > 			(configured from DT); while the sensor
> > devices stick to using "thresholds".
> > 
> > 			-> Queuing from above, sensors, most of the
> > time, only need to know a high and a low temp threshold; which
> > 			essentially is a subset of the
> > active/passive etc. trip points. Calculation of that based on the
> > current temp, as of today is replicated across all the sensor
> > drivers and can be hoisted up to the of-thermal.
> > 
> 
> There is no intention to add such logic to of thermal. The main reason
> is of-thermal should never be competing with thermal core. Any
> extension in of-thermal needs to be supported by thermal core.
> 
> I believe all the above is left currently to thermal zone device
> drivers. The problem with of-thermal is that it had to be born as a
> thermal zone device driver. And that is because..
> 
> > Seems like this is the opportune time to make a call about the role
> > of of-thermal?
> > 
> 
> .. the point one may be missing here is the fact that with current
> thermal subsystem implementation, handling thermal devices is somewhat
> different from other devices within the kernel.
> 
> The real design issue (or not an issue) we have is the fact that
> thermal drivers adds both the driver and the device. Changing that is
> somewhat disruptive. Not impossible, but requires considering the
> existing driver's code.
> 
> If we had a better split from who adds the device and who provides the
> driver, then of-thermal would never be a "glue layer" or born as
> thermal zone device driver. It would simply, parse DT, add the
> devices, done.
> 
> Then, drivers would register themselves as handlers for specific
> thermal devices. That is the correct design, based on common design
> found in other parts of the kernel. Another little missing end would
> be the "compatible" string for thermal zone devices in DT. But as I
> said, it should not be a blocker.
> 
> So, given the current situation, we have essentially two options: (a)
> stick to the same design we have for now, having of-thermal as dummy
> as possible, and grow in small steps towards the correct design; or
> (b) redesign thermal core to have a better split between devices and
> drivers, then adjust of-thermal.
> 
> 
> For now, the path we are taking is (a). In fact, to fit the needs of
> coming drivers, specially considering they are based on DT booting
> platforms, it is always tempting to add intelligence to of-thermal.
> But, as I mentioned, I want to avoid growing intelligence in it, for
> obvious reasons.

Eventually, it is your call if we make __thermal_trip [1] exported to
sensors (with or without struct device_node *np)
 or if we have new structure (e.g. struct thermal_trip) which is a read
only reference to relevant fields of [1]?

> 
> > On 11/26/2014 07:18 AM, Eduardo Valentin wrote:
> > > * PGP Signed by an unknown key
> > > 
> > > Hello,
> > > 
> > > On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote:
> > >> Hi Eduardo,
> > >>
> > >>> Hello Lukasz,
> > >>>
> > >>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote:
> > >>>> This patch extends the of-thermal.c to export copy of trip
> > >>>> points for a given thermal zone.
> > >>>>
> > >>>> Thermal drivers should use of_thermal_get_trip_points() method
> > >>>> to get pointer to table of thermal trip points.
> > >>>>
> > >>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > >>>> ---
> > >>>> Changes for v2:
> > >>>> - New patch - as suggested by Eduardo Valentin
> > >>>> ---
> > >>>>  drivers/thermal/of-thermal.c   | 33
> > >>>> +++++++++++++++++++++++++++++++++
> > >>>> drivers/thermal/thermal_core.h | 7 +++++++
> > >>>> include/linux/thermal.h        | 14 ++++++++++++++ 3 files
> > >>>> changed, 54 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/thermal/of-thermal.c
> > >>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644
> > >>>> --- a/drivers/thermal/of-thermal.c
> > >>>> +++ b/drivers/thermal/of-thermal.c
> > >>>> @@ -89,6 +89,7 @@ struct __thermal_zone {
> > >>>>  	/* trip data */
> > >>>>  	int ntrips;
> > >>>>  	struct __thermal_trip *trips;
> > >>>> +	struct thermal_trip *gtrips;
> > Do we really need this duplication ?
> > >>>>  
> > >>>>  	/* cooling binding data */
> > >>>>  	int num_tbps;
> > >>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct
> > >>>> thermal_zone_device *tz, int trip) return true;
> > >>>>  }
> > >>>>  
> > >>>> +/**
> > >>>> + * of_thermal_get_trip_points - function to get access to a
> > >>>> globally exported
> > >>>> + *				trip points
> > >>>> + *
> > >>>> + * @tz:	pointer to a thermal zone
> > >>>> + *
> > >>>> + * This function provides a pointer to the copy of trip points
> > >>>> table
> > >>>> + *
> > >>>> + * Return: pointer to trip points table, NULL otherwise
> > >>>> + */
> > >>>> +const struct thermal_trip * const
> > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz)
> > >>>> +{
> > >>>> +	struct __thermal_zone *data = tz->devdata;
> > >>>> +
> > >>>> +	if (!data)
> > >>>> +		return NULL;
> > >>>> +
> > >>>> +	return data->gtrips;
> > >>>> +}
> > >>>> +
> > >>>
> > >>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
> > >>
> > >> Ok.
> > >>
> > >>>
> > >>>>  static int of_thermal_get_trend(struct thermal_zone_device
> > >>>> *tz, int trip, enum thermal_trend *trend)
> > >>>>  {
> > >>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct
> > >>>> device_node *np) goto free_tbps;
> > >>>>  	}
> > >>>>  
> > >>>> +	tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips),
> > >>>> GFP_KERNEL);
> > >>>> +	if (!tz->gtrips) {
> > >>>> +		ret = -ENOMEM;
> > >>>> +		goto free_tbps;
> > >>>> +	}
> > >>>> +
> > >>>> +	for (i = 0; i < tz->ntrips; i++)
> > >>>> +		memcpy(&(tz->gtrips[i]),
> > >>>> &(tz->trips[i].temperature),
> > >>>> +		       sizeof(*tz->gtrips));
> > >>>> +
> > >>>>  finish:
> > >>>>  	of_node_put(child);
> > >>>>  	tz->mode = THERMAL_DEVICE_DISABLED;
> > >>>> @@ -793,6 +825,7 @@ static inline void
> > >>>> of_thermal_free_zone(struct __thermal_zone *tz) {
> > >>>>  	int i;
> > >>>>  
> > >>>> +	kfree(tz->gtrips);
> > >>>>  	for (i = 0; i < tz->num_tbps; i++)
> > >>>>  		of_node_put(tz->tbps[i].cooling_device);
> > >>>>  	kfree(tz->tbps);
> > Couldn't find the code that updates *gtrips as a result of
> > set_trip_temp via sysfs.
> > 
> > >>>> diff --git a/drivers/thermal/thermal_core.h
> > >>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644
> > >>>> --- a/drivers/thermal/thermal_core.h
> > >>>> +++ b/drivers/thermal/thermal_core.h
> > >>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
> > >>>>  void of_thermal_destroy_zones(void);
> > >>>>  int of_thermal_get_ntrips(struct thermal_zone_device *);
> > >>>>  bool of_thermal_is_trip_en(struct thermal_zone_device *, int);
> > >>>> +const struct thermal_trip * const
> > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *);
> > >>>>  #else
> > >>>>  static inline int of_parse_thermal_zones(void) { return 0; }
> > >>>>  static inline void of_thermal_destroy_zones(void) { }
> > >>>> @@ -102,6 +104,11 @@ static inline bool
> > >>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) {
> > >>>
> > >>> This produces compilation error when CONFIG_THERMAL_OF is not
> > >>> set. Name the parameters to fix.
> > >>
> > >> As all the other cases, I will fix that.
> > >>
> > >>>
> > >>>>  	return 0;
> > >>>>  }
> > >>>> +static inline const struct thermal_trip * const
> > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *)
> > >>>> +{
> > >>>> +	return NULL;
> > >>>> +}
> > >>>>  #endif
> > >>>>  
> > >>>>  #endif /* __THERMAL_CORE_H__ */
> > >>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > >>>> index 5bc28a7..88d7249 100644
> > >>>> --- a/include/linux/thermal.h
> > >>>> +++ b/include/linux/thermal.h
> > >>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops {
> > >>>>  	int (*get_trend)(void *, long *);
> > >>>>  };
> > >>>>  
> > >>>> +/**
> > >>>> + * struct thermal_trip - Structure representing themal trip
> > >>>> points exported from
> > >>>> + *                       of-thermal
> > >>>> + *
> > >>>
> > >>> The only problem I have with this name is that would look like
> > >>> it is in use in all thermal framework, which is not really the
> > >>> case. But I do think having a type here is a good thing. So,
> > >>> not sure :-)
> > >>
> > >> It can stay to be struct thermal_trip or we can rename it to
> > >> struct of_thermal_trip.
> > >>
> > >> I'm fine with both names.
> > > 
> > > Leave it as 'thermal_trip'.
> > > 
> > >>
> > >>>
> > >>>> + * @temperature:	trip point temperature
> > >>>> + * @hysteresis:		trip point hysteresis
> > >>>> + * @type:		trip point type
> > >>>> + */
> > >>>> +struct thermal_trip {
> > >>>> +	unsigned long int temperature;
> > >>>> +	unsigned long int hysteresis;
> > >>>> +	enum thermal_trip_type type;
> > >>>> +};
> > >>>> +
> > >>>>  /* Function declarations */
> > >>>>  #ifdef CONFIG_THERMAL_OF
> > >>>>  struct thermal_zone_device *
> > >>>> -- 
> > >>>> 2.0.0.rc2
> > >>>>
> > >>
> > >>
> > >>
> > >> -- 
> > >> Best regards,
> > >>
> > >> Lukasz Majewski
> > >>
> > >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> > > 
> > > * Unknown Key
> > > * 0x7DA4E256
> > > 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[PATCH v3 0/5] thermal: of: of-thermal.c API extensions

[Lukasz Majewski]

Provided patch series extend of-thermal.c file API. It is necessary 
for ongoing Exynos TMU rework.

First version of this code can be found at:
http://www.spinics.net/lists/linux-samsung-soc/msg37719.html

Second version:
http://www.spinics.net/lists/kernel/msg1872274.html

This code provides methods to access some of-thermal local data.
Moreover it provides a read only access to of-thermal trip points.

Lukasz Majewski (5):
  thermal: of: Extend of-thermal.c to provide number of trip points
  thermal: of: Extend of-thermal.c to provide check if trip point is
    valid
  thermal: of: Rename struct __thermal_trip to struct thermal_trip
  thermal: of: Extend of-thermal to export table of trip points
  thermal: of: Extend current of-thermal.c code to allow setting
    emulated temp

 drivers/thermal/of-thermal.c   | 109 ++++++++++++++++++++++++++++++++++-------
 drivers/thermal/thermal_core.h |  18 +++++++
 include/linux/thermal.h        |  18 +++++++
 3 files changed, 127 insertions(+), 18 deletions(-)

-- 
2.0.0.rc2

[PATCH v3 1/5] thermal: of: Extend of-thermal.c to provide number of trip points

[Lukasz Majewski]

This patch extends the of-thermal.c to provide information about number of
available trip points.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v3:
- Exporting of_thermal_get_ntrips symbol as a GPL
- Fix build error when CONFIG_THERMAL_OF is disabled

Changes for v2:
- Provide detailed (doxygen like) description of the of_thermal_get_ntrips()
  method
- Check for data pointer not being NULL
---
 drivers/thermal/of-thermal.c   | 21 +++++++++++++++++++++
 drivers/thermal/thermal_core.h |  5 +++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index b7982f0..7facd23 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -112,6 +112,27 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
 	return data->ops->get_temp(data->sensor_data, temp);
 }
 
+/**
+ * of_thermal_get_ntrips - function to export number of available trip
+ *			   points.
+ * @tz: pointer to a thermal zone
+ *
+ * This function is a globally visible wrapper to get number of trip points
+ * stored in the local struct __thermal_zone
+ *
+ * Return: number of available trip points, -ENODEV when data not available
+ */
+int of_thermal_get_ntrips(struct thermal_zone_device *tz)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data || IS_ERR(data))
+		return -ENODEV;
+
+	return data->ntrips;
+}
+EXPORT_SYMBOL_GPL(of_thermal_get_ntrips);
+
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index d15d243..1cc5041 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -89,9 +89,14 @@ static inline void thermal_gov_user_space_unregister(void) {}
 #ifdef CONFIG_THERMAL_OF
 int of_parse_thermal_zones(void);
 void of_thermal_destroy_zones(void);
+int of_thermal_get_ntrips(struct thermal_zone_device *);
 #else
 static inline int of_parse_thermal_zones(void) { return 0; }
 static inline void of_thermal_destroy_zones(void) { }
+static inline int of_thermal_get_ntrips(struct thermal_zone_device *tz)
+{
+	return 0;
+}
 #endif
 
 #endif /* __THERMAL_CORE_H__ */
-- 
2.0.0.rc2

[PATCH v3 2/5] thermal: of: Extend of-thermal.c to provide check if trip point is valid

[Lukasz Majewski]

This patch extends the of-thermal.c to provide check if trip point is
valid.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v3:
- Rename of_thermal_is_trip_en() to of_thermal_is_trip_valid()
- Export of_thermal_is_trip_valid symbol as GPL
- Fix the build error when CONFIG_THERMAL_OF is not defined

Changes for v2:
- Replace int with bool
- Replace 1/0 with true/false
- Add check if data pointer is not NULL
- Add missing doxygen style comment for the function
---
 drivers/thermal/of-thermal.c   | 21 +++++++++++++++++++++
 drivers/thermal/thermal_core.h |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 7facd23..87b9cfe 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -133,6 +133,27 @@ int of_thermal_get_ntrips(struct thermal_zone_device *tz)
 }
 EXPORT_SYMBOL_GPL(of_thermal_get_ntrips);
 
+/**
+ * of_thermal_is_trip_valid - function to check if trip point is valid
+ *
+ * @tz:	pointer to a thermal zone
+ * @trip:	trip point to evaluate
+ *
+ * This function is responsible for checking if passed trip point is valid
+ *
+ * Return: true if trip point is valid, false otherwise
+ */
+bool of_thermal_is_trip_valid(struct thermal_zone_device *tz, int trip)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data || trip >= data->ntrips || trip < 0)
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(of_thermal_is_trip_valid);
+
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 1cc5041..58a0dfa 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -90,6 +90,7 @@ static inline void thermal_gov_user_space_unregister(void) {}
 int of_parse_thermal_zones(void);
 void of_thermal_destroy_zones(void);
 int of_thermal_get_ntrips(struct thermal_zone_device *);
+bool of_thermal_is_trip_valid(struct thermal_zone_device *, int);
 #else
 static inline int of_parse_thermal_zones(void) { return 0; }
 static inline void of_thermal_destroy_zones(void) { }
@@ -97,6 +98,11 @@ static inline int of_thermal_get_ntrips(struct thermal_zone_device *tz)
 {
 	return 0;
 }
+static inline bool of_thermal_is_trip_valid(struct thermal_zone_device *tz,
+					    int trip)
+{
+	return 0;
+}
 #endif
 
 #endif /* __THERMAL_CORE_H__ */
-- 
2.0.0.rc2

[PATCH v3 3/5] thermal: of: Rename struct __thermal_trip to struct thermal_trip

[Lukasz Majewski]

This patch changes name of struct __thermal_trip to thermal_trip and moves
declaration of the latter to ./include/linux/thermal.h for better visibility.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v3:
- New patch - as suggested by Navneet Kumar
---
 drivers/thermal/of-thermal.c | 21 +++------------------
 include/linux/thermal.h      | 15 +++++++++++++++
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 87b9cfe..d3ac117 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -37,21 +37,6 @@
 /***   Private data structures to represent thermal device tree data ***/
 
 /**
- * struct __thermal_trip - representation of a point in temperature domain
- * @np: pointer to struct device_node that this trip point was created from
- * @temperature: temperature value in miliCelsius
- * @hysteresis: relative hysteresis in miliCelsius
- * @type: trip point type
- */
-
-struct __thermal_trip {
-	struct device_node *np;
-	unsigned long int temperature;
-	unsigned long int hysteresis;
-	enum thermal_trip_type type;
-};
-
-/**
  * struct __thermal_bind_param - a match between trip and cooling device
  * @cooling_device: a pointer to identify the referred cooling device
  * @trip_id: the trip point index
@@ -88,7 +73,7 @@ struct __thermal_zone {
 
 	/* trip data */
 	int ntrips;
-	struct __thermal_trip *trips;
+	struct thermal_trip *trips;
 
 	/* cooling binding data */
 	int num_tbps;
@@ -538,7 +523,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_unregister);
  */
 static int thermal_of_populate_bind_params(struct device_node *np,
 					   struct __thermal_bind_params *__tbp,
-					   struct __thermal_trip *trips,
+					   struct thermal_trip *trips,
 					   int ntrips)
 {
 	struct of_phandle_args cooling_spec;
@@ -641,7 +626,7 @@ static int thermal_of_get_trip_type(struct device_node *np,
  * Return: 0 on success, proper error code otherwise
  */
 static int thermal_of_populate_trip(struct device_node *np,
-				    struct __thermal_trip *trip)
+				    struct thermal_trip *trip)
 {
 	int prop;
 	int ret;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 5bc28a7..b8d91ef 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -303,6 +303,21 @@ struct thermal_zone_of_device_ops {
 	int (*get_trend)(void *, long *);
 };
 
+/**
+ * struct thermal_trip - representation of a point in temperature domain
+ * @np: pointer to struct device_node that this trip point was created from
+ * @temperature: temperature value in miliCelsius
+ * @hysteresis: relative hysteresis in miliCelsius
+ * @type: trip point type
+ */
+
+struct thermal_trip {
+	struct device_node *np;
+	unsigned long int temperature;
+	unsigned long int hysteresis;
+	enum thermal_trip_type type;
+};
+
 /* Function declarations */
 #ifdef CONFIG_THERMAL_OF
 struct thermal_zone_device *
-- 
2.0.0.rc2

[PATCH v3 4/5] thermal: of: Extend of-thermal to export table of trip points

[Lukasz Majewski]

This patch extends the of-thermal.c to export trip points for a given
thermal zone.

Thermal drivers should use of_thermal_get_trip_points() method to get
pointer to table of thermal trip points.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v3:
- Export of_thermal_get_trip_points symbol as GPL
- Fix the build error when CONFIG_THERMAL_OF is not defined

Changes for v2:
- New patch - as suggested by Eduardo Valentin
---
 drivers/thermal/of-thermal.c   | 22 ++++++++++++++++++++++
 drivers/thermal/thermal_core.h |  7 +++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index d3ac117..e062bf5 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -139,6 +139,28 @@ bool of_thermal_is_trip_valid(struct thermal_zone_device *tz, int trip)
 }
 EXPORT_SYMBOL_GPL(of_thermal_is_trip_valid);
 
+/**
+ * of_thermal_get_trip_points - function to get access to a globally exported
+ *				trip points
+ *
+ * @tz:	pointer to a thermal zone
+ *
+ * This function provides a pointer to trip points table
+ *
+ * Return: pointer to trip points table, NULL otherwise
+ */
+const struct thermal_trip * const
+of_thermal_get_trip_points(struct thermal_zone_device *tz)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data)
+		return NULL;
+
+	return data->trips;
+}
+EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
+
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 58a0dfa..9083e75 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -91,6 +91,8 @@ int of_parse_thermal_zones(void);
 void of_thermal_destroy_zones(void);
 int of_thermal_get_ntrips(struct thermal_zone_device *);
 bool of_thermal_is_trip_valid(struct thermal_zone_device *, int);
+const struct thermal_trip * const
+of_thermal_get_trip_points(struct thermal_zone_device *);
 #else
 static inline int of_parse_thermal_zones(void) { return 0; }
 static inline void of_thermal_destroy_zones(void) { }
@@ -103,6 +105,11 @@ static inline bool of_thermal_is_trip_valid(struct thermal_zone_device *tz,
 {
 	return 0;
 }
+static inline const struct thermal_trip * const
+of_thermal_get_trip_points(struct thermal_zone_device *tz)
+{
+	return NULL;
+}
 #endif
 
 #endif /* __THERMAL_CORE_H__ */
-- 
2.0.0.rc2

[PATCH v3 5/5] thermal: of: Extend current of-thermal.c code to allow setting emulated temp

[Lukasz Majewski]

Before this change it was only possible to set get_temp() and get_trend()
methods to be used in the common code handling passing parameters via
device tree to "cpu-thermal" CPU thermal zone device.

Now it is possible to also set emulated value of temperature for debug
purposes.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Acked-by: Eduardo Valentin <edubezval@gmail.com>
---
Changes for v3:
- Update comment for .set_emul_temp method to indicate that it is optional

Changes for v2:
- Rework the emulated temperature setting code to use of_thermal_sensor_ops
structure
---
 drivers/thermal/of-thermal.c | 24 ++++++++++++++++++++++++
 include/linux/thermal.h      |  3 +++
 2 files changed, 27 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index e062bf5..e145b66 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -161,6 +161,28 @@ of_thermal_get_trip_points(struct thermal_zone_device *tz)
 }
 EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
 
+/**
+ * of_thermal_set_emul_temp - function to set emulated temperature
+ *
+ * @tz:	pointer to a thermal zone
+ * @temp:	temperature to set
+ *
+ * This function gives the ability to set emulated value of temperature,
+ * which is handy for debugging
+ *
+ * Return: zero on success, error code otherwise
+ */
+static int of_thermal_set_emul_temp(struct thermal_zone_device *tz,
+				    unsigned long temp)
+{
+	struct __thermal_zone *data = tz->devdata;
+
+	if (!data->ops || !data->ops->set_emul_temp)
+		return -EINVAL;
+
+	return data->ops->set_emul_temp(data->sensor_data, temp);
+}
+
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
@@ -392,6 +414,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 
 	tzd->ops->get_temp = of_thermal_get_temp;
 	tzd->ops->get_trend = of_thermal_get_trend;
+	tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
 	mutex_unlock(&tzd->lock);
 
 	return tzd;
@@ -520,6 +543,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
 	mutex_lock(&tzd->lock);
 	tzd->ops->get_temp = NULL;
 	tzd->ops->get_trend = NULL;
+	tzd->ops->set_emul_temp = NULL;
 
 	tz->ops = NULL;
 	tz->sensor_data = NULL;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index b8d91ef..99be7fc 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -297,10 +297,13 @@ struct thermal_genl_event {
  *
  * Optional:
  * @get_trend: a pointer to a function that reads the sensor temperature trend.
+ * @set_emul_temp: a pointer to a function that sets sensor emulated
+ *		   temperature.
  */
 struct thermal_zone_of_device_ops {
 	int (*get_temp)(void *, long *);
 	int (*get_trend)(void *, long *);
+	int (*set_emul_temp)(void *, unsigned long);
 };
 
 /**
-- 
2.0.0.rc2

Re: [PATCH v3 1/5] thermal: of: Extend of-thermal.c to provide number of trip points

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 2938 bytes --]

On Mon, Dec 08, 2014 at 06:04:17PM +0100, Lukasz Majewski wrote:
> This patch extends the of-thermal.c to provide information about number of
> available trip points.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
> Changes for v3:
> - Exporting of_thermal_get_ntrips symbol as a GPL
> - Fix build error when CONFIG_THERMAL_OF is disabled
> 
> Changes for v2:
> - Provide detailed (doxygen like) description of the of_thermal_get_ntrips()
>   method
> - Check for data pointer not being NULL
> ---
>  drivers/thermal/of-thermal.c   | 21 +++++++++++++++++++++
>  drivers/thermal/thermal_core.h |  5 +++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index b7982f0..7facd23 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -112,6 +112,27 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>  	return data->ops->get_temp(data->sensor_data, temp);
>  }
>  
> +/**
> + * of_thermal_get_ntrips - function to export number of available trip
> + *			   points.
> + * @tz: pointer to a thermal zone
> + *
> + * This function is a globally visible wrapper to get number of trip points
> + * stored in the local struct __thermal_zone
> + *
> + * Return: number of available trip points, -ENODEV when data not available
> + */
> +int of_thermal_get_ntrips(struct thermal_zone_device *tz)
> +{
> +	struct __thermal_zone *data = tz->devdata;
> +
> +	if (!data || IS_ERR(data))
> +		return -ENODEV;
> +
> +	return data->ntrips;
> +}
> +EXPORT_SYMBOL_GPL(of_thermal_get_ntrips);
> +
>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>  				enum thermal_trend *trend)
>  {
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index d15d243..1cc5041 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -89,9 +89,14 @@ static inline void thermal_gov_user_space_unregister(void) {}
>  #ifdef CONFIG_THERMAL_OF
>  int of_parse_thermal_zones(void);
>  void of_thermal_destroy_zones(void);
> +int of_thermal_get_ntrips(struct thermal_zone_device *);

Lukasz,

Here is a question that applies to all patches in this series adding
functions in this header.

i suppose you intend to use these new functions in driver code, right?
Add them here, will limit their usage to code inside drivers/thermal.

Shouldn't these new APIs be declared under include/linux/thermal.h?

This way, they can be be available to other parts of the kernel too.


>  #else
>  static inline int of_parse_thermal_zones(void) { return 0; }
>  static inline void of_thermal_destroy_zones(void) { }
> +static inline int of_thermal_get_ntrips(struct thermal_zone_device *tz)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #endif /* __THERMAL_CORE_H__ */
> -- 
> 2.0.0.rc2
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 1/5] thermal: of: Extend of-thermal.c to provide number of trip points

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3586 bytes --]

On Mon, 8 Dec 2014 15:52:49 -0400
Eduardo Valentin <edubezval@gmail.com> wrote:

Hi Eduardo,

> On Mon, Dec 08, 2014 at 06:04:17PM +0100, Lukasz Majewski wrote:
> > This patch extends the of-thermal.c to provide information about
> > number of available trip points.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> > Changes for v3:
> > - Exporting of_thermal_get_ntrips symbol as a GPL
> > - Fix build error when CONFIG_THERMAL_OF is disabled
> > 
> > Changes for v2:
> > - Provide detailed (doxygen like) description of the
> > of_thermal_get_ntrips() method
> > - Check for data pointer not being NULL
> > ---
> >  drivers/thermal/of-thermal.c   | 21 +++++++++++++++++++++
> >  drivers/thermal/thermal_core.h |  5 +++++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/thermal/of-thermal.c
> > b/drivers/thermal/of-thermal.c index b7982f0..7facd23 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -112,6 +112,27 @@ static int of_thermal_get_temp(struct
> > thermal_zone_device *tz, return
> > data->ops->get_temp(data->sensor_data, temp); }
> >  
> > +/**
> > + * of_thermal_get_ntrips - function to export number of available
> > trip
> > + *			   points.
> > + * @tz: pointer to a thermal zone
> > + *
> > + * This function is a globally visible wrapper to get number of
> > trip points
> > + * stored in the local struct __thermal_zone
> > + *
> > + * Return: number of available trip points, -ENODEV when data not
> > available
> > + */
> > +int of_thermal_get_ntrips(struct thermal_zone_device *tz)
> > +{
> > +	struct __thermal_zone *data = tz->devdata;
> > +
> > +	if (!data || IS_ERR(data))
> > +		return -ENODEV;
> > +
> > +	return data->ntrips;
> > +}
> > +EXPORT_SYMBOL_GPL(of_thermal_get_ntrips);
> > +
> >  static int of_thermal_get_trend(struct thermal_zone_device *tz,
> > int trip, enum thermal_trend *trend)
> >  {
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h index d15d243..1cc5041 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -89,9 +89,14 @@ static inline void
> > thermal_gov_user_space_unregister(void) {} #ifdef CONFIG_THERMAL_OF
> >  int of_parse_thermal_zones(void);
> >  void of_thermal_destroy_zones(void);
> > +int of_thermal_get_ntrips(struct thermal_zone_device *);
> 
> Lukasz,
> 
> Here is a question that applies to all patches in this series adding
> functions in this header.
> 
> i suppose you intend to use these new functions in driver code, right?

Yes, right.

> Add them here, will limit their usage to code inside drivers/thermal.

This is my intention - at least for now the use case is to configure the
thermal driver (i.e. Exynos TMU). Other drivers (e.g. TI) can benefit
from this code too.

> 
> Shouldn't these new APIs be declared under include/linux/thermal.h?

I'd prefer to leave this API in ./drivers/thermal and move them to
./include/linux/thermal.h only when other parts of the kernel require
it.

> 
> This way, they can be be available to other parts of the kernel too.
> 
> 
> >  #else
> >  static inline int of_parse_thermal_zones(void) { return 0; }
> >  static inline void of_thermal_destroy_zones(void) { }
> > +static inline int of_thermal_get_ntrips(struct thermal_zone_device
> > *tz) +{
> > +	return 0;
> > +}
> >  #endif
> >  
> >  #endif /* __THERMAL_CORE_H__ */
> > -- 
> > 2.0.0.rc2
> > 

Best Regards,

Lukasz Majewski

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 0/5] thermal: of: of-thermal.c API extensions

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

On Mon, Dec 08, 2014 at 06:04:16PM +0100, Lukasz Majewski wrote:
> Provided patch series extend of-thermal.c file API. It is necessary 
> for ongoing Exynos TMU rework.
> 
> First version of this code can be found at:
> http://www.spinics.net/lists/linux-samsung-soc/msg37719.html
> 
> Second version:
> http://www.spinics.net/lists/kernel/msg1872274.html
> 
> This code provides methods to access some of-thermal local data.
> Moreover it provides a read only access to of-thermal trip points.
> 
> Lukasz Majewski (5):
>   thermal: of: Extend of-thermal.c to provide number of trip points
>   thermal: of: Extend of-thermal.c to provide check if trip point is
>     valid
>   thermal: of: Rename struct __thermal_trip to struct thermal_trip
>   thermal: of: Extend of-thermal to export table of trip points
>   thermal: of: Extend current of-thermal.c code to allow setting
>     emulated temp

Applied the series in my -linus branch.

Cheers

> 
>  drivers/thermal/of-thermal.c   | 109 ++++++++++++++++++++++++++++++++++-------
>  drivers/thermal/thermal_core.h |  18 +++++++
>  include/linux/thermal.h        |  18 +++++++
>  3 files changed, 127 insertions(+), 18 deletions(-)
> 
> -- 
> 2.0.0.rc2
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]