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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 21 Jun 2022 02:48:46 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> 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?

Whilst I'd prefer it earlier, if it's a real pain, just put a note on
that in the cover letter and I'll cope :)

> 
> >>  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,

That looks sensible to me.
> 
> 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'd assume there won't be too many different versions that need separate
support and have a chipinfo for each of those versions.  So you
select the chipinfo based on both the device part number and version.

>
Thanks,

J