Re: [PATCH v2] iio: imu: mpu6050: Fix FIFO layout for ICM20602

From: Jonathan Cameron
Date: Sun Apr 14 2019 - 06:46:44 EST

Next message: Masami Hiramatsu: "Kconfig dependency issue on function-graph tracer and frame pointer on arm"
Previous message: Jonathan Cameron: "Re: [PATCH v2 4/4] power: supply: add Ingenic JZ47xx battery driver."
In reply to: Jean-Baptiste Maneyrol: "Re: [PATCH v2] iio: imu: mpu6050: Fix FIFO layout for ICM20602"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, 8 Apr 2019 14:32:00 +0000
Jean-Baptiste Maneyrol <JManeyrol@xxxxxxxxxxxxxx> wrote:

> Hello,
>
> overall looks good for me.
>
> I would just prefer to change the define name for temperature to INV_ICM20602_SCAN_TEMP. It is the chip temperature that can be used for temperature compensation for both accel and gyro data.
>
> But it is really just a details.
>
> Best regards,
> Jean-Baptiste Maneyrol

Rather than go around again, I've made the change and applied the
patch to the fixes-togreg branch of iio.git. I also added a
fixes tag.

Thanks,

Jonathan

>
>
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, April 7, 2019 13:45
> To: stevemo@xxxxxxxxxx
> Cc: Jean-Baptiste Maneyrol; Hartmut Knaack; Lars-Peter Clausen; Peter Meerwald-Stadler; Martin Kelly; Jonathan Marek; Brian Masney; Randolph MaaÃen; Douglas Fischer; linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] iio: imu: mpu6050: Fix FIFO layout for ICM20602
> Â
> Â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 Tue,Â 2 Apr 2019 23:28:56 -0700
> stevemo@xxxxxxxxxx wrote:
>
> > From: Steve Moskovchenko <stevemo@xxxxxxxxxx>
> >
> > The MPU6050 driver has recently gained support for the
> > ICM20602 IMU, which is very similar to MPU6xxx. However,
> > the ICM20602's FIFO data specifically includes temperature
> > readings, which were not present on MPU6xxx parts. As a
> > result, the driver will under-read the ICM20602's FIFO
> > register, causing the same (partial) sample to be returned
> > for all reads, until the FIFO overflows.
> >
> > Fix this by adding a table of scan elements specifically
> > for the ICM20602, which takes the extra temperature data
> > into consideration.
> >
> > While we're at it, fix the temperature offset and scaling
> > on ICM20602, since it uses different scale/offset constants
> > than the rest of the MPU6xxx devices.
> >
> > Signed-off-by: Steve Moskovchenko <stevemo@xxxxxxxxxx>
> I'd like a reviewed-by or acked-by from Jean-Baptiste on this before
> I take it.
>
> thanks,
>
> Jonathan
>
> > ---
> > v2: Read temperature when running in accel-only mode, too.
> >
> >Â drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 46 ++++++++++++++++++++--
> >Â drivers/iio/imu/inv_mpu6050/inv_mpu_iio.hÂ | 20 +++++++++-
> >Â drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c |Â 3 ++
> >Â 3 files changed, 64 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index 650de0fefb7b..fedd3f2b0135 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > @@ -471,7 +471,10 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev,
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return IIO_VAL_INT_PLUS_MICRO;
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ case IIO_TEMP:
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *val = 0;
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *val2 = INV_MPU6050_TEMP_SCALE;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (st->chip_type == INV_ICM20602)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *val2 = INV_ICM20602_TEMP_SCALE;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ else
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *val2 = INV_MPU6050_TEMP_SCALE;
> >Â
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return IIO_VAL_INT_PLUS_MICRO;
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ default:
> > @@ -480,7 +483,10 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev,
> >ÂÂÂÂÂÂÂ case IIO_CHAN_INFO_OFFSET:
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ switch (chan->type) {
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ case IIO_TEMP:
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *val = INV_MPU6050_TEMP_OFFSET;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (st->chip_type == INV_ICM20602)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *val = INV_ICM20602_TEMP_OFFSET;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ else
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *val = INV_MPU6050_TEMP_OFFSET;
> >Â
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return IIO_VAL_INT;
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ default:
> > @@ -845,6 +851,32 @@ static const struct iio_chan_spec inv_mpu_channels[] = {
> >ÂÂÂÂÂÂÂ INV_MPU6050_CHAN(IIO_ACCEL, IIO_MOD_Z, INV_MPU6050_SCAN_ACCL_Z),
> >Â };
> >Â
> > +static const struct iio_chan_spec inv_icm20602_channels[] = {
> > +ÂÂÂÂ IIO_CHAN_SOFT_TIMESTAMP(INV_ICM20602_SCAN_TIMESTAMP),
> > +ÂÂÂÂ {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ .type = IIO_TEMP,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | BIT(IIO_CHAN_INFO_OFFSET)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | BIT(IIO_CHAN_INFO_SCALE),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ .scan_index = INV_ICM20602_SCAN_GYRO_TEMP,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ .scan_type = {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ .sign = 's',
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ .realbits = 16,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ .storagebits = 16,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ .shift = 0,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ .endianness = IIO_BE,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ },
> > +ÂÂÂÂ },
> > +
> > +ÂÂÂÂ INV_MPU6050_CHAN(IIO_ANGL_VEL, IIO_MOD_X, INV_ICM20602_SCAN_GYRO_X),
> > +ÂÂÂÂ INV_MPU6050_CHAN(IIO_ANGL_VEL, IIO_MOD_Y, INV_ICM20602_SCAN_GYRO_Y),
> > +ÂÂÂÂ INV_MPU6050_CHAN(IIO_ANGL_VEL, IIO_MOD_Z, INV_ICM20602_SCAN_GYRO_Z),
> > +
> > +ÂÂÂÂ INV_MPU6050_CHAN(IIO_ACCEL, IIO_MOD_Y, INV_ICM20602_SCAN_ACCL_Y),
> > +ÂÂÂÂ INV_MPU6050_CHAN(IIO_ACCEL, IIO_MOD_X, INV_ICM20602_SCAN_ACCL_X),
> > +ÂÂÂÂ INV_MPU6050_CHAN(IIO_ACCEL, IIO_MOD_Z, INV_ICM20602_SCAN_ACCL_Z),
> > +};
> > +
> >Â /*
> >ÂÂ * The user can choose any frequency between INV_MPU6050_MIN_FIFO_RATE and
> >ÂÂ * INV_MPU6050_MAX_FIFO_RATE, but only these frequencies are matched by the
> > @@ -1100,8 +1132,14 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ indio_dev->name = name;
> >ÂÂÂÂÂÂÂ else
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ indio_dev->name = dev_name(dev);
> > -ÂÂÂÂ indio_dev->channels = inv_mpu_channels;
> > -ÂÂÂÂ indio_dev->num_channels = ARRAY_SIZE(inv_mpu_channels);
> > +
> > +ÂÂÂÂ if (chip_type == INV_ICM20602) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ indio_dev->channels = inv_icm20602_channels;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ indio_dev->num_channels = ARRAY_SIZE(inv_icm20602_channels);
> > +ÂÂÂÂ } else {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ indio_dev->channels = inv_mpu_channels;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ indio_dev->num_channels = ARRAY_SIZE(inv_mpu_channels);
> > +ÂÂÂÂ }
> >Â
> >ÂÂÂÂÂÂÂ indio_dev->info = &mpu_info;
> >ÂÂÂÂÂÂÂ indio_dev->modes = INDIO_BUFFER_TRIGGERED;
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > index 325afd9f5f61..2ed4b98e0cd7 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > @@ -208,6 +208,9 @@ struct inv_mpu6050_state {
> >Â #define INV_MPU6050_BYTES_PER_3AXIS_SENSORÂÂ 6
> >Â #define INV_MPU6050_FIFO_COUNT_BYTEÂÂÂÂÂÂÂÂÂ 2
> >Â
> > +/* ICM20602 FIFO samples include temperature readings */
> > +#define INV_ICM20602_BYTES_PER_TEMP_SENSORÂÂ 2
> > +
> >Â /* mpu6500 registers */
> >Â #define INV_MPU6500_REG_ACCEL_CONFIG_2ÂÂÂÂÂ 0x1D
> >Â #define INV_MPU6500_REG_ACCEL_OFFSETÂÂÂÂÂÂÂ 0x77
> > @@ -229,6 +232,9 @@ struct inv_mpu6050_state {
> >Â #define INV_MPU6050_GYRO_CONFIG_FSR_SHIFTÂÂÂ 3
> >Â #define INV_MPU6050_ACCL_CONFIG_FSR_SHIFTÂÂÂ 3
> >Â
> > +#define INV_ICM20602_TEMP_OFFSETÂÂÂÂÂÂÂÂÂ 8170
> > +#define INV_ICM20602_TEMP_SCALEÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 3060
> > +
> >Â /* 6 + 6 round up and plus 8 */
> >Â #define INV_MPU6050_OUTPUT_DATA_SIZEÂÂÂÂÂÂÂÂ 24
> >Â
> > @@ -270,7 +276,7 @@ struct inv_mpu6050_state {
> >Â #define INV_ICM20608_WHOAMI_VALUEÂÂÂÂÂÂÂÂÂÂÂ 0xAF
> >Â #define INV_ICM20602_WHOAMI_VALUEÂÂÂÂÂÂÂÂÂÂÂ 0x12
> >Â
> > -/* scan element definition */
> > +/* scan element definition for generic MPU6xxx devices */
> >Â enum inv_mpu6050_scan {
> >ÂÂÂÂÂÂÂ INV_MPU6050_SCAN_ACCL_X,
> >ÂÂÂÂÂÂÂ INV_MPU6050_SCAN_ACCL_Y,
> > @@ -281,6 +287,18 @@ enum inv_mpu6050_scan {
> >ÂÂÂÂÂÂÂ INV_MPU6050_SCAN_TIMESTAMP,
> >Â };
> >Â
> > +/* scan element definition for ICM20602, which includes temperature */
> > +enum inv_icm20602_scan {
> > +ÂÂÂÂ INV_ICM20602_SCAN_ACCL_X,
> > +ÂÂÂÂ INV_ICM20602_SCAN_ACCL_Y,
> > +ÂÂÂÂ INV_ICM20602_SCAN_ACCL_Z,
> > +ÂÂÂÂ INV_ICM20602_SCAN_GYRO_TEMP,
> > +ÂÂÂÂ INV_ICM20602_SCAN_GYRO_X,
> > +ÂÂÂÂ INV_ICM20602_SCAN_GYRO_Y,
> > +ÂÂÂÂ INV_ICM20602_SCAN_GYRO_Z,
> > +ÂÂÂÂ INV_ICM20602_SCAN_TIMESTAMP,
> > +};
> > +
> >Â enum inv_mpu6050_filter_e {
> >ÂÂÂÂÂÂÂ INV_MPU6050_FILTER_256HZ_NOLPF2 = 0,
> >ÂÂÂÂÂÂÂ INV_MPU6050_FILTER_188HZ,
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> > index 548e042f7b5b..57bd11bde56b 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> > @@ -207,6 +207,9 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> >ÂÂÂÂÂÂÂ if (st->chip_config.gyro_fifo_enable)
> >ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bytes_per_datum += INV_MPU6050_BYTES_PER_3AXIS_SENSOR;
> >Â
> > +ÂÂÂÂ if (st->chip_type == INV_ICM20602)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ bytes_per_datum += INV_ICM20602_BYTES_PER_TEMP_SENSOR;
> > +
> >ÂÂÂÂÂÂÂ /*
> >ÂÂÂÂÂÂÂÂ * read fifo_count register to know how many bytes are inside the FIFO
> >ÂÂÂÂÂÂÂÂ * right now
>

Next message: Masami Hiramatsu: "Kconfig dependency issue on function-graph tracer and frame pointer on arm"
Previous message: Jonathan Cameron: "Re: [PATCH v2 4/4] power: supply: add Ingenic JZ47xx battery driver."
In reply to: Jean-Baptiste Maneyrol: "Re: [PATCH v2] iio: imu: mpu6050: Fix FIFO layout for ICM20602"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]