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
