Re: [PATCH RFC 1/2] thermal: qcom: tsens-v1: Add support for MSM8994 TSENS

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 09/06/2021 15:31, Konrad Dybcio wrote:
> Hi,
> 
> 
>> Please split binding and code into two separate patches.
> 
> It's a oneliner, but I might as well.
> 
>  
> 
>> That deserves a cartdrige with a good explanation of why this function
>> is doing all this. Without enough details, it is hard to review the code.
> 
> I don't really know *why* it's doing all of this. Qualcomm doesn't share any documentation.
> 
> It' just based on the freely-available msm-3.10 kernel driver. Probably just a HW specific.

Oh, ok. Let's assume we are in hacking mode then. Hopefully Bjorn can
give some inputs.

>>> +static void compute_intercept_slope_8994(struct tsens_priv *priv,
>>> +			      u32 *base0, u32 *base1, u32 *p, u32 mode)
>>> +{
>>> +	int adc_code_of_tempx, i, num, den;
>>> +
>>> +	for (i = 0; i < priv->num_sensors; i++) {
>>> +		dev_dbg(priv->dev,
>>> +			"%s: sensor%d - data_point1:%#x data_point2:%#x\n",
>>> +			__func__, i, base0[i], base1[i]);
>>> +
>>> +		priv->sensor[i].slope = SLOPE_DEFAULT;
>>> +		if (mode == TWO_PT_CALIB) {
>>> +			/*
>>> +			 * slope (m) = adc_code2 - adc_code1 (y2 - y1)/
>>> +			 *	temp_120_degc - temp_30_degc (x2 - x1)
>>> +			 */
>>> +			num = base1[i] - base0[i];
>> As the caller of the function is copying the value of base[0] to the
>> entire array, whatever 'i', base[i] == base[0], so the parameters can be
>> replaced by a single int.
>>
>> Then the code becomes:
>>
>> 	num = base1 - base0;
>> 	num *= SLOPE_FACTOR;
>> 	den = CAL_DEGC_PT2 - CAL_DEGC_PT1;
>> 	slope = num / den;
>>
>> There is no change in the values, so 'slope' can be precomputed before
>> the loop. We end up with:
>>
>> 	int adc_code_of_tempx, i, num, den;
>> 	int slope;
>>
>> 	/*
>> 	 * slope (m) = adc_code2 - adc_code1 (y2 - y1)/
>> 	 *	temp_120_degc - temp_30_degc (x2 - x1)
>> 	 */
>> 	num = base1 - base0;
>> 	num *= SLOPE_FACTOR;
>> 	den = CAL_DEGC_PT2 - CAL_DEGC_PT1;
>> 	slope = num / den;
>>
>> 	for (i = 0; i < priv->num_sensors; i++) {
>>
>> 		priv->sensor[i].slope = mode == TWO_PT_CALIB ? slope :
>> 			SLOPE_DEFAULT;
> 
> That's sounds very good. I did not think of this approach, but I will incorporate it
> 
> into the next revision.
> 
> 
> 
>>> +		adc_code_of_tempx = base0[i] + p[i];
>>> +
>>> +		priv->sensor[i].offset = (adc_code_of_tempx * SLOPE_FACTOR) -
>>> +				(CAL_DEGC_PT1 *	priv->sensor[i].slope);
>>> +		dev_dbg(priv->dev, "%s: offset:%d\n", __func__,
>>> +			priv->sensor[i].offset);
>>> +	}
>>> +}
>>> +
>>>  static int calibrate_v1(struct tsens_priv *priv)
>>>  {
>>>  	u32 base0 = 0, base1 = 0;
>>> @@ -297,14 +421,143 @@ static int calibrate_8976(struct tsens_priv *priv)
>>>  	return 0;
>>>  }
>> Same comment as above. The more the details, the easier for the people
>> to review the code.
> 
> Sorry, I am not sure what you're referring to, the calibrate_8976 function?

I was referring to explaining a bit more the code but a comment saying
it is a verbatim translation of the msm downstream driver should be ok.

>>> -/* v1.x: msm8956,8976,qcs404,405 */
>>> +static int calibrate_8994(struct tsens_priv *priv)
>>> +{
>>> +	int base0[16] = { 0 }, base1[16] = { 0 }, i;
>>> +	u32 p[16];
>> p stands for ?
> 
> No idea, but judging by the line:
> 
> " adc_code_of_tempx = base0[i] + p[i]; "
> 
> it's probably some hw-specific offset value.
> 
> 
> 
>>> +	int mode = 0;
>>> +	u32 *calib0, *calib1, *calib2, *calib_mode, *calib_rsel;
>>> +	u32 calib_redun_sel;
>>> +
>>> +	/* 0x40d0-0x40dc */
>>> +	calib0 = (u32 *)qfprom_read(priv->dev, "calib");
>> Fix qfprom_read, by returning an int and using nvmem_cell_read_u32
>> (separate series).
>>
>> It seems like all call sites are expecting an int.
> 
> Weird. I did not get slope calculation issues even with this, but perhaps
> 
> I was just lucky.
> 
> 
> 
>>> +			p[9] = (calib2[0] & MSM8994_S9_REDUN_MASK) >> MSM8994_S9_REDUN_SHIFT;
>>> +			p[10] = (calib2[0] & MSM8994_S10_REDUN_MASK) >> MSM8994_S10_REDUN_SHIFT;
>>> +			p[11] = (calib2[0] & MSM8994_S11_REDUN_MASK) >> MSM8994_S11_REDUN_SHIFT;
>>> +			p[12] = (calib2[0] & MSM8994_S12_REDUN_MASK) >> MSM8994_S12_REDUN_SHIFT;
>>> +			p[13] = (calib2[0] & MSM8994_S13_REDUN_MASK) >> MSM8994_S13_REDUN_SHIFT;
>>> +			p[14] = (calib2[0] & MSM8994_S14_REDUN_MASK) >> MSM8994_S14_REDUN_SHIFT;
>>> +			p[15] = (calib2[0] & MSM8994_S15_REDUN_MASK) >> MSM8994_S15_REDUN_SHIFT;
>> IMO, it is possible to do something simpler (probably bits.h could have
>> interesting helpers).
> 
> All TSENS consumers had this style, probably to make it easier to compare with the
> 
> downstream driver should there arise any human errors.
> 
> 
> 
>>> +		} else {
>>> +			dev_dbg(priv->dev, "%s: REDUN NON-TWO_PT mode, mode = %i",
>>> +			__func__, mode);
>>> +			for (i = 0; i < 16; i++)
>>> +				p[i] = 532;
>> No litterals, macros please
> 
> Does MSM8994_NON_TWOPT_DEFAULT_VALUE sound good? It doesn't exactly
> 
> roll of the tongue but I don't have many better ideas..

Is this driver msm8994 specific ?


>> And it would be simpler to iniatialize the array with the value.
>>
>> u32 p[16] = { [ 0 ... 15 ] = MY_532_MACRO };
> 
>> So no need to use this loop and the other one beliw.
> 
> Thanks, didn't know about this.
> 
> 
> 
>> What about replacing 16 by TSENS_SENSOR_MAX ?
> 
> If you mean this 8994-specific function exactly, then it'd probably cause
> 
> more confusion than help as we might find out that some SoC using TSENSv1
> 
> has even more sensors.
> 
> 
> 
>>>  static struct tsens_features tsens_v1_feat = {
>>>  	.ver_major	= VER_1_X,
>>>  	.crit_int	= 0,
>>>  	.adc		= 1,
>>>  	.srot_split	= 1,
>>> -	.max_sensors	= 11,
>>> +	.max_sensors	= 16,
> 
> Here TSENS_SENSOR_MAX does make sense.
> 
> 
> 
>>> +
>>> +struct tsens_plat_data data_8994 = {
>>> +	.num_sensors	= 16,
>>> +	.ops		= &ops_8994,
>>> +	.hw_ids		= (unsigned int []){ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 },
>> If you have time, in another series, replace this by a single int used
>> as a bitmask and fix the hw_id loop in tsens.c.
> 
> I will add this to my to-do list, but no promises on this landing anytime soon :/
> 
> 
> 
> Thanks for the thorough review,
> 
> Konrad
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog