Re: [PATCH v3 2/2] iio:temperature:mlx90632: Adding extended calibration option

[Andy Shevchenko]

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