[v3,18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads.

Commit Message

Jonathan Cameron July 22, 2020, 3:50 p.m. UTC

From: Jonathan Cameron <[e-mail]>

We should not be assuming that we are reading a sequence of
registers as here we are doing a read of a lot of data from
a single register address.

Suggested-by: Jean-Baptiste Maneyrol <[e-mail]>
Signed-off-by: Jonathan Cameron <[e-mail]>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko July 23, 2020, 12:15 p.m. UTC | #1

On Wed, Jul 22, 2020 at 6:53 PM Jonathan Cameron <[e-mail]> wrote:
>
> From: Jonathan Cameron <[e-mail]>
>
> We should not be assuming that we are reading a sequence of
> registers as here we are doing a read of a lot of data from
> a single register address.

> -               result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> -                                         st->data, bytes_per_datum);
> +               result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
> +                                          st->data, bytes_per_datum);

I don't know the difference between these APIs, but AFAIU in this case
we always ask for a minimum data (like one item of 2 bytes or so) per
access. Because registers are defined like 16-bit wide we are fine. Is
that correct?

Jonathan Cameron July 23, 2020, 12:28 p.m. UTC | #2

On Thu, 23 Jul 2020 15:15:46 +0300
Andy Shevchenko <[e-mail]> wrote:

> On Wed, Jul 22, 2020 at 6:53 PM Jonathan Cameron <[e-mail]> wrote:
> >
> > From: Jonathan Cameron <[e-mail]>
> >
> > We should not be assuming that we are reading a sequence of
> > registers as here we are doing a read of a lot of data from
> > a single register address.  
> 
> > -               result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> > -                                         st->data, bytes_per_datum);
> > +               result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
> > +                                          st->data, bytes_per_datum);  
> 
> I don't know the difference between these APIs, but AFAIU in this case
> we always ask for a minimum data (like one item of 2 bytes or so) per
> access. Because registers are defined like 16-bit wide we are fine. Is
> that correct?
Yes.   There is only really a different in these two APIs if caching
is enabled in regmap.  Conceptually noinc is repeated reading of the same
register, whilst build_read reads a bunch of registers starting at this
location.   If any of them happen to be cacheable, regmap_bulk_read will
update the cached value for any registers it happens to read.  In this
particular case that would be incorrect because the hardware is not
doing any auto increment of the address during repeated reads, it's just
reading the same register lots of times.

Jonathan

Jean-Baptiste Maneyrol July 24, 2020, 8:29 a.m. UTC | #3

Hi Jonathan,

perfect.

Thanks for the fix,
JB

Reviewed-by: Jean-Baptiste Maneyrol <[e-mail]>


From: [e-mail] <[e-mail]> on behalf of Jonathan Cameron <[e-mail]>
Sent: Wednesday, July 22, 2020 17:50
To: [e-mail] <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>; Lars-Peter Clausen <[e-mail]>; Peter Meerwald <[e-mail]>; Jonathan Cameron <[e-mail]>; Jean-Baptiste Maneyrol <[e-mail]>
Subject: [PATCH v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads. 
 
 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.

From: Jonathan Cameron <[e-mail]>

We should not be assuming that we are reading a sequence of
registers as here we are doing a read of a lot of data from
a single register address.

Suggested-by: Jean-Baptiste Maneyrol <[e-mail]>
Signed-off-by: Jonathan Cameron <[e-mail]>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index d8e6b88ddffc..45c37525c2f1 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -179,8 +179,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
         nb = fifo_count / bytes_per_datum;
         inv_mpu6050_update_period(st, pf->timestamp, nb);
         for (i = 0; i < nb; ++i) {
-               result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
-                                         st->data, bytes_per_datum);
+               result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
+                                          st->data, bytes_per_datum);
                 if (result)
                         goto flush_fifo;
                 /* skip first samples if needed */

Jonathan Cameron Sept. 19, 2020, 4:55 p.m. UTC | #4

On Fri, 24 Jul 2020 08:29:56 +0000
Jean-Baptiste Maneyrol <[e-mail]> wrote:

> Hi Jonathan,
> 
> perfect.
> 
> Thanks for the fix,
> JB
> 
> Reviewed-by: Jean-Baptiste Maneyrol <[e-mail]>
> 
> 
> From: [e-mail] <[e-mail]> on behalf of Jonathan Cameron <[e-mail]>
> Sent: Wednesday, July 22, 2020 17:50
> To: [e-mail] <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>; Lars-Peter Clausen <[e-mail]>; Peter Meerwald <[e-mail]>; Jonathan Cameron <[e-mail]>; Jean-Baptiste Maneyrol <[e-mail]>
> Subject: [PATCH v3 18/27] iio:imu:inv_mpu6050: Use regmap_noinc_read for fifo reads. 
>  
>  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.
> 
> From: Jonathan Cameron <[e-mail]>
> 
> We should not be assuming that we are reading a sequence of
> registers as here we are doing a read of a lot of data from
> a single register address.
> 
> Suggested-by: Jean-Baptiste Maneyrol <[e-mail]>
> Signed-off-by: Jonathan Cameron <[e-mail]>
Applied to the togreg branch of iio.git.  I've explicitly added that
this one isn't marked for stable as it doesn't actually have
any affect beyond being semantically correct.

thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index d8e6b88ddffc..45c37525c2f1 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -179,8 +179,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>          nb = fifo_count / bytes_per_datum;
>          inv_mpu6050_update_period(st, pf->timestamp, nb);
>          for (i = 0; i < nb; ++i) {
> -               result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> -                                         st->data, bytes_per_datum);
> +               result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
> +                                          st->data, bytes_per_datum);
>                  if (result)
>                          goto flush_fifo;
>                  /* skip first samples if needed */

Patch

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index d8e6b88ddffc..45c37525c2f1 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -179,8 +179,8 @@  irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	nb = fifo_count / bytes_per_datum;
 	inv_mpu6050_update_period(st, pf->timestamp, nb);
 	for (i = 0; i < nb; ++i) {
-		result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
-					  st->data, bytes_per_datum);
+		result = regmap_noinc_read(st->map, st->reg->fifo_r_w,
+					   st->data, bytes_per_datum);
 		if (result)
 			goto flush_fifo;
 		/* skip first samples if needed */