| Message ID | 20200608204250.3291-7-jmaneyrol@invensense.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: new inv_icm42600 driver | expand |
On 6/8/20 10:42 PM, Jean-Baptiste Maneyrol wrote: > + case IIO_CHAN_INFO_PROCESSED: > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + ret = inv_icm42600_temp_read(st, &temp); > + iio_device_release_direct_mode(indio_dev); > + if (ret) > + return ret; > + *val = temp; > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + *val = 483; > + *val2 = 91787; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_OFFSET: > + *val = 25000; > + return IIO_VAL_INT; If the data is returned processed there is no need to specify scale and offset. But since the transformation to turn the data into standard units is a simple linear transform the preferred way to handle this is to return RAW data and specify scale and offset.
Hello Lars, for the temperature data, the problem is that temperature in the FIFO (used in buffer) is not in the same format than when reading the register. Reading the temperature register return a full precision value on 16 bits. I am using a PROCESSED attribute for it. Temperature data in buffer (coming from the FIFO) is on 8 bits in lower precision. It is reported as raw data, thus the need of the offset and scale values. So offset and scale values are only for transforming the temperature data from the buffer, and direct read is a full precision already processed in m°C. Thanks for the review, JB From: Lars-Peter Clausen <lars@metafoo.de> Sent: Sunday, June 14, 2020 17:10 To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>; jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v3 06/13] iio: imu: inv_icm42600: add temperature sensor support CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe. On 6/8/20 10:42 PM, Jean-Baptiste Maneyrol wrote: > + case IIO_CHAN_INFO_PROCESSED: > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + ret = inv_icm42600_temp_read(st, &temp); > + iio_device_release_direct_mode(indio_dev); > + if (ret) > + return ret; > + *val = temp; > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + *val = 483; > + *val2 = 91787; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_OFFSET: > + *val = 25000; > + return IIO_VAL_INT; If the data is returned processed there is no need to specify scale and offset. But since the transformation to turn the data into standard units is a simple linear transform the preferred way to handle this is to return RAW data and specify scale and offset.
On Sun, 14 Jun 2020 20:35:13 +0000 Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote: > Hello Lars, > > for the temperature data, the problem is that temperature in the FIFO (used in buffer) is not in the same format than when reading the register. > > Reading the temperature register return a full precision value on 16 bits. I am using a PROCESSED attribute for it. > Temperature data in buffer (coming from the FIFO) is on 8 bits in lower precision. It is reported as raw data, thus the need of the offset and scale values. > > So offset and scale values are only for transforming the temperature data from the buffer, and direct read is a full precision already processed in m°C. That is a problem. We have no means of describing it in IIO. If the channel is processed via sysfs the assumption would normally be that it is processed in the buffer as well. We don't really have any means of describing the two separately. In cases where we've seen this before the way around it was to change the data in the fifo so that the scaling was the same as the sysfs channel (usually it's just a bit of padding). Can we do that here? Looks like the ratio is 1:64 so should be possible. > > Thanks for the review, > JB > > From: Lars-Peter Clausen <lars@metafoo.de> > Sent: Sunday, June 14, 2020 17:10 > To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>; jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> > Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v3 06/13] iio: imu: inv_icm42600: add temperature sensor support > > CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe. > > On 6/8/20 10:42 PM, Jean-Baptiste Maneyrol wrote: > > + case IIO_CHAN_INFO_PROCESSED: > > + ret = iio_device_claim_direct_mode(indio_dev); > > + if (ret) > > + return ret; > > + ret = inv_icm42600_temp_read(st, &temp); > > + iio_device_release_direct_mode(indio_dev); > > + if (ret) > > + return ret; > > + *val = temp; > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SCALE: > > + *val = 483; > > + *val2 = 91787; > > + return IIO_VAL_INT_PLUS_MICRO; > > + case IIO_CHAN_INFO_OFFSET: > > + *val = 25000; > > + return IIO_VAL_INT; > > If the data is returned processed there is no need to specify scale and > offset. > > But since the transformation to turn the data into standard units is a > simple linear transform the preferred way to handle this is to return > RAW data and specify scale and offset.
Hi Jonathan, no problem, I can rework temperature to have the same raw output for both cases (with just decreased resolution for the FIFO case). v4 series is coming soon with the fix. Thanks, JB From: Jonathan Cameron <jic23@kernel.org> Sent: Saturday, June 20, 2020 17:57 To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com> Cc: Lars-Peter Clausen <lars@metafoo.de>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v3 06/13] iio: imu: inv_icm42600: add temperature sensor support CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe. On Sun, 14 Jun 2020 20:35:13 +0000 Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote: > Hello Lars, > > for the temperature data, the problem is that temperature in the FIFO (used in buffer) is not in the same format than when reading the register. > > Reading the temperature register return a full precision value on 16 bits. I am using a PROCESSED attribute for it. > Temperature data in buffer (coming from the FIFO) is on 8 bits in lower precision. It is reported as raw data, thus the need of the offset and scale values. > > So offset and scale values are only for transforming the temperature data from the buffer, and direct read is a full precision already processed in m°C. That is a problem. We have no means of describing it in IIO. If the channel is processed via sysfs the assumption would normally be that it is processed in the buffer as well. We don't really have any means of describing the two separately. In cases where we've seen this before the way around it was to change the data in the fifo so that the scaling was the same as the sysfs channel (usually it's just a bit of padding). Can we do that here? Looks like the ratio is 1:64 so should be possible. > > Thanks for the review, > JB > > From: Lars-Peter Clausen <lars@metafoo.de> > Sent: Sunday, June 14, 2020 17:10 > To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>; jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> > Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v3 06/13] iio: imu: inv_icm42600: add temperature sensor support > > CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe. > > On 6/8/20 10:42 PM, Jean-Baptiste Maneyrol wrote: > > + case IIO_CHAN_INFO_PROCESSED: > > + ret = iio_device_claim_direct_mode(indio_dev); > > + if (ret) > > + return ret; > > + ret = inv_icm42600_temp_read(st, &temp); > > + iio_device_release_direct_mode(indio_dev); > > + if (ret) > > + return ret; > > + *val = temp; > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SCALE: > > + *val = 483; > > + *val2 = 91787; > > + return IIO_VAL_INT_PLUS_MICRO; > > + case IIO_CHAN_INFO_OFFSET: > > + *val = 25000; > > + return IIO_VAL_INT; > > If the data is returned processed there is no need to specify scale and > offset. > > But since the transformation to turn the data into standard units is a > simple linear transform the preferred way to handle this is to return > RAW data and specify scale and offset.
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c index 717c6b0869fc..3f214df44093 100644 --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c @@ -13,6 +13,7 @@ #include <linux/iio/iio.h> #include "inv_icm42600.h" +#include "inv_icm42600_temp.h" #define INV_ICM42600_ACCEL_CHAN(_modifier, _index, _ext_info) \ { \ @@ -45,6 +46,7 @@ enum inv_icm42600_accel_scan { INV_ICM42600_ACCEL_SCAN_X, INV_ICM42600_ACCEL_SCAN_Y, INV_ICM42600_ACCEL_SCAN_Z, + INV_ICM42600_ACCEL_SCAN_TEMP, }; static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = { @@ -59,6 +61,7 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = { inv_icm42600_accel_ext_infos), INV_ICM42600_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42600_ACCEL_SCAN_Z, inv_icm42600_accel_ext_infos), + INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP), }; static int inv_icm42600_accel_read_sensor(struct inv_icm42600_state *st, @@ -450,8 +453,14 @@ static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev, int16_t data; int ret; - if (chan->type != IIO_ACCEL) + switch (chan->type) { + case IIO_ACCEL: + break; + case IIO_TEMP: + return inv_icm42600_temp_read_raw(indio_dev, chan, val, val2, mask); + default: return -EINVAL; + } switch (mask) { case IIO_CHAN_INFO_RAW: diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c index 3875ecbee67e..6a0e7661fa48 100644 --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c @@ -13,6 +13,7 @@ #include <linux/iio/iio.h> #include "inv_icm42600.h" +#include "inv_icm42600_temp.h" #define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info) \ { \ @@ -45,6 +46,7 @@ enum inv_icm42600_gyro_scan { INV_ICM42600_GYRO_SCAN_X, INV_ICM42600_GYRO_SCAN_Y, INV_ICM42600_GYRO_SCAN_Z, + INV_ICM42600_GYRO_SCAN_TEMP, }; static const struct iio_chan_spec_ext_info inv_icm42600_gyro_ext_infos[] = { @@ -59,6 +61,7 @@ static const struct iio_chan_spec inv_icm42600_gyro_channels[] = { inv_icm42600_gyro_ext_infos), INV_ICM42600_GYRO_CHAN(IIO_MOD_Z, INV_ICM42600_GYRO_SCAN_Z, inv_icm42600_gyro_ext_infos), + INV_ICM42600_TEMP_CHAN(INV_ICM42600_GYRO_SCAN_TEMP), }; static int inv_icm42600_gyro_read_sensor(struct inv_icm42600_state *st, @@ -461,8 +464,14 @@ static int inv_icm42600_gyro_read_raw(struct iio_dev *indio_dev, int16_t data; int ret; - if (chan->type != IIO_ANGL_VEL) + switch (chan->type) { + case IIO_ANGL_VEL: + break; + case IIO_TEMP: + return inv_icm42600_temp_read_raw(indio_dev, chan, val, val2, mask); + default: return -EINVAL; + } switch (mask) { case IIO_CHAN_INFO_RAW: diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.c new file mode 100644 index 000000000000..b0871352fe39 --- /dev/null +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 Invensense, Inc. + */ + +#include <linux/kernel.h> +#include <linux/device.h> +#include <linux/mutex.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/math64.h> +#include <linux/iio/iio.h> + +#include "inv_icm42600.h" +#include "inv_icm42600_temp.h" + +static int inv_icm42600_temp_read(struct inv_icm42600_state *st, int32_t *temp) +{ + struct device *dev = regmap_get_device(st->map); + int64_t data; + __be16 *raw; + int16_t val; + int ret; + + pm_runtime_get_sync(dev); + mutex_lock(&st->lock); + + ret = inv_icm42600_set_temp_conf(st, true, NULL); + if (ret) + goto exit; + + raw = (__be16 *)&st->buffer[0]; + ret = regmap_bulk_read(st->map, INV_ICM42600_REG_TEMP_DATA, raw, sizeof(*raw)); + if (ret) + goto exit; + + val = (int16_t)be16_to_cpup(raw); + if (val == INV_ICM42600_DATA_INVALID) { + ret = -EINVAL; + goto exit; + } + /* + * T°C = (val / 132.48) + 25 = ((val * 100) / 13248) + 25 + * Tm°C = (val * 100 * 1000) / 13248 + 25000 + */ + data = (int64_t)(val) * 100LL * 1000LL; + *temp = div_s64(data, 13248) + 25000; +exit: + mutex_unlock(&st->lock); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + return ret; +} + +int inv_icm42600_temp_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev); + int32_t temp; + int ret; + + if (chan->type != IIO_TEMP) + return -EINVAL; + + switch (mask) { + case IIO_CHAN_INFO_PROCESSED: + ret = iio_device_claim_direct_mode(indio_dev); + if (ret) + return ret; + ret = inv_icm42600_temp_read(st, &temp); + iio_device_release_direct_mode(indio_dev); + if (ret) + return ret; + *val = temp; + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + *val = 483; + *val2 = 91787; + return IIO_VAL_INT_PLUS_MICRO; + case IIO_CHAN_INFO_OFFSET: + *val = 25000; + return IIO_VAL_INT; + default: + return -EINVAL; + } +} diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.h b/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.h new file mode 100644 index 000000000000..2c3a932faa94 --- /dev/null +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.h @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2020 Invensense, Inc. + */ + +#ifndef INV_ICM42600_TEMP_H_ +#define INV_ICM42600_TEMP_H_ + +#include <linux/iio/iio.h> + +#define INV_ICM42600_TEMP_CHAN(_index) \ + { \ + .type = IIO_TEMP, \ + .info_mask_separate = \ + BIT(IIO_CHAN_INFO_PROCESSED) | \ + BIT(IIO_CHAN_INFO_OFFSET) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .scan_index = _index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 8, \ + .storagebits = 8, \ + }, \ + } + +int inv_icm42600_temp_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask); + +#endif
Add temperature channel in gyroscope and accelerometer devices. Temperature is available in full 16 bits resolution as a processed channel. Scale and offset attributes are also provided for the low 8 bits resolution raw temperature found in the FIFO. Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> --- .../iio/imu/inv_icm42600/inv_icm42600_accel.c | 11 ++- .../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 11 ++- .../iio/imu/inv_icm42600/inv_icm42600_temp.c | 87 +++++++++++++++++++ .../iio/imu/inv_icm42600/inv_icm42600_temp.h | 30 +++++++ 4 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_temp.c create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_temp.h