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

[Thara Gopinath]

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