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/[email protected]/T/#t
Changes since v1:
- Fixup a couple of typos
- Add review tag
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.17.GIT
We're actually reading the temperature from the status register. Fix the
variable name to reflect that.
Signed-off-by: Amit Kucheria <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
---
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 c22dc18c7c65..25e7f247b027 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 f40150fd7eae..908e3dcb2d5c 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.17.GIT
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 <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
---
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 25e7f247b027..6207d8d92351 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.17.GIT
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 <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
---
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 908e3dcb2d5c..44da02f594ac 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 temperature from deciCelsius to milliCelsius */
+ *temp = sign_extend32(last_temp, fls(LAST_TEMP_MASK) - 1) * 100;
return 0;
}
--
2.17.GIT
On Thu 26 Jul 03:33 PDT 2018, 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 <[email protected]>
> Reviewed-by: Matthias Kaehlcke <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
> 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 c22dc18c7c65..25e7f247b027 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 f40150fd7eae..908e3dcb2d5c 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.17.GIT
>
On Thu 26 Jul 03:33 PDT 2018, 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 <[email protected]>
> Reviewed-by: Matthias Kaehlcke <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
> 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 908e3dcb2d5c..44da02f594ac 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 temperature from deciCelsius to milliCelsius */
> + *temp = sign_extend32(last_temp, fls(LAST_TEMP_MASK) - 1) * 100;
>
> return 0;
> }
> --
> 2.17.GIT
>
On Thu 26 Jul 03:33 PDT 2018, 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 <[email protected]>
> Reviewed-by: Matthias Kaehlcke <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
> 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 25e7f247b027..6207d8d92351 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.17.GIT
>
Hey Amit,
On Thu, Jul 26, 2018 at 04:03:07PM +0530, Amit Kucheria wrote:
> 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/[email protected]/T/#t
Given it is already rc7 time, even if this is fixes series, I will only consider this for the merge window.
>
> Changes since v1:
> - Fixup a couple of typos
> - Add review tag
>
> 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.17.GIT
>
--
All the best,
Eduardo Valentin
On Fri, Jul 27, 2018 at 2:38 AM Eduardo Valentin <[email protected]> wrote:
>
> Hey Amit,
>
>
> On Thu, Jul 26, 2018 at 04:03:07PM +0530, Amit Kucheria wrote:
> > 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/[email protected]/T/#t
>
>
> Given it is already rc7 time, even if this is fixes series, I will only consider this for the merge window.
Sure, I wasn't expecting to land this series in 4.19. I'm hoping that
[1] can be landed though.
Regards,
Amit