Hi Jonathan, I already sent v2 of the driver with some of your comments in. Will fix remainder in v3 - I am meanwhile also interested what responses I will get with including into int_sqrt(). Thanks for comments. Best regards, Crt On 5 December 2017 at 12:23, Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote: > On Sat, 2 Dec 2017 21:30:54 +0100 > Crt Mori <cmo@xxxxxxxxxxx> wrote: > >> Hi Jonathan, >> Definitely did not think there will be so many comments inside, but I >> expected few of them indeed. >> >> See my replies inside. >> >> On 2 December 2017 at 15:33, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: >> > On Wed, 29 Nov 2017 23:07:49 +0100 >> > Crt Mori <cmo@xxxxxxxxxxx> wrote: >> > >> >> Melexis has just released Infra Red temperature sensor MLX90632 used >> >> for contact-less temperature measurement. Driver provides basic >> >> functionality for reporting object (and ambient) temperature with >> >> support for object emissivity. >> >> >> >> Signed-off-by: Crt Mori <cmo@xxxxxxxxxxx> >> > >> > Various comments inline. >> > >> >> --- >> >> MAINTAINERS | 7 + >> >> drivers/iio/temperature/Kconfig | 12 + >> >> drivers/iio/temperature/Makefile | 1 + >> >> drivers/iio/temperature/mlx90632.c | 802 +++++++++++++++++++++++++++++++++++++ >> >> 4 files changed, 822 insertions(+) >> >> create mode 100644 drivers/iio/temperature/mlx90632.c >> >> >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> >> index 2d3d750b19c0..81aec02b08b8 100644 >> >> --- a/MAINTAINERS >> >> +++ b/MAINTAINERS >> >> @@ -8690,6 +8690,13 @@ W: http://www.melexis.com >> >> S: Supported >> >> F: drivers/iio/temperature/mlx90614.c >> >> >> >> +MELEXIS MLX90632 DRIVER >> >> +M: Crt Mori <cmo@xxxxxxxxxxx> >> >> +L: linux-iio@xxxxxxxxxxxxxxx >> >> +W: http://www.melexis.com >> >> +S: Supported >> >> +F: drivers/iio/temperature/mlx90632.c >> >> + >> >> MELFAS MIP4 TOUCHSCREEN DRIVER >> >> M: Sangwon Jee <jeesw@xxxxxxxxxx> >> >> W: http://www.melfas.com >> >> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig >> >> index 5378976d6d27..82e4a62745e2 100644 >> >> --- a/drivers/iio/temperature/Kconfig >> >> +++ b/drivers/iio/temperature/Kconfig >> >> @@ -43,6 +43,18 @@ config MLX90614 >> >> This driver can also be built as a module. If so, the module will >> >> be called mlx90614. >> >> >> >> +config MLX90632 >> >> + tristate "MLX90632 contact-less infrared sensor with medical accuracy" >> >> + depends on I2C >> >> + select REGMAP_I2C >> >> + help >> >> + If you say yes here you get support for the Melexis >> >> + MLX90632 contact-less infrared sensor with medical accuracy >> >> + connected with I2C. >> >> + >> >> + This driver can also be built as a module. If so, the module will >> >> + be called mlx90632. >> >> + >> >> config TMP006 >> >> tristate "TMP006 infrared thermopile sensor" >> >> depends on I2C >> >> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile >> >> index ad1d668de546..44644fe01bc9 100644 >> >> --- a/drivers/iio/temperature/Makefile >> >> +++ b/drivers/iio/temperature/Makefile >> >> @@ -5,6 +5,7 @@ >> >> obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o >> >> obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o >> >> obj-$(CONFIG_MLX90614) += mlx90614.o >> >> +obj-$(CONFIG_MLX90632) += mlx90632.o >> >> obj-$(CONFIG_TMP006) += tmp006.o >> >> obj-$(CONFIG_TMP007) += tmp007.o >> >> obj-$(CONFIG_TSYS01) += tsys01.o >> >> diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c >> >> new file mode 100644 >> >> index 000000000000..05c7d943e504 >> >> --- /dev/null >> >> +++ b/drivers/iio/temperature/mlx90632.c >> >> @@ -0,0 +1,802 @@ >> >> +/* >> >> + * mlx90632.c - Melexis MLX90632 contactless IR temperature sensor >> >> + * >> >> + * Copyright (c) 2017 Melexis <cmo@xxxxxxxxxxx> >> >> + * >> >> + * This file is subject to the terms and conditions of version 2 of >> >> + * the GNU General Public License. See the file COPYING in the main >> >> + * directory of this archive for more details. >> >> + * >> >> + * Driver for the Melexis MLX90632 I2C 16-bit IR thermopile sensor >> >> + */ >> >> +#include <asm/byteorder.h> >> > Don't think this should ever be included in a driver. >> > What do you need it for? >> > >> >> +#include <linux/delay.h> >> >> +#include <linux/err.h> >> >> +#include <linux/gpio/consumer.h> >> >> +#include <linux/i2c.h> >> >> +#include <linux/kernel.h> >> >> +#include <linux/module.h> >> >> +#include <linux/math64.h> >> >> +#include <linux/of.h> >> >> +#include <linux/pm_runtime.h> >> >> +#include <linux/regmap.h> >> >> +#include <linux/unaligned/be_byteshift.h> >> > why? >> > >> >> + >> >> +#include <linux/iio/iio.h> >> >> +#include <linux/iio/sysfs.h> >> >> + >> >> +/* Memory sections addresses */ >> >> +#define MLX90632_ADDR_RAM 0x4000 /* Start address of ram */ >> >> +#define MLX90632_ADDR_EEPROM 0x2480 /* Start address of user eeprom */ >> >> + >> >> +/* EEPROM addresses - used at startup */ >> >> +#define MLX90632_EE_CTRL 0x24d4 /* Control register initial value */ >> >> +#define MLX90632_EE_I2C_ADDR 0x24d5 /* I2C address register initial value */ >> >> +#define MLX90632_EE_VERSION 0x240b /* EEPROM version reg address */ >> >> +#define MLX90632_EE_P_R 0x240c /* P_R calibration register 32bit */ >> >> +#define MLX90632_EE_P_G 0x240e /* P_G calibration register 32bit */ >> >> +#define MLX90632_EE_P_T 0x2410 /* P_T calibration register 32bit */ >> >> +#define MLX90632_EE_P_O 0x2412 /* P_O calibration register 32bit */ >> >> +#define MLX90632_EE_Aa 0x2414 /* Aa calibration register 32bit */ >> >> +#define MLX90632_EE_Ab 0x2416 /* Ab calibration register 32bit */ >> >> +#define MLX90632_EE_Ba 0x2418 /* Ba calibration register 32bit */ >> >> +#define MLX90632_EE_Bb 0x241a /* Bb calibration register 32bit */ >> >> +#define MLX90632_EE_Ca 0x241c /* Ca calibration register 32bit */ >> >> +#define MLX90632_EE_Cb 0x241e /* Cb calibration register 32bit */ >> >> +#define MLX90632_EE_Da 0x2420 /* Da calibration register 32bit */ >> >> +#define MLX90632_EE_Db 0x2422 /* Db calibration register 32bit */ >> >> +#define MLX90632_EE_Ea 0x2424 /* Ea calibration register 32bit */ >> >> +#define MLX90632_EE_Eb 0x2426 /* Eb calibration register 32bit */ >> >> +#define MLX90632_EE_Fa 0x2428 /* Fa calibration register 32bit */ >> >> +#define MLX90632_EE_Fb 0x242a /* Fb calibration register 32bit */ >> >> +#define MLX90632_EE_Ga 0x242c /* Ga calibration register 32bit */ >> >> + >> >> +#define MLX90632_EE_Gb 0x242e /* Gb calibration register 16bit */ >> >> +#define MLX90632_EE_Ka 0x242f /* Ka calibration register 16bit */ >> >> + >> >> +#define MLX90632_EE_Ha 0x2481 /* Ha customer calib value reg 16bit */ >> >> +#define MLX90632_EE_Hb 0x2482 /* Hb customer calib value reg 16bit */ >> >> + >> >> +/* Register addresses - volatile */ >> >> +#define MLX90632_REG_I2C_ADDR 0x3000 /* Chip I2C address register */ >> >> + >> >> +/* Control register address - volatile */ >> >> +#define MLX90632_REG_CONTROL 0x3001 /* Control Register address */ >> >> +#define MLX90632_CFG_PWR_MASK GENMASK(2, 1) /* PowerMode Mask */ >> >> +/* PowerModes statuses */ >> >> +#define MLX90632_PWR_STATUS(ctrl_val) (ctrl_val << 1) >> >> +#define MLX90632_PWR_STATUS_HALT MLX90632_PWR_STATUS(0) /* hold */ >> >> +#define MLX90632_PWR_STATUS_SLEEP_STEP MLX90632_PWR_STATUS(1) /* sleep step*/ >> >> +#define MLX90632_PWR_STATUS_STEP MLX90632_PWR_STATUS(2) /* step */ >> >> +#define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous*/ >> >> + >> >> +/* Device status register - volatile */ >> >> +#define MLX90632_REG_STATUS 0x3fff /* Device status register */ >> >> +#define MLX90632_STAT_BUSY BIT(10) /* Device busy indicator */ >> >> +#define MLX90632_STAT_EE_BUSY BIT(9) /* EEPROM busy indicator */ >> >> +#define MLX90632_STAT_BRST BIT(8) /* Brown out reset indicator */ >> >> +#define MLX90632_STAT_CYCLE_POS GENMASK(6, 2) /* Data position */ >> >> +#define MLX90632_STAT_DATA_RDY BIT(0) /* Data ready indicator */ >> >> + >> >> +/* RAM_MEAS address-es for each channel */ >> >> +#define MLX90632_RAM_1(meas_num) (MLX90632_ADDR_RAM + 3 * meas_num) >> >> +#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) >> >> + >> >> +/* Magic constants */ >> >> +#define MLX90632_EEPROM_VERSION 0xff05 /* EEPROM DSP version for constants */ >> >> +#define MLX90632_ID_MEDICAL 0x01ff /* EEPROM Medical device id */ >> >> +#define MLX90632_ID_CONSUMER 0x02ff /* EEPROM Consumer device id */ >> >> +#define MLX90632_EEPROM_WRITE_KEY 0x554C /* EEPROM write key 0x55 and 0x4c */ >> >> +#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 TENTO3 1000LL >> >> +#define TENTO4 10000LL >> >> +#define TENTO5 100000LL >> >> +#define TENTO6 1000000LL >> >> +#define TENTO7 10000000LL >> >> +#define TENTO10 10000000000LL >> >> +#define TENTO12 1000000000000LL >> > Umm. The numbers describe the constants rather better than the defines ;) >> > Just use the numbers if you need them! >> > >> >> This is way shorter (line length) and more readable in my opinion. >> Also writing 12 times 0 is error prone :D > > Needs care but I really don't like these constants. > >> >> >> + >> >> +struct mlx90632_data { >> >> + struct i2c_client *client; >> >> + struct mutex lock; /* Multiple reads for single measurement */ >> >> + struct regmap *regmap; >> >> + u16 emissivity; >> >> +}; >> >> + >> >> +static const struct regmap_range mlx90632_volatile_reg_range[] = { >> >> + regmap_reg_range(MLX90632_REG_CONTROL, MLX90632_REG_I2C_ADDR), >> >> + regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS), >> >> + regmap_reg_range(MLX90632_RAM_1(0), >> >> + MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)), >> >> +}; >> >> + >> >> +static const struct regmap_access_table mlx90632_volatile_regs_tbl = { >> >> + .yes_ranges = mlx90632_volatile_reg_range, >> >> + .n_yes_ranges = ARRAY_SIZE(mlx90632_volatile_reg_range), >> >> +}; >> >> + >> >> +static const struct regmap_range mlx90632_read_reg_range[] = { >> >> + regmap_reg_range(MLX90632_EE_VERSION, MLX90632_EE_Ka), >> >> + regmap_reg_range(MLX90632_EE_CTRL, MLX90632_EE_I2C_ADDR), >> >> + regmap_reg_range(MLX90632_EE_Ha, MLX90632_EE_Hb), >> >> + regmap_reg_range(MLX90632_REG_CONTROL, MLX90632_REG_I2C_ADDR), >> >> + regmap_reg_range(MLX90632_REG_STATUS, MLX90632_REG_STATUS), >> >> + regmap_reg_range(MLX90632_RAM_1(0), >> >> + MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)), >> >> +}; >> >> + >> >> +static const struct regmap_access_table mlx90632_readable_regs_tbl = { >> >> + .yes_ranges = mlx90632_read_reg_range, >> >> + .n_yes_ranges = ARRAY_SIZE(mlx90632_read_reg_range), >> >> +}; >> >> + >> >> +static const struct regmap_range mlx90632_no_write_reg_range[] = { >> >> + regmap_reg_range(MLX90632_EE_VERSION, MLX90632_EE_Ka), >> >> + regmap_reg_range(MLX90632_RAM_1(0), >> >> + MLX90632_RAM_3(MLX90632_MAX_MEAS_NUM)), >> >> +}; >> >> + >> >> +static const struct regmap_access_table mlx90632_writeable_regs_tbl = { >> >> + .no_ranges = mlx90632_no_write_reg_range, >> >> + .n_no_ranges = ARRAY_SIZE(mlx90632_no_write_reg_range), >> >> +}; >> >> + >> >> +static const struct regmap_config mlx90632_regmap = { >> >> + .reg_bits = 16, >> >> + .val_bits = 16, >> >> + >> >> + .volatile_table = &mlx90632_volatile_regs_tbl, >> >> + .rd_table = &mlx90632_readable_regs_tbl, >> >> + .wr_table = &mlx90632_writeable_regs_tbl, >> >> + >> >> + .use_single_rw = true, >> >> + .reg_format_endian = REGMAP_ENDIAN_BIG, >> >> + .val_format_endian = REGMAP_ENDIAN_BIG, >> >> + .cache_type = REGCACHE_RBTREE, >> >> +}; >> >> + >> >> +static u64 mlx90632_int_sqrt(u64 x) >> >> +{ >> >> + u64 b, m, y = 0; >> >> + >> >> + if (BITS_PER_LONG != 32) >> >> + return int_sqrt(x); >> > >> > needs a comment on why... >> > Ideally propose a standard 64 bit routine if one is needed for >> > 32 bit machines. >> > >> >> Problem is that int_sqrt is not strongly typed. That means 64 bit >> number gets truncated to 32bit on 32bit machine as you mentioned >> and that caused quite many headaches in my testing (worked on >> mobile phone, but not on beaglebone black). This solution is just >> stronger typing of int_sqrt function. >> >> If you think I should put it onto math.h as a patch I can try it. > > Sure, let's see if that goes anywhere. Seems the right approach to me. > >> >> >> + >> >> + if (x <= 1) >> >> + return x; >> >> + >> >> + m = 1ULL << (64 - 2); >> >> + while (m != 0) { >> >> + b = y + m; >> >> + y >>= 1; >> >> + >> >> + if (x >= b) { >> >> + x -= b; >> >> + y += m; >> >> + } >> >> + m >>= 2; >> >> + } >> >> + return y; >> >> +} >> >> + >> >> +static s32 mlx90632_pwr_set_sleep_step(struct regmap *regmap) >> >> +{ >> >> + return regmap_update_bits(regmap, MLX90632_REG_CONTROL, >> >> + MLX90632_CFG_PWR_MASK, >> >> + MLX90632_PWR_STATUS_SLEEP_STEP); >> >> +} >> >> + >> >> +static s32 mlx90632_pwr_set_step(struct regmap *regmap) >> >> +{ >> >> + return regmap_update_bits(regmap, MLX90632_REG_CONTROL, >> >> + MLX90632_CFG_PWR_MASK, >> >> + MLX90632_PWR_STATUS_STEP); >> >> +} >> >> + >> >> +static s32 mlx90632_pwr_continuous(struct regmap *regmap) >> >> +{ >> >> + return regmap_update_bits(regmap, MLX90632_REG_CONTROL, >> >> + MLX90632_CFG_PWR_MASK, >> >> + MLX90632_PWR_STATUS_CONTINUOUS); >> >> +} >> >> + >> >> +static int mlx90632_start_measurement(struct mlx90632_data *data) >> > >> > Looks superficially to me like this is actually waiting for a reading >> > to complete rather than just starting it? If so change the name >> > to reflect that. >> > >> >> ok >> >> >> +{ >> >> + int ret, tries = 100; >> >> + unsigned int reg_status; >> >> + >> >> + ret = regmap_update_bits(data->regmap, MLX90632_REG_STATUS, >> >> + MLX90632_STAT_DATA_RDY, 0); >> >> + if (ret < 0) >> >> + return ret; >> >> + >> >> + while (tries-- > 0) { >> >> + 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); >> >> + } >> >> + >> >> + if (tries < 0) { >> >> + dev_err(&data->client->dev, "data not ready"); >> >> + return -ETIMEDOUT; >> >> + } >> >> + >> >> + return (reg_status & MLX90632_STAT_CYCLE_POS) >> 2; >> > >> > This is a non obvious return value so I would suggest adding >> > some documentation to say what is going on here.. >> > >> >> Yes, it is indeed. >> >> >> +} >> >> + >> >> +static int mlx9032_channel_new_select(int ret, uint8_t *channel_new, >> >> + uint8_t *channel_old) >> > >> > don't use ret as a name for a variable being passed in. Rather >> > confusing! >> > >> >> OK. I think I should just copy setting of channel_old and new in the >> only spot it is used. >> >> >> +{ >> >> + if (ret == 1) { >> >> + *channel_new = 1; >> >> + *channel_old = 2; >> >> + } else if (ret == 2) { >> >> + *channel_new = 2; >> >> + *channel_old = 1; >> >> + } else { >> >> + return ret; >> >> + } >> >> + return 0; >> > Can't get here... >> > >> >> Not true. You get here in every example other than else where channel_new >> and channel_old are successfully set. but I agree there is a bug in case ret=0 >> when it will return 0, but it will not set channels. > > Ah good point, I missed that. I would do it as a switch with direct returns. > Yes, that is already changed to switch in v2. >> >> >> +} >> >> + >> >> +static int mlx90632_read_ambient_raw(struct regmap *regmap, >> >> + s16 *ambient_new_raw, s16 *ambient_old_raw) >> >> +{ >> >> + int ret; >> >> + unsigned int read_tmp; >> >> + >> >> + ret = regmap_read(regmap, MLX90632_RAM_3(1), &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + *ambient_new_raw = (s16)read_tmp; >> >> + >> >> + ret = regmap_read(regmap, MLX90632_RAM_3(2), &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + *ambient_old_raw = (s16)read_tmp; >> >> + >> >> + return ret; >> >> +} >> >> + >> >> +static int mlx90632_read_object_raw(struct regmap *regmap, >> >> + int start_measurement_ret, >> >> + s16 *object_new_raw, s16 *object_old_raw) >> >> +{ >> >> + int ret; >> >> + unsigned int read_tmp; >> >> + s16 read; >> >> + u8 channel = 0; >> >> + u8 channel_old = 0; >> >> + >> >> + ret = mlx90632_channel_new_select(start_measurement_ret, &channel, >> >> + &channel_old); >> >> + if (ret != 0) >> >> + return ret; >> >> + >> >> + ret = regmap_read(regmap, MLX90632_RAM_2(channel), &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + >> >> + read = (s16)read_tmp; >> >> + >> >> + ret = regmap_read(regmap, MLX90632_RAM_1(channel), &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + *object_new_raw = (read + (s16)read_tmp) / 2; >> >> + >> >> + ret = regmap_read(regmap, MLX90632_RAM_2(channel_old), &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + read = (s16)read_tmp; >> >> + >> >> + ret = regmap_read(regmap, MLX90632_RAM_1(channel_old), &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + *object_old_raw = (read + (s16)read_tmp) / 2; >> >> + >> >> + return ret; >> >> +} >> >> + >> >> +static int mlx90632_read_all_channel(struct mlx90632_data *data, >> >> + s16 *ambient_new_raw, s16 *ambient_old_raw, >> >> + s16 *object_new_raw, s16 *object_old_raw) >> >> +{ >> >> + s32 ret, tm_ret; >> >> + >> >> + mutex_lock(&data->lock); >> >> + tm_ret = mlx90632_start_measurement(data); >> >> + if (tm_ret >= 0) { >> >> + ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw, >> >> + ambient_old_raw); >> >> + if (ret >= 0) { >> >> + ret = mlx90632_read_object_raw(data->regmap, tm_ret, >> >> + object_new_raw, >> >> + object_old_raw); >> >> + } >> >> + } else { >> >> + ret = tm_ret; >> >> + } >> > Use something cleaner like >> > mutex_lock(&data->lock); >> > ret = mlx90632_start_measurement(data); >> > if (ret < 0) >> > goto unlock; >> > ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw, >> > ambient_old_raw); >> > if (ret < 0) >> > goto unlock; >> > >> > ret = mlx90632_read_object_raw(data->regmap, tm_ret, >> > object_new_raw, >> > object_old_raw); >> > unlock: >> >> Ok sorry, professional deformation (goto). >> >> >> + mutex_unlock(&data->lock); >> >> + return ret; >> >> +} >> >> + >> >> +static int mlx90632_read_ee_register(struct regmap *regmap, u16 reg_lsb, >> >> + s32 *reg_value) >> >> +{ >> >> + s32 ret; >> >> + unsigned int read; >> >> + __le32 value; >> >> + >> >> + ret = regmap_read(regmap, reg_lsb, &read); >> >> + if (ret < 0) >> >> + return ret; >> >> + >> >> + value = cpu_to_le32(read); >> > >> > Why would you be converting to le32 to then do >> > calculations in here using it at cpu endianness? >> > (guessing you want le32_to_cpu..) >> > >> > hmm isn't any relevant endian conversion wrapped up in regmap >> > anyway? >> > >> >> Regmap reads 16 bit values, but some values are 32 bit. To avoid CPU >> endianess affecting this, I am putting it to known time and shift values. >> This way I ensured it is working on any CPU. le32_to_cpu is in return value. >> > > I'm far from convinced this is a good idea. I would ensure all values > are in cpu endianness before combining them based on the read order > (which is endian independant) > OK, I see the point. Will try it and send v3 (which will be needed anyway). >> >> + >> >> + ret = regmap_read(regmap, reg_lsb + 1, &read); >> >> + if (ret < 0) >> >> + return ret; >> >> + >> >> + value = (cpu_to_le32(read) << 16) | (value & 0xffff); >> >> + >> >> + *reg_value = le32_to_cpu(value); >> >> + return 0; >> >> +} >> >> + >> >> +static s64 mlx90632_preprocess_temp_amb(s16 ambient_new_raw, >> >> + s16 ambient_old_raw, s16 Gb) >> >> +{ >> >> + s64 VR_Ta, kGb, tmp; >> >> + >> >> + kGb = ((s64)Gb * TENTO3) >> 10ULL; >> >> + VR_Ta = (s64)ambient_old_raw * TENTO6 + >> >> + kGb * div64_s64(((s64)ambient_new_raw * TENTO3), >> >> + (MLX90632_REF_3)); >> >> + tmp = div64_s64( >> >> + div64_s64(((s64)ambient_new_raw * TENTO12), >> >> + (MLX90632_REF_3)), VR_Ta); >> >> + return div64_s64(tmp << 19ULL, TENTO3); >> >> +} >> >> + >> >> +static s64 mlx90632_preprocess_temp_obj(s16 object_new_raw, s16 object_old_raw, >> >> + s16 ambient_new_raw, >> >> + s16 ambient_old_raw, s16 Ka) >> >> +{ >> >> + s64 VR_IR, kKa, tmp; >> >> + >> >> + kKa = ((s64)Ka * TENTO3) >> 10ULL; >> >> + VR_IR = (s64)ambient_old_raw * TENTO6 + >> >> + kKa * div64_s64(((s64)ambient_new_raw * TENTO3), >> >> + (MLX90632_REF_3)); >> >> + tmp = div64_s64( >> >> + div64_s64(((s64)((object_new_raw + object_old_raw) / 2) >> >> + * TENTO12), (MLX90632_REF_12)), VR_IR); >> >> + return div64_s64(tmp << 19ULL), TENTO3); >> >> +} >> >> + >> >> +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) >> >> +{ >> >> + s64 Asub, Bsub, Ablock, Bblock, Cblock, AMB, sum; >> >> + >> >> + AMB = mlx90632_preprocess_temp_amb(ambient_new_raw, ambient_old_raw, >> >> + Gb); >> >> + Asub = ((s64)P_T * TENTO10) >> 44ULL; >> >> + Bsub = AMB - (((s64)P_R * TENTO3) >> 8ULL); >> >> + Ablock = Asub * (Bsub * Bsub); >> >> + Bblock = (div64_s64(Bsub * TENTO7, P_G)) << 20ULL; >> >> + Cblock = ((s64)P_O * TENTO10) >> 8ULL; >> >> + >> >> + sum = div64_s64(Ablock, TENTO6) + Bblock + Cblock; >> >> + >> >> + return div64_s64(sum, TENTO7); >> >> +} >> >> + >> >> +static s32 mlx90632_calc_temp_object_iteration(s32 prev_object_temp, s64 object, >> >> + s64 TAdut, s32 Fa, s32 Fb, >> >> + s32 Ga, s16 Ha, s16 Hb, >> >> + u16 emissivity) >> >> +{ >> >> + s64 calcedKsTO, calcedKsTA, ir_Alpha, TAdut4, Alpha_corr; >> >> + s64 Ha_customer, Hb_customer; >> >> + >> >> + Ha_customer = ((s64)Ha * TENTO6) >> 14ULL; >> >> + Hb_customer = ((s64)Hb * 100) >> 10ULL; >> >> + >> >> + calcedKsTO = ((s64)((s64)Ga * (prev_object_temp - 25 * TENTO3) >> >> + * TENTO3)) >> 36LL; >> >> + calcedKsTA = ((s64)(Fb * (TAdut - 25 * TENTO6))) >> 36LL; >> >> + Alpha_corr = div64_s64((((s64)(Fa * TENTO10) >> 46LL) * Ha_customer), >> >> + TENTO3); >> >> + Alpha_corr *= ((s64)(1 * TENTO6 + calcedKsTO + calcedKsTA)); >> >> + Alpha_corr = emissivity * div64_s64(Alpha_corr, TENTO5); >> >> + Alpha_corr = div64_s64(Alpha_corr, TENTO3); >> >> + ir_Alpha = div64_s64((s64)object * TENTO7, Alpha_corr); >> >> + TAdut4 = (div64_s64(TAdut, TENTO4) + 27315) * >> >> + (div64_s64(TAdut, TENTO4) + 27315) * >> >> + (div64_s64(TAdut, TENTO4) + 27315) * >> >> + (div64_s64(TAdut, TENTO4) + 27315); >> >> + >> >> + return (mlx90632_int_sqrt( >> >> + mlx90632_int_sqrt(ir_Alpha * TENTO12 + TAdut4) >> >> + ) - 27315 - Hb_customer) * 10; >> >> +} >> >> + >> >> +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 temp = 25000; >> >> + s8 i; >> >> + >> >> + kTA = (Ea * TENTO3) >> 16LL; >> >> + kTA0 = (Eb * TENTO3) >> 8LL; >> >> + TAdut = div64_s64(((ambient - kTA0) * TENTO6), kTA) + 25 * TENTO6; >> >> + >> >> + for (i = 0; i < 5; ++i) { >> > comment on why iterations are needed would be good here. >> >> This is described in datasheet (the whole calculation). Will add >> explanatory comment. >> >> >> + temp = mlx90632_calc_temp_object_iteration(temp, object, TAdut, >> >> + Fa, Fb, Ga, Ha, Hb, >> >> + tmp_emi); >> >> + } >> >> + return temp; >> >> +} >> >> + >> >> +static int mlx90632_calc_object_dsp105(struct mlx90632_data *data, int *val) >> >> +{ >> >> + s32 ret; >> >> + s32 Ea, Eb, Fa, Fb, Ga; >> >> + unsigned int read_tmp; >> >> + s16 Ha, Hb, Gb, Ka; >> >> + s16 ambient_new_raw, ambient_old_raw, object_new_raw, object_old_raw; >> >> + s64 object, ambient; >> >> + >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_Ea, &Ea); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_Eb, &Eb); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_Fa, &Fa); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_Fb, &Fb); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_Ga, &Ga); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = regmap_read(data->regmap, MLX90632_EE_Ha, &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + Ha = (s16)read_tmp; >> >> + ret = regmap_read(data->regmap, MLX90632_EE_Hb, &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + Hb = (s16)read_tmp; >> >> + ret = regmap_read(data->regmap, MLX90632_EE_Gb, &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + Gb = (s16)read_tmp; >> >> + ret = regmap_read(data->regmap, MLX90632_EE_Ka, &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + Ka = (s16)read_tmp; >> >> + >> >> + ret = mlx90632_read_all_channel(data, >> >> + &ambient_new_raw, &ambient_old_raw, >> >> + &object_new_raw, &object_old_raw); >> >> + if (ret < 0) >> >> + return ret; >> >> + >> >> + ambient = mlx90632_preprocess_temp_amb(ambient_new_raw, >> >> + ambient_old_raw, Gb); >> >> + object = mlx90632_preprocess_temp_obj(object_new_raw, >> >> + object_old_raw, >> >> + ambient_new_raw, >> >> + ambient_old_raw, Ka); >> >> + >> >> + *val = mlx90632_calc_temp_object(object, ambient, Ea, Eb, Fa, Fb, Ga, >> >> + Ha, Hb, data->emissivity); >> >> + return 0; >> >> +} >> >> + >> >> +static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val) >> >> +{ >> >> + s32 ret; >> >> + unsigned int read_tmp; >> >> + s32 PT, PR, PG, PO; >> >> + s16 Gb; >> >> + s16 ambient_new_raw, ambient_old_raw; >> >> + >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_P_R, &PR); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_P_G, &PG); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_P_T, &PT); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = mlx90632_read_ee_register(data->regmap, MLX90632_EE_P_O, &PO); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = regmap_read(data->regmap, MLX90632_EE_Gb, &read_tmp); >> >> + if (ret < 0) >> >> + return ret; >> >> + Gb = (s16)read_tmp; >> >> + >> >> + ret = mlx90632_read_ambient_raw(data->regmap, &ambient_new_raw, >> >> + &ambient_old_raw); >> >> + *val = mlx90632_calc_temp_ambient(ambient_new_raw, ambient_old_raw, >> >> + PT, PR, PG, PO, Gb); >> >> + return ret; >> >> +} >> >> + >> >> +static int mlx90632_read_raw(struct iio_dev *indio_dev, >> >> + struct iio_chan_spec const *channel, int *val, >> >> + int *val2, long mask) >> >> +{ >> >> + struct mlx90632_data *data = iio_priv(indio_dev); >> >> + int ret; >> >> + >> >> + switch (mask) { >> >> + case IIO_CHAN_INFO_PROCESSED: >> >> + switch (channel->channel2) { >> >> + case IIO_MOD_TEMP_AMBIENT: >> >> + ret = mlx90632_calc_ambient_dsp105(data, val); >> >> + if (ret < 0) >> >> + return ret; >> >> + return IIO_VAL_INT; >> >> + case IIO_MOD_TEMP_OBJECT: >> >> + ret = mlx90632_calc_object_dsp105(data, val); >> >> + if (ret < 0) >> >> + return ret; >> >> + return IIO_VAL_INT; >> >> + default: >> >> + return -EINVAL; >> >> + } >> >> + case IIO_CHAN_INFO_CALIBEMISSIVITY: >> >> + if (data->emissivity == 1000) { >> >> + *val = 1; >> >> + *val2 = 0; >> >> + } else { >> >> + *val = 0; >> >> + *val2 = data->emissivity; >> > Odd given you are reporting as int + nano >> > this goes from 1.0 to 0.000000999 in one step? >> > Seems unlikely.. If it is true then a comment >> > is needed. >> > >> > The write is int + micro but this would still be wrong >> > without a factor of 1000. >> > >> >> In input (write) below I am adding factor of 1000. I do not know >> if resolution like that is needed, but you never know what they >> come up with in real life so I rather left some of the breating >> space. > The above still doesn't make much sense unless your range > is disjoint 0-0.000000999, 1.0 which would be surprising and > also doesn't correspond to the write. > I see the error - will fix. Will change in int + micro in the process. >> >> >> + } >> >> + return IIO_VAL_INT_PLUS_NANO; >> >> + >> >> + default: >> >> + return -EINVAL; >> >> + } >> >> +} >> >> + >> >> +static int mlx90632_write_raw(struct iio_dev *indio_dev, >> >> + struct iio_chan_spec const *channel, int val, >> >> + int val2, long mask) >> >> +{ >> >> + struct mlx90632_data *data = iio_priv(indio_dev); >> >> + >> >> + switch (mask) { >> >> + case IIO_CHAN_INFO_CALIBEMISSIVITY: >> >> + if (val < 0 || val2 < 0 || val > 1 || >> >> + (val == 1 && val2 != 0)) >> > >> > I'd add a comment describing what this is doing. I think it >> > is checking for 0-1.0 inclusive? >> > >> >> Yes, exactly. In case you set emissivity outside of range it should >> not change it and return EINVAL. >> >> >> + return -EINVAL; >> >> + data->emissivity = val * 1000 + val2 / 1000; >> >> + return 0; >> >> + default: >> >> + return -EINVAL; >> >> + } >> >> +} >> >> + >> >> +static const struct iio_chan_spec mlx90632_channels[] = { >> >> + { >> >> + .type = IIO_TEMP, >> >> + .modified = 1, >> >> + .channel2 = IIO_MOD_TEMP_AMBIENT, >> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), >> >> + }, >> >> + { >> >> + .type = IIO_TEMP, >> >> + .modified = 1, >> >> + .channel2 = IIO_MOD_TEMP_OBJECT, >> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | >> >> + BIT(IIO_CHAN_INFO_CALIBEMISSIVITY), >> >> + }, >> >> +}; >> >> + >> >> +static const struct iio_info mlx90632_info = { >> >> + .read_raw = mlx90632_read_raw, >> >> + .write_raw = mlx90632_write_raw, >> >> +}; >> >> + >> >> +#ifdef CONFIG_PM >> >> +static int mlx90632_sleep(struct mlx90632_data *data) >> >> +{ >> >> + dev_dbg(&data->client->dev, "Requesting sleep"); >> >> + return mlx90632_pwr_set_sleep_step(data->regmap); >> >> +} >> >> + >> >> +static int mlx90632_wakeup(struct mlx90632_data *data) >> >> +{ >> >> + dev_dbg(&data->client->dev, "Requesting wake-up"); >> >> + return mlx90632_pwr_continuous(data->regmap); >> >> +} >> >> +#endif >> >> + >> >> +static int mlx90632_probe(struct i2c_client *client, >> >> + const struct i2c_device_id *id) >> >> +{ >> >> + struct iio_dev *indio_dev; >> >> + struct mlx90632_data *mlx90632; >> >> + struct regmap *regmap; >> >> + int ret; >> >> + unsigned int read; >> >> + >> >> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*mlx90632)); >> >> + if (!indio_dev) { >> >> + dev_err(&client->dev, "Failed to allocate device"); >> >> + return -ENOMEM; >> >> + } >> >> + >> >> + regmap = devm_regmap_init_i2c(client, &mlx90632_regmap); >> >> + if (IS_ERR(regmap)) { >> >> + ret = PTR_ERR(regmap); >> >> + dev_err(&client->dev, "Failed to allocate regmap: %d\n", ret); >> >> + return ret; >> >> + } >> >> + >> >> + mlx90632 = iio_priv(indio_dev); >> >> + i2c_set_clientdata(client, indio_dev); >> >> + mlx90632->client = client; >> >> + mlx90632->regmap = regmap; >> >> + >> >> + mutex_init(&mlx90632->lock); >> >> + mlx90632_wakeup(mlx90632); >> >> + >> >> + indio_dev->dev.parent = &client->dev; >> >> + indio_dev->name = id->name; >> >> + indio_dev->modes = INDIO_DIRECT_MODE; >> >> + indio_dev->info = &mlx90632_info; >> >> + indio_dev->channels = mlx90632_channels; >> >> + indio_dev->num_channels = ARRAY_SIZE(mlx90632_channels); >> >> + >> >> + ret = regmap_read(mlx90632->regmap, MLX90632_EE_VERSION, &read); >> >> + if (ret < 0) { >> >> + dev_err(&client->dev, "read of version failed: %d\n", ret); >> >> + return ret; >> >> + } >> >> + if (read == (MLX90632_EEPROM_VERSION & MLX90632_ID_MEDICAL)) { >> > >> > This is odd. Why the bitwise and of what look to be two different things entirely? >> > >> >> If you look at how they are set (high vs low bits) you will see they >> are complimentary and >> prepared exactly for bitwise check. ID_MEDICAL might not be best name, but >> EEPROM_ID_MEDICAL was just too long. You read 16 bits and that is why >> lower 8 bits >> are in fact EEPROM_VERSION while higher 8 are ID of calibration. Since >> calculations >> are currently dependent on lower 8 bits, I left some of the wiggle >> space for future. >> In case we add another one in future, but I would rather have a strict >> check for this to >> avoid any kind of confusion what is supported. > > Surely should be | then? > > read == (0xff05 & 0x01ff) > > read == (0x105) > So only 3 bits of overlap? > > Definitely needs a comment to explain this int the code. > Nothing overlaps - thats the point. Just lower 8 bits are EEPROM_VERSION and the higher are ID. Will add a comment. >> >> >> + dev_dbg(&client->dev, >> >> + "Detected Medical EEPROM calibration %x", read); >> >> + } else if (read == (MLX90632_EEPROM_VERSION & MLX90632_ID_CONSUMER)) { >> >> + dev_dbg(&client->dev, >> >> + "Detected Consumer EEPROM calibration %x", read); >> >> + } else { >> >> + dev_err(&client->dev, >> >> + "Chip EEPROM version mismatch %x (expected %x)", >> >> + read, MLX90632_EEPROM_VERSION); >> >> + return -EPROTONOSUPPORT; >> >> + } >> >> + >> >> + mlx90632->emissivity = 1000; >> >> + >> >> + return devm_iio_device_register(&client->dev, indio_dev); >> > >> > Don't use devm version. You are (correctly) manually unwinding >> > this in the remove (as you have pm to deal with after removing the >> > interfaces). This will result in a double free I think... >> > return iio_device_register is the way to go. >> > >> > I'm a bit confused that you don't seem to set up runtime pm anywhere... >> > I would assume we would be looking at autosuspend for a device >> > like this but it isn't enabled.. >> > >> >> OK, will check how to use iio_device_register instead. >> >> I did not do much of PM on device, so I can't really say. That is >> why autosuspend is not enabled. > > If you are dropping the PM for now, then devm_iio_device_register is > fine, just drop the remove function entirely as it won't need to do > anything. In v3 I kept it as it seemed like simple fix, but if it is not really, then I will remove it. >> >> >> +} >> >> + >> >> +static int mlx90632_remove(struct i2c_client *client) >> >> +{ >> >> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >> >> + struct mlx90632_data *data = iio_priv(indio_dev); >> >> + >> >> + iio_device_unregister(indio_dev); >> >> + >> >> + pm_runtime_disable(&client->dev); >> >> + if (!pm_runtime_status_suspended(&client->dev)) >> >> + mlx90632_sleep(data); >> >> + pm_runtime_set_suspended(&client->dev); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static const struct i2c_device_id mlx90632_id[] = { >> >> + { "mlx90632", 0 }, >> >> + { } >> >> +}; >> >> +MODULE_DEVICE_TABLE(i2c, mlx90632_id); >> >> + >> >> +static const struct of_device_id mlx90632_of_match[] = { >> >> + { .compatible = "melexis,mlx90632" }, >> >> + { } >> >> +}; >> >> +MODULE_DEVICE_TABLE(of, mlx90632_of_match); >> >> + >> >> +#ifdef CONFIG_PM_SLEEP >> >> +static int mlx90632_pm_suspend(struct device *dev) >> >> +{ >> >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >> >> + struct mlx90632_data *data = iio_priv(indio_dev); >> >> + >> >> + if (pm_runtime_active(dev)) >> >> + return mlx90632_sleep(data); >> > >> > I'm a little confused as to why, if the device is powered >> > up fully and a suspend comes in we have to do less than we do >> > in runtime pm case. >> > >> > Am I missing something? >> > >> >> I think I have mostly tested runtime pm case, so this might have >> been missed. I did get wrong results if I did not mark regcache as dirty. >> >> I will just remove the whole PM thing to start with and add it up later, OK? > > Makes sense. > >> >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +static int mlx90632_pm_resume(struct device *dev) >> >> +{ >> >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >> >> + struct mlx90632_data *data = iio_priv(indio_dev); >> >> + int err; >> >> + >> >> + err = mlx90632_wakeup(data); >> >> + if (err < 0) >> >> + return err; >> >> + >> >> + pm_runtime_disable(dev); >> >> + pm_runtime_set_active(dev); >> >> + pm_runtime_enable(dev); >> >> + >> >> + return 0; >> >> +} >> >> +#endif >> >> + >> >> +#ifdef CONFIG_PM >> >> +static int mlx90632_pm_runtime_suspend(struct device *dev) >> >> +{ >> >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >> >> + struct mlx90632_data *mlx90632 = iio_priv(indio_dev); >> >> + >> >> + regcache_sync(mlx90632->regmap); >> >> + >> >> + return mlx90632_sleep(mlx90632); >> >> +} >> >> + >> >> +static int mlx90632_pm_runtime_resume(struct device *dev) >> >> +{ >> >> + s32 ret; >> >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >> >> + struct mlx90632_data *mlx90632 = iio_priv(indio_dev); >> >> + >> >> + regcache_mark_dirty(mlx90632->regmap); >> >> + regcache_cache_only(mlx90632->regmap, false); >> >> + ret = regcache_sync(mlx90632->regmap); >> >> + if (ret < 0) { >> >> + dev_err(dev, "Failed to sync regmap registers: %d\n", ret); >> >> + return ret; >> >> + } >> >> + >> >> + return mlx90632_wakeup(mlx90632); >> >> +} >> >> +#endif >> >> + >> >> +static const struct dev_pm_ops mlx90632_pm_ops = { >> >> + SET_SYSTEM_SLEEP_PM_OPS(mlx90632_pm_suspend, mlx90632_pm_resume) >> >> + SET_RUNTIME_PM_OPS(mlx90632_pm_runtime_suspend, >> >> + mlx90632_pm_runtime_resume, NULL) >> >> +}; >> >> + >> >> +static struct i2c_driver mlx90632_driver = { >> >> + .driver = { >> >> + .name = "mlx90632", >> >> + .of_match_table = mlx90632_of_match, >> >> + .pm = &mlx90632_pm_ops, >> >> + }, >> >> + .probe = mlx90632_probe, >> >> + .remove = mlx90632_remove, >> >> + .id_table = mlx90632_id, >> >> +}; >> >> +module_i2c_driver(mlx90632_driver); >> >> + >> >> +MODULE_AUTHOR("Crt Mori <cmo@xxxxxxxxxxx>"); >> >> +MODULE_DESCRIPTION("Melexis MLX90632 contactless Infra Red temperature sensor driver"); >> >> +MODULE_LICENSE("GPL v2"); >> > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> > the body of a message to majordomo@xxxxxxxxxxxxxxx >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
