Re: [PATCH v3 4/8] iio: magnetometer: yas530: Correct temperature handling

[Jakob Hauser <jahau@rocketmail.com>]

Hi Jonathan,

On 18.06.22 16:53, Jonathan Cameron wrote:
>
> On Sat, 18 Jun 2022 02:13:12 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:
...
>>  /* These variant IDs are known from code dumps */
>>  #define YAS537_DEVICE_ID		0x07 /* YAS537 (MS-3T) */
>> @@ -314,7 +315,7 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
> 
> Hmm. I'm not a great fun of big hydra functions to handle differences
> between devices.  This could easily all be one code flow with some
> lookups into chip specific constant data (as btw could a lot of
> the other switch statements in the existing driver).

I'll try to implement the chip_info approach. This should become a
separate patch.

Concerning the patchset, I would prefer to introduce the chip_info
approach rather late. That would mean to leave this patch unchanged and
introduce your suggestions later within the patchset. I think it's
easier to follow the changes along the patchset.

However, you probably would prefer to place the chip_info patch rather
early in the patchset?

>>  static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
>>  {
>>  	struct yas5xx_calibration *c = &yas5xx->calibration;
>> -	u16 t, x, y1, y2;
>> +	u16 t_ref, t, x, y1, y2;
>>  	/* These are "signed x, signed y1 etc */
>>  	s32 sx, sy1, sy2, sy, sz;
>>  	int ret;
>> @@ -329,16 +330,46 @@ static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
>>  	sy1 = yas5xx_linearize(yas5xx, y1, 1);
>>  	sy2 = yas5xx_linearize(yas5xx, y2, 2);
>>  
>> -	/*
>> -	 * Temperature compensation for x, y1, y2 respectively:
>> -	 *
>> -	 *          Cx * t
>> -	 * x' = x - ------
>> -	 *           100
>> -	 */
>> -	sx = sx - (c->Cx * t) / 100;
>> -	sy1 = sy1 - (c->Cy1 * t) / 100;
>> -	sy2 = sy2 - (c->Cy2 * t) / 100;
>> +	/* Set the temperature reference value (unit: counts) */
>> +	switch (yas5xx->devid) {
>> +	case YAS530_DEVICE_ID:
>> +		t_ref = YAS530_20DEGREES;
> 
> One thought to simplify the divergent flow below.
> 
> 		t_ref2 = 0;
>> +		break;
>> +	case YAS532_DEVICE_ID:
>> +		t_ref = YAS532_20DEGREES;
> 		if (yas5xx->version == YAS532_VERSION_AC)
> 			t_ref2 = YAS432_20DEGREES;
> 		else
> 			t_ref2 = 0;

The t_ref2 approach looks confusing to me. Because for the most version
it's "t_ref2 = 0", only one version out of four needs this.

Another approach: I would rather introduce t_comp (for compensation). In
the chip_info, for the most version it would be...

        .t_comp = t,

... and for the one variant it would be:

        .t_comp = t - t_ref,

A problem: I would include the YAS variants like YAS530, YAS532 etc. in
the chip_info. The versions like "AB" and "AC", on the other hand, I
wouldn't include into the chip_info, instead I would handle these in the
functions. In that case the, "t_comp" thing would need to be done in the
function using an if statement, similar to what you suggested up here.
I'll have a closer look when setting up patchset v4.

> Possibly with moving some of the comments below up here.
> As mentioned below, this stuff would be even better if
> in a chip type specific const structure rather than as code.
> That is have one switch statement in probe that picks from
> an array of 
> 
> struct yas5xx_chip_info {
> 	/* COMMENTS ON WHAT these are.. *
> 	u16 tref;
> 	u16 tref2;
> 	int ref_temp_celsius;
> 	int min_temp_celsuis;
> };
> static const struct yas5xx_chip_info[] = {
> 	[ENUM value we will use to pick right on in probe] = {
> 		...
> 
> etc

Thanks for writing down what it's supposed to look, that's helpful to
compare with other examples.

...

>> @@ -347,11 +378,37 @@ static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
>>  	sy = sy1 - sy2;
>>  	sz = -sy1 - sy2;
>>  
>> -	/*
>> -	 * FIXME: convert to Celsius? Just guessing this is given
>> -	 * as 1/10:s of degrees so multiply by 100 to get millicentigrades.
>> -	 */
>> -	*to = t * 100;
>> +	/* Process temperature readout */
>> +	switch (yas5xx->devid) {
>> +	case YAS530_DEVICE_ID:
>> +		/*
>> +		 * Raw temperature value t is the number of counts starting
>> +		 * at -62 °C. Reference value t_ref is the number of counts
>> +		 * between -62 °C and 20 °C (82 °C range).
> Roll this info into the maths and only have the constants set in the switch
> statement.  Even better if you just move them into chip specific data so
> look them up directly rather than via a switch of devid.  The whole driver
> would benefit from moving this stuff to const data rather than switch
> statements all over the place.
> 
> 	int min_temp_x10 = yas5xx->chip_info.min_temp_x10;
> 	const int ref_temp_x10 = 200;
> 
> 	*to = (min_temp_x10 + ((ref_temp_x10 - min_temp_x10) * t / t_ref)) * 100;
> 
> That should make the code self explanatory and remove need for the comments.

I'll have a look and will try to implement this.

...

Kind regards,
Jakob