[PATCH v1 0/3] thermal: tsens: bugfixes, cleanups

[PATCH v1 0/3] thermal: tsens: bugfixes, cleanups

[Amit Kucheria]

Here is another series of tsens-related bugfixes and cleanups to prepare
for IRQ support, among other things. It applies on top of another series[1]
that did some code consolidation to add support for sdm845.

[1] https://lore.kernel.org/lkml/cover.1531895128.git.amit.kucheria@linaro.org/T/#t


Amit Kucheria (3):
  thermal: tsens: Rename variable
  thermal: tsens: switch from of_iomap() to devm_ioremap_resource()
  thermal: tsens: Fix negative temperature reporting

 drivers/thermal/qcom/tsens-common.c | 19 ++++++++++---------
 drivers/thermal/qcom/tsens-v2.c     | 23 ++++++++++-------------
 2 files changed, 20 insertions(+), 22 deletions(-)

-- 
2.7.4

[PATCH v1 1/3] thermal: tsens: Rename variable

[Amit Kucheria]

We're actually reading the temperature from the status register. Fix the
variable name to reflect that.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-common.c |  6 +++---
 drivers/thermal/qcom/tsens-v2.c     | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index c22dc18..25e7f24 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -104,11 +104,11 @@ int get_temp_common(struct tsens_device *tmdev, int id, int *temp)
 {
 	struct tsens_sensor *s = &tmdev->sensor[id];
 	u32 code;
-	unsigned int sensor_addr;
+	unsigned int status_reg;
 	int last_temp = 0, ret;
 
-	sensor_addr = S0_ST_ADDR + s->hw_id * SN_ADDR_OFFSET;
-	ret = regmap_read(tmdev->map, sensor_addr, &code);
+	status_reg = S0_ST_ADDR + s->hw_id * SN_ADDR_OFFSET;
+	ret = regmap_read(tmdev->map, status_reg, &code);
 	if (ret)
 		return ret;
 	last_temp = code & SN_ST_TEMP_MASK;
diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index f40150f..908e3dc 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -16,11 +16,11 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
 {
 	struct tsens_sensor *s = &tmdev->sensor[id];
 	u32 code;
-	unsigned int sensor_addr;
+	unsigned int status_reg;
 	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
 
-	sensor_addr = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
-	ret = regmap_read(tmdev->map, sensor_addr, &code);
+	status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
+	ret = regmap_read(tmdev->map, status_reg, &code);
 	if (ret)
 		return ret;
 	last_temp = code & LAST_TEMP_MASK;
@@ -28,7 +28,7 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
 		goto done;
 
 	/* Try a second time */
-	ret = regmap_read(tmdev->map, sensor_addr, &code);
+	ret = regmap_read(tmdev->map, status_reg, &code);
 	if (ret)
 		return ret;
 	if (code & STATUS_VALID_BIT) {
@@ -39,7 +39,7 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
 	}
 
 	/* Try a third/last time */
-	ret = regmap_read(tmdev->map, sensor_addr, &code);
+	ret = regmap_read(tmdev->map, status_reg, &code);
 	if (ret)
 		return ret;
 	if (code & STATUS_VALID_BIT) {
-- 
2.7.4

[PATCH v1 2/3] thermal: tsens: switch from of_iomap() to devm_ioremap_resource()

[Amit Kucheria]

devm_ioremap_resources() automatically requests resources (so that the I/O
region shows up in /proc/iomem) and devm_ wrappers do better error handling
and unmapping of the I/O region when needed.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-common.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index 25e7f24..6207d8d 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -127,13 +127,11 @@ static const struct regmap_config tsens_config = {
 int __init init_common(struct tsens_device *tmdev)
 {
 	void __iomem *base;
+	struct resource *res;
 	struct platform_device *op = of_find_device_by_node(tmdev->dev->of_node);
 
 	if (!op)
 		return -EINVAL;
-	base = of_iomap(tmdev->dev->of_node, 0);
-	if (!base)
-		return -EINVAL;
 
 	/* The driver only uses the TM register address space for now */
 	if (op->num_resources > 1) {
@@ -143,11 +141,14 @@ int __init init_common(struct tsens_device *tmdev)
 		tmdev->tm_offset = 0x1000;
 	}
 
+	res = platform_get_resource(op, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&op->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
 	tmdev->map = devm_regmap_init_mmio(tmdev->dev, base, &tsens_config);
-	if (IS_ERR(tmdev->map)) {
-		iounmap(base);
+	if (IS_ERR(tmdev->map))
 		return PTR_ERR(tmdev->map);
-	}
 
 	return 0;
 }
-- 
2.7.4

[PATCH v1 3/3] thermal: tsens: Fix negative temperature reporting

[Amit Kucheria]

The current code will always return 0xffffffff in case of negative
temperatures due to a bug in how the binary sign extension is being done.

Use sign_extend32() instead.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-v2.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index 908e3dc..46abc21 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -5,19 +5,20 @@
  */
 
 #include <linux/regmap.h>
+#include <linux/bitops.h>
 #include "tsens.h"
 
 #define STATUS_OFFSET		0xa0
 #define LAST_TEMP_MASK		0xfff
 #define STATUS_VALID_BIT	BIT(21)
-#define CODE_SIGN_BIT		BIT(11)
 
 static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
 {
 	struct tsens_sensor *s = &tmdev->sensor[id];
 	u32 code;
 	unsigned int status_reg;
-	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
+	u32 last_temp = 0, last_temp2 = 0, last_temp3 = 0;
+	int ret;
 
 	status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
 	ret = regmap_read(tmdev->map, status_reg, &code);
@@ -54,12 +55,8 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
 	else if (last_temp2 == last_temp3)
 		last_temp = last_temp3;
 done:
-	/* Code sign bit is the sign extension for a negative value */
-	if (last_temp & CODE_SIGN_BIT)
-		last_temp |= ~CODE_SIGN_BIT;
-
-	/* Temperatures are in deciCelicius */
-	*temp = last_temp * 100;
+	/* Convert temperatures to milliCelicius */
+	*temp = sign_extend32(last_temp, fls(LAST_TEMP_MASK) - 1) * 100;
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v1 1/3] thermal: tsens: Rename variable

[Matthias Kaehlcke]

On Wed, Jul 18, 2018 at 12:55:01PM +0530, Amit Kucheria wrote:
> We're actually reading the temperature from the status register. Fix the
> variable name to reflect that.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/qcom/tsens-common.c |  6 +++---
>  drivers/thermal/qcom/tsens-v2.c     | 10 +++++-----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index c22dc18..25e7f24 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -104,11 +104,11 @@ int get_temp_common(struct tsens_device *tmdev, int id, int *temp)
>  {
>  	struct tsens_sensor *s = &tmdev->sensor[id];
>  	u32 code;
> -	unsigned int sensor_addr;
> +	unsigned int status_reg;
>  	int last_temp = 0, ret;
>  
> -	sensor_addr = S0_ST_ADDR + s->hw_id * SN_ADDR_OFFSET;
> -	ret = regmap_read(tmdev->map, sensor_addr, &code);
> +	status_reg = S0_ST_ADDR + s->hw_id * SN_ADDR_OFFSET;
> +	ret = regmap_read(tmdev->map, status_reg, &code);
>  	if (ret)
>  		return ret;
>  	last_temp = code & SN_ST_TEMP_MASK;
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index f40150f..908e3dc 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -16,11 +16,11 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>  {
>  	struct tsens_sensor *s = &tmdev->sensor[id];
>  	u32 code;
> -	unsigned int sensor_addr;
> +	unsigned int status_reg;
>  	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
>  
> -	sensor_addr = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
> -	ret = regmap_read(tmdev->map, sensor_addr, &code);
> +	status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
> +	ret = regmap_read(tmdev->map, status_reg, &code);
>  	if (ret)
>  		return ret;
>  	last_temp = code & LAST_TEMP_MASK;
> @@ -28,7 +28,7 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>  		goto done;
>  
>  	/* Try a second time */
> -	ret = regmap_read(tmdev->map, sensor_addr, &code);
> +	ret = regmap_read(tmdev->map, status_reg, &code);
>  	if (ret)
>  		return ret;
>  	if (code & STATUS_VALID_BIT) {
> @@ -39,7 +39,7 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>  	}
>  
>  	/* Try a third/last time */
> -	ret = regmap_read(tmdev->map, sensor_addr, &code);
> +	ret = regmap_read(tmdev->map, status_reg, &code);
>  	if (ret)
>  		return ret;
>  	if (code & STATUS_VALID_BIT) {

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v1 2/3] thermal: tsens: switch from of_iomap() to devm_ioremap_resource()

[Matthias Kaehlcke]

On Wed, Jul 18, 2018 at 12:55:02PM +0530, Amit Kucheria wrote:
> devm_ioremap_resources() automatically requests resources (so that the I/O
> region shows up in /proc/iomem) and devm_ wrappers do better error handling
> and unmapping of the I/O region when needed.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/qcom/tsens-common.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 25e7f24..6207d8d 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -127,13 +127,11 @@ static const struct regmap_config tsens_config = {
>  int __init init_common(struct tsens_device *tmdev)
>  {
>  	void __iomem *base;
> +	struct resource *res;
>  	struct platform_device *op = of_find_device_by_node(tmdev->dev->of_node);
>  
>  	if (!op)
>  		return -EINVAL;
> -	base = of_iomap(tmdev->dev->of_node, 0);
> -	if (!base)
> -		return -EINVAL;
>  
>  	/* The driver only uses the TM register address space for now */
>  	if (op->num_resources > 1) {
> @@ -143,11 +141,14 @@ int __init init_common(struct tsens_device *tmdev)
>  		tmdev->tm_offset = 0x1000;
>  	}
>  
> +	res = platform_get_resource(op, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&op->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
>  	tmdev->map = devm_regmap_init_mmio(tmdev->dev, base, &tsens_config);
> -	if (IS_ERR(tmdev->map)) {
> -		iounmap(base);
> +	if (IS_ERR(tmdev->map))
>  		return PTR_ERR(tmdev->map);
> -	}
>  
>  	return 0;
>  }

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v1 3/3] thermal: tsens: Fix negative temperature reporting

[Matthias Kaehlcke]

On Wed, Jul 18, 2018 at 12:55:03PM +0530, Amit Kucheria wrote:
> The current code will always return 0xffffffff in case of negative
> temperatures due to a bug in how the binary sign extension is being done.
> 
> Use sign_extend32() instead.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/qcom/tsens-v2.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 908e3dc..46abc21 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -5,19 +5,20 @@
>   */
>  
>  #include <linux/regmap.h>
> +#include <linux/bitops.h>
>  #include "tsens.h"
>  
>  #define STATUS_OFFSET		0xa0
>  #define LAST_TEMP_MASK		0xfff
>  #define STATUS_VALID_BIT	BIT(21)
> -#define CODE_SIGN_BIT		BIT(11)
>  
>  static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>  {
>  	struct tsens_sensor *s = &tmdev->sensor[id];
>  	u32 code;
>  	unsigned int status_reg;
> -	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
> +	u32 last_temp = 0, last_temp2 = 0, last_temp3 = 0;
> +	int ret;
>  
>  	status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
>  	ret = regmap_read(tmdev->map, status_reg, &code);
> @@ -54,12 +55,8 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>  	else if (last_temp2 == last_temp3)
>  		last_temp = last_temp3;
>  done:
> -	/* Code sign bit is the sign extension for a negative value */
> -	if (last_temp & CODE_SIGN_BIT)
> -		last_temp |= ~CODE_SIGN_BIT;
> -
> -	/* Temperatures are in deciCelicius */
> -	*temp = last_temp * 100;
> +	/* Convert temperatures to milliCelicius */

nits:

s/temperatures/temperature/
s/milliCelicius/milliCelsius/

> +	*temp = sign_extend32(last_temp, fls(LAST_TEMP_MASK) - 1) * 100;
>  
>  	return 0;
>  }

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v1 3/3] thermal: tsens: Fix negative temperature reporting

[Amit Kucheria]

On Thu, Jul 26, 2018 at 5:19 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
> On Wed, Jul 18, 2018 at 12:55:03PM +0530, Amit Kucheria wrote:
>> The current code will always return 0xffffffff in case of negative
>> temperatures due to a bug in how the binary sign extension is being done.
>>
>> Use sign_extend32() instead.
>>
>> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
>> ---
>>  drivers/thermal/qcom/tsens-v2.c | 13 +++++--------
>>  1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
>> index 908e3dc..46abc21 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -5,19 +5,20 @@
>>   */
>>
>>  #include <linux/regmap.h>
>> +#include <linux/bitops.h>
>>  #include "tsens.h"
>>
>>  #define STATUS_OFFSET                0xa0
>>  #define LAST_TEMP_MASK               0xfff
>>  #define STATUS_VALID_BIT     BIT(21)
>> -#define CODE_SIGN_BIT                BIT(11)
>>
>>  static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>>  {
>>       struct tsens_sensor *s = &tmdev->sensor[id];
>>       u32 code;
>>       unsigned int status_reg;
>> -     int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
>> +     u32 last_temp = 0, last_temp2 = 0, last_temp3 = 0;
>> +     int ret;
>>
>>       status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
>>       ret = regmap_read(tmdev->map, status_reg, &code);
>> @@ -54,12 +55,8 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>>       else if (last_temp2 == last_temp3)
>>               last_temp = last_temp3;
>>  done:
>> -     /* Code sign bit is the sign extension for a negative value */
>> -     if (last_temp & CODE_SIGN_BIT)
>> -             last_temp |= ~CODE_SIGN_BIT;
>> -
>> -     /* Temperatures are in deciCelicius */
>> -     *temp = last_temp * 100;
>> +     /* Convert temperatures to milliCelicius */
>
> nits:
>
> s/temperatures/temperature/
> s/milliCelicius/milliCelsius/

Embarrassing to have two typos in a single sentence. :-)

>> +     *temp = sign_extend32(last_temp, fls(LAST_TEMP_MASK) - 1) * 100;
>>
>>       return 0;
>>  }
>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Thanks for the review.