On Fri, Aug 7, 2020 at 12:21 PM Crt Mori <cmo@xxxxxxxxxxx> wrote:
Oh yeah, you are right, there will be some comments :-)
> For some time market wants medical grade accuracy in medical range,
the market
> 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 error which can be eliminated. Currently this temperature
errors
> is fixed at 25, but interface to adjust it by user (with external sensor
the interface
> or just IR measurement of the another object which acts as ambient),
'of another' or 'the other' if we know what it is exactly.
> will be introduced in another commit.
...
> 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 */
Perhaps better to switch this struct to follow kernel doc in one of
preparatory patches and add the description of this field accordingly.
> + u32 object_ambient_temperature;
> };
...
> +static int mlx90632_set_meas_type(struct regmap *regmap, u8 type)
> +{
> + int ret;
> +
> + if ((type != MLX90632_MTYP_MEDICAL) & (type != MLX90632_MTYP_EXTENDED))
> + return -EINVAL;
Not sure I understand the point of & vs. && here.
> + ret = regmap_write(regmap, MLX90632_REG_I2C_CMD, MLX90632_RESET_CMD);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_write_bits(regmap, MLX90632_REG_CONTROL,
> + (MLX90632_CFG_MTYP_MASK | MLX90632_CFG_PWR_MASK),
> + (MLX90632_MTYP_STATUS(type) | MLX90632_PWR_STATUS_HALT));
> + if (ret < 0)
> + return ret;
> +
> + mlx90632_pwr_continuous(regmap);
> +
> + return ret;
Since you are using ' < 0' above and below (and I think it doesn't
worth it, i.o.w. you may drop them) here is something interesting
might be returned (actually not, see first part of this sentence).
Should be
return 0;
> +}
...
> +static int mlx90632_read_ambient_raw_extended(struct regmap *regmap,
> + s16 *ambient_new_raw, s16 *ambient_old_raw)
> +{
> + int ret;
> + unsigned int read_tmp;
Please keep them in reversed xmas tree format.
> +
> + ret = regmap_read(regmap, MLX90632_RAM_3(17), &read_tmp);
> + if (ret < 0)
> + return ret;
> + *ambient_new_raw = (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_3(18), &read_tmp);
> + if (ret < 0)
> + return ret;
> + *ambient_old_raw = (s16)read_tmp;
> + return ret;
Same comments as per previous function.
> +}
> +static int mlx90632_read_object_raw_extended(struct regmap *regmap, s16 *object_new_raw)
> +{
> + int ret;
> + unsigned int read_tmp;
> + s32 read;
Besides all above comments being applicable here...
> + ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = read - (s16)read_tmp;
...I'm wondering if you can use bulk reads of those registers.
Also I'm not sure you need explicit castings.
> + ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = read - (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = (read + (s16)read_tmp) / 2;
Ditto.
> + ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = read + (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = read + (s16)read_tmp;
> + if (read > 32767 || read < -32768)
These are defined as S16_MIN and S16_MAX. Use limits.h.
> + return -EINVAL;
-ERANGE
> + *object_new_raw = (int16_t)read;
Oh, no. Please avoid user space types in the kernel. And what's the
point anyway after checking the range?
> + return ret;
> +}
...
> +static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *object_new_raw,
> + s16 *ambient_new_raw, s16 *ambient_old_raw)
> +{
> + s32 ret;
> + int tries = 4;
> +
> + mutex_lock(&data->lock);
> + ret = mlx90632_set_meas_type(data->regmap, MLX90632_MTYP_EXTENDED);
> + if (ret < 0)
> + goto read_unlock;
> + while (tries-- > 0) {
> + ret = mlx90632_perform_measurement(data);
> + if (ret < 0)
> + goto read_unlock;
> +
> + if (ret == 19)
It's funny. What does this magic mean?
> + break;
> + }
> + if (tries < 0) {
> + ret = -ETIMEDOUT;
> + goto read_unlock;
> + }
Timeout loops are much better in a following style
unsigned int iterations = 4;
do {
...
} while (--iterations);
if (!iterations) {
...-ETIMEDOUT...
}
Besides that consider the iopoll.h APIs, perhaps it may be applied here.
> + ret = mlx90632_read_object_raw_extended(data->regmap, object_new_raw);
> + if (ret < 0)
> + goto read_unlock;
> +
> + ret = mlx90632_read_ambient_raw_extended(data->regmap, ambient_new_raw, ambient_old_raw);
> +
> +read_unlock:
> + (void) mlx90632_set_meas_type(data->regmap, MLX90632_MTYP_MEDICAL);
> +
> + mutex_unlock(&data->lock);
> + return ret;
> +}
...
> +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));
And the point of using parentheses? It's not a Lisp language :-)
(Applicable everywhere in your code, the rule of thumb that any
particular comment given by reviewer should be considered against
entire code where it's appropriate)
> + tmp = div64_s64(
> + div64_s64((((s64)object_new_raw) * 1000000000000LL), MLX90632_REF_12),
> + VR_IR);
> + return div64_s64((tmp << 19ULL), 1000LL);
> +}
...
> + TAdut = div64_s64(((ambient - kTA0) * 1000000LL), kTA) + 25 * 1000000LL;
> + Tr4 = (div64_long(reflected, 10) + 27315) *
> + (div64_long(reflected, 10) + 27315) *
> + (div64_long(reflected, 10) + 27315) *
> + (div64_long(reflected, 10) + 27315);
> + TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) *
> + (div64_s64(TAdut, 10000LL) + 27315) *
> + (div64_s64(TAdut, 10000LL) + 27315) *
> + (div64_s64(TAdut, 10000LL) + 27315);
Okay, looking at this I definitely think that this patch should be
split into a few smaller logically separated pieces like introducing
some helpers to calculate above with them.
...
> + mlx90632->object_ambient_temperature = 25000; /* 25 degrees Celsius */
Comment is lying. milliCelsius.
--
With Best Regards,
Andy Shevchenko
