On Sat, Aug 8, 2020 at 2:24 AM Crt Mori <cmo@xxxxxxxxxxx> wrote: > > For some time the market wants medical grade accuracy in medical range, > while still retaining the declared accuracy outside of the medical range > within the same sensor. That is why we created extended calibration > which is automatically switched to when object temperature is too high. > > This patch also introduces the object_ambient_temperature variable which > is needed for more accurate calculation of the object infra-red > footprint as sensor's ambient temperature might be totally different > than what the ambient temperature is at object and that is why we can > have some more errors which can be eliminated. Currently this temperature > is fixed at 25, but the interface to adjust it by user (with external > sensor or just IR measurement of the other object which acts as ambient), > will be introduced in another commit. Thank you for an update! My comments below. ... > struct mlx90632_data { > struct i2c_client *client; > struct mutex lock; /* Multiple reads for single measurement */ > struct regmap *regmap; > u16 emissivity; > + u8 mtyp; /* measurement type - to enable extended range calculations */ > + u32 object_ambient_temperature; > }; As a separate patch to have a kernel doc for this, there are plenty of examples in the kernel, but I will show below an example /** * struct foo - private data for the MLX90632 device * @client: I2C client of the device * @bar: ... ... */ struct foo { struct i2c_client *client; ... bar; ... }; ... > + if ((type != MLX90632_MTYP_MEDICAL) && (type != MLX90632_MTYP_EXTENDED)) > + return -EINVAL; So, just to point out what the difference between & and && (even for boolean): the former one forces both sides of operand to be executed, while && checks for only first in case of false. ... > +static int mlx90632_read_ambient_raw_extended(struct regmap *regmap, > + s16 *ambient_new_raw, s16 *ambient_old_raw) > +{ > + unsigned int read_tmp; > + int ret; > + > + ret = regmap_read(regmap, MLX90632_RAM_3(17), &read_tmp); > + if (ret < 0) > + return ret; > + *ambient_new_raw = (s16)read_tmp; Again the same question, do you need all these castings? > + ret = regmap_read(regmap, MLX90632_RAM_3(18), &read_tmp); These 17, 18 and 19 should be defined with descriptive names. > + if (ret < 0) > + return ret; > + *ambient_old_raw = (s16)read_tmp; > + > + return 0; > +} ... > + int tries = 4; > + while (tries-- > 0) { > + ret = mlx90632_perform_measurement(data); > + if (ret < 0) > + goto read_unlock; > + > + if (ret == 19) > + break; > + } > + if (tries < 0) { > + ret = -ETIMEDOUT; > + goto read_unlock; > + } Again, please use unsigned int tries = 4; // or should be 5? do { ... } while (ret != ..._ LAST_CHANNEL && --tries); if (!tries) ... It will really make the code easier to read. ... > +static s64 mlx90632_preprocess_temp_obj_extended(s16 object_new_raw, s16 ambient_new_raw, > + s16 ambient_old_raw, s16 Ka) > +{ > + s64 VR_IR, kKa, tmp; > + > + kKa = ((s64)Ka * 1000LL) >> 10ULL; > + VR_IR = (s64)ambient_old_raw * 1000000LL + > + kKa * div64_s64((s64)ambient_new_raw * 1000LL, > + MLX90632_REF_3); > + tmp = div64_s64( > + div64_s64((s64) object_new_raw * 1000000000000LL, MLX90632_REF_12), > + VR_IR); > + return div64_s64(tmp << 19ULL, 1000LL); > +} It would be nice to have a comment on this voodoo arithmetics. -- With Best Regards, Andy Shevchenko