[PATCH 1/3] thermal: of: support writable trips via dt

[PATCH 1/3] thermal: of: support writable trips via dt

[Navneet Kumar]

From: navneet kumar <navneetk@nvidia.com>

Support writable trip points configuration from the
device tree. 'OF' reads this configuration and adjusts
the 'trips' mask accordingly to allow the 'set_trip_xxx'
calls to be effective.

Signed-off-by: Diwakar Tundlam <dtundlam@nvidia.com>
---
 drivers/thermal/of-thermal.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 62143ba31001..cf9ee3e82fee 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -604,7 +604,8 @@ 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,
+				    bool *trip_writable)
 {
 	int prop;
 	int ret;
@@ -629,6 +630,8 @@ static int thermal_of_populate_trip(struct device_node *np,
 		return ret;
 	}
 
+	*trip_writable = of_property_read_bool(np, "writable");
+
 	/* Required for cooling map matching */
 	trip->np = np;
 	of_node_get(np);
@@ -657,6 +660,8 @@ thermal_of_build_thermal_zone(struct device_node *np)
 	struct __thermal_zone *tz;
 	int ret, i;
 	u32 prop;
+	bool trip_writable;
+	u64 m = 0;
 
 	if (!np) {
 		pr_err("no thermal zone np\n");
@@ -700,9 +705,14 @@ thermal_of_build_thermal_zone(struct device_node *np)
 
 	i = 0;
 	for_each_child_of_node(child, gchild) {
-		ret = thermal_of_populate_trip(gchild, &tz->trips[i++]);
+		trip_writable = false;
+		ret = thermal_of_populate_trip(gchild, &tz->trips[i],
+					       &trip_writable);
 		if (ret)
 			goto free_trips;
+		if (trip_writable)
+			m |= 1ULL << i;
+		i++;
 	}
 
 	of_node_put(child);
-- 
1.8.1.5

[PATCH 2/3] thermal: of: consolidate sensor callbacks as ops

[Navneet Kumar]

From: navneet kumar <navneetk@nvidia.com>

Consolidate all the sensor callbacks (get_temp/get_trend)
into a 'thermal_of_sensor_ops' struct.

As a part of this, introduce a 'thermal_zone_of_sensor_register2'
sensor API that accepts sensor_ops instead of the two callbacks
as separate arguments to the register function.

Modify the older version of register function to call register2.

Adjust all the references to get_temp/get_trend callbacks.

Signed-off-by: navneet kumar <navneetk@nvidia.com>
---
 drivers/thermal/of-thermal.c | 78 ++++++++++++++++++++++++++++----------------
 include/linux/thermal.h      | 14 ++++++++
 2 files changed, 64 insertions(+), 28 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index cf9ee3e82fee..3d47a0cf3825 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -96,8 +96,7 @@ struct __thermal_zone {
 
 	/* sensor interface */
 	void *sensor_data;
-	int (*get_temp)(void *, long *);
-	int (*get_trend)(void *, long *);
+	struct thermal_of_sensor_ops sops;
 };
 
 /***   DT thermal zone device callbacks   ***/
@@ -107,10 +106,10 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
 {
 	struct __thermal_zone *data = tz->devdata;
 
-	if (!data->get_temp)
+	if (!data->sops.get_temp)
 		return -EINVAL;
 
-	return data->get_temp(data->sensor_data, temp);
+	return data->sops.get_temp(data->sensor_data, temp);
 }
 
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
@@ -120,10 +119,10 @@ static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 	long dev_trend;
 	int r;
 
-	if (!data->get_trend)
+	if (!data->sops.get_trend)
 		return -EINVAL;
 
-	r = data->get_trend(data->sensor_data, &dev_trend);
+	r = data->sops.get_trend(data->sensor_data, &dev_trend);
 	if (r)
 		return r;
 
@@ -324,8 +323,7 @@ static struct thermal_zone_device_ops of_thermal_ops = {
 static struct thermal_zone_device *
 thermal_zone_of_add_sensor(struct device_node *zone,
 			   struct device_node *sensor, void *data,
-			   int (*get_temp)(void *, long *),
-			   int (*get_trend)(void *, long *))
+			   struct thermal_of_sensor_ops *sops)
 {
 	struct thermal_zone_device *tzd;
 	struct __thermal_zone *tz;
@@ -337,8 +335,9 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 	tz = tzd->devdata;
 
 	mutex_lock(&tzd->lock);
-	tz->get_temp = get_temp;
-	tz->get_trend = get_trend;
+	if (sops)
+		memcpy(&(tz->sops), sops, sizeof(*sops));
+
 	tz->sensor_data = data;
 
 	tzd->ops->get_temp = of_thermal_get_temp;
@@ -349,15 +348,15 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 }
 
 /**
- * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
+ * thermal_zone_of_sensor_register2 - registers a sensor to a DT thermal zone
  * @dev: a valid struct device pointer of a sensor device. Must contain
  *       a valid .of_node, for the sensor node.
  * @sensor_id: a sensor identifier, in case the sensor IP has more
  *             than one sensors
  * @data: a private pointer (owned by the caller) that will be passed
  *        back, when a temperature reading is needed.
- * @get_temp: a pointer to a function that reads the sensor temperature.
- * @get_trend: a pointer to a function that reads the sensor temperature trend.
+ * @sops: handle to the sensor ops (get_temp/get_trend etc.) provided by the
+ *		sensor to OF.
  *
  * This function will search the list of thermal zones described in device
  * tree and look for the zone that refer to the sensor device pointed by
@@ -370,21 +369,13 @@ thermal_zone_of_add_sensor(struct device_node *zone,
  * The thermal zone temperature trend is provided by the @get_trend function
  * pointer. When called, it will have the private pointer @data back.
  *
- * TODO:
- * 01 - This function must enqueue the new sensor instead of using
- * it as the only source of temperature values.
- *
- * 02 - There must be a way to match the sensor with all thermal zones
- * that refer to it.
- *
  * Return: On success returns a valid struct thermal_zone_device,
  * otherwise, it returns a corresponding ERR_PTR(). Caller must
  * check the return value with help of IS_ERR() helper.
  */
 struct thermal_zone_device *
-thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
-				void *data, int (*get_temp)(void *, long *),
-				int (*get_trend)(void *, long *))
+thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
+				void *data, struct thermal_of_sensor_ops *sops)
 {
 	struct device_node *np, *child, *sensor_np;
 	struct thermal_zone_device *tzd = ERR_PTR(-ENODEV);
@@ -426,9 +417,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
 
 		if (sensor_specs.np == sensor_np && id == sensor_id) {
 			tzd = thermal_zone_of_add_sensor(child, sensor_np,
-							 data,
-							 get_temp,
-							 get_trend);
+							  data,
+							  sops);
 			of_node_put(sensor_specs.np);
 			of_node_put(child);
 			goto exit;
@@ -441,6 +431,38 @@ exit:
 
 	return tzd;
 }
+EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_register2);
+
+/**
+ * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
+ * @dev: a valid struct device pointer of a sensor device. Must contain
+ *       a valid .of_node, for the sensor node.
+ * @sensor_id: a sensor identifier, in case the sensor IP has more
+ *             than one sensors
+ * @data: a private pointer (owned by the caller) that will be passed
+ *        back, when a temperature reading is needed.
+ * @get_temp: a pointer to a function that reads the sensor temperature.
+ * @get_trend: a pointer to a function that reads the sensor temperature trend.
+ *
+ * This function calls thermal_zone_of_sensor_register2 after translating
+ * the sensor callbacks into a single structi (sops).
+ *
+ * Return: Bubbles up the return status from thermal_zone_of_register2
+ *
+ */
+struct thermal_zone_device *
+thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
+				void *data, int (*get_temp)(void *, long *),
+				int (*get_trend)(void *, long *))
+{
+	struct thermal_of_sensor_ops sops = {
+		.get_temp = get_temp,
+		.get_trend = get_trend,
+	};
+
+	return thermal_zone_of_sensor_register2(dev, sensor_id, data, &sops);
+
+}
 EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_register);
 
 /**
@@ -476,8 +498,8 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
 	tzd->ops->get_temp = NULL;
 	tzd->ops->get_trend = NULL;
 
-	tz->get_temp = NULL;
-	tz->get_trend = NULL;
+	tz->sops.get_temp = NULL;
+	tz->sops.get_trend = NULL;
 	tz->sensor_data = NULL;
 	mutex_unlock(&tzd->lock);
 }
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index ef90838b36a0..58341c56a01f 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -289,6 +289,11 @@ struct thermal_genl_event {
 	enum events event;
 };
 
+struct thermal_of_sensor_ops {
+	int (*get_temp)(void *, long *);
+	int (*get_trend)(void *, long *);
+};
+
 /* Function declarations */
 #ifdef CONFIG_THERMAL_OF
 struct thermal_zone_device *
@@ -297,6 +302,9 @@ thermal_zone_of_sensor_register(struct device *dev, int id,
 				int (*get_trend)(void *, long *));
 void thermal_zone_of_sensor_unregister(struct device *dev,
 				       struct thermal_zone_device *tz);
+struct thermal_zone_device *
+thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
+				void *data, struct thermal_of_sensor_ops *sops);
 #else
 static inline struct thermal_zone_device *
 thermal_zone_of_sensor_register(struct device *dev, int id,
@@ -312,6 +320,12 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
 {
 }
 
+static inline struct thermal_zone_device *
+thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
+				void *data, struct thermal_of_sensor_ops *sops)
+{
+	return NULL;
+}
 #endif
 struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
 		void *, struct thermal_zone_device_ops *,
-- 
1.8.1.5

[PATCH 3/3] thermal: of: notify sensor driver on trip updates

[Navneet Kumar]

From: navneet kumar <navneetk@nvidia.com>

some thermal sensor hardwares include logic which
can raise interrupts at certain programmed temperature
thresholds.

Drivers for such sensors should be able to learn the
appropriate threshold temperatures for interrupts by querying
the thermal framework.

This change provides a mechanism to allow a sensor driver to
update it's thresholds when userspace changes a trip point
temperature.

While this behavior may not make sense in thermal zones
with more than one sensor, no such examples exist in
the kernel.

Signed-off-by: navneet kumar <navneetk@nvidia.com>
---
 drivers/thermal/of-thermal.c | 7 +++++++
 include/linux/thermal.h      | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 3d47a0cf3825..3568e4a586dc 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -258,6 +258,9 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
 	/* thermal framework should take care of data->mask & (1 << trip) */
 	data->trips[trip].temperature = temp;
 
+	if (data->sops.trip_update)
+		data->sops.trip_update(data->sensor_data, trip);
+
 	return 0;
 }
 
@@ -285,6 +288,9 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
 	/* thermal framework should take care of data->mask & (1 << trip) */
 	data->trips[trip].hysteresis = hyst;
 
+	if (data->sops.trip_update)
+		data->sops.trip_update(data->sensor_data, trip);
+
 	return 0;
 }
 
@@ -500,6 +506,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
 
 	tz->sops.get_temp = NULL;
 	tz->sops.get_trend = NULL;
+	tz->sops.trip_update = NULL;
 	tz->sensor_data = NULL;
 	mutex_unlock(&tzd->lock);
 }
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 58341c56a01f..b93e65815175 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -292,6 +292,7 @@ struct thermal_genl_event {
 struct thermal_of_sensor_ops {
 	int (*get_temp)(void *, long *);
 	int (*get_trend)(void *, long *);
+	int (*trip_update)(void *, int);
 };
 
 /* Function declarations */
-- 
1.8.1.5

Re: [PATCH 1/3] thermal: of: support writable trips via dt

[Eduardo Valentin]

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

Hello Navneet

On Wed, Nov 26, 2014 at 05:16:27PM -0800, Navneet Kumar wrote:
> From: navneet kumar <navneetk@nvidia.com>
> 
> Support writable trip points configuration from the
> device tree. 'OF' reads this configuration and adjusts
> the 'trips' mask accordingly to allow the 'set_trip_xxx'
> calls to be effective.
> 
> Signed-off-by: Diwakar Tundlam <dtundlam@nvidia.com>

Thanks for sharing your patches!

> ---
>  drivers/thermal/of-thermal.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 62143ba31001..cf9ee3e82fee 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -604,7 +604,8 @@ 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,
> +				    bool *trip_writable)
>  {
>  	int prop;
>  	int ret;
> @@ -629,6 +630,8 @@ static int thermal_of_populate_trip(struct device_node *np,
>  		return ret;
>  	}
>  
> +	*trip_writable = of_property_read_bool(np, "writable");

New DT properties needs to be properly discussed in device tree mainling
list. From what I see here, this property does not describe hardware,
does it? 

A simple git grep writable Documentation/devicetree/bindings/
returns nothing. So, I am a bit skeptic having this property is
allowable.

In any case, can you please send your proposal also copying device tree mailing list?
devicetree@vger.kernel.org

Besides, you need to document this new property:
Documentation/devicetree/bindings/thermal/thermal.txt 


All the best,

Eduardo Valentin

> +
>  	/* Required for cooling map matching */
>  	trip->np = np;
>  	of_node_get(np);
> @@ -657,6 +660,8 @@ thermal_of_build_thermal_zone(struct device_node *np)
>  	struct __thermal_zone *tz;
>  	int ret, i;
>  	u32 prop;
> +	bool trip_writable;
> +	u64 m = 0;
>  
>  	if (!np) {
>  		pr_err("no thermal zone np\n");
> @@ -700,9 +705,14 @@ thermal_of_build_thermal_zone(struct device_node *np)
>  
>  	i = 0;
>  	for_each_child_of_node(child, gchild) {
> -		ret = thermal_of_populate_trip(gchild, &tz->trips[i++]);
> +		trip_writable = false;
> +		ret = thermal_of_populate_trip(gchild, &tz->trips[i],
> +					       &trip_writable);
>  		if (ret)
>  			goto free_trips;
> +		if (trip_writable)
> +			m |= 1ULL << i;
> +		i++;
>  	}
>  
>  	of_node_put(child);
> -- 
> 1.8.1.5
> 

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

Re: [PATCH 2/3] thermal: of: consolidate sensor callbacks as ops

[Eduardo Valentin]

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


Hello Navneet,

On Wed, Nov 26, 2014 at 05:16:28PM -0800, Navneet Kumar wrote:
> From: navneet kumar <navneetk@nvidia.com>
> 
> Consolidate all the sensor callbacks (get_temp/get_trend)
> into a 'thermal_of_sensor_ops' struct.
> 
> As a part of this, introduce a 'thermal_zone_of_sensor_register2'
> sensor API that accepts sensor_ops instead of the two callbacks
> as separate arguments to the register function.

This is usually not a good thing to do. Specially about the suffix
'.*2', sounds really broken :-(. Best thing to do is to update the API
with the improvement, and update all the users of that old API. 

This is one of the key Linux design decision. Please, have a look at:
Documentation/stable_api_nonsense.txt

To understand why doing such thing is a bad thing to do.

> 
> Modify the older version of register function to call register2.
> 
> Adjust all the references to get_temp/get_trend callbacks.
> 
> Signed-off-by: navneet kumar <navneetk@nvidia.com>
> ---
>  drivers/thermal/of-thermal.c | 78 ++++++++++++++++++++++++++++----------------
>  include/linux/thermal.h      | 14 ++++++++
>  2 files changed, 64 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index cf9ee3e82fee..3d47a0cf3825 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -96,8 +96,7 @@ struct __thermal_zone {
>  
>  	/* sensor interface */
>  	void *sensor_data;
> -	int (*get_temp)(void *, long *);
> -	int (*get_trend)(void *, long *);
> +	struct thermal_of_sensor_ops sops;

Have you seen this patch:
https://git.kernel.org/cgit/linux/kernel/git/evalenti/linux-soc-thermal.git/commit/?h=linus&id=2251aef64a38db60f4ae7a4a83f9203c6791f196

?

Please rebase your changes on top of my -linus branch:
git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git linus


BR,

Eduardo Valentin
>  };
>  
>  /***   DT thermal zone device callbacks   ***/
> @@ -107,10 +106,10 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>  {
>  	struct __thermal_zone *data = tz->devdata;
>  
> -	if (!data->get_temp)
> +	if (!data->sops.get_temp)
>  		return -EINVAL;
>  
> -	return data->get_temp(data->sensor_data, temp);
> +	return data->sops.get_temp(data->sensor_data, temp);
>  }
>  
>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
> @@ -120,10 +119,10 @@ static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>  	long dev_trend;
>  	int r;
>  
> -	if (!data->get_trend)
> +	if (!data->sops.get_trend)
>  		return -EINVAL;
>  
> -	r = data->get_trend(data->sensor_data, &dev_trend);
> +	r = data->sops.get_trend(data->sensor_data, &dev_trend);
>  	if (r)
>  		return r;
>  
> @@ -324,8 +323,7 @@ static struct thermal_zone_device_ops of_thermal_ops = {
>  static struct thermal_zone_device *
>  thermal_zone_of_add_sensor(struct device_node *zone,
>  			   struct device_node *sensor, void *data,
> -			   int (*get_temp)(void *, long *),
> -			   int (*get_trend)(void *, long *))
> +			   struct thermal_of_sensor_ops *sops)
>  {
>  	struct thermal_zone_device *tzd;
>  	struct __thermal_zone *tz;
> @@ -337,8 +335,9 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  	tz = tzd->devdata;
>  
>  	mutex_lock(&tzd->lock);
> -	tz->get_temp = get_temp;
> -	tz->get_trend = get_trend;
> +	if (sops)
> +		memcpy(&(tz->sops), sops, sizeof(*sops));
> +
>  	tz->sensor_data = data;
>  
>  	tzd->ops->get_temp = of_thermal_get_temp;
> @@ -349,15 +348,15 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  }
>  
>  /**
> - * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
> + * thermal_zone_of_sensor_register2 - registers a sensor to a DT thermal zone
>   * @dev: a valid struct device pointer of a sensor device. Must contain
>   *       a valid .of_node, for the sensor node.
>   * @sensor_id: a sensor identifier, in case the sensor IP has more
>   *             than one sensors
>   * @data: a private pointer (owned by the caller) that will be passed
>   *        back, when a temperature reading is needed.
> - * @get_temp: a pointer to a function that reads the sensor temperature.
> - * @get_trend: a pointer to a function that reads the sensor temperature trend.
> + * @sops: handle to the sensor ops (get_temp/get_trend etc.) provided by the
> + *		sensor to OF.
>   *
>   * This function will search the list of thermal zones described in device
>   * tree and look for the zone that refer to the sensor device pointed by
> @@ -370,21 +369,13 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>   * The thermal zone temperature trend is provided by the @get_trend function
>   * pointer. When called, it will have the private pointer @data back.
>   *
> - * TODO:
> - * 01 - This function must enqueue the new sensor instead of using
> - * it as the only source of temperature values.
> - *
> - * 02 - There must be a way to match the sensor with all thermal zones
> - * that refer to it.
> - *
>   * Return: On success returns a valid struct thermal_zone_device,
>   * otherwise, it returns a corresponding ERR_PTR(). Caller must
>   * check the return value with help of IS_ERR() helper.
>   */
>  struct thermal_zone_device *
> -thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
> -				void *data, int (*get_temp)(void *, long *),
> -				int (*get_trend)(void *, long *))
> +thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
> +				void *data, struct thermal_of_sensor_ops *sops)
>  {
>  	struct device_node *np, *child, *sensor_np;
>  	struct thermal_zone_device *tzd = ERR_PTR(-ENODEV);
> @@ -426,9 +417,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>  
>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
> -							 data,
> -							 get_temp,
> -							 get_trend);
> +							  data,
> +							  sops);
>  			of_node_put(sensor_specs.np);
>  			of_node_put(child);
>  			goto exit;
> @@ -441,6 +431,38 @@ exit:
>  
>  	return tzd;
>  }
> +EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_register2);
> +
> +/**
> + * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
> + * @dev: a valid struct device pointer of a sensor device. Must contain
> + *       a valid .of_node, for the sensor node.
> + * @sensor_id: a sensor identifier, in case the sensor IP has more
> + *             than one sensors
> + * @data: a private pointer (owned by the caller) that will be passed
> + *        back, when a temperature reading is needed.
> + * @get_temp: a pointer to a function that reads the sensor temperature.
> + * @get_trend: a pointer to a function that reads the sensor temperature trend.
> + *
> + * This function calls thermal_zone_of_sensor_register2 after translating
> + * the sensor callbacks into a single structi (sops).
> + *
> + * Return: Bubbles up the return status from thermal_zone_of_register2
> + *
> + */
> +struct thermal_zone_device *
> +thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
> +				void *data, int (*get_temp)(void *, long *),
> +				int (*get_trend)(void *, long *))
> +{
> +	struct thermal_of_sensor_ops sops = {
> +		.get_temp = get_temp,
> +		.get_trend = get_trend,
> +	};
> +
> +	return thermal_zone_of_sensor_register2(dev, sensor_id, data, &sops);
> +
> +}
>  EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_register);
>  
>  /**
> @@ -476,8 +498,8 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>  	tzd->ops->get_temp = NULL;
>  	tzd->ops->get_trend = NULL;
>  
> -	tz->get_temp = NULL;
> -	tz->get_trend = NULL;
> +	tz->sops.get_temp = NULL;
> +	tz->sops.get_trend = NULL;
>  	tz->sensor_data = NULL;
>  	mutex_unlock(&tzd->lock);
>  }
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index ef90838b36a0..58341c56a01f 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -289,6 +289,11 @@ struct thermal_genl_event {
>  	enum events event;
>  };
>  
> +struct thermal_of_sensor_ops {
> +	int (*get_temp)(void *, long *);
> +	int (*get_trend)(void *, long *);
> +};
> +
>  /* Function declarations */
>  #ifdef CONFIG_THERMAL_OF
>  struct thermal_zone_device *
> @@ -297,6 +302,9 @@ thermal_zone_of_sensor_register(struct device *dev, int id,
>  				int (*get_trend)(void *, long *));
>  void thermal_zone_of_sensor_unregister(struct device *dev,
>  				       struct thermal_zone_device *tz);
> +struct thermal_zone_device *
> +thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
> +				void *data, struct thermal_of_sensor_ops *sops);
>  #else
>  static inline struct thermal_zone_device *
>  thermal_zone_of_sensor_register(struct device *dev, int id,
> @@ -312,6 +320,12 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>  {
>  }
>  
> +static inline struct thermal_zone_device *
> +thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
> +				void *data, struct thermal_of_sensor_ops *sops)
> +{
> +	return NULL;
> +}
>  #endif
>  struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
>  		void *, struct thermal_zone_device_ops *,
> -- 
> 1.8.1.5
> 

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

Re: [PATCH 3/3] thermal: of: notify sensor driver on trip updates

[Eduardo Valentin]

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

Hello Navneet,

On Wed, Nov 26, 2014 at 05:16:29PM -0800, Navneet Kumar wrote:
> From: navneet kumar <navneetk@nvidia.com>
> 
> some thermal sensor hardwares include logic which
> can raise interrupts at certain programmed temperature
> thresholds.
> 
> Drivers for such sensors should be able to learn the
> appropriate threshold temperatures for interrupts by querying
> the thermal framework.
> 
> This change provides a mechanism to allow a sensor driver to
> update it's thresholds when userspace changes a trip point
> temperature.
> 
> While this behavior may not make sense in thermal zones
> with more than one sensor, no such examples exist in
> the kernel.
> 
> Signed-off-by: navneet kumar <navneetk@nvidia.com>
> ---
>  drivers/thermal/of-thermal.c | 7 +++++++
>  include/linux/thermal.h      | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 3d47a0cf3825..3568e4a586dc 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -258,6 +258,9 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>  	/* thermal framework should take care of data->mask & (1 << trip) */
>  	data->trips[trip].temperature = temp;
>  
> +	if (data->sops.trip_update)
> +		data->sops.trip_update(data->sensor_data, trip);
> +
>  	return 0;
>  }
>  
> @@ -285,6 +288,9 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>  	/* thermal framework should take care of data->mask & (1 << trip) */
>  	data->trips[trip].hysteresis = hyst;
>  
> +	if (data->sops.trip_update)
> +		data->sops.trip_update(data->sensor_data, trip);
> +
>  	return 0;
>  }
>  
> @@ -500,6 +506,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>  
>  	tz->sops.get_temp = NULL;
>  	tz->sops.get_trend = NULL;
> +	tz->sops.trip_update = NULL;
>  	tz->sensor_data = NULL;
>  	mutex_unlock(&tzd->lock);
>  }
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 58341c56a01f..b93e65815175 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -292,6 +292,7 @@ struct thermal_genl_event {
>  struct thermal_of_sensor_ops {
>  	int (*get_temp)(void *, long *);
>  	int (*get_trend)(void *, long *);
> +	int (*trip_update)(void *, int);

First thing I ask you is to update your work on top of my -linus branch,
as I already mentioned. Reasoning is that part of the changes you are
sending is already there.

As for this new callback, I am fine with it as long as it is also
available for drivers that do not use of-thermal. Once again, of-thermal
is not a competitor of thermal core. It will never be. It is not a new
thermal API. 

That said, it does not make sense to have functionality in of-thermal that
do not belong to thermal core. Exceptions are, of course, for helping
doing the same operations we already have in thermal core.

All the best,

Eduardo Valentin

>  };
>  
>  /* Function declarations */
> -- 
> 1.8.1.5
> 

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

Re: [PATCH 2/3] thermal: of: consolidate sensor callbacks as ops

[navneet kumar]

Hi Eduardo,

On 11/27/2014 06:28 AM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> 
> Hello Navneet,
> 
> On Wed, Nov 26, 2014 at 05:16:28PM -0800, Navneet Kumar wrote:
>> From: navneet kumar <navneetk@nvidia.com>
>>
>> Consolidate all the sensor callbacks (get_temp/get_trend)
>> into a 'thermal_of_sensor_ops' struct.
>>
>> As a part of this, introduce a 'thermal_zone_of_sensor_register2'
>> sensor API that accepts sensor_ops instead of the two callbacks
>> as separate arguments to the register function.
> 
> This is usually not a good thing to do. Specially about the suffix
> '.*2', sounds really broken :-(. Best thing to do is to update the API
> with the improvement, and update all the users of that old API. 
> 
> This is one of the key Linux design decision. Please, have a look at:
> Documentation/stable_api_nonsense.txt
> 
> To understand why doing such thing is a bad thing to do.
> 
Thanks for correcting me. I was thinking on the lines of staging this patch as-
1. release a *register2
2. fixup rest of the drivers ( as a separate patch) to use *register2
3. rename all the references of register2 as register and eventually terminate
the use of the old signature'd API.

Either ways, i see your patch now, and will do the needful rebase too!

thanks again.
>>
>> Modify the older version of register function to call register2.
>>
>> Adjust all the references to get_temp/get_trend callbacks.
>>
>> Signed-off-by: navneet kumar <navneetk@nvidia.com>
>> ---
>>  drivers/thermal/of-thermal.c | 78 ++++++++++++++++++++++++++++----------------
>>  include/linux/thermal.h      | 14 ++++++++
>>  2 files changed, 64 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index cf9ee3e82fee..3d47a0cf3825 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -96,8 +96,7 @@ struct __thermal_zone {
>>  
>>  	/* sensor interface */
>>  	void *sensor_data;
>> -	int (*get_temp)(void *, long *);
>> -	int (*get_trend)(void *, long *);
>> +	struct thermal_of_sensor_ops sops;
> 
> Have you seen this patch:
> https://git.kernel.org/cgit/linux/kernel/git/evalenti/linux-soc-thermal.git/commit/?h=linus&id=2251aef64a38db60f4ae7a4a83f9203c6791f196
> 
> ?
> 
> Please rebase your changes on top of my -linus branch:
> git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git linus
> 
> 
> BR,
> 
> Eduardo Valentin
>>  };
>>  
>>  /***   DT thermal zone device callbacks   ***/
>> @@ -107,10 +106,10 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>>  {
>>  	struct __thermal_zone *data = tz->devdata;
>>  
>> -	if (!data->get_temp)
>> +	if (!data->sops.get_temp)
>>  		return -EINVAL;
>>  
>> -	return data->get_temp(data->sensor_data, temp);
>> +	return data->sops.get_temp(data->sensor_data, temp);
>>  }
>>  
>>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>> @@ -120,10 +119,10 @@ static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>>  	long dev_trend;
>>  	int r;
>>  
>> -	if (!data->get_trend)
>> +	if (!data->sops.get_trend)
>>  		return -EINVAL;
>>  
>> -	r = data->get_trend(data->sensor_data, &dev_trend);
>> +	r = data->sops.get_trend(data->sensor_data, &dev_trend);
>>  	if (r)
>>  		return r;
>>  
>> @@ -324,8 +323,7 @@ static struct thermal_zone_device_ops of_thermal_ops = {
>>  static struct thermal_zone_device *
>>  thermal_zone_of_add_sensor(struct device_node *zone,
>>  			   struct device_node *sensor, void *data,
>> -			   int (*get_temp)(void *, long *),
>> -			   int (*get_trend)(void *, long *))
>> +			   struct thermal_of_sensor_ops *sops)
>>  {
>>  	struct thermal_zone_device *tzd;
>>  	struct __thermal_zone *tz;
>> @@ -337,8 +335,9 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>>  	tz = tzd->devdata;
>>  
>>  	mutex_lock(&tzd->lock);
>> -	tz->get_temp = get_temp;
>> -	tz->get_trend = get_trend;
>> +	if (sops)
>> +		memcpy(&(tz->sops), sops, sizeof(*sops));
>> +
>>  	tz->sensor_data = data;
>>  
>>  	tzd->ops->get_temp = of_thermal_get_temp;
>> @@ -349,15 +348,15 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>>  }
>>  
>>  /**
>> - * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
>> + * thermal_zone_of_sensor_register2 - registers a sensor to a DT thermal zone
>>   * @dev: a valid struct device pointer of a sensor device. Must contain
>>   *       a valid .of_node, for the sensor node.
>>   * @sensor_id: a sensor identifier, in case the sensor IP has more
>>   *             than one sensors
>>   * @data: a private pointer (owned by the caller) that will be passed
>>   *        back, when a temperature reading is needed.
>> - * @get_temp: a pointer to a function that reads the sensor temperature.
>> - * @get_trend: a pointer to a function that reads the sensor temperature trend.
>> + * @sops: handle to the sensor ops (get_temp/get_trend etc.) provided by the
>> + *		sensor to OF.
>>   *
>>   * This function will search the list of thermal zones described in device
>>   * tree and look for the zone that refer to the sensor device pointed by
>> @@ -370,21 +369,13 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>>   * The thermal zone temperature trend is provided by the @get_trend function
>>   * pointer. When called, it will have the private pointer @data back.
>>   *
>> - * TODO:
>> - * 01 - This function must enqueue the new sensor instead of using
>> - * it as the only source of temperature values.
>> - *
>> - * 02 - There must be a way to match the sensor with all thermal zones
>> - * that refer to it.
>> - *
>>   * Return: On success returns a valid struct thermal_zone_device,
>>   * otherwise, it returns a corresponding ERR_PTR(). Caller must
>>   * check the return value with help of IS_ERR() helper.
>>   */
>>  struct thermal_zone_device *
>> -thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>> -				void *data, int (*get_temp)(void *, long *),
>> -				int (*get_trend)(void *, long *))
>> +thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
>> +				void *data, struct thermal_of_sensor_ops *sops)
>>  {
>>  	struct device_node *np, *child, *sensor_np;
>>  	struct thermal_zone_device *tzd = ERR_PTR(-ENODEV);
>> @@ -426,9 +417,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>>  
>>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>> -							 data,
>> -							 get_temp,
>> -							 get_trend);
>> +							  data,
>> +							  sops);
>>  			of_node_put(sensor_specs.np);
>>  			of_node_put(child);
>>  			goto exit;
>> @@ -441,6 +431,38 @@ exit:
>>  
>>  	return tzd;
>>  }
>> +EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_register2);
>> +
>> +/**
>> + * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
>> + * @dev: a valid struct device pointer of a sensor device. Must contain
>> + *       a valid .of_node, for the sensor node.
>> + * @sensor_id: a sensor identifier, in case the sensor IP has more
>> + *             than one sensors
>> + * @data: a private pointer (owned by the caller) that will be passed
>> + *        back, when a temperature reading is needed.
>> + * @get_temp: a pointer to a function that reads the sensor temperature.
>> + * @get_trend: a pointer to a function that reads the sensor temperature trend.
>> + *
>> + * This function calls thermal_zone_of_sensor_register2 after translating
>> + * the sensor callbacks into a single structi (sops).
>> + *
>> + * Return: Bubbles up the return status from thermal_zone_of_register2
>> + *
>> + */
>> +struct thermal_zone_device *
>> +thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>> +				void *data, int (*get_temp)(void *, long *),
>> +				int (*get_trend)(void *, long *))
>> +{
>> +	struct thermal_of_sensor_ops sops = {
>> +		.get_temp = get_temp,
>> +		.get_trend = get_trend,
>> +	};
>> +
>> +	return thermal_zone_of_sensor_register2(dev, sensor_id, data, &sops);
>> +
>> +}
>>  EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_register);
>>  
>>  /**
>> @@ -476,8 +498,8 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>>  	tzd->ops->get_temp = NULL;
>>  	tzd->ops->get_trend = NULL;
>>  
>> -	tz->get_temp = NULL;
>> -	tz->get_trend = NULL;
>> +	tz->sops.get_temp = NULL;
>> +	tz->sops.get_trend = NULL;
>>  	tz->sensor_data = NULL;
>>  	mutex_unlock(&tzd->lock);
>>  }
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index ef90838b36a0..58341c56a01f 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -289,6 +289,11 @@ struct thermal_genl_event {
>>  	enum events event;
>>  };
>>  
>> +struct thermal_of_sensor_ops {
>> +	int (*get_temp)(void *, long *);
>> +	int (*get_trend)(void *, long *);
>> +};
>> +
>>  /* Function declarations */
>>  #ifdef CONFIG_THERMAL_OF
>>  struct thermal_zone_device *
>> @@ -297,6 +302,9 @@ thermal_zone_of_sensor_register(struct device *dev, int id,
>>  				int (*get_trend)(void *, long *));
>>  void thermal_zone_of_sensor_unregister(struct device *dev,
>>  				       struct thermal_zone_device *tz);
>> +struct thermal_zone_device *
>> +thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
>> +				void *data, struct thermal_of_sensor_ops *sops);
>>  #else
>>  static inline struct thermal_zone_device *
>>  thermal_zone_of_sensor_register(struct device *dev, int id,
>> @@ -312,6 +320,12 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>>  {
>>  }
>>  
>> +static inline struct thermal_zone_device *
>> +thermal_zone_of_sensor_register2(struct device *dev, int sensor_id,
>> +				void *data, struct thermal_of_sensor_ops *sops)
>> +{
>> +	return NULL;
>> +}
>>  #endif
>>  struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
>>  		void *, struct thermal_zone_device_ops *,
>> -- 
>> 1.8.1.5
>>
> 
> * Unknown Key
> * 0x7DA4E256
> 

Re: [PATCH 3/3] thermal: of: notify sensor driver on trip updates

[navneet kumar]

Hi Eduardo,

On 11/27/2014 06:32 AM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> Hello Navneet,
> 
> On Wed, Nov 26, 2014 at 05:16:29PM -0800, Navneet Kumar wrote:
>> From: navneet kumar <navneetk@nvidia.com>
>>
>> some thermal sensor hardwares include logic which
>> can raise interrupts at certain programmed temperature
>> thresholds.
>>
>> Drivers for such sensors should be able to learn the
>> appropriate threshold temperatures for interrupts by querying
>> the thermal framework.
>>
>> This change provides a mechanism to allow a sensor driver to
>> update it's thresholds when userspace changes a trip point
>> temperature.
>>
>> While this behavior may not make sense in thermal zones
>> with more than one sensor, no such examples exist in
>> the kernel.
>>
>> Signed-off-by: navneet kumar <navneetk@nvidia.com>
>> ---
>>  drivers/thermal/of-thermal.c | 7 +++++++
>>  include/linux/thermal.h      | 1 +
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index 3d47a0cf3825..3568e4a586dc 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -258,6 +258,9 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>>  	/* thermal framework should take care of data->mask & (1 << trip) */
>>  	data->trips[trip].temperature = temp;
>>  
>> +	if (data->sops.trip_update)
>> +		data->sops.trip_update(data->sensor_data, trip);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -285,6 +288,9 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>>  	/* thermal framework should take care of data->mask & (1 << trip) */
>>  	data->trips[trip].hysteresis = hyst;
>>  
>> +	if (data->sops.trip_update)
>> +		data->sops.trip_update(data->sensor_data, trip);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -500,6 +506,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>>  
>>  	tz->sops.get_temp = NULL;
>>  	tz->sops.get_trend = NULL;
>> +	tz->sops.trip_update = NULL;
>>  	tz->sensor_data = NULL;
>>  	mutex_unlock(&tzd->lock);
>>  }
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index 58341c56a01f..b93e65815175 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -292,6 +292,7 @@ struct thermal_genl_event {
>>  struct thermal_of_sensor_ops {
>>  	int (*get_temp)(void *, long *);
>>  	int (*get_trend)(void *, long *);
>> +	int (*trip_update)(void *, int);
> 
> First thing I ask you is to update your work on top of my -linus branch,
> as I already mentioned. Reasoning is that part of the changes you are
> sending is already there.
will do.
> 
> As for this new callback, I am fine with it as long as it is also
> available for drivers that do not use of-thermal. Once again, of-thermal
> is not a competitor of thermal core. It will never be. It is not a new
> thermal API. 
I agree that this callback is not a part of the thermal_core functionality.
However, when a driver registers directly with the thermal_core (doesn't use
of-thermal), it 'owns' the set_trip_XX callbacks in the first place; which is
the sole purpose of using the 'trip_update' callback in of-thermal.

Adding an additional 'update' to the thermal_core ops would be a no-op. right?
> 
> That said, it does not make sense to have functionality in of-thermal that
> do not belong to thermal core. Exceptions are, of course, for helping
> doing the same operations we already have in thermal core.
> 
> All the best,
> 
> Eduardo Valentin
> 
>>  };
>>  
>>  /* Function declarations */
>> -- 
>> 1.8.1.5
>>
> 
> * Unknown Key
> * 0x7DA4E256
> 

Re: [PATCH 3/3] thermal: of: notify sensor driver on trip updates

[Eduardo Valentin]

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

On Mon, Dec 01, 2014 at 12:45:52PM -0800, navneet kumar wrote:
> Hi Eduardo,
> 
> On 11/27/2014 06:32 AM, Eduardo Valentin wrote:
> > * PGP Signed by an unknown key
> > 
> > Hello Navneet,
> > 
> > On Wed, Nov 26, 2014 at 05:16:29PM -0800, Navneet Kumar wrote:
> >> From: navneet kumar <navneetk@nvidia.com>
> >>
> >> some thermal sensor hardwares include logic which
> >> can raise interrupts at certain programmed temperature
> >> thresholds.
> >>
> >> Drivers for such sensors should be able to learn the
> >> appropriate threshold temperatures for interrupts by querying
> >> the thermal framework.
> >>
> >> This change provides a mechanism to allow a sensor driver to
> >> update it's thresholds when userspace changes a trip point
> >> temperature.
> >>
> >> While this behavior may not make sense in thermal zones
> >> with more than one sensor, no such examples exist in
> >> the kernel.
> >>
> >> Signed-off-by: navneet kumar <navneetk@nvidia.com>
> >> ---
> >>  drivers/thermal/of-thermal.c | 7 +++++++
> >>  include/linux/thermal.h      | 1 +
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >> index 3d47a0cf3825..3568e4a586dc 100644
> >> --- a/drivers/thermal/of-thermal.c
> >> +++ b/drivers/thermal/of-thermal.c
> >> @@ -258,6 +258,9 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
> >>  	/* thermal framework should take care of data->mask & (1 << trip) */
> >>  	data->trips[trip].temperature = temp;
> >>  
> >> +	if (data->sops.trip_update)
> >> +		data->sops.trip_update(data->sensor_data, trip);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -285,6 +288,9 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
> >>  	/* thermal framework should take care of data->mask & (1 << trip) */
> >>  	data->trips[trip].hysteresis = hyst;
> >>  
> >> +	if (data->sops.trip_update)
> >> +		data->sops.trip_update(data->sensor_data, trip);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -500,6 +506,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
> >>  
> >>  	tz->sops.get_temp = NULL;
> >>  	tz->sops.get_trend = NULL;
> >> +	tz->sops.trip_update = NULL;
> >>  	tz->sensor_data = NULL;
> >>  	mutex_unlock(&tzd->lock);
> >>  }
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index 58341c56a01f..b93e65815175 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -292,6 +292,7 @@ struct thermal_genl_event {
> >>  struct thermal_of_sensor_ops {
> >>  	int (*get_temp)(void *, long *);
> >>  	int (*get_trend)(void *, long *);
> >> +	int (*trip_update)(void *, int);
> > 
> > First thing I ask you is to update your work on top of my -linus branch,
> > as I already mentioned. Reasoning is that part of the changes you are
> > sending is already there.
> will do.
> > 
> > As for this new callback, I am fine with it as long as it is also
> > available for drivers that do not use of-thermal. Once again, of-thermal
> > is not a competitor of thermal core. It will never be. It is not a new
> > thermal API. 
> I agree that this callback is not a part of the thermal_core functionality.
> However, when a driver registers directly with the thermal_core (doesn't use
> of-thermal), it 'owns' the set_trip_XX callbacks in the first place; which is
> the sole purpose of using the 'trip_update' callback in of-thermal.
> 
> Adding an additional 'update' to the thermal_core ops would be a no-op. right?

Yes, you are right. Now I understand your point. 

Can we then re-use the .set_trips nomenclature?

Cheers,

> > 
> > That said, it does not make sense to have functionality in of-thermal that
> > do not belong to thermal core. Exceptions are, of course, for helping
> > doing the same operations we already have in thermal core.
> > 
> > All the best,
> > 
> > Eduardo Valentin
> > 
> >>  };
> >>  
> >>  /* Function declarations */
> >> -- 
> >> 1.8.1.5
> >>
> > 
> > * Unknown Key
> > * 0x7DA4E256
> > 

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

Re: [PATCH 3/3] thermal: of: notify sensor driver on trip updates

[navneet kumar]

On 12/01/2014 01:23 PM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Dec 01, 2014 at 12:45:52PM -0800, navneet kumar wrote:
>> Hi Eduardo,
>>
>> On 11/27/2014 06:32 AM, Eduardo Valentin wrote:
>>>> Old Signed by an unknown key
>>>
>>> Hello Navneet,
>>>
>>> On Wed, Nov 26, 2014 at 05:16:29PM -0800, Navneet Kumar wrote:
>>>> From: navneet kumar <navneetk@nvidia.com>
>>>>
>>>> some thermal sensor hardwares include logic which
>>>> can raise interrupts at certain programmed temperature
>>>> thresholds.
>>>>
>>>> Drivers for such sensors should be able to learn the
>>>> appropriate threshold temperatures for interrupts by querying
>>>> the thermal framework.
>>>>
>>>> This change provides a mechanism to allow a sensor driver to
>>>> update it's thresholds when userspace changes a trip point
>>>> temperature.
>>>>
>>>> While this behavior may not make sense in thermal zones
>>>> with more than one sensor, no such examples exist in
>>>> the kernel.
>>>>
>>>> Signed-off-by: navneet kumar <navneetk@nvidia.com>
>>>> ---
>>>>  drivers/thermal/of-thermal.c | 7 +++++++
>>>>  include/linux/thermal.h      | 1 +
>>>>  2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>> index 3d47a0cf3825..3568e4a586dc 100644
>>>> --- a/drivers/thermal/of-thermal.c
>>>> +++ b/drivers/thermal/of-thermal.c
>>>> @@ -258,6 +258,9 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>>>>  	/* thermal framework should take care of data->mask & (1 << trip) */
>>>>  	data->trips[trip].temperature = temp;
>>>>  
>>>> +	if (data->sops.trip_update)
>>>> +		data->sops.trip_update(data->sensor_data, trip);
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -285,6 +288,9 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>>>>  	/* thermal framework should take care of data->mask & (1 << trip) */
>>>>  	data->trips[trip].hysteresis = hyst;
>>>>  
>>>> +	if (data->sops.trip_update)
>>>> +		data->sops.trip_update(data->sensor_data, trip);
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -500,6 +506,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>>>>  
>>>>  	tz->sops.get_temp = NULL;
>>>>  	tz->sops.get_trend = NULL;
>>>> +	tz->sops.trip_update = NULL;
>>>>  	tz->sensor_data = NULL;
>>>>  	mutex_unlock(&tzd->lock);
>>>>  }
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index 58341c56a01f..b93e65815175 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -292,6 +292,7 @@ struct thermal_genl_event {
>>>>  struct thermal_of_sensor_ops {
>>>>  	int (*get_temp)(void *, long *);
>>>>  	int (*get_trend)(void *, long *);
>>>> +	int (*trip_update)(void *, int);
>>>
>>> First thing I ask you is to update your work on top of my -linus branch,
>>> as I already mentioned. Reasoning is that part of the changes you are
>>> sending is already there.
>> will do.
>>>
>>> As for this new callback, I am fine with it as long as it is also
>>> available for drivers that do not use of-thermal. Once again, of-thermal
>>> is not a competitor of thermal core. It will never be. It is not a new
>>> thermal API. 
>> I agree that this callback is not a part of the thermal_core functionality.
>> However, when a driver registers directly with the thermal_core (doesn't use
>> of-thermal), it 'owns' the set_trip_XX callbacks in the first place; which is
>> the sole purpose of using the 'trip_update' callback in of-thermal.
>>
>> Adding an additional 'update' to the thermal_core ops would be a no-op. right?
> 
> Yes, you are right. Now I understand your point. 
> 
> Can we then re-use the .set_trips nomenclature?
Sorry, I fail to understand. Are you suggesting to re-use the interface for set_trip 'temp' as well as 'hyst'?
If so, is it just to maintain the commonality across thermal_core and of-thermal interfaces?

The way i see it, the driver just needs to get some kind of 'update' that 'something' changed with
a trip point; and can later query the trips from of-thermal. (Lukasz's patch helps with that).
Functionality-wise, using two callbacks seems excessive. But i may be wrong :-)

> 
> Cheers,
> 
>>>
>>> That said, it does not make sense to have functionality in of-thermal that
>>> do not belong to thermal core. Exceptions are, of course, for helping
>>> doing the same operations we already have in thermal core.
>>>
>>> All the best,
>>>
>>> Eduardo Valentin
>>>
>>>>  };
>>>>  
>>>>  /* Function declarations */
>>>> -- 
>>>> 1.8.1.5
>>>>
>>>
>>> * Unknown Key
>>> * 0x7DA4E256
>>>
> 
> * Unknown Key
> * 0x7DA4E256
>