[PATCH 0/2] thermal: lookup temperature

[PATCH 0/2] thermal: lookup temperature

[Eduardo Valentin]

hello Rui,

Here is a temperature lookup helper function.

The usage of it is exemplified on patch 02.

Please review. This may overlap with Durgas recent work. But I am resending
as no one has commented my RFC.

Cheers,

Eduardo Valentin (2):
  thermal: introduce thermal_zone_lookup_temperature helper function
  staging: ti-soc-thermal: remove external heat while extrapolating
    hotspot

 drivers/staging/ti-soc-thermal/ti-thermal-common.c |    5 +--
 drivers/thermal/thermal_sys.c                      |   32 ++++++++++++++++++++
 include/linux/thermal.h                            |    1 +
 3 files changed, 35 insertions(+), 3 deletions(-)

-- 
1.7.7.1.488.ge8e1c

[PATCH 0/2] thermal: lookup temperature

[Eduardo Valentin]

hello Rui,

Here is a temperature lookup helper function.

The usage of it is exemplified on patch 02.

Please review. This may overlap with Durgas recent work. But I am resending
as no one has commented my RFC.

Cheers,

Eduardo Valentin (2):
  thermal: introduce thermal_zone_lookup_temperature helper function
  staging: ti-soc-thermal: remove external heat while extrapolating
    hotspot

 drivers/staging/ti-soc-thermal/ti-thermal-common.c |    5 +--
 drivers/thermal/thermal_sys.c                      |   32 ++++++++++++++++++++
 include/linux/thermal.h                            |    1 +
 3 files changed, 35 insertions(+), 3 deletions(-)

-- 
1.7.7.1.488.ge8e1c

[PATCH 1/2] thermal: introduce thermal_zone_lookup_temperature helper function

[Eduardo Valentin]

This patch adds a helper function to get temperature of
a thermal zone, based on the zone type name.

It will perform a zone name lookup and return the last
sensor temperature reading. In case the zone is not found
or if the required parameters are invalid, it will return
the corresponding error code.

Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/thermal/thermal_sys.c |   32 ++++++++++++++++++++++++++++++++
 include/linux/thermal.h       |    1 +
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 5bd95d4..f0caa13 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1790,6 +1790,38 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
 
+/**
+ * thermal_lookup_temperature - search for a zone and returns its temperature
+ * @name: thermal zone name to fetch the temperature
+ * @temperature: pointer to store the zone temperature, in case it is found
+ *
+ * When the zone is found, updates @temperature and returns 0.
+ *
+ * Return: -EINVAL in case of wrong parameters, -ENODEV in case the zone
+ * is not found and 0 when it is successfully found.
+ */
+int thermal_zone_lookup_temperature(const char *name, int *temperature)
+{
+	struct thermal_zone_device *pos = NULL;
+	bool found = false;
+
+	if (!name || !temperature)
+		return -EINVAL;
+
+	mutex_lock(&thermal_list_lock);
+	list_for_each_entry(pos, &thermal_tz_list, node)
+		if (!strcmp(pos->type, name)) {
+			found = true;
+			break;
+		}
+	if (found)
+		*temperature = pos->last_temperature;
+	mutex_unlock(&thermal_list_lock);
+
+	return found ? 0 : -ENODEV;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_lookup_temperature);
+
 #ifdef CONFIG_NET
 static struct genl_family thermal_event_genl_family = {
 	.id = GENL_ID_GENERATE,
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 542a39c..2b2f902 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -237,6 +237,7 @@ void thermal_zone_device_update(struct thermal_zone_device *);
 struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
 		const struct thermal_cooling_device_ops *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
+int thermal_zone_lookup_temperature(const char *name, int *temperature);
 
 int thermal_zone_trend_get(struct thermal_zone_device *, int);
 struct thermal_instance *thermal_instance_get(struct thermal_zone_device *,
-- 
1.7.7.1.488.ge8e1c

[PATCH 1/2] thermal: introduce thermal_zone_lookup_temperature helper function

[Eduardo Valentin]

This patch adds a helper function to get temperature of
a thermal zone, based on the zone type name.

It will perform a zone name lookup and return the last
sensor temperature reading. In case the zone is not found
or if the required parameters are invalid, it will return
the corresponding error code.

Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/thermal/thermal_sys.c |   32 ++++++++++++++++++++++++++++++++
 include/linux/thermal.h       |    1 +
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 5bd95d4..f0caa13 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1790,6 +1790,38 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
 
+/**
+ * thermal_lookup_temperature - search for a zone and returns its temperature
+ * @name: thermal zone name to fetch the temperature
+ * @temperature: pointer to store the zone temperature, in case it is found
+ *
+ * When the zone is found, updates @temperature and returns 0.
+ *
+ * Return: -EINVAL in case of wrong parameters, -ENODEV in case the zone
+ * is not found and 0 when it is successfully found.
+ */
+int thermal_zone_lookup_temperature(const char *name, int *temperature)
+{
+	struct thermal_zone_device *pos = NULL;
+	bool found = false;
+
+	if (!name || !temperature)
+		return -EINVAL;
+
+	mutex_lock(&thermal_list_lock);
+	list_for_each_entry(pos, &thermal_tz_list, node)
+		if (!strcmp(pos->type, name)) {
+			found = true;
+			break;
+		}
+	if (found)
+		*temperature = pos->last_temperature;
+	mutex_unlock(&thermal_list_lock);
+
+	return found ? 0 : -ENODEV;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_lookup_temperature);
+
 #ifdef CONFIG_NET
 static struct genl_family thermal_event_genl_family = {
 	.id = GENL_ID_GENERATE,
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 542a39c..2b2f902 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -237,6 +237,7 @@ void thermal_zone_device_update(struct thermal_zone_device *);
 struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
 		const struct thermal_cooling_device_ops *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
+int thermal_zone_lookup_temperature(const char *name, int *temperature);
 
 int thermal_zone_trend_get(struct thermal_zone_device *, int);
 struct thermal_instance *thermal_instance_get(struct thermal_zone_device *,
-- 
1.7.7.1.488.ge8e1c

[PATCH 2/2] staging: ti-soc-thermal: remove external heat while extrapolating hotspot

[Eduardo Valentin]

For boards that provide a PCB sensor close to SoC junction
temperature, it is possible to remove the cumulative heat
reported by the SoC temperature sensor.

This patch changes the extrapolation computation to consider
an external sensor in the extrapolation equations.

Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/staging/ti-soc-thermal/ti-thermal-common.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ti-soc-thermal/ti-thermal-common.c b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
index 231c549..52d3c1b 100644
--- a/drivers/staging/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
@@ -92,10 +92,9 @@ static inline int ti_thermal_get_temp(struct thermal_zone_device *thermal,
 	if (ret)
 		return ret;
 
-	pcb_temp = 0;
-	/* TODO: Introduce pcb temperature lookup */
+	ret = thermal_zone_lookup_temperature("pcb", &pcb_temp);
 	/* In case pcb zone is available, use the extrapolation rule with it */
-	if (pcb_temp) {
+	if (!ret) {
 		tmp -= pcb_temp;
 		slope = s->slope_pcb;
 		constant = s->constant_pcb;
-- 
1.7.7.1.488.ge8e1c

[PATCH 2/2] staging: ti-soc-thermal: remove external heat while extrapolating hotspot

[Eduardo Valentin]

For boards that provide a PCB sensor close to SoC junction
temperature, it is possible to remove the cumulative heat
reported by the SoC temperature sensor.

This patch changes the extrapolation computation to consider
an external sensor in the extrapolation equations.

Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/staging/ti-soc-thermal/ti-thermal-common.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ti-soc-thermal/ti-thermal-common.c b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
index 231c549..52d3c1b 100644
--- a/drivers/staging/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
@@ -92,10 +92,9 @@ static inline int ti_thermal_get_temp(struct thermal_zone_device *thermal,
 	if (ret)
 		return ret;
 
-	pcb_temp = 0;
-	/* TODO: Introduce pcb temperature lookup */
+	ret = thermal_zone_lookup_temperature("pcb", &pcb_temp);
 	/* In case pcb zone is available, use the extrapolation rule with it */
-	if (pcb_temp) {
+	if (!ret) {
 		tmp -= pcb_temp;
 		slope = s->slope_pcb;
 		constant = s->constant_pcb;
-- 
1.7.7.1.488.ge8e1c

Re: [PATCH 1/2] thermal: introduce thermal_zone_lookup_temperature helper function

[Zhang Rui]

On Fri, 2013-03-22 at 17:13 -0400, Eduardo Valentin wrote:
> This patch adds a helper function to get temperature of
> a thermal zone, based on the zone type name.
> 
> It will perform a zone name lookup and return the last
> sensor temperature reading. In case the zone is not found
> or if the required parameters are invalid, it will return
> the corresponding error code.
> 
> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> ---
>  drivers/thermal/thermal_sys.c |   32 ++++++++++++++++++++++++++++++++
>  include/linux/thermal.h       |    1 +
>  2 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 5bd95d4..f0caa13 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -1790,6 +1790,38 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
>  
> +/**
> + * thermal_lookup_temperature - search for a zone and returns its temperature
> + * @name: thermal zone name to fetch the temperature
> + * @temperature: pointer to store the zone temperature, in case it is found
> + *
> + * When the zone is found, updates @temperature and returns 0.
> + *
> + * Return: -EINVAL in case of wrong parameters, -ENODEV in case the zone
> + * is not found and 0 when it is successfully found.
> + */
> +int thermal_zone_lookup_temperature(const char *name, int *temperature)
> +{
> +	struct thermal_zone_device *pos = NULL;
> +	bool found = false;
> +
> +	if (!name || !temperature)
> +		return -EINVAL;
> +
> +	mutex_lock(&thermal_list_lock);
> +	list_for_each_entry(pos, &thermal_tz_list, node)
> +		if (!strcmp(pos->type, name)) {
> +			found = true;
> +			break;
> +		}
> +	if (found)
> +		*temperature = pos->last_temperature;
> +	mutex_unlock(&thermal_list_lock);
> +
> +	return found ? 0 : -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_lookup_temperature);
> +
please do not use thermal zone type as the parameter because unique
thermal zone type string is not a hard rule.
If this is really needed, I'd prefer two APIs instead
1. struct thermal_zone_device * thermal_zone_get_zone_by_name(char
*name);
2. int thermal_zone_get_temperature(struct thermal_zone_device *, int
*temperature);

And in thermal_zone_get_zone_by_name(), you should parse all the thermal
zone list and return an error code instead if multiple zones are found.

thanks,
rui
>  #ifdef CONFIG_NET
>  static struct genl_family thermal_event_genl_family = {
>  	.id = GENL_ID_GENERATE,
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 542a39c..2b2f902 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -237,6 +237,7 @@ void thermal_zone_device_update(struct thermal_zone_device *);
>  struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
>  		const struct thermal_cooling_device_ops *);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
> +int thermal_zone_lookup_temperature(const char *name, int *temperature);
>  
>  int thermal_zone_trend_get(struct thermal_zone_device *, int);
>  struct thermal_instance *thermal_instance_get(struct thermal_zone_device *,

RE: [PATCH 1/2] thermal: introduce thermal_zone_lookup_temperature helper function

[R, Durgadoss]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3169 bytes --]

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Zhang Rui
> Sent: Monday, March 25, 2013 11:41 AM
> To: Eduardo Valentin
> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] thermal: introduce
> thermal_zone_lookup_temperature helper function
> 
> On Fri, 2013-03-22 at 17:13 -0400, Eduardo Valentin wrote:
> > This patch adds a helper function to get temperature of
> > a thermal zone, based on the zone type name.
> >
> > It will perform a zone name lookup and return the last
> > sensor temperature reading. In case the zone is not found
> > or if the required parameters are invalid, it will return
> > the corresponding error code.
> >
> > Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> > ---
> >  drivers/thermal/thermal_sys.c |   32
> ++++++++++++++++++++++++++++++++
> >  include/linux/thermal.h       |    1 +
> >  2 files changed, 33 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 5bd95d4..f0caa13 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -1790,6 +1790,38 @@ void thermal_zone_device_unregister(struct
> thermal_zone_device *tz)
> >  }
> >  EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
> >
> > +/**
> > + * thermal_lookup_temperature - search for a zone and returns its
> temperature
> > + * @name: thermal zone name to fetch the temperature
> > + * @temperature: pointer to store the zone temperature, in case it is
> found
> > + *
> > + * When the zone is found, updates @temperature and returns 0.
> > + *
> > + * Return: -EINVAL in case of wrong parameters, -ENODEV in case the
> zone
> > + * is not found and 0 when it is successfully found.
> > + */
> > +int thermal_zone_lookup_temperature(const char *name, int
> *temperature)
> > +{
> > +	struct thermal_zone_device *pos = NULL;
> > +	bool found = false;
> > +
> > +	if (!name || !temperature)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&thermal_list_lock);
> > +	list_for_each_entry(pos, &thermal_tz_list, node)
> > +		if (!strcmp(pos->type, name)) {
> > +			found = true;
> > +			break;
> > +		}
> > +	if (found)
> > +		*temperature = pos->last_temperature;
> > +	mutex_unlock(&thermal_list_lock);
> > +
> > +	return found ? 0 : -ENODEV;
> > +}
> > +EXPORT_SYMBOL_GPL(thermal_zone_lookup_temperature);
> > +
> please do not use thermal zone type as the parameter because unique
> thermal zone type string is not a hard rule.

Okay, agree with this. This is what I am implementing in
my changes as well.

> If this is really needed, I'd prefer two APIs instead
> 1. struct thermal_zone_device * thermal_zone_get_zone_by_name(char
> *name);
> 2. int thermal_zone_get_temperature(struct thermal_zone_device *, int
> *temperature);

Why do we need this second API?
If the driver has a 'tz' pointer, it can use tz->ops->get_temp
to retrieve the temperature, right ?

Thanks,
Durga
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 1/2] thermal: introduce thermal_zone_lookup_temperature helper function

[R, Durgadoss]

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Zhang Rui
> Sent: Monday, March 25, 2013 11:41 AM
> To: Eduardo Valentin
> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] thermal: introduce
> thermal_zone_lookup_temperature helper function
> 
> On Fri, 2013-03-22 at 17:13 -0400, Eduardo Valentin wrote:
> > This patch adds a helper function to get temperature of
> > a thermal zone, based on the zone type name.
> >
> > It will perform a zone name lookup and return the last
> > sensor temperature reading. In case the zone is not found
> > or if the required parameters are invalid, it will return
> > the corresponding error code.
> >
> > Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> > ---
> >  drivers/thermal/thermal_sys.c |   32
> ++++++++++++++++++++++++++++++++
> >  include/linux/thermal.h       |    1 +
> >  2 files changed, 33 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 5bd95d4..f0caa13 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -1790,6 +1790,38 @@ void thermal_zone_device_unregister(struct
> thermal_zone_device *tz)
> >  }
> >  EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
> >
> > +/**
> > + * thermal_lookup_temperature - search for a zone and returns its
> temperature
> > + * @name: thermal zone name to fetch the temperature
> > + * @temperature: pointer to store the zone temperature, in case it is
> found
> > + *
> > + * When the zone is found, updates @temperature and returns 0.
> > + *
> > + * Return: -EINVAL in case of wrong parameters, -ENODEV in case the
> zone
> > + * is not found and 0 when it is successfully found.
> > + */
> > +int thermal_zone_lookup_temperature(const char *name, int
> *temperature)
> > +{
> > +	struct thermal_zone_device *pos = NULL;
> > +	bool found = false;
> > +
> > +	if (!name || !temperature)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&thermal_list_lock);
> > +	list_for_each_entry(pos, &thermal_tz_list, node)
> > +		if (!strcmp(pos->type, name)) {
> > +			found = true;
> > +			break;
> > +		}
> > +	if (found)
> > +		*temperature = pos->last_temperature;
> > +	mutex_unlock(&thermal_list_lock);
> > +
> > +	return found ? 0 : -ENODEV;
> > +}
> > +EXPORT_SYMBOL_GPL(thermal_zone_lookup_temperature);
> > +
> please do not use thermal zone type as the parameter because unique
> thermal zone type string is not a hard rule.

Okay, agree with this. This is what I am implementing in
my changes as well.

> If this is really needed, I'd prefer two APIs instead
> 1. struct thermal_zone_device * thermal_zone_get_zone_by_name(char
> *name);
> 2. int thermal_zone_get_temperature(struct thermal_zone_device *, int
> *temperature);

Why do we need this second API?
If the driver has a 'tz' pointer, it can use tz->ops->get_temp
to retrieve the temperature, right ?

Thanks,
Durga

Re: [PATCH 2/2] staging: ti-soc-thermal: remove external heat while extrapolating hotspot

[Zhang Rui]

On Fri, 2013-03-22 at 17:13 -0400, Eduardo Valentin wrote:
> For boards that provide a PCB sensor close to SoC junction
> temperature, it is possible to remove the cumulative heat
> reported by the SoC temperature sensor.
> 
> This patch changes the extrapolation computation to consider
> an external sensor in the extrapolation equations.
> 
> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> ---
>  drivers/staging/ti-soc-thermal/ti-thermal-common.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/ti-soc-thermal/ti-thermal-common.c b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
> index 231c549..52d3c1b 100644
> --- a/drivers/staging/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
> @@ -92,10 +92,9 @@ static inline int ti_thermal_get_temp(struct thermal_zone_device *thermal,
>  	if (ret)
>  		return ret;
>  
> -	pcb_temp = 0;
> -	/* TODO: Introduce pcb temperature lookup */
> +	ret = thermal_zone_lookup_temperature("pcb", &pcb_temp);
>  	/* In case pcb zone is available, use the extrapolation rule with it */
> -	if (pcb_temp) {
> +	if (!ret) {
>  		tmp -= pcb_temp;
>  		slope = s->slope_pcb;
>  		constant = s->constant_pcb;

I can not see this piece of code.
But I assume that the thermal_zone_device is registered in another
driver, right?
or else you can use the thermal_zone_device pointer directly instead.

thanks,
rui

RE: [PATCH 1/2] thermal: introduce thermal_zone_lookup_temperature helper function

[Zhang Rui]

On Mon, 2013-03-25 at 00:20 -0600, R, Durgadoss wrote:
> > -----Original Message-----
> > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > owner@vger.kernel.org] On Behalf Of Zhang Rui
> > Sent: Monday, March 25, 2013 11:41 AM
> > To: Eduardo Valentin
> > Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 1/2] thermal: introduce
> > thermal_zone_lookup_temperature helper function
> > 
> > On Fri, 2013-03-22 at 17:13 -0400, Eduardo Valentin wrote:
> > > This patch adds a helper function to get temperature of
> > > a thermal zone, based on the zone type name.
> > >
> > > It will perform a zone name lookup and return the last
> > > sensor temperature reading. In case the zone is not found
> > > or if the required parameters are invalid, it will return
> > > the corresponding error code.
> > >
> > > Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> > > ---
> > >  drivers/thermal/thermal_sys.c |   32
> > ++++++++++++++++++++++++++++++++
> > >  include/linux/thermal.h       |    1 +
> > >  2 files changed, 33 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > > index 5bd95d4..f0caa13 100644
> > > --- a/drivers/thermal/thermal_sys.c
> > > +++ b/drivers/thermal/thermal_sys.c
> > > @@ -1790,6 +1790,38 @@ void thermal_zone_device_unregister(struct
> > thermal_zone_device *tz)
> > >  }
> > >  EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
> > >
> > > +/**
> > > + * thermal_lookup_temperature - search for a zone and returns its
> > temperature
> > > + * @name: thermal zone name to fetch the temperature
> > > + * @temperature: pointer to store the zone temperature, in case it is
> > found
> > > + *
> > > + * When the zone is found, updates @temperature and returns 0.
> > > + *
> > > + * Return: -EINVAL in case of wrong parameters, -ENODEV in case the
> > zone
> > > + * is not found and 0 when it is successfully found.
> > > + */
> > > +int thermal_zone_lookup_temperature(const char *name, int
> > *temperature)
> > > +{
> > > +	struct thermal_zone_device *pos = NULL;
> > > +	bool found = false;
> > > +
> > > +	if (!name || !temperature)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&thermal_list_lock);
> > > +	list_for_each_entry(pos, &thermal_tz_list, node)
> > > +		if (!strcmp(pos->type, name)) {
> > > +			found = true;
> > > +			break;
> > > +		}
> > > +	if (found)
> > > +		*temperature = pos->last_temperature;
> > > +	mutex_unlock(&thermal_list_lock);
> > > +
> > > +	return found ? 0 : -ENODEV;
> > > +}
> > > +EXPORT_SYMBOL_GPL(thermal_zone_lookup_temperature);
> > > +
> > please do not use thermal zone type as the parameter because unique
> > thermal zone type string is not a hard rule.
> 
> Okay, agree with this. This is what I am implementing in
> my changes as well.
> 
> > If this is really needed, I'd prefer two APIs instead
> > 1. struct thermal_zone_device * thermal_zone_get_zone_by_name(char
> > *name);
> > 2. int thermal_zone_get_temperature(struct thermal_zone_device *, int
> > *temperature);
> 
> Why do we need this second API?
> If the driver has a 'tz' pointer, it can use tz->ops->get_temp
> to retrieve the temperature, right ?
> 

probably it is because one driver want to get the temperature of a
sensor registered by another driver.

thanks,
rui

Re: [PATCH 2/2] staging: ti-soc-thermal: remove external heat while extrapolating hotspot

[Eduardo Valentin]

On 25-03-2013 02:23, Zhang Rui wrote:
> On Fri, 2013-03-22 at 17:13 -0400, Eduardo Valentin wrote:
>> For boards that provide a PCB sensor close to SoC junction
>> temperature, it is possible to remove the cumulative heat
>> reported by the SoC temperature sensor.
>>
>> This patch changes the extrapolation computation to consider
>> an external sensor in the extrapolation equations.
>>
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
>> ---
>>   drivers/staging/ti-soc-thermal/ti-thermal-common.c |    5 ++---
>>   1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/ti-soc-thermal/ti-thermal-common.c b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
>> index 231c549..52d3c1b 100644
>> --- a/drivers/staging/ti-soc-thermal/ti-thermal-common.c
>> +++ b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
>> @@ -92,10 +92,9 @@ static inline int ti_thermal_get_temp(struct thermal_zone_device *thermal,
>>   	if (ret)
>>   		return ret;
>>
>> -	pcb_temp = 0;
>> -	/* TODO: Introduce pcb temperature lookup */
>> +	ret = thermal_zone_lookup_temperature("pcb", &pcb_temp);
>>   	/* In case pcb zone is available, use the extrapolation rule with it */
>> -	if (pcb_temp) {
>> +	if (!ret) {
>>   		tmp -= pcb_temp;
>>   		slope = s->slope_pcb;
>>   		constant = s->constant_pcb;
>
> I can not see this piece of code.
> But I assume that the thermal_zone_device is registered in another
> driver, right?

It is because I made this patch based on staging-next. But you will find 
the same under drivers/stating/omap-thermal/omap-bandgap.c


Yes. The sensor managed by another driver. And the temperature lookup is 
assumed to be done by the fw.

> or else you can use the thermal_zone_device pointer directly instead.
>

I'd not recommend this because of the above statements

> thanks,
> rui
>
>
>

Re: [PATCH 2/2] staging: ti-soc-thermal: remove external heat while extrapolating hotspot

[Eduardo Valentin]

On 25-03-2013 02:23, Zhang Rui wrote:
> On Fri, 2013-03-22 at 17:13 -0400, Eduardo Valentin wrote:
>> For boards that provide a PCB sensor close to SoC junction
>> temperature, it is possible to remove the cumulative heat
>> reported by the SoC temperature sensor.
>>
>> This patch changes the extrapolation computation to consider
>> an external sensor in the extrapolation equations.
>>
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
>> ---
>>   drivers/staging/ti-soc-thermal/ti-thermal-common.c |    5 ++---
>>   1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/ti-soc-thermal/ti-thermal-common.c b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
>> index 231c549..52d3c1b 100644
>> --- a/drivers/staging/ti-soc-thermal/ti-thermal-common.c
>> +++ b/drivers/staging/ti-soc-thermal/ti-thermal-common.c
>> @@ -92,10 +92,9 @@ static inline int ti_thermal_get_temp(struct thermal_zone_device *thermal,
>>   	if (ret)
>>   		return ret;
>>
>> -	pcb_temp = 0;
>> -	/* TODO: Introduce pcb temperature lookup */
>> +	ret = thermal_zone_lookup_temperature("pcb", &pcb_temp);
>>   	/* In case pcb zone is available, use the extrapolation rule with it */
>> -	if (pcb_temp) {
>> +	if (!ret) {
>>   		tmp -= pcb_temp;
>>   		slope = s->slope_pcb;
>>   		constant = s->constant_pcb;
>
> I can not see this piece of code.
> But I assume that the thermal_zone_device is registered in another
> driver, right?

It is because I made this patch based on staging-next. But you will find 
the same under drivers/stating/omap-thermal/omap-bandgap.c


Yes. The sensor managed by another driver. And the temperature lookup is 
assumed to be done by the fw.

> or else you can use the thermal_zone_device pointer directly instead.
>

I'd not recommend this because of the above statements

> thanks,
> rui
>
>
>

Re: [PATCH 1/2] thermal: introduce thermal_zone_lookup_temperature helper function

[Eduardo Valentin]

On 25-03-2013 02:26, Zhang Rui wrote:
> On Mon, 2013-03-25 at 00:20 -0600, R, Durgadoss wrote:
>>> -----Original Message-----
>>> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
>>> owner@vger.kernel.org] On Behalf Of Zhang Rui
>>> Sent: Monday, March 25, 2013 11:41 AM
>>> To: Eduardo Valentin
>>> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH 1/2] thermal: introduce
>>> thermal_zone_lookup_temperature helper function
>>>
>>> On Fri, 2013-03-22 at 17:13 -0400, Eduardo Valentin wrote:
>>>> This patch adds a helper function to get temperature of
>>>> a thermal zone, based on the zone type name.
>>>>
>>>> It will perform a zone name lookup and return the last
>>>> sensor temperature reading. In case the zone is not found
>>>> or if the required parameters are invalid, it will return
>>>> the corresponding error code.
>>>>
>>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
>>>> ---
>>>>   drivers/thermal/thermal_sys.c |   32
>>> ++++++++++++++++++++++++++++++++
>>>>   include/linux/thermal.h       |    1 +
>>>>   2 files changed, 33 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>>>> index 5bd95d4..f0caa13 100644
>>>> --- a/drivers/thermal/thermal_sys.c
>>>> +++ b/drivers/thermal/thermal_sys.c
>>>> @@ -1790,6 +1790,38 @@ void thermal_zone_device_unregister(struct
>>> thermal_zone_device *tz)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
>>>>
>>>> +/**
>>>> + * thermal_lookup_temperature - search for a zone and returns its
>>> temperature
>>>> + * @name: thermal zone name to fetch the temperature
>>>> + * @temperature: pointer to store the zone temperature, in case it is
>>> found
>>>> + *
>>>> + * When the zone is found, updates @temperature and returns 0.
>>>> + *
>>>> + * Return: -EINVAL in case of wrong parameters, -ENODEV in case the
>>> zone
>>>> + * is not found and 0 when it is successfully found.
>>>> + */
>>>> +int thermal_zone_lookup_temperature(const char *name, int
>>> *temperature)
>>>> +{
>>>> +	struct thermal_zone_device *pos = NULL;
>>>> +	bool found = false;
>>>> +
>>>> +	if (!name || !temperature)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&thermal_list_lock);
>>>> +	list_for_each_entry(pos, &thermal_tz_list, node)
>>>> +		if (!strcmp(pos->type, name)) {
>>>> +			found = true;
>>>> +			break;
>>>> +		}
>>>> +	if (found)
>>>> +		*temperature = pos->last_temperature;
>>>> +	mutex_unlock(&thermal_list_lock);
>>>> +
>>>> +	return found ? 0 : -ENODEV;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(thermal_zone_lookup_temperature);
>>>> +
>>> please do not use thermal zone type as the parameter because unique
>>> thermal zone type string is not a hard rule.

OK. I didn't consider this, as I was just resending the patch and the FW 
has evolve since last time I sent it.

>>
>> Okay, agree with this. This is what I am implementing in
>> my changes as well.
>>
>>> If this is really needed, I'd prefer two APIs instead
>>> 1. struct thermal_zone_device * thermal_zone_get_zone_by_name(char
>>> *name);
>>> 2. int thermal_zone_get_temperature(struct thermal_zone_device *, int
>>> *temperature);
>>
>> Why do we need this second API?
>> If the driver has a 'tz' pointer, it can use tz->ops->get_temp
>> to retrieve the temperature, right ?
>>
>
> probably it is because one driver want to get the temperature of a
> sensor registered by another driver.

Unless API 1. can be used on other cases, why do we need two APIs? Why 
not keep the same signature I proposed, but with the same care on 'type' 
you mentioned for 1.?

>
> thanks,
> rui
>
>
>