diff mbox series

[v10,5/8] drivers: thermal: tsens: Fix bug in sensor enable for msm8960

Message ID 20210217194011.22649-6-ansuelsmth@gmail.com
State New
Headers show
Series [v10,1/8] drivers: thermal: tsens: Add VER_0 tsens version | expand

Commit Message

Christian Marangi Feb. 17, 2021, 7:40 p.m. UTC
It's present a hardware bug in tsens VER_0 where if sensors upper to id
6 are enabled selectively, underfined results are expected. Fix this by
enabling all the remaining sensor in one step.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/thermal/qcom/tsens-8960.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comments

Thara Gopinath March 18, 2021, 9:15 p.m. UTC | #1
On 2/17/21 2:40 PM, Ansuel Smith wrote:
> It's present a hardware bug in tsens VER_0 where if sensors upper to id

> 6 are enabled selectively, underfined results are expected. Fix this by

> enabling all the remaining sensor in one step.



It took me a while to understand this. It is most likely me! But please 
consider rewording.

> 

> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> ---

>   drivers/thermal/qcom/tsens-8960.c | 19 +++++++++++++++++--

>   1 file changed, 17 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c

> index 86585f439985..248aaa65b5b0 100644

> --- a/drivers/thermal/qcom/tsens-8960.c

> +++ b/drivers/thermal/qcom/tsens-8960.c

> @@ -27,9 +27,9 @@

>   #define EN			BIT(0)

>   #define SW_RST			BIT(1)

>   #define SENSOR0_EN		BIT(3)

> +#define MEASURE_PERIOD		BIT(18)

>   #define SLP_CLK_ENA		BIT(26)

>   #define SLP_CLK_ENA_8660	BIT(24)

> -#define MEASURE_PERIOD		1

>   #define SENSOR0_SHIFT		3

>   

>   /* INT_STATUS_ADDR bitmasks */

> @@ -132,11 +132,26 @@ static int enable_8960(struct tsens_priv *priv, int id)

>   	if (ret)

>   		return ret;

>   

> -	mask = BIT(id + SENSOR0_SHIFT);

> +	/* HARDWARE BUG:

> +	 * On platform with more than 5 sensors, all the remaining


Isn't it 6 ? At least according to code below it is.. You are checking 
for id > 5.

> +	 * sensors needs to be enabled all togheder or underfined

> +	 * results are expected. (Sensor 6-7 disabled, Sensor 3

> +	 * disabled...) In the original driver, all the sensors

> +	 * are enabled in one step hence this bug is not triggered.


Also with this change, you should add a check in this function to see if 
the sensors are already enabled and if yes return back. The enabling 
call from tsens.c happens for every single sensor. But at sensor number 
6 you are enabling rest of the sensors. There is absolutely no reason to 
keep doing this for rest of the sensors.

> +	 */

> +	if (id > 5)

> +		mask = GENMASK(10, 6);

> +	else

> +		mask = BIT(id);

> +

> +	mask <<= SENSOR0_SHIFT;

> +

>   	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg | SW_RST);


I know this is not part of this patch. But you mention above that 
earlier you were enabling all sensors one shot. Now that this is being 
done one at a time, is it needed to do a SW_RST every time ?

>   	if (ret)

>   		return ret;

>   

> +	reg |= MEASURE_PERIOD;

> +

>   	if (priv->num_sensors > 1)

>   		reg |= mask | SLP_CLK_ENA | EN;

>   	else

> 


-- 
Warm Regards
Thara
diff mbox series

Patch

diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
index 86585f439985..248aaa65b5b0 100644
--- a/drivers/thermal/qcom/tsens-8960.c
+++ b/drivers/thermal/qcom/tsens-8960.c
@@ -27,9 +27,9 @@ 
 #define EN			BIT(0)
 #define SW_RST			BIT(1)
 #define SENSOR0_EN		BIT(3)
+#define MEASURE_PERIOD		BIT(18)
 #define SLP_CLK_ENA		BIT(26)
 #define SLP_CLK_ENA_8660	BIT(24)
-#define MEASURE_PERIOD		1
 #define SENSOR0_SHIFT		3
 
 /* INT_STATUS_ADDR bitmasks */
@@ -132,11 +132,26 @@  static int enable_8960(struct tsens_priv *priv, int id)
 	if (ret)
 		return ret;
 
-	mask = BIT(id + SENSOR0_SHIFT);
+	/* HARDWARE BUG:
+	 * On platform with more than 5 sensors, all the remaining
+	 * sensors needs to be enabled all togheder or underfined
+	 * results are expected. (Sensor 6-7 disabled, Sensor 3
+	 * disabled...) In the original driver, all the sensors
+	 * are enabled in one step hence this bug is not triggered.
+	 */
+	if (id > 5)
+		mask = GENMASK(10, 6);
+	else
+		mask = BIT(id);
+
+	mask <<= SENSOR0_SHIFT;
+
 	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg | SW_RST);
 	if (ret)
 		return ret;
 
+	reg |= MEASURE_PERIOD;
+
 	if (priv->num_sensors > 1)
 		reg |= mask | SLP_CLK_ENA | EN;
 	else