| Message ID | 20200807092014.967262-1-cmo@melexis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: temperature: mlx90632: Add extended calibration calculations | expand |
On Fri, Aug 7, 2020 at 12:21 PM Crt Mori <cmo@melexis.com> 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.
On Fri, 7 Aug 2020 at 12:29, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Aug 7, 2020 at 12:21 PM Crt Mori <cmo@melexis.com> wrote: > > Oh yeah, you are right, there will be some comments :-) > Told ya. No matter how many times I go through it, I always find something. I will prepare v3 with fixes, except for some additional questions below. > > 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. > Can you explain a bit more? I was looking in kernel doc, but could not find much about how to comment these members. > > + 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. > Should indeed be &&, if it is needed at all. Both are boolean types. > > + 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. I cant, sensor does not support it and single read case did not work few years back, but maybe regmap now improved... > 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? > That we should break the loop once channels up to 19 are filled (we read 17 18 and 19 in this case, we read 1 2 in normal case). A comment maybe here? > > + 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
On Fri, 7 Aug 2020 at 13:13, Crt Mori <cmo@melexis.com> wrote: > > On Fri, 7 Aug 2020 at 12:29, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Aug 7, 2020 at 12:21 PM Crt Mori <cmo@melexis.com> wrote: > > > > Oh yeah, you are right, there will be some comments :-) > > > > Told ya. No matter how many times I go through it, I always find > something. I will prepare v3 with fixes, except for some additional > questions below. > I tried some suggestions and it was just not working. See the explanation below. I am resending v3 without those. > > > 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. > > > > Can you explain a bit more? I was looking in kernel doc, but could not > find much about how to comment these members. > > > > + 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. > > > > Should indeed be &&, if it is needed at all. Both are boolean types. > > > > + 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. > > I cant, sensor does not support it and single read case did not work > few years back, but maybe regmap now improved... > > > 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? > > > > That we should break the loop once channels up to 19 are filled (we > read 17 18 and 19 in this case, we read 1 2 in normal case). A comment > maybe here? > > > > + 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. > > I tried to apply the iopoll.h, but it is not appropriate enough as timeout_us would have to be timeout_ms, because if you want 4 cycles of 10ms, then you run out of range of usleep. I can create a helper, but it does not seem like someone needs it. I am keeping current style, because also function above (old) has the same style of poll loop. > > > + 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
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c index 763a148a0095..bb35a65bb9f0 100644 --- a/drivers/iio/temperature/mlx90632.c +++ b/drivers/iio/temperature/mlx90632.c @@ -58,6 +58,8 @@ /* Control register address - volatile */ #define MLX90632_REG_CONTROL 0x3001 /* Control Register address */ #define MLX90632_CFG_PWR_MASK GENMASK(2, 1) /* PowerMode Mask */ +#define MLX90632_CFG_MTYP_MASK GENMASK(8, 4) /* Meas select Mask */ + /* PowerModes statuses */ #define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1) #define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */ @@ -65,6 +67,18 @@ #define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */ #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/ +/* Measurement types */ +#define MLX90632_MTYP_MEDICAL 0 +#define MLX90632_MTYP_EXTENDED 17 + +/* Measurement type select*/ +#define MLX90632_MTYP_STATUS(ctrl_val) (ctrl_val << 4) +#define MLX90632_MTYP_STATUS_MEDICAL MLX90632_MTYP_STATUS(MLX90632_MTYP_MEDICAL) +#define MLX90632_MTYP_STATUS_EXTENDED MLX90632_MTYP_STATUS(MLX90632_MTYP_EXTENDED) + +/* I2C command register - volatile */ +#define MLX90632_REG_I2C_CMD 0x3005 /* I2C command Register address */ + /* Device status register - volatile */ #define MLX90632_REG_STATUS 0x3fff /* Device status register */ #define MLX90632_STAT_BUSY BIT(10) /* Device busy indicator */ @@ -81,6 +95,8 @@ /* Magic constants */ #define MLX90632_ID_MEDICAL 0x0105 /* EEPROM DSPv5 Medical device id */ #define MLX90632_ID_CONSUMER 0x0205 /* EEPROM DSPv5 Consumer device id */ +#define MLX90632_ID_EXTENDED 0x0505 /* EEPROM DSPv5 Extended range device id */ +#define MLX90632_ID_MASK GENMASK(14, 0) /* DSP version and device ID in EE_VERSION */ #define MLX90632_DSP_VERSION 5 /* DSP version */ #define MLX90632_DSP_MASK GENMASK(7, 0) /* DSP version in EE_VERSION */ #define MLX90632_RESET_CMD 0x0006 /* Reset sensor (address or global) */ @@ -88,16 +104,20 @@ #define MLX90632_REF_3 12LL /**< ResCtrlRef value of Channel 3 */ #define MLX90632_MAX_MEAS_NUM 31 /**< Maximum measurements in list */ #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */ +#define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */ 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; }; static const struct regmap_range mlx90632_volatile_reg_range[] = { regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL), + regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD), regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS), regmap_reg_range(MLX90632_RAM_1(0), MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)), @@ -113,6 +133,7 @@ static const struct regmap_range mlx90632_read_reg_range[] = { regmap_reg_range(MLX90632_EE_CTRL, MLX90632_EE_I2C_ADDR), regmap_reg_range(MLX90632_EE_Ha, MLX90632_EE_Hb), regmap_reg_range(MLX90632_REG_I2C_ADDR, MLX90632_REG_CONTROL), + regmap_reg_range(MLX90632_REG_I2C_CMD, MLX90632_REG_I2C_CMD), regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS), regmap_reg_range(MLX90632_RAM_1(0), MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)), @@ -199,6 +220,28 @@ static int mlx90632_perform_measurement(struct mlx90632_data *data) return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2; } +static int mlx90632_set_meas_type(struct regmap *regmap, u8 type) +{ + int ret; + + if ((type != MLX90632_MTYP_MEDICAL) & (type != MLX90632_MTYP_EXTENDED)) + return -EINVAL; + + 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; +} + static int mlx90632_channel_new_select(int perform_ret, uint8_t *channel_new, uint8_t *channel_old) { @@ -300,6 +343,106 @@ static int mlx90632_read_all_channel(struct mlx90632_data *data, return ret; } +static int mlx90632_read_ambient_raw_extended(struct regmap *regmap, + s16 *ambient_new_raw, s16 *ambient_old_raw) +{ + int ret; + unsigned int read_tmp; + + 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; +} + +static int mlx90632_read_object_raw_extended(struct regmap *regmap, s16 *object_new_raw) +{ + int ret; + unsigned int read_tmp; + s32 read; + + 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; + + 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; + + 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) + return -EINVAL; + + *object_new_raw = (int16_t)read; + + 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) + break; + } + if (tries < 0) { + ret = -ETIMEDOUT; + goto read_unlock; + } + + 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 int mlx90632_read_ee_register(struct regmap *regmap, u16 reg_lsb, s32 *reg_value) { @@ -354,9 +497,23 @@ static s64 mlx90632_preprocess_temp_obj(s16 object_new_raw, s16 object_old_raw, return div64_s64((tmp << 19ULL), 1000LL); } +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); +} + static s32 mlx90632_calc_temp_ambient(s16 ambient_new_raw, s16 ambient_old_raw, - s32 P_T, s32 P_R, s32 P_G, s32 P_O, - s16 Gb) + s32 P_T, s32 P_R, s32 P_G, s32 P_O, s16 Gb) { s64 Asub, Bsub, Ablock, Bblock, Cblock, AMB, sum; @@ -423,6 +580,37 @@ static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb, return temp; } +static s32 mlx90632_calc_temp_object_extended(s64 object, s64 ambient, s64 reflected, + s32 Ea, s32 Eb, s32 Fa, s32 Fb, s32 Ga, + s16 Ha, s16 Hb, u16 tmp_emi) +{ + s64 kTA, kTA0, TAdut, TAdut4, Tr4, TaTr4; + s64 temp = 25000; + s8 i; + + kTA = (Ea * 1000LL) >> 16LL; + kTA0 = (Eb * 1000LL) >> 8LL; + 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); + TaTr4 = Tr4 - div64_long(Tr4 - TAdut4, tmp_emi) * 1000; + + /* Iterations of calculation as described in datasheet */ + for (i = 0; i < 5; ++i) { + temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, TaTr4, + Fa / 2, Fb, Ga, Ha, Hb, + tmp_emi); + } + + return temp; +} + static int mlx90632_calc_object_dsp105(struct mlx90632_data *data, int *val) { s32 ret; @@ -470,6 +658,26 @@ static int mlx90632_calc_object_dsp105(struct mlx90632_data *data, int *val) if (ret < 0) return ret; + if (object_new_raw > MLX90632_EXTENDED_LIMIT && + data->mtyp == MLX90632_MTYP_EXTENDED) { + ret = mlx90632_read_all_channel_extended(data, &object_new_raw, + &ambient_new_raw, &ambient_old_raw); + if (ret < 0) + return ret; + + /* Use extended mode calculations */ + ambient = mlx90632_preprocess_temp_amb(ambient_new_raw, + ambient_old_raw, Gb); + object = mlx90632_preprocess_temp_obj_extended(object_new_raw, + ambient_new_raw, + ambient_old_raw, Ka); + *val = mlx90632_calc_temp_object_extended(object, ambient, + data->object_ambient_temperature, + Ea, Eb, Fa, Fb, Ga, + Ha, Hb, data->emissivity); + return 0; + } + ambient = mlx90632_preprocess_temp_amb(ambient_new_raw, ambient_old_raw, Gb); object = mlx90632_preprocess_temp_obj(object_new_raw, @@ -643,6 +851,7 @@ static int mlx90632_probe(struct i2c_client *client, i2c_set_clientdata(client, indio_dev); mlx90632->client = client; mlx90632->regmap = regmap; + mlx90632->mtyp = MLX90632_MTYP_MEDICAL; mutex_init(&mlx90632->lock); indio_dev->name = id->name; @@ -662,12 +871,17 @@ static int mlx90632_probe(struct i2c_client *client, dev_err(&client->dev, "read of version failed: %d\n", ret); return ret; } + read = read & MLX90632_ID_MASK; if (read == MLX90632_ID_MEDICAL) { dev_dbg(&client->dev, "Detected Medical EEPROM calibration %x\n", read); } else if (read == MLX90632_ID_CONSUMER) { dev_dbg(&client->dev, "Detected Consumer EEPROM calibration %x\n", read); + } else if (read == MLX90632_ID_EXTENDED) { + dev_dbg(&client->dev, + "Detected Extended range EEPROM calibration %x\n", read); + mlx90632->mtyp = MLX90632_MTYP_EXTENDED; } else if ((read & MLX90632_DSP_MASK) == MLX90632_DSP_VERSION) { dev_dbg(&client->dev, "Detected Unknown EEPROM calibration %x\n", read); @@ -679,6 +893,7 @@ static int mlx90632_probe(struct i2c_client *client, } mlx90632->emissivity = 1000; + mlx90632->object_ambient_temperature = 25000; /* 25 degrees Celsius */ pm_runtime_disable(&client->dev); ret = pm_runtime_set_active(&client->dev);
For some time 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 error which can be eliminated. Currently this temperature is fixed at 25, but interface to adjust it by user (with external sensor or just IR measurement of the another object which acts as ambient), will be introduced in another commit. Signed-off-by: Crt Mori <cmo@melexis.com> --- drivers/iio/temperature/mlx90632.c | 219 ++++++++++++++++++++++++++++- 1 file changed, 217 insertions(+), 2 deletions(-)