[PATCH v4 0/4] iio: temperature: mlx90632: Add extended calibration calculations

[PATCH v4 0/4] iio: temperature: mlx90632: Add extended calibration calculations

[Crt Mori]

Since the second patch is dependent on the first and was still not
merged, I have decided to send them together. First patch just makes
second one more readable as it splits out the repeated calculation and
that enables the second patch to tweak the variable to the new
condition.

V4 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
	 - Move the function creation for Ta4 to first patch
	 - Add kernel doc patch for documenting internal struct
	 - Add patch to convert while loops to do-while loops for
	   polling

V3 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
	 - Change commit message text to more proper English as per suggestions
	 - Drop unneeded brackets and parentheses
	 - Use defines from limits.h
	 - Remove userspace typedefs as leftovers from porting
	 - Testing of timeout loops with iopoll.h was no successful,
	   because delay between measurements is 10ms, but we need to
	   fill at least 3 channels, so final timeout should be 40ms
	   which is out of scope of usleep function
	 - Fixing some typos in comments

V2 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
	 - Convert divison back to shifts to make it more readable

Crt Mori (4):
  iio:temperature:mlx90632: Reduce number of equal calulcations
  iio:temperature:mlx90632: Adding extended calibration option
  iio:temperature:mlx90632: Add kerneldoc to the internal struct
  iio:temperature:mlx90632: Convert polling while loops to do-while

 drivers/iio/temperature/mlx90632.c | 251 +++++++++++++++++++++++++++--
 1 file changed, 236 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH v4 1/4] iio:temperature:mlx90632: Reduce number of equal calulcations

[Crt Mori]

TAdut4 was calculated each iteration although it did not change. In light
of near future additions of the Extended range DSP calculations, this
function refactoring will help reduce unrelated changes in that series as
well as reduce the number of new functions needed.

Signed-off-by: Crt Mori <cmo@melexis.com>
---
 drivers/iio/temperature/mlx90632.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 51b812bcff2e..c3de10ba5b1e 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -374,11 +374,11 @@ static s32 mlx90632_calc_temp_ambient(s16 ambient_new_raw, s16 ambient_old_raw,
 }
 
 static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 object,
-					       s64 TAdut, s32 Fa, s32 Fb,
+					       s64 TAdut, s64 TAdut4, s32 Fa, s32 Fb,
 					       s32 Ga, s16 Ha, s16 Hb,
 					       u16 emissivity)
 {
-	s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
+	s64 calcedKsTO, calcedKsTA, ir_Alpha, Alpha_corr;
 	s64 Ha_customer, Hb_customer;
 
 	Ha_customer = ((s64)Ha * 1000000LL) >> 14ULL;
@@ -393,30 +393,35 @@ static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 object,
 	Alpha_corr = emissivity * div64_s64(Alpha_corr, 100000LL);
 	Alpha_corr = div64_s64(Alpha_corr, 1000LL);
 	ir_Alpha = div64_s64((s64)object * 10000000LL, Alpha_corr);
-	TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) *
-		(div64_s64(TAdut, 10000LL) + 27315) *
-		(div64_s64(TAdut, 10000LL)  + 27315) *
-		(div64_s64(TAdut, 10000LL) + 27315);
 
 	return (int_sqrt64(int_sqrt64(ir_Alpha * 1000000000000LL + TAdut4))
 		- 27315 - Hb_customer) * 10;
 }
 
+static s64 mlx90632_calc_ta4(s64 TAdut, s64 scale)
+{
+	return (div64_s64(TAdut, scale) + 27315) *
+		(div64_s64(TAdut, scale) + 27315) *
+		(div64_s64(TAdut, scale) + 27315) *
+		(div64_s64(TAdut, scale) + 27315);
+}
+
 static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb,
 				     s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
 				     u16 tmp_emi)
 {
-	s64 kTA, kTA0, TAdut;
+	s64 kTA, kTA0, TAdut, TAdut4;
 	s64 temp = 25000;
 	s8 i;
 
 	kTA = (Ea * 1000LL) >> 16LL;
 	kTA0 = (Eb * 1000LL) >> 8LL;
 	TAdut = div64_s64(((ambient - kTA0) * 1000000LL), kTA) + 25 * 1000000LL;
+	TAdut4 = mlx90632_calc_ta4(TAdut, 10000LL);
 
 	/* Iterations of calculation as described in datasheet */
 	for (i = 0; i < 5; ++i) {
-		temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
+		temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, TAdut4,
 							   Fa, Fb, Ga, Ha, Hb,
 							   tmp_emi);
 	}
-- 
2.25.1

Re: [PATCH v4 1/4] iio:temperature:mlx90632: Reduce number of equal calulcations

[Andy Shevchenko]

On Sat, Aug 8, 2020 at 3:10 PM Crt Mori <cmo@melexis.com> wrote:
>
> TAdut4 was calculated each iteration although it did not change. In light
> of near future additions of the Extended range DSP calculations, this
> function refactoring will help reduce unrelated changes in that series as
> well as reduce the number of new functions needed.

Okay, unifying these two changes (helper function and move the place
where it's called) perhaps good.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Crt Mori <cmo@melexis.com>
> ---
>  drivers/iio/temperature/mlx90632.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
> index 51b812bcff2e..c3de10ba5b1e 100644
> --- a/drivers/iio/temperature/mlx90632.c
> +++ b/drivers/iio/temperature/mlx90632.c
> @@ -374,11 +374,11 @@ static s32 mlx90632_calc_temp_ambient(s16 ambient_new_raw, s16 ambient_old_raw,
>  }
>
>  static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 object,
> -                                              s64 TAdut, s32 Fa, s32 Fb,
> +                                              s64 TAdut, s64 TAdut4, s32 Fa, s32 Fb,
>                                                s32 Ga, s16 Ha, s16 Hb,
>                                                u16 emissivity)
>  {
> -       s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr;
> +       s64 calcedKsTO, calcedKsTA, ir_Alpha, Alpha_corr;
>         s64 Ha_customer, Hb_customer;
>
>         Ha_customer = ((s64)Ha * 1000000LL) >> 14ULL;
> @@ -393,30 +393,35 @@ static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 object,
>         Alpha_corr = emissivity * div64_s64(Alpha_corr, 100000LL);
>         Alpha_corr = div64_s64(Alpha_corr, 1000LL);
>         ir_Alpha = div64_s64((s64)object * 10000000LL, Alpha_corr);
> -       TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) *
> -               (div64_s64(TAdut, 10000LL) + 27315) *
> -               (div64_s64(TAdut, 10000LL)  + 27315) *
> -               (div64_s64(TAdut, 10000LL) + 27315);
>
>         return (int_sqrt64(int_sqrt64(ir_Alpha * 1000000000000LL + TAdut4))
>                 - 27315 - Hb_customer) * 10;
>  }
>
> +static s64 mlx90632_calc_ta4(s64 TAdut, s64 scale)
> +{
> +       return (div64_s64(TAdut, scale) + 27315) *
> +               (div64_s64(TAdut, scale) + 27315) *
> +               (div64_s64(TAdut, scale) + 27315) *
> +               (div64_s64(TAdut, scale) + 27315);
> +}
> +
>  static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb,
>                                      s32 Fa, s32 Fb, s32 Ga, s16 Ha, s16 Hb,
>                                      u16 tmp_emi)
>  {
> -       s64 kTA, kTA0, TAdut;
> +       s64 kTA, kTA0, TAdut, TAdut4;
>         s64 temp = 25000;
>         s8 i;
>
>         kTA = (Ea * 1000LL) >> 16LL;
>         kTA0 = (Eb * 1000LL) >> 8LL;
>         TAdut = div64_s64(((ambient - kTA0) * 1000000LL), kTA) + 25 * 1000000LL;
> +       TAdut4 = mlx90632_calc_ta4(TAdut, 10000LL);
>
>         /* Iterations of calculation as described in datasheet */
>         for (i = 0; i < 5; ++i) {
> -               temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut,
> +               temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, TAdut4,
>                                                            Fa, Fb, Ga, Ha, Hb,
>                                                            tmp_emi);
>         }
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 2/4] iio:temperature:mlx90632: Adding extended calibration option

[Crt Mori]

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.

Signed-off-by: Crt Mori <cmo@melexis.com>
---
 drivers/iio/temperature/mlx90632.c | 212 ++++++++++++++++++++++++++++-
 1 file changed, 210 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index c3de10ba5b1e..049c1217a166 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -11,6 +11,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
+#include <linux/limits.h>
 #include <linux/module.h>
 #include <linux/math64.h>
 #include <linux/of.h>
@@ -58,6 +59,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 +68,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 +96,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 +105,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 +134,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 +221,26 @@ 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;
+
+	return mlx90632_pwr_continuous(regmap);
+}
+
 static int mlx90632_channel_new_select(int perform_ret, uint8_t *channel_new,
 				       uint8_t *channel_old)
 {
@@ -300,6 +342,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)
+{
+	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;
+
+	ret = regmap_read(regmap, MLX90632_RAM_3(18), &read_tmp);
+	if (ret < 0)
+		return ret;
+	*ambient_old_raw = (s16)read_tmp;
+
+	return 0;
+}
+
+static int mlx90632_read_object_raw_extended(struct regmap *regmap, s16 *object_new_raw)
+{
+	unsigned int read_tmp;
+	s32 read;
+	int ret;
+
+	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 > S16_MAX || read < S16_MIN)
+		return -ERANGE;
+
+	*object_new_raw = read;
+
+	return 0;
+}
+
+static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *object_new_raw,
+					      s16 *ambient_new_raw, s16 *ambient_old_raw)
+{
+	int tries = 4;
+	int ret;
+
+	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 +496,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;
 
@@ -428,6 +584,31 @@ 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 = mlx90632_calc_ta4(reflected, 10);
+	TAdut4 = mlx90632_calc_ta4(TAdut, 10000LL);
+	TaTr4 = Tr4 - div64_s64(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;
@@ -475,6 +656,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,
@@ -648,6 +849,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;
@@ -667,12 +869,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);	
@@ -684,6 +891,7 @@ static int mlx90632_probe(struct i2c_client *client,
 	}
 
 	mlx90632->emissivity = 1000;
+	mlx90632->object_ambient_temperature = 25000; /* 25 degrees milliCelsius */
 
 	pm_runtime_disable(&client->dev);
 	ret = pm_runtime_set_active(&client->dev);
-- 
2.25.1

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

[Andy Shevchenko]

On Sat, Aug 8, 2020 at 3:11 PM Crt Mori <cmo@melexis.com> 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.

The kernel doc patch should go before this patch.

...

> +       *ambient_new_raw = (s16)read_tmp;

> +       *ambient_old_raw = (s16)read_tmp;

Sorry, did I miss your answer about these castings all over the patch?

...

> +       ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);

What so special about these magic 17, 18, 19? Can you provide definitions?

...

> +       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;
> +       }

Please avoid ping-pong type of changes in the same series (similar way
as for kernel doc), which means don't introduce something you are
going to change later on. Patch to move to do {} while () should go
before this one.

-- 
With Best Regards,
Andy Shevchenko

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

[Crt Mori]

Hi,
I am very sorry you missed them, I thought you saw it (reply on v3 of
the patch). Maybe something happened to that mail, as it contained
link to datasheet, so I will omit it now.

Except for the order, only the remarks below are still open (did you
get the polling trail I did?)

On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Sat, Aug 8, 2020 at 3:11 PM Crt Mori <cmo@melexis.com> 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.
>
> The kernel doc patch should go before this patch.
>
> ...
>
> > +       *ambient_new_raw = (s16)read_tmp;
>
> > +       *ambient_old_raw = (s16)read_tmp;
>
> Sorry, did I miss your answer about these castings all over the patch?
>

These castings are in fact needed. You read unsigned integer, but the
return value is signed integer. Without the cast it did not extend the
signed bit, but just wrote the value to signed. Also I find it more
obvious with casts, that I did not "accidentally" convert to signed.

> ...
>
> > +       ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > +       ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> > +       ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> > +       ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> > +       ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> > +       ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);
>
> What so special about these magic 17, 18, 19? Can you provide definitions?
>
When we started 0 to 19 were all open for access, from userspace, then
only 1 and 2 were used with calculations, and now we use 17, 18 and
19. Matter of fact is, I can't provide a descriptive name as it
depends on DSP version and as you can see now within the same DSP
version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
that might not be true in the next configuration, so I rather keep the
naming as in the datasheet.

> ...
>
> > +       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;
> > +       }
>
> Please avoid ping-pong type of changes in the same series (similar way
> as for kernel doc), which means don't introduce something you are
> going to change later on. Patch to move to do {} while () should go
> before this one.

OK, will fix that ordering in v5, but will wait till we solve also
above discussions to avoid adding new versions.

>
> --
> With Best Regards,
> Andy Shevchenko

And about that voodoo stuff with numbers:

Honestly, the equation is in the datasheet[1] and this is just making
floating point to fixed point with proper intermediate scaling
(initially I had defines of TENTOX, but that was not desired). There
is no better explanation of this voodoo.

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

[Jonathan Cameron]

On Sat, 8 Aug 2020 23:57:59 +0200
Crt Mori <cmo@melexis.com> wrote:

> Hi,
> I am very sorry you missed them, I thought you saw it (reply on v3 of
> the patch). Maybe something happened to that mail, as it contained
> link to datasheet, so I will omit it now.
> 
> Except for the order, only the remarks below are still open (did you
> get the polling trail I did?)
> 
> On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > On Sat, Aug 8, 2020 at 3:11 PM Crt Mori <cmo@melexis.com> 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.  
> >
> > The kernel doc patch should go before this patch.
> >
> > ...
> >  
> > > +       *ambient_new_raw = (s16)read_tmp;  
> >  
> > > +       *ambient_old_raw = (s16)read_tmp;  
> >
> > Sorry, did I miss your answer about these castings all over the patch?
> >  
> 
> These castings are in fact needed. You read unsigned integer, but the
> return value is signed integer. Without the cast it did not extend the
> signed bit, but just wrote the value to signed. Also I find it more
> obvious with casts, that I did not "accidentally" convert to signed.

Should we perhaps be making this explicit for the cases where we
are sign extending?  That doesn't include these two as the lvalue
is s16, but does include some of the others.

sign_extend32(read_tmp, 15)

> 
> > ...
> >  
> > > +       ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > > +       ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> > > +       ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> > > +       ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> > > +       ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> > > +       ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);  
> >
> > What so special about these magic 17, 18, 19? Can you provide definitions?
> >  
> When we started 0 to 19 were all open for access, from userspace, then
> only 1 and 2 were used with calculations, and now we use 17, 18 and
> 19. Matter of fact is, I can't provide a descriptive name as it
> depends on DSP version and as you can see now within the same DSP
> version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
> named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
> that might not be true in the next configuration, so I rather keep the
> naming as in the datasheet.
Normal solution for that is to version the defines as well.

MLX90632_FW3_RAM_1_AMBIENT etc
When a new version changes this, then you introduced new defines to
support that firmware.

> 
> > ...
> >  
> > > +       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;
> > > +       }  
> >
> > Please avoid ping-pong type of changes in the same series (similar way
> > as for kernel doc), which means don't introduce something you are
> > going to change later on. Patch to move to do {} while () should go
> > before this one.  
> 
> OK, will fix that ordering in v5, but will wait till we solve also
> above discussions to avoid adding new versions.
> 
> >
> > --
> > With Best Regards,
> > Andy Shevchenko  
> 
> And about that voodoo stuff with numbers:
> 
> Honestly, the equation is in the datasheet[1] and this is just making
> floating point to fixed point with proper intermediate scaling
> (initially I had defines of TENTOX, but that was not desired). There
> is no better explanation of this voodoo.

We all love fixed point arithmetic :)

Jonathan

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

[Crt Mori]

On Sun, 9 Aug 2020 at 15:32, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 8 Aug 2020 23:57:59 +0200
> Crt Mori <cmo@melexis.com> wrote:
>
> > Hi,
> > I am very sorry you missed them, I thought you saw it (reply on v3 of
> > the patch). Maybe something happened to that mail, as it contained
> > link to datasheet, so I will omit it now.
> >
> > Except for the order, only the remarks below are still open (did you
> > get the polling trail I did?)
> >
> > On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Sat, Aug 8, 2020 at 3:11 PM Crt Mori <cmo@melexis.com> 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.
> > >
> > > The kernel doc patch should go before this patch.
> > >
> > > ...
> > >
> > > > +       *ambient_new_raw = (s16)read_tmp;
> > >
> > > > +       *ambient_old_raw = (s16)read_tmp;
> > >
> > > Sorry, did I miss your answer about these castings all over the patch?
> > >
> >
> > These castings are in fact needed. You read unsigned integer, but the
> > return value is signed integer. Without the cast it did not extend the
> > signed bit, but just wrote the value to signed. Also I find it more
> > obvious with casts, that I did not "accidentally" convert to signed.
>
> Should we perhaps be making this explicit for the cases where we
> are sign extending?  That doesn't include these two as the lvalue
> is s16, but does include some of the others.
>
> sign_extend32(read_tmp, 15)
>

So for you lines like
s32 read;
read = (read + (s16)read_tmp) / 2;

would actually be better as:
read = (read + sign_extend32(read_tmp, 15)) / 2;

Hm, strange. I would read that more align the read_tmp to 32 bit than
the value you have in read_tmp is actually a signed 16 bit integer...

> >
> > > ...
> > >
> > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);
> > >
> > > What so special about these magic 17, 18, 19? Can you provide definitions?
> > >
> > When we started 0 to 19 were all open for access, from userspace, then
> > only 1 and 2 were used with calculations, and now we use 17, 18 and
> > 19. Matter of fact is, I can't provide a descriptive name as it
> > depends on DSP version and as you can see now within the same DSP
> > version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
> > named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
> > that might not be true in the next configuration, so I rather keep the
> > naming as in the datasheet.
> Normal solution for that is to version the defines as well.
>
> MLX90632_FW3_RAM_1_AMBIENT etc
> When a new version changes this, then you introduced new defines to
> support that firmware.
>

OK will add those, but it is ending up as:
MLX90632_RAM_DSP5_AMBIENT
MLX90632_RAM_DSP5_EXTENDED_AMBIENT
MLX90632_RAM_DSP5_OBJECT_1
MLX90632_RAM_DSP5_EXTENDED_OBJECT_1
MLX90632_RAM_DSP5_OBJECT_2
MLX90632_RAM_DSP5_EXTENDED_OBJECT_2

ok?
> >
> > > ...
> > >
> > > > +       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;
> > > > +       }
> > >
> > > Please avoid ping-pong type of changes in the same series (similar way
> > > as for kernel doc), which means don't introduce something you are
> > > going to change later on. Patch to move to do {} while () should go
> > > before this one.
> >
> > OK, will fix that ordering in v5, but will wait till we solve also
> > above discussions to avoid adding new versions.
> >
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> >
> > And about that voodoo stuff with numbers:
> >
> > Honestly, the equation is in the datasheet[1] and this is just making
> > floating point to fixed point with proper intermediate scaling
> > (initially I had defines of TENTOX, but that was not desired). There
> > is no better explanation of this voodoo.
>
> We all love fixed point arithmetic :)
>
> Jonathan

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

[Crt Mori]

On Sun, 9 Aug 2020 at 23:05, Crt Mori <cmo@melexis.com> wrote:
>
> On Sun, 9 Aug 2020 at 15:32, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 8 Aug 2020 23:57:59 +0200
> > Crt Mori <cmo@melexis.com> wrote:
> >
> > > Hi,
> > > I am very sorry you missed them, I thought you saw it (reply on v3 of
> > > the patch). Maybe something happened to that mail, as it contained
> > > link to datasheet, so I will omit it now.
> > >
> > > Except for the order, only the remarks below are still open (did you
> > > get the polling trail I did?)
> > >
> > > On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > On Sat, Aug 8, 2020 at 3:11 PM Crt Mori <cmo@melexis.com> 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.
> > > >
> > > > The kernel doc patch should go before this patch.
> > > >
> > > > ...
> > > >
> > > > > +       *ambient_new_raw = (s16)read_tmp;
> > > >
> > > > > +       *ambient_old_raw = (s16)read_tmp;
> > > >
> > > > Sorry, did I miss your answer about these castings all over the patch?
> > > >
> > >
> > > These castings are in fact needed. You read unsigned integer, but the
> > > return value is signed integer. Without the cast it did not extend the
> > > signed bit, but just wrote the value to signed. Also I find it more
> > > obvious with casts, that I did not "accidentally" convert to signed.
> >
> > Should we perhaps be making this explicit for the cases where we
> > are sign extending?  That doesn't include these two as the lvalue
> > is s16, but does include some of the others.
> >
> > sign_extend32(read_tmp, 15)
> >
>
> So for you lines like
> s32 read;
> read = (read + (s16)read_tmp) / 2;
>
> would actually be better as:
> read = (read + sign_extend32(read_tmp, 15)) / 2;
>
> Hm, strange. I would read that more align the read_tmp to 32 bit than
> the value you have in read_tmp is actually a signed 16 bit integer...
>

OK, I did some trails without the casts and had deja-vu from the first
series of patches I submitted.  I noticed that without a cast the
value that ends up in variable is not extended to signed, but it is
unsigned value truncated. This same finding leads to have these casts
already in current ambient and object raw read functions.

So now only debate is if sign_extend32 is useful in this case, as read
in the current case is 32 bit (before it was also 16 bit).

My preference is to leave unified across the driver.

> > >
> > > > ...
> > > >
> > > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> > > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> > > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> > > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> > > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);
> > > >
> > > > What so special about these magic 17, 18, 19? Can you provide definitions?
> > > >
> > > When we started 0 to 19 were all open for access, from userspace, then
> > > only 1 and 2 were used with calculations, and now we use 17, 18 and
> > > 19. Matter of fact is, I can't provide a descriptive name as it
> > > depends on DSP version and as you can see now within the same DSP
> > > version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
> > > named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
> > > that might not be true in the next configuration, so I rather keep the
> > > naming as in the datasheet.
> > Normal solution for that is to version the defines as well.
> >
> > MLX90632_FW3_RAM_1_AMBIENT etc
> > When a new version changes this, then you introduced new defines to
> > support that firmware.
> >
>
> OK will add those, but it is ending up as:
> MLX90632_RAM_DSP5_AMBIENT
> MLX90632_RAM_DSP5_EXTENDED_AMBIENT
> MLX90632_RAM_DSP5_OBJECT_1
> MLX90632_RAM_DSP5_EXTENDED_OBJECT_1
> MLX90632_RAM_DSP5_OBJECT_2
> MLX90632_RAM_DSP5_EXTENDED_OBJECT_2
>
> ok?
> > >
> > > > ...
> > > >
> > > > > +       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;
> > > > > +       }
> > > >
> > > > Please avoid ping-pong type of changes in the same series (similar way
> > > > as for kernel doc), which means don't introduce something you are
> > > > going to change later on. Patch to move to do {} while () should go
> > > > before this one.
> > >
> > > OK, will fix that ordering in v5, but will wait till we solve also
> > > above discussions to avoid adding new versions.
> > >
> > > >
> > > > --
> > > > With Best Regards,
> > > > Andy Shevchenko
> > >
> > > And about that voodoo stuff with numbers:
> > >
> > > Honestly, the equation is in the datasheet[1] and this is just making
> > > floating point to fixed point with proper intermediate scaling
> > > (initially I had defines of TENTOX, but that was not desired). There
> > > is no better explanation of this voodoo.
> >
> > We all love fixed point arithmetic :)
> >
> > Jonathan

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

[Jonathan Cameron]

On Tue, 11 Aug 2020 09:53:55 +0200
Crt Mori <cmo@melexis.com> wrote:

> On Sun, 9 Aug 2020 at 23:05, Crt Mori <cmo@melexis.com> wrote:
> >
> > On Sun, 9 Aug 2020 at 15:32, Jonathan Cameron <jic23@kernel.org> wrote:  
> > >
> > > On Sat, 8 Aug 2020 23:57:59 +0200
> > > Crt Mori <cmo@melexis.com> wrote:
> > >  
> > > > Hi,
> > > > I am very sorry you missed them, I thought you saw it (reply on v3 of
> > > > the patch). Maybe something happened to that mail, as it contained
> > > > link to datasheet, so I will omit it now.
> > > >
> > > > Except for the order, only the remarks below are still open (did you
> > > > get the polling trail I did?)
> > > >
> > > > On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > >
> > > > > On Sat, Aug 8, 2020 at 3:11 PM Crt Mori <cmo@melexis.com> 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.  
> > > > >
> > > > > The kernel doc patch should go before this patch.
> > > > >
> > > > > ...
> > > > >  
> > > > > > +       *ambient_new_raw = (s16)read_tmp;  
> > > > >  
> > > > > > +       *ambient_old_raw = (s16)read_tmp;  
> > > > >
> > > > > Sorry, did I miss your answer about these castings all over the patch?
> > > > >  
> > > >
> > > > These castings are in fact needed. You read unsigned integer, but the
> > > > return value is signed integer. Without the cast it did not extend the
> > > > signed bit, but just wrote the value to signed. Also I find it more
> > > > obvious with casts, that I did not "accidentally" convert to signed.  
> > >
> > > Should we perhaps be making this explicit for the cases where we
> > > are sign extending?  That doesn't include these two as the lvalue
> > > is s16, but does include some of the others.
> > >
> > > sign_extend32(read_tmp, 15)
> > >  
> >
> > So for you lines like
> > s32 read;
> > read = (read + (s16)read_tmp) / 2;
> >
> > would actually be better as:
> > read = (read + sign_extend32(read_tmp, 15)) / 2;
> >
> > Hm, strange. I would read that more align the read_tmp to 32 bit than
> > the value you have in read_tmp is actually a signed 16 bit integer...
> >  
> 
> OK, I did some trails without the casts and had deja-vu from the first
> series of patches I submitted.  I noticed that without a cast the
> value that ends up in variable is not extended to signed, but it is
> unsigned value truncated. This same finding leads to have these casts
> already in current ambient and object raw read functions.
> 
> So now only debate is if sign_extend32 is useful in this case, as read
> in the current case is 32 bit (before it was also 16 bit).
> 
> My preference is to leave unified across the driver.

It is fairly obvious to me what is going on as things stand, but
if others are being confused, the sign_extend32 does make it explicit
that this is all about sign extension.

> 
> > > >  
> > > > > ...
> > > > >  
> > > > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> > > > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> > > > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> > > > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> > > > > > +       ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> > > > > > +       ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);  
> > > > >
> > > > > What so special about these magic 17, 18, 19? Can you provide definitions?
> > > > >  
> > > > When we started 0 to 19 were all open for access, from userspace, then
> > > > only 1 and 2 were used with calculations, and now we use 17, 18 and
> > > > 19. Matter of fact is, I can't provide a descriptive name as it
> > > > depends on DSP version and as you can see now within the same DSP
> > > > version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
> > > > named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
> > > > that might not be true in the next configuration, so I rather keep the
> > > > naming as in the datasheet.  
> > > Normal solution for that is to version the defines as well.
> > >
> > > MLX90632_FW3_RAM_1_AMBIENT etc
> > > When a new version changes this, then you introduced new defines to
> > > support that firmware.
> > >  
> >
> > OK will add those, but it is ending up as:
> > MLX90632_RAM_DSP5_AMBIENT
> > MLX90632_RAM_DSP5_EXTENDED_AMBIENT
> > MLX90632_RAM_DSP5_OBJECT_1
> > MLX90632_RAM_DSP5_EXTENDED_OBJECT_1
> > MLX90632_RAM_DSP5_OBJECT_2
> > MLX90632_RAM_DSP5_EXTENDED_OBJECT_2
> >
> > ok?  
That's fine.

> > > >  
> > > > > ...
> > > > >  
> > > > > > +       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;
> > > > > > +       }  
> > > > >
> > > > > Please avoid ping-pong type of changes in the same series (similar way
> > > > > as for kernel doc), which means don't introduce something you are
> > > > > going to change later on. Patch to move to do {} while () should go
> > > > > before this one.  
> > > >
> > > > OK, will fix that ordering in v5, but will wait till we solve also
> > > > above discussions to avoid adding new versions.
> > > >  
> > > > >
> > > > > --
> > > > > With Best Regards,
> > > > > Andy Shevchenko  
> > > >
> > > > And about that voodoo stuff with numbers:
> > > >
> > > > Honestly, the equation is in the datasheet[1] and this is just making
> > > > floating point to fixed point with proper intermediate scaling
> > > > (initially I had defines of TENTOX, but that was not desired). There
> > > > is no better explanation of this voodoo.  
> > >
> > > We all love fixed point arithmetic :)
> > >
> > > Jonathan  

[PATCH v4 3/4] iio:temperature:mlx90632: Add kerneldoc to the internal struct

[Crt Mori]

Document internal/private struct for mlx90632 device.

Signed-off-by: Crt Mori <cmo@melexis.com>
---
 drivers/iio/temperature/mlx90632.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 049c1217a166..4e0131705c11 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -107,12 +107,23 @@
 #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
 #define MLX90632_EXTENDED_LIMIT 27000 /* Extended mode raw value limit */
 
+/**
+ * struct mlx90632_data - private data for the MLX90632 device
+ * @client: I2C client of the device
+ * @lock: Internal mutex for multiple reads for single measurement
+ * @regmap: Regmap of the device
+ * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
+ * @mtyp: Measurement type physical sensor configuration for extended range
+ *        calculations
+ * @object_ambient_temperature: Ambient temperature at object (might differ of
+ *                              the ambient temperature of sensor.
+ */
 struct mlx90632_data {
 	struct i2c_client *client;
-	struct mutex lock; /* Multiple reads for single measurement */
+	struct mutex lock;
 	struct regmap *regmap;
 	u16 emissivity;
-	u8 mtyp; /* measurement type - to enable extended range calculations */
+	u8 mtyp;
 	u32 object_ambient_temperature;
 };
 
-- 
2.25.1

Re: [PATCH v4 3/4] iio:temperature:mlx90632: Add kerneldoc to the internal struct

[Andy Shevchenko]

On Sat, Aug 8, 2020 at 3:11 PM Crt Mori <cmo@melexis.com> wrote:
>
> Document internal/private struct for mlx90632 device.

Thanks. The only issue with it is that it should go before the one
that adds member(s) to the struct.


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 4/4] iio:temperature:mlx90632: Convert polling while loops to do-while

[Crt Mori]

Reduce number of lines and improve readability to convert polling while
loops to do-while. The iopoll.h interface was not used, because we
require more than 20ms timeout, because time for sensor to perform a
measurement is around 10ms and it needs to perform measurements for each
channel (which currently is 3).

Signed-off-by: Crt Mori <cmo@melexis.com>
---
 drivers/iio/temperature/mlx90632.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 4e0131705c11..24ffcce9ad74 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -214,15 +214,13 @@ static int mlx90632_perform_measurement(struct mlx90632_data *data)
 	if (ret < 0)
 		return ret;
 
-	while (tries-- > 0) {
+	do {
 		ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
 				  &reg_status);
 		if (ret < 0)
 			return ret;
-		if (reg_status & MLX90632_STAT_DATA_RDY)
-			break;
 		usleep_range(10000, 11000);
-	}
+	} while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--);
 
 	if (tries < 0) {
 		dev_err(&data->client->dev, "data not ready");
@@ -419,7 +417,7 @@ static int mlx90632_read_object_raw_extended(struct regmap *regmap, s16 *object_
 static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *object_new_raw,
 					      s16 *ambient_new_raw, s16 *ambient_old_raw)
 {
-	int tries = 4;
+	int tries = 5;
 	int ret;
 
 	mutex_lock(&data->lock);
@@ -427,14 +425,13 @@ static int mlx90632_read_all_channel_extended(struct mlx90632_data *data, s16 *o
 	if (ret < 0)
 		goto read_unlock;
 
-	while (tries-- > 0) {
+	do {
 		ret = mlx90632_perform_measurement(data);
 		if (ret < 0)
 			goto read_unlock;
 
-		if (ret == 19)
-			break;
-	}
+	} while ((ret != 19) && tries--);
+
 	if (tries < 0) {
 		ret = -ETIMEDOUT;
 		goto read_unlock;
-- 
2.25.1

Re: [PATCH v4 0/4] iio: temperature: mlx90632: Add extended calibration calculations

[Andy Shevchenko]

On Sat, Aug 8, 2020 at 3:10 PM Crt Mori <cmo@melexis.com> wrote:
>
> Since the second patch is dependent on the first and was still not
> merged, I have decided to send them together. First patch just makes
> second one more readable as it splits out the repeated calculation and
> that enables the second patch to tweak the variable to the new
> condition.

I guess the above is not true anymore, b/c it's already 4 in the series.

Thanks for the changelog, nevertheless!

> V4 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
>          - Move the function creation for Ta4 to first patch
>          - Add kernel doc patch for documenting internal struct
>          - Add patch to convert while loops to do-while loops for
>            polling
>
> V3 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
>          - Change commit message text to more proper English as per suggestions
>          - Drop unneeded brackets and parentheses
>          - Use defines from limits.h
>          - Remove userspace typedefs as leftovers from porting
>          - Testing of timeout loops with iopoll.h was no successful,
>            because delay between measurements is 10ms, but we need to
>            fill at least 3 channels, so final timeout should be 40ms
>            which is out of scope of usleep function
>          - Fixing some typos in comments
>
> V2 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
>          - Convert divison back to shifts to make it more readable
>
> Crt Mori (4):
>   iio:temperature:mlx90632: Reduce number of equal calulcations
>   iio:temperature:mlx90632: Adding extended calibration option
>   iio:temperature:mlx90632: Add kerneldoc to the internal struct
>   iio:temperature:mlx90632: Convert polling while loops to do-while
>
>  drivers/iio/temperature/mlx90632.c | 251 +++++++++++++++++++++++++++--
>  1 file changed, 236 insertions(+), 15 deletions(-)
>
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/4] iio: temperature: mlx90632: Add extended calibration calculations

[Andy Shevchenko]

On Sat, Aug 8, 2020 at 3:10 PM Crt Mori <cmo@melexis.com> wrote:
>
> Since the second patch is dependent on the first and was still not
> merged, I have decided to send them together. First patch just makes
> second one more readable as it splits out the repeated calculation and
> that enables the second patch to tweak the variable to the new
> condition.
>
> V4 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
>          - Move the function creation for Ta4 to first patch
>          - Add kernel doc patch for documenting internal struct
>          - Add patch to convert while loops to do-while loops for
>            polling
>
> V3 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
>          - Change commit message text to more proper English as per suggestions
>          - Drop unneeded brackets and parentheses
>          - Use defines from limits.h
>          - Remove userspace typedefs as leftovers from porting
>          - Testing of timeout loops with iopoll.h was no successful,
>            because delay between measurements is 10ms, but we need to
>            fill at least 3 channels, so final timeout should be 40ms
>            which is out of scope of usleep function
>          - Fixing some typos in comments
>
> V2 review comments from Andy Shevchenko <andy.shevchenko@gmail.com>:
>          - Convert divison back to shifts to make it more readable
>
> Crt Mori (4):
>   iio:temperature:mlx90632: Reduce number of equal calulcations
>   iio:temperature:mlx90632: Adding extended calibration option
>   iio:temperature:mlx90632: Add kerneldoc to the internal struct
>   iio:temperature:mlx90632: Convert polling while loops to do-while
>
>  drivers/iio/temperature/mlx90632.c | 251 +++++++++++++++++++++++++++--
>  1 file changed, 236 insertions(+), 15 deletions(-)
>
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/4] iio: temperature: mlx90632: Add extended calibration calculations

[Andy Shevchenko]

On Sat, Aug 8, 2020 at 3:10 PM Crt Mori <cmo@melexis.com> wrote:
>
> Since the second patch is dependent on the first and was still not
> merged, I have decided to send them together. First patch just makes
> second one more readable as it splits out the repeated calculation and
> that enables the second patch to tweak the variable to the new
> condition.

So, we are closer, but here is one remark, it's not good to send a new
version if you are not going to address _all_ comments (it's fine to
do, but you need to answer first why you are not going to satisfy
them). For example, explicit castings here and there, voodoo
arithmetics is left uncommented.


-- 
With Best Regards,
Andy Shevchenko