Add extended calibration calculations for the new subversion of DSP5.
V5 review comments from Andy Shevchenko <[email protected]:
- Swap order of patches to avoid re-doing the calculations
- Add fixed name defines for Ambient and Object RAM temperature
channels as per suggestion of the Jonathan Cameron <[email protected]>
V5:
- Add style changes patch along with current series.
V4 review comments from Andy Shevchenko <[email protected]>:
- 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 <[email protected]>:
- 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 <[email protected]>:
- Convert divison back to shifts to make it more readable
Crt Mori (5):
iio:temperature:mlx90632: Reduce number of equal calulcations
iio:temperature:mlx90632: Add kerneldoc to the internal struct
iio:temperature:mlx90632: Convert polling while loop to do-while
iio:temperature:mlx90632: Adding extended calibration option
iio:temperature:mlx90632: Some stylefixing leftovers
drivers/iio/temperature/mlx90632.c | 301 +++++++++++++++++++++++++----
1 file changed, 267 insertions(+), 34 deletions(-)
--
2.25.1
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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
Document internal/private struct for mlx90632 device.
Signed-off-by: Crt Mori <[email protected]>
---
drivers/iio/temperature/mlx90632.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index c3de10ba5b1e..ce75f5a3486b 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -89,9 +89,16 @@
#define MLX90632_MAX_MEAS_NUM 31 /**< Maximum measurements in list */
#define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
+/**
+ * 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.
+ */
struct mlx90632_data {
struct i2c_client *client;
- struct mutex lock; /* Multiple reads for single measurement */
+ struct mutex lock;
struct regmap *regmap;
u16 emissivity;
};
--
2.25.1
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 <[email protected]>
---
drivers/iio/temperature/mlx90632.c | 224 ++++++++++++++++++++++++++++-
1 file changed, 222 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index d87c792b7e72..9fa8c078c037 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 */
@@ -78,9 +93,21 @@
#define MLX90632_RAM_2(meas_num) (MLX90632_ADDR_RAM + 3 * meas_num + 1)
#define MLX90632_RAM_3(meas_num) (MLX90632_ADDR_RAM + 3 * meas_num + 2)
+/* Name important RAM_MEAS channels */
+#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_1 MLX90632_RAM_3(17)
+#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_2 MLX90632_RAM_3(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_1 MLX90632_RAM_1(17)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_2 MLX90632_RAM_2(17)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_3 MLX90632_RAM_1(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_4 MLX90632_RAM_2(18)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_5 MLX90632_RAM_1(19)
+#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_6 MLX90632_RAM_2(19)
+
/* 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,6 +115,7 @@
#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 - private data for the MLX90632 device
@@ -95,16 +123,23 @@
* @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;
struct regmap *regmap;
u16 emissivity;
+ u8 mtyp;
+ 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)),
@@ -120,6 +155,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)),
@@ -204,6 +240,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)
{
@@ -305,6 +361,104 @@ 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_DSP5_EXTENDED_AMBIENT_1, &read_tmp);
+ if (ret < 0)
+ return ret;
+ *ambient_new_raw = (s16)read_tmp;
+
+ ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_AMBIENT_2, &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_DSP5_EXTENDED_OBJECT_1, &read_tmp);
+ if (ret < 0)
+ return ret;
+ read = (s16)read_tmp;
+
+ ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_2, &read_tmp);
+ if (ret < 0)
+ return ret;
+ read = read - (s16)read_tmp;
+
+ ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_3, &read_tmp);
+ if (ret < 0)
+ return ret;
+ read = read - (s16)read_tmp;
+
+ ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_4, &read_tmp);
+ if (ret < 0)
+ return ret;
+ read = (read + (s16)read_tmp) / 2;
+
+ ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_5, &read_tmp);
+ if (ret < 0)
+ return ret;
+ read = read + (s16)read_tmp;
+
+ ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_6, &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;
+
+ do {
+ ret = mlx90632_perform_measurement(data);
+ if (ret < 0)
+ goto read_unlock;
+ } while ((ret != 19) && tries--);
+
+ 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)
{
@@ -359,9 +513,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;
@@ -433,6 +601,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;
@@ -480,6 +673,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,
@@ -653,6 +866,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;
@@ -672,12 +886,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);
@@ -689,6 +908,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
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 <[email protected]>
---
drivers/iio/temperature/mlx90632.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index ce75f5a3486b..d87c792b7e72 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -188,15 +188,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,
®_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");
--
2.25.1
There is some inconsistency and whitespace cleanup performed in this
patch. It was done on top of my other patches, but I can rebase to head
of the togreg branch if it would go in sooner.
Signed-off-by: Crt Mori <[email protected]>
---
drivers/iio/temperature/mlx90632.c | 41 ++++++++++++++++--------------
1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
index 9fa8c078c037..4ef13509fb0f 100644
--- a/drivers/iio/temperature/mlx90632.c
+++ b/drivers/iio/temperature/mlx90632.c
@@ -94,6 +94,9 @@
#define MLX90632_RAM_3(meas_num) (MLX90632_ADDR_RAM + 3 * meas_num + 2)
/* Name important RAM_MEAS channels */
+#define MLX90632_RAM_DSP5_AMBIENT_1 MLX90632_RAM_3(1)
+#define MLX90632_RAM_DSP5_AMBIENT_2 MLX90632_RAM_3(2)
+
#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_1 MLX90632_RAM_3(17)
#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_2 MLX90632_RAM_3(18)
#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_1 MLX90632_RAM_1(17)
@@ -111,10 +114,10 @@
#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) */
-#define MLX90632_REF_12 12LL /**< ResCtrlRef value of Ch 1 or Ch 2 */
-#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_REF_12 12LL /* ResCtrlRef value of Ch 1 or Ch 2 */
+#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 */
/**
@@ -285,12 +288,12 @@ static int mlx90632_read_ambient_raw(struct regmap *regmap,
int ret;
unsigned int read_tmp;
- ret = regmap_read(regmap, MLX90632_RAM_3(1), &read_tmp);
+ ret = regmap_read(regmap, MLX90632_RAM_DSP5_AMBIENT_1, &read_tmp);
if (ret < 0)
return ret;
*ambient_new_raw = (s16)read_tmp;
- ret = regmap_read(regmap, MLX90632_RAM_3(2), &read_tmp);
+ ret = regmap_read(regmap, MLX90632_RAM_DSP5_AMBIENT_2, &read_tmp);
if (ret < 0)
return ret;
*ambient_old_raw = (s16)read_tmp;
@@ -488,11 +491,11 @@ static s64 mlx90632_preprocess_temp_amb(s16 ambient_new_raw,
kGb = ((s64)Gb * 1000LL) >> 10ULL;
VR_Ta = (s64)ambient_old_raw * 1000000LL +
- kGb * div64_s64(((s64)ambient_new_raw * 1000LL),
- (MLX90632_REF_3));
+ kGb * div64_s64((s64)ambient_new_raw * 1000LL,
+ MLX90632_REF_3);
tmp = div64_s64(
- div64_s64(((s64)ambient_new_raw * 1000000000000LL),
- (MLX90632_REF_3)), VR_Ta);
+ div64_s64((s64)ambient_new_raw * 1000000000000LL,
+ MLX90632_REF_3), VR_Ta);
return div64_s64(tmp << 19ULL, 1000LL);
}
@@ -504,13 +507,13 @@ static s64 mlx90632_preprocess_temp_obj(s16 object_new_raw, s16 object_old_raw,
kKa = ((s64)Ka * 1000LL) >> 10ULL;
VR_IR = (s64)ambient_old_raw * 1000000LL +
- kKa * div64_s64(((s64)ambient_new_raw * 1000LL),
- (MLX90632_REF_3));
+ kKa * div64_s64((s64)ambient_new_raw * 1000LL,
+ MLX90632_REF_3);
tmp = div64_s64(
- div64_s64(((s64)((object_new_raw + object_old_raw) / 2)
- * 1000000000000LL), (MLX90632_REF_12)),
+ div64_s64((s64)((object_new_raw + object_old_raw) / 2)
+ * 1000000000000LL, MLX90632_REF_12),
VR_IR);
- return div64_s64((tmp << 19ULL), 1000LL);
+ return div64_s64(tmp << 19ULL, 1000LL);
}
static s64 mlx90632_preprocess_temp_obj_extended(s16 object_new_raw, s16 ambient_new_raw,
@@ -560,8 +563,8 @@ static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 object,
calcedKsTO = ((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
* 1000LL)) >> 36LL;
calcedKsTA = ((s64)(Fb * (TAdut - 25 * 1000000LL))) >> 36LL;
- Alpha_corr = div64_s64((((s64)(Fa * 10000000000LL) >> 46LL)
- * Ha_customer), 1000LL);
+ Alpha_corr = div64_s64(((s64)(Fa * 10000000000LL) >> 46LL)
+ * Ha_customer, 1000LL);
Alpha_corr *= ((s64)(1 * 1000000LL + calcedKsTO + calcedKsTA));
Alpha_corr = emissivity * div64_s64(Alpha_corr, 100000LL);
Alpha_corr = div64_s64(Alpha_corr, 1000LL);
@@ -589,7 +592,7 @@ static s32 mlx90632_calc_temp_object(s64 object, s64 ambient, s32 Ea, s32 Eb,
kTA = (Ea * 1000LL) >> 16LL;
kTA0 = (Eb * 1000LL) >> 8LL;
- TAdut = div64_s64(((ambient - kTA0) * 1000000LL), kTA) + 25 * 1000000LL;
+ TAdut = div64_s64((ambient - kTA0) * 1000000LL, kTA) + 25 * 1000000LL;
TAdut4 = mlx90632_calc_ta4(TAdut, 10000LL);
/* Iterations of calculation as described in datasheet */
@@ -899,7 +902,7 @@ static int mlx90632_probe(struct i2c_client *client,
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);
+ "Detected Unknown EEPROM calibration %x\n", read);
} else {
dev_err(&client->dev,
"Wrong DSP version %x (expected %x)\n",
--
2.25.1
On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
>
> Document internal/private struct for mlx90632 device.
Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Crt Mori <[email protected]>
> ---
> drivers/iio/temperature/mlx90632.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
> index c3de10ba5b1e..ce75f5a3486b 100644
> --- a/drivers/iio/temperature/mlx90632.c
> +++ b/drivers/iio/temperature/mlx90632.c
> @@ -89,9 +89,16 @@
> #define MLX90632_MAX_MEAS_NUM 31 /**< Maximum measurements in list */
> #define MLX90632_SLEEP_DELAY_MS 3000 /**< Autosleep delay */
>
> +/**
> + * 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.
> + */
> struct mlx90632_data {
> struct i2c_client *client;
> - struct mutex lock; /* Multiple reads for single measurement */
> + struct mutex lock;
> struct regmap *regmap;
> u16 emissivity;
> };
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
>
> There is some inconsistency and whitespace cleanup performed in this
> patch. It was done on top of my other patches, but I can rebase to head
> of the togreg branch if it would go in sooner.
...
> -#define MLX90632_REF_12 12LL /**< ResCtrlRef value of Ch 1 or Ch 2 */
> -#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_REF_12 12LL /* ResCtrlRef value of Ch 1 or Ch 2 */
> +#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 */
This was actually in doxy (perhaps kernel doc also understands this)
format of description.
Can you double check that the kernel doc is okay / not okay with it?
If it is okay, perhaps it's better to convert others to that format
rather than dropping it.
--
With Best Regards,
Andy Shevchenko
On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
>
> 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).
I don't see how it prevents using iopoll.h. It uses usleep_range()
under the hood in the same way you did here, but open coded.
...
> - while (tries-- > 0) {
> + do {
> ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> ®_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");
--
With Best Regards,
Andy Shevchenko
On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> 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.
FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Crt Mori <[email protected]>
> ---
> drivers/iio/temperature/mlx90632.c | 224 ++++++++++++++++++++++++++++-
> 1 file changed, 222 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c
> index d87c792b7e72..9fa8c078c037 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 */
> @@ -78,9 +93,21 @@
> #define MLX90632_RAM_2(meas_num) (MLX90632_ADDR_RAM + 3 * meas_num + 1)
> #define MLX90632_RAM_3(meas_num) (MLX90632_ADDR_RAM + 3 * meas_num + 2)
>
> +/* Name important RAM_MEAS channels */
> +#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_1 MLX90632_RAM_3(17)
> +#define MLX90632_RAM_DSP5_EXTENDED_AMBIENT_2 MLX90632_RAM_3(18)
> +#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_1 MLX90632_RAM_1(17)
> +#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_2 MLX90632_RAM_2(17)
> +#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_3 MLX90632_RAM_1(18)
> +#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_4 MLX90632_RAM_2(18)
> +#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_5 MLX90632_RAM_1(19)
> +#define MLX90632_RAM_DSP5_EXTENDED_OBJECT_6 MLX90632_RAM_2(19)
> +
> /* 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,6 +115,7 @@
> #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 - private data for the MLX90632 device
> @@ -95,16 +123,23 @@
> * @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;
> struct regmap *regmap;
> u16 emissivity;
> + u8 mtyp;
> + 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)),
> @@ -120,6 +155,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)),
> @@ -204,6 +240,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)
> {
> @@ -305,6 +361,104 @@ 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_DSP5_EXTENDED_AMBIENT_1, &read_tmp);
> + if (ret < 0)
> + return ret;
> + *ambient_new_raw = (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_AMBIENT_2, &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_DSP5_EXTENDED_OBJECT_1, &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_2, &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = read - (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_3, &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = read - (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_4, &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = (read + (s16)read_tmp) / 2;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_5, &read_tmp);
> + if (ret < 0)
> + return ret;
> + read = read + (s16)read_tmp;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_DSP5_EXTENDED_OBJECT_6, &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;
> +
> + do {
> + ret = mlx90632_perform_measurement(data);
> + if (ret < 0)
> + goto read_unlock;
> + } while ((ret != 19) && tries--);
> +
> + 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)
> {
> @@ -359,9 +513,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;
>
> @@ -433,6 +601,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;
> @@ -480,6 +673,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,
> @@ -653,6 +866,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;
> @@ -672,12 +886,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);
> @@ -689,6 +908,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
>
--
With Best Regards,
Andy Shevchenko
On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <[email protected]> wrote:
>
> On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
> >
> > 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).
>
> I don't see how it prevents using iopoll.h. It uses usleep_range()
> under the hood in the same way you did here, but open coded.
>
One loop is indeed 10ms and that is not the problem, the problem is
that timeout is at least 3 calls of this data ready (3 channels), so
that is at minimum 30ms of timeout, or it could even be 4 in worse
case scenario and that is outside of the range for usleep to measure.
So in case of the other loop, where we wait 200ms for channel refresh
it is also out of scope. Timeout should be in number of tries or in
msleep range if you ask me.
> ...
>
> > - while (tries-- > 0) {
> > + do {
> > ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> > ®_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");
>
> --
> With Best Regards,
> Andy Shevchenko
On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <[email protected]> wrote:
>
> On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <[email protected]> wrote:
> >
> > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
> > >
> > > 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).
> >
> > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > under the hood in the same way you did here, but open coded.
> >
>
> One loop is indeed 10ms and that is not the problem, the problem is
> that timeout is at least 3 calls of this data ready (3 channels), so
> that is at minimum 30ms of timeout, or it could even be 4 in worse
> case scenario and that is outside of the range for usleep to measure.
> So in case of the other loop, where we wait 200ms for channel refresh
> it is also out of scope. Timeout should be in number of tries or in
> msleep range if you ask me.
I still didn't buy it. You have in both cases usleep_range(). Why in
your case it's okay and in regmap_read_poll_timeout() is not?
> > ...
> >
> > > - while (tries-- > 0) {
> > > + do {
> > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> > > ®_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");
--
With Best Regards,
Andy Shevchenko
On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <[email protected]> wrote:
>
> On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <[email protected]> wrote:
> >
> > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <[email protected]> wrote:
> > >
> > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
> > > >
> > > > 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).
> > >
> > > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > > under the hood in the same way you did here, but open coded.
> > >
> >
> > One loop is indeed 10ms and that is not the problem, the problem is
> > that timeout is at least 3 calls of this data ready (3 channels), so
> > that is at minimum 30ms of timeout, or it could even be 4 in worse
> > case scenario and that is outside of the range for usleep to measure.
> > So in case of the other loop, where we wait 200ms for channel refresh
> > it is also out of scope. Timeout should be in number of tries or in
> > msleep range if you ask me.
>
> I still didn't buy it. You have in both cases usleep_range(). Why in
> your case it's okay and in regmap_read_poll_timeout() is not?
>
I tried and it did not work, so then I read the manual. Looking into
* regmap_read_poll_timeout_atomic - Poll until a condition is met or a
timeout occurs
...
* @delay_us: Time to udelay between reads in us (0 tight-loops).
* Should be less than ~10us since udelay is used
* (see Documentation/timers/timers-howto.rst).
* @timeout_us: Timeout in us, 0 means never timeout
So I went to read Documentation/timers/timers-howto.rst
SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
* Use usleep_range
- Why not msleep for (1ms - 20ms)?
Explained originally here:
http://lkml.org/lkml/2007/8/3/250
msleep(1~20) may not do what the caller intends, and
will often sleep longer (~20 ms actual sleep for any
value given in the 1~20ms range). In many cases this
is not the desired behavior.
Since I am above the 20ms range, it is too much for usleep_range and
that proved to be a case as I got -ETIMEOUT and the desired channels
were not read.
> > > ...
> > >
> > > > - while (tries-- > 0) {
> > > > + do {
> > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> > > > ®_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");
>
> --
> With Best Regards,
> Andy Shevchenko
On Thu, 13 Aug 2020 at 13:01, Andy Shevchenko <[email protected]> wrote:
>
> On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
> >
> > There is some inconsistency and whitespace cleanup performed in this
> > patch. It was done on top of my other patches, but I can rebase to head
> > of the togreg branch if it would go in sooner.
>
> ...
>
> > -#define MLX90632_REF_12 12LL /**< ResCtrlRef value of Ch 1 or Ch 2 */
> > -#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_REF_12 12LL /* ResCtrlRef value of Ch 1 or Ch 2 */
> > +#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 */
>
> This was actually in doxy (perhaps kernel doc also understands this)
> format of description.
> Can you double check that the kernel doc is okay / not okay with it?
>
> If it is okay, perhaps it's better to convert others to that format
> rather than dropping it.
>
It is indeed from doxygen and looking at other drivers it should not
be OK. I checked the docs and it does not say in fact. Maybe Jonathan
knows, but he was already OK with these changes in v1.
> --
> With Best Regards,
> Andy Shevchenko
On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <[email protected]> wrote:
> On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <[email protected]> wrote:
> > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <[email protected]> wrote:
> > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <[email protected]> wrote:
> > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
...
> > > > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > > > under the hood in the same way you did here, but open coded.
> > > >
> > >
> > > One loop is indeed 10ms and that is not the problem, the problem is
> > > that timeout is at least 3 calls of this data ready (3 channels), so
> > > that is at minimum 30ms of timeout, or it could even be 4 in worse
> > > case scenario and that is outside of the range for usleep to measure.
> > > So in case of the other loop, where we wait 200ms for channel refresh
> > > it is also out of scope. Timeout should be in number of tries or in
> > > msleep range if you ask me.
> >
> > I still didn't buy it. You have in both cases usleep_range(). Why in
> > your case it's okay and in regmap_read_poll_timeout() is not?
> >
>
> I tried and it did not work, so then I read the manual. Looking into
>
> * regmap_read_poll_timeout_atomic - Poll until a condition is met or a
> timeout occurs
Why _atomic?!
> ...
> * @delay_us: Time to udelay between reads in us (0 tight-loops).
> * Should be less than ~10us since udelay is used
> * (see Documentation/timers/timers-howto.rst).
> * @timeout_us: Timeout in us, 0 means never timeout
>
>
> So I went to read Documentation/timers/timers-howto.rst
>
> SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
> * Use usleep_range
>
> - Why not msleep for (1ms - 20ms)?
> Explained originally here:
> http://lkml.org/lkml/2007/8/3/250
>
> msleep(1~20) may not do what the caller intends, and
> will often sleep longer (~20 ms actual sleep for any
> value given in the 1~20ms range). In many cases this
> is not the desired behavior.
>
> Since I am above the 20ms range, it is too much for usleep_range and
> that proved to be a case as I got -ETIMEOUT and the desired channels
> were not read.
> > > > ...
> > > >
> > > > > - while (tries-- > 0) {
> > > > > + do {
> > > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> > > > > ®_status);
> > > > > if (ret < 0)
> > > > > return ret;
> > > > > - if (reg_status & MLX90632_STAT_DATA_RDY)
> > > > > - break;
> > > > > usleep_range(10000, 11000);
You use here usleep_range(). The same is used for
regmap_read_poll_timeout(). What's the difference?
Since it uses 1/4 of the range you probably need to update tries and
timeout_us to make it work.
> > > > > - }
> > > > > + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--);
> > > > >
> > > > > if (tries < 0) {
> > > > > dev_err(&data->client->dev, "data not ready");
--
With Best Regards,
Andy Shevchenko
On Thu, Aug 13, 2020 at 4:12 PM Crt Mori <[email protected]> wrote:
> On Thu, 13 Aug 2020 at 13:01, Andy Shevchenko <[email protected]> wrote:
> > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
...
> > > -#define MLX90632_REF_12 12LL /**< ResCtrlRef value of Ch 1 or Ch 2 */
> > > -#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_REF_12 12LL /* ResCtrlRef value of Ch 1 or Ch 2 */
> > > +#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 */
> >
> > This was actually in doxy (perhaps kernel doc also understands this)
> > format of description.
> > Can you double check that the kernel doc is okay / not okay with it?
> >
> > If it is okay, perhaps it's better to convert others to that format
> > rather than dropping it.
> >
> It is indeed from doxygen and looking at other drivers it should not
> be OK. I checked the docs and it does not say in fact. Maybe Jonathan
> knows, but he was already OK with these changes in v1.
I'm fine with either choice.
--
With Best Regards,
Andy Shevchenko
On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko <[email protected]> wrote:
>
> On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <[email protected]> wrote:
> > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <[email protected]> wrote:
> > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <[email protected]> wrote:
> > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <[email protected]> wrote:
> > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
>
> ...
>
> > > > > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > > > > under the hood in the same way you did here, but open coded.
> > > > >
> > > >
> > > > One loop is indeed 10ms and that is not the problem, the problem is
> > > > that timeout is at least 3 calls of this data ready (3 channels), so
> > > > that is at minimum 30ms of timeout, or it could even be 4 in worse
> > > > case scenario and that is outside of the range for usleep to measure.
> > > > So in case of the other loop, where we wait 200ms for channel refresh
> > > > it is also out of scope. Timeout should be in number of tries or in
> > > > msleep range if you ask me.
> > >
> > > I still didn't buy it. You have in both cases usleep_range(). Why in
> > > your case it's okay and in regmap_read_poll_timeout() is not?
> > >
> >
> > I tried and it did not work, so then I read the manual. Looking into
> >
> > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a
> > timeout occurs
>
> Why _atomic?!
I just pasted something, it is the same as for non _atomic
>
> > ...
> > * @delay_us: Time to udelay between reads in us (0 tight-loops).
> > * Should be less than ~10us since udelay is used
> > * (see Documentation/timers/timers-howto.rst).
> > * @timeout_us: Timeout in us, 0 means never timeout
> >
> >
> > So I went to read Documentation/timers/timers-howto.rst
> >
> > SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
> > * Use usleep_range
> >
> > - Why not msleep for (1ms - 20ms)?
> > Explained originally here:
> > http://lkml.org/lkml/2007/8/3/250
> >
> > msleep(1~20) may not do what the caller intends, and
> > will often sleep longer (~20 ms actual sleep for any
> > value given in the 1~20ms range). In many cases this
> > is not the desired behavior.
> >
> > Since I am above the 20ms range, it is too much for usleep_range and
> > that proved to be a case as I got -ETIMEOUT and the desired channels
> > were not read.
> > > > > ...
> > > > >
> > > > > > - while (tries-- > 0) {
> > > > > > + do {
> > > > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS,
> > > > > > ®_status);
> > > > > > if (ret < 0)
> > > > > > return ret;
> > > > > > - if (reg_status & MLX90632_STAT_DATA_RDY)
> > > > > > - break;
> > > > > > usleep_range(10000, 11000);
>
> You use here usleep_range(). The same is used for
> regmap_read_poll_timeout(). What's the difference?
>
> Since it uses 1/4 of the range you probably need to update tries and
> timeout_us to make it work.
>
Timeout_us here needs to be in one case 100 * 10ms (maybe not
realistic as we could live with number of around 40 * 10ms), but this
is a lot more than proposed range of usleep which Is up to 20ms. Even
in best case this timeout should be 40 ms to give enough time to
measure 2 channels for sure. So with the current timeout_us
requirement we are outside of the range of the udelay timer and that
is why I would need a macro with number of tries, not with the timeout
value (or timeout value of ms).
> > > > > > - }
> > > > > > + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--);
> > > > > >
> > > > > > if (tries < 0) {
> > > > > > dev_err(&data->client->dev, "data not ready");
>
> --
> With Best Regards,
> Andy Shevchenko
On Fri, Aug 14, 2020 at 10:33 AM Crt Mori <[email protected]> wrote:
> On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko <[email protected]> wrote:
> > On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <[email protected]> wrote:
> > > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <[email protected]> wrote:
> > > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <[email protected]> wrote:
> > > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <[email protected]> wrote:
> > > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
> >
> > ...
> >
> > > > > > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > > > > > under the hood in the same way you did here, but open coded.
> > > > > >
> > > > >
> > > > > One loop is indeed 10ms and that is not the problem, the problem is
> > > > > that timeout is at least 3 calls of this data ready (3 channels), so
> > > > > that is at minimum 30ms of timeout, or it could even be 4 in worse
> > > > > case scenario and that is outside of the range for usleep to measure.
> > > > > So in case of the other loop, where we wait 200ms for channel refresh
> > > > > it is also out of scope. Timeout should be in number of tries or in
> > > > > msleep range if you ask me.
> > > >
> > > > I still didn't buy it. You have in both cases usleep_range(). Why in
> > > > your case it's okay and in regmap_read_poll_timeout() is not?
> > > >
> > >
> > > I tried and it did not work, so then I read the manual. Looking into
> > >
> > > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a
> > > timeout occurs
> >
> > Why _atomic?!
>
> I just pasted something, it is the same as for non _atomic
OK.
...
> > > * @delay_us: Time to udelay between reads in us (0 tight-loops).
> > > * Should be less than ~10us since udelay is used
> > > * (see Documentation/timers/timers-howto.rst).
> > > * @timeout_us: Timeout in us, 0 means never timeout
...
> > > > > > > usleep_range(10000, 11000);
> >
> > You use here usleep_range(). The same is used for
> > regmap_read_poll_timeout(). What's the difference?
> >
> > Since it uses 1/4 of the range you probably need to update tries and
> > timeout_us to make it work.
> >
>
> Timeout_us here needs to be in one case 100 * 10ms (maybe not
> realistic as we could live with number of around 40 * 10ms), but this
> is a lot more than proposed range of usleep which Is up to 20ms. Even
> in best case this timeout should be 40 ms to give enough time to
> measure 2 channels for sure. So with the current timeout_us
> requirement we are outside of the range of the udelay timer and that
> is why I would need a macro with number of tries, not with the timeout
> value (or timeout value of ms).
I do not understand. The regmap_read_poll_timeout() is a macro which
unrolls in the very similar loop you have now in the code.
What prevents it from using it?
I think there is a big misunderstanding about the parameters of that macro.
delay_us (must be small enough), timeout_us can be any long.
--
With Best Regards,
Andy Shevchenko
On Fri, 14 Aug 2020 at 11:32, Andy Shevchenko <[email protected]> wrote:
>
> On Fri, Aug 14, 2020 at 10:33 AM Crt Mori <[email protected]> wrote:
> > On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko <[email protected]> wrote:
> > > On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <[email protected]> wrote:
> > > > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <[email protected]> wrote:
> > > > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <[email protected]> wrote:
> > > > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <[email protected]> wrote:
> > > > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
> > >
> > > ...
> > >
> > > > > > > I don't see how it prevents using iopoll.h. It uses usleep_range()
> > > > > > > under the hood in the same way you did here, but open coded.
> > > > > > >
> > > > > >
> > > > > > One loop is indeed 10ms and that is not the problem, the problem is
> > > > > > that timeout is at least 3 calls of this data ready (3 channels), so
> > > > > > that is at minimum 30ms of timeout, or it could even be 4 in worse
> > > > > > case scenario and that is outside of the range for usleep to measure.
> > > > > > So in case of the other loop, where we wait 200ms for channel refresh
> > > > > > it is also out of scope. Timeout should be in number of tries or in
> > > > > > msleep range if you ask me.
> > > > >
> > > > > I still didn't buy it. You have in both cases usleep_range(). Why in
> > > > > your case it's okay and in regmap_read_poll_timeout() is not?
> > > > >
> > > >
> > > > I tried and it did not work, so then I read the manual. Looking into
> > > >
> > > > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a
> > > > timeout occurs
> > >
> > > Why _atomic?!
> >
> > I just pasted something, it is the same as for non _atomic
>
> OK.
>
> ...
>
> > > > * @delay_us: Time to udelay between reads in us (0 tight-loops).
> > > > * Should be less than ~10us since udelay is used
> > > > * (see Documentation/timers/timers-howto.rst).
> > > > * @timeout_us: Timeout in us, 0 means never timeout
>
> ...
>
> > > > > > > > usleep_range(10000, 11000);
> > >
> > > You use here usleep_range(). The same is used for
> > > regmap_read_poll_timeout(). What's the difference?
> > >
> > > Since it uses 1/4 of the range you probably need to update tries and
> > > timeout_us to make it work.
> > >
> >
> > Timeout_us here needs to be in one case 100 * 10ms (maybe not
> > realistic as we could live with number of around 40 * 10ms), but this
> > is a lot more than proposed range of usleep which Is up to 20ms. Even
> > in best case this timeout should be 40 ms to give enough time to
> > measure 2 channels for sure. So with the current timeout_us
> > requirement we are outside of the range of the udelay timer and that
> > is why I would need a macro with number of tries, not with the timeout
> > value (or timeout value of ms).
>
> I do not understand. The regmap_read_poll_timeout() is a macro which
> unrolls in the very similar loop you have now in the code.
> What prevents it from using it?
>
> I think there is a big misunderstanding about the parameters of that macro.
> delay_us (must be small enough), timeout_us can be any long.
>
I tested on Beaglebone with the 100 * 10000 as timeout_us and I always
got the -ETIMEDOUT error. I also tested in the other case where
delay_us is 250000 and then timeout_us would be 4*250000 and I have
also received -ETIMEDOUT as a response.
I can prepare a patch with the iopoll.h API and maybe you will spot
the mistake, as after rechecking timeout_us is indeed 64bit and is
only used in the time comparison operations and not with timers.
> --
> With Best Regards,
> Andy Shevchenko
On Fri, Aug 14, 2020 at 12:42 PM Crt Mori <[email protected]> wrote:
> On Fri, 14 Aug 2020 at 11:32, Andy Shevchenko <[email protected]> wrote:
> > On Fri, Aug 14, 2020 at 10:33 AM Crt Mori <[email protected]> wrote:
> > > On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko <[email protected]> wrote:
> > > > On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <[email protected]> wrote:
> > > > > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <[email protected]> wrote:
...
> > > > > usleep_range(10000, 11000);
Above for 4 attempts is up to 44000us.
With iopoll.h case we may set
delay_us = 18000 (the result range will be 4500, 18000)
timeout_us = 72000 (this will give up to 16 attempts to read)
> I tested on Beaglebone with the 100 * 10000 as timeout_us and I always
> got the -ETIMEDOUT error. I also tested in the other case where
> delay_us is 250000 and then timeout_us would be 4*250000 and I have
> also received -ETIMEDOUT as a response.
I don't see how delay_us should be bigger than 20ms (20000).
> I can prepare a patch with the iopoll.h API and maybe you will spot
> the mistake, as after rechecking timeout_us is indeed 64bit and is
> only used in the time comparison operations and not with timers.
Perhaps above will clarify.
--
With Best Regards,
Andy Shevchenko
On Thu, 13 Aug 2020 22:41:51 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, Aug 13, 2020 at 4:12 PM Crt Mori <[email protected]> wrote:
> > On Thu, 13 Aug 2020 at 13:01, Andy Shevchenko <[email protected]> wrote:
> > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <[email protected]> wrote:
>
> ...
>
> > > > -#define MLX90632_REF_12 12LL /**< ResCtrlRef value of Ch 1 or Ch 2 */
> > > > -#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_REF_12 12LL /* ResCtrlRef value of Ch 1 or Ch 2 */
> > > > +#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 */
> > >
> > > This was actually in doxy (perhaps kernel doc also understands this)
> > > format of description.
> > > Can you double check that the kernel doc is okay / not okay with it?
> > >
> > > If it is okay, perhaps it's better to convert others to that format
> > > rather than dropping it.
> > >
> > It is indeed from doxygen and looking at other drivers it should not
> > be OK. I checked the docs and it does not say in fact. Maybe Jonathan
> > knows, but he was already OK with these changes in v1.
>
> I'm fine with either choice.
>
I'd prefer to get rid of them as you have done.
Jonathan