Message ID | b4ba111a1e33f60ed4d3d21421c3250243d40fba.1463582011.git.leonard.crestez@intel.com |
---|---|
State | Not Applicable |
Headers | show |
On 18/05/16 16:00, Crestez Dan Leonard wrote: > Using regmap_read_bulk is wrong because it assumes that a range of > registers is being read. In our case reading from the fifo register will > return multiple values but this is *not* auto-increment. > > This currently works by accident. > > Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com> I'd really prefer to see this in the regmap core. It really doesn't look like it will be hard to do. Basically it's just an cut and paste job from regmap_bulk_read. 1. Check the register in question is volatile - anything else would be bonkers. 2. The emulation of bulk reads relies on volatile or no caching anyway. However, that will probably take a while, so I suppose we might want to take this in the meantime as a fix - be it not a terribly urgent one as you observe it works by accident at the moment. Clearly it will be needed for your regcache support though. > --- > drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 33 ++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > index d070062..8455af0 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > @@ -21,6 +21,7 @@ > #include <linux/interrupt.h> > #include <linux/kfifo.h> > #include <linux/poll.h> > +#include <linux/spi/spi.h> > #include "inv_mpu_iio.h" > > static void inv_clear_kfifo(struct inv_mpu6050_state *st) > @@ -128,6 +129,13 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > u16 fifo_count; > s64 timestamp; > > + struct device *regmap_dev = regmap_get_device(st->map); > + struct i2c_client *i2c; > + struct spi_device *spi = NULL; > + > + i2c = i2c_verify_client(regmap_dev); > + spi = i2c ? NULL: to_spi_device(regmap_dev); > + > mutex_lock(&indio_dev->mlock); > if (!(st->chip_config.accl_fifo_enable | > st->chip_config.gyro_fifo_enable)) > @@ -160,10 +168,27 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR) > goto flush_fifo; > while (fifo_count >= bytes_per_datum) { > - result = regmap_bulk_read(st->map, st->reg->fifo_r_w, > - data, bytes_per_datum); > - if (result) > - goto flush_fifo; > + /* > + * We need to do a large burst read from a single register. > + * > + * regmap_read_bulk assumes that multiple registers are > + * involved but in our case st->reg->fifo_r_w + 1 is something > + * completely unrelated. > + */ > + if (spi) { > + u8 cmd = st->reg->fifo_r_w | 0x80; > + result = spi_write_then_read(spi, > + &cmd, 1, > + data, bytes_per_datum); > + if (result) > + goto flush_fifo; > + } else { > + result = i2c_smbus_read_i2c_block_data(i2c, > + st->reg->fifo_r_w, > + bytes_per_datum, data); > + if (result != bytes_per_datum) > + goto flush_fifo; > + } > > result = kfifo_out(&st->timestamps, ×tamp, 1); > /* when there is no timestamp, put timestamp as 0 */ > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c index d070062..8455af0 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c @@ -21,6 +21,7 @@ #include <linux/interrupt.h> #include <linux/kfifo.h> #include <linux/poll.h> +#include <linux/spi/spi.h> #include "inv_mpu_iio.h" static void inv_clear_kfifo(struct inv_mpu6050_state *st) @@ -128,6 +129,13 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) u16 fifo_count; s64 timestamp; + struct device *regmap_dev = regmap_get_device(st->map); + struct i2c_client *i2c; + struct spi_device *spi = NULL; + + i2c = i2c_verify_client(regmap_dev); + spi = i2c ? NULL: to_spi_device(regmap_dev); + mutex_lock(&indio_dev->mlock); if (!(st->chip_config.accl_fifo_enable | st->chip_config.gyro_fifo_enable)) @@ -160,10 +168,27 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR) goto flush_fifo; while (fifo_count >= bytes_per_datum) { - result = regmap_bulk_read(st->map, st->reg->fifo_r_w, - data, bytes_per_datum); - if (result) - goto flush_fifo; + /* + * We need to do a large burst read from a single register. + * + * regmap_read_bulk assumes that multiple registers are + * involved but in our case st->reg->fifo_r_w + 1 is something + * completely unrelated. + */ + if (spi) { + u8 cmd = st->reg->fifo_r_w | 0x80; + result = spi_write_then_read(spi, + &cmd, 1, + data, bytes_per_datum); + if (result) + goto flush_fifo; + } else { + result = i2c_smbus_read_i2c_block_data(i2c, + st->reg->fifo_r_w, + bytes_per_datum, data); + if (result != bytes_per_datum) + goto flush_fifo; + } result = kfifo_out(&st->timestamps, ×tamp, 1); /* when there is no timestamp, put timestamp as 0 */
Using regmap_read_bulk is wrong because it assumes that a range of registers is being read. In our case reading from the fifo register will return multiple values but this is *not* auto-increment. This currently works by accident. Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com> --- drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 33 ++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)