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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxx>
---
  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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux