Hi Jonathan, On 18.06.22 16:53, Jonathan Cameron wrote: > > On Sat, 18 Jun 2022 02:13:12 +0200 > Jakob Hauser <jahau@xxxxxxxxxxxxxx> 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