On Sun, 10 Dec 2017 19:35:03 +0000 Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On Mon, 4 Dec 2017 16:31:32 +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> > > Comments inline. Some of which were not fixed from V1. > > Also note this won't compile as the sqrt patch is after it. Doh and I didn't review latest version... Jonathan > > Jonathan > > > --- > > MAINTAINERS | 7 + > > drivers/iio/temperature/Kconfig | 12 + > > drivers/iio/temperature/Makefile | 1 + > > drivers/iio/temperature/mlx90632.c | 793 +++++++++++++++++++++++++++++++++++++ > > 4 files changed, 813 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..3bf3572bb81b > > --- /dev/null > > +++ b/drivers/iio/temperature/mlx90632.c > > @@ -0,0 +1,793 @@ > > +/* > > + * 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 <linux/delay.h> > > +#include <linux/err.h> > > +#include <linux/gpio/consumer.h> > Why? > > > +#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/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 */ > Long comments that only just fit on the line. Better just to put > them on the line above in all cases. > > +#define MLX90632_ID_MEDICAL 0x01ff /* EEPROM Medical device id */ > > +#define MLX90632_ID_CONSUMER 0x02ff /* EEPROM Consumer device id */ > > So to go back to the very odd code below. What you are checking is that > the register contains: > 0xff05 & 0x1ff = 0x105 vs 0xff05 & 0x2ff = 0x205. > That means from the kernel point of view that the IDs > are effectively 0x105 and 0x205 as that is what we can see.. > > If you want to leave them in this odd form you must explain which part is > visible in the eeprom and give some justification for it. > Right now it just looks like a bug as I raised the first time I reviewed this. > > Note that the datasheet I found doesn't seem to have any information on > this register at all so I am only guessing at what was intended. > > > > +#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 MLX90632_MAX_MEAS_NUM 31 /**< Maximum measurements in list */ > > + > > +#define TENTO3 1000LL > > +#define TENTO4 10000LL > > +#define TENTO5 100000LL > > +#define TENTO6 1000000LL > > +#define TENTO7 10000000LL > > +#define TENTO10 10000000000LL > > +#define TENTO12 1000000000000LL > > I still really really don't like these... From my point of view > they make the code harder to read. We only have to check they > were right once so I don't think the chance of bug increases much > by just using the numbers inline. > You could if you really want to do this at least use a table > lookup and have > pow_10[] = { 1, 10, 100, 1000, ..) > > Then we can have pow_10[3] inline which isn't quite as > horrible. (still pretty horrible though) > > > + > > +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 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_continuous(struct regmap *regmap) > > +{ > > + return regmap_update_bits(regmap, MLX90632_REG_CONTROL, > > + MLX90632_CFG_PWR_MASK, > > + MLX90632_PWR_STATUS_CONTINUOUS); > > +} > > + > > +/** > > + * mlx90632_perform_measurement - Trigger and retrieve current measurement cycle > > + * @*data: pointer to mlx90632_data object containing regmap information > > + * > > + * Perform a measurement and return latest measurement cycle position reported > > + * by sensor. This is a blocking function for 500ms, as that is default sensor > > + * refresh rate. > > + */ > > +static int mlx90632_perform_measurement(struct mlx90632_data *data) > > +{ > > + 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; > > +} > > + > > +static int mlx90632_channel_new_select(int perform_ret, uint8_t *channel_new, > > + uint8_t *channel_old) > > +{ > > + switch (perform_ret) { > > + case 1: > > + *channel_new = 1; > > + *channel_old = 2; > > + break; > > + case 2: > > + *channel_new = 2; > > + *channel_old = 1; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +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 perform_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(perform_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, measurement; > > + > > + mutex_lock(&data->lock); > > + measurement = mlx90632_perform_measurement(data); > > + if (measurement < 0) { > > + ret = measurement; > > + goto read_unlock; > > + } > > + ret = mlx90632_read_ambient_raw(data->regmap, ambient_new_raw, > > + ambient_old_raw); > > + if (ret < 0) > > + goto read_unlock; > > + > > + ret = mlx90632_read_object_raw(data->regmap, measurement, > > + object_new_raw, object_old_raw); > > +read_unlock: > > + 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); > > + > > + ret = regmap_read(regmap, reg_lsb + 1, &read); > > + if (ret < 0) > > + return ret; > > + > > + value = (cpu_to_le32(read) << 16) | (value & 0xffff); > No. This is horrible as I stated before. You have two 16 bit > values and you know which is the high byte and which the low. > You can simply combine them in cpu endiannenss. > *reg_value = (highword) << 16 | lowword; > > This works what ever your endiannes.. > > + > > + *reg_value = le32_to_cpu(value); > blank line here please. > > > + 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); > Odd alignment... > > > + 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 (int_sqrt64(int_sqrt64(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; > > + > > + /* Iterations of calculation as described in datasheet */ > > + for (i = 0; i < 5; ++i) { > > + 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; > > + } > > + return IIO_VAL_INT_PLUS_NANO; > > So emmissivty = 900 translates to val2 = 900. > With IIO_VAL_INTO_PLUS_NANO this is > 0.000000900 which is not what you want. > I raised this in v1. > > > + > > + 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)) > > + 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 > > General kernel thing these days is not to do this for > PM as the config options are a mess, but instead > mark them as __maybe_unused and let the compiler > drop them if they are unused. > > > +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)) { > > These still make no sense - you are taking the bitwise and of > two values that only overlap by a couple of bits. Unless this > register is some weird hash of the two this looks to be wrong. > > If this is really the case a comment explaining how these two > values are combined and why it makes any sense at all is needed. > You have a mask that mostly doesn't seem to overlap with the thing > being masked. > > > + 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 iio_device_register(indio_dev); > > +} > > + > > +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); > > + > > + regcache_sync(data->regmap); > > + > > + if (pm_runtime_active(dev)) > > + return mlx90632_sleep(data); > > + > > + 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; > > + > > + regcache_mark_dirty(data->regmap); > > + regcache_cache_only(data->regmap, false); > > + err = regcache_sync(data->regmap); > > + if (err < 0) { > > + dev_err(dev, "Failed to sync regmap registers: %d\n", err); > > + return err; > > + } > > + > > + err = mlx90632_wakeup(data); > > + if (err < 0) > > + return err; > > + > > + pm_runtime_disable(dev); > I'd like to see a comment on this 'dance'. Given you have > just done a pm resume I don't immediately see why you need > a set_active... > > > + 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); > This all seems a little odd in the resume path. We never put the device > into cache_only that I can see so why are we coming out of it? > > > + 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
