Skip to contentSkip to conversation

iio: imu: st_lsm6dsx: do not use a fixed read len in read_oneshot

PATCH Superseded Lorenzo Bianconi 2 participants 4 messages Nov 12, 2018 – Nov 18, 2018 1 message not in the archive

Subsystems (2)
IIO ST LSM6DSx IMU IIO driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Lorenzo Bianconi

author
#1 lore raw

PATCH 1/1 iio: imu: st_lsm6dsx: do not use a fixed read len in read_oneshot

Generalize st_lsm6dsx_shub_read_oneshot in order to not use a fixed read length and take into account iio channel realbits for single read operations

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c | 31 +++++++++++++-------
 1 file changed, 20 insertions(+), 11 deletions(-)
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +20 -11
index ee59b0cac84f..b908df0380b3 100644
@@ -27,6 +27,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/bitfield.h>
+#include <linux/slab.h>
 
 #include "st_lsm6dsx.h"
 

@@ -432,31 +433,39 @@ st_lsm6dsx_shub_read_oneshot(struct st_lsm6dsx_sensor *sensor,
 			     struct iio_chan_spec const *ch,
 			     int *val)
 {
-	int err, delay, len = ch->scan_type.realbits >> 3;
-	__le16 data;
+	int ret, delay, len = ch->scan_type.realbits >> 3;
+	u8 *data;
 
-	err = st_lsm6dsx_shub_set_enable(sensor, true);
-	if (err < 0)
-		return err;
+	ret = st_lsm6dsx_shub_set_enable(sensor, true);
+	if (ret < 0)
+		return ret;
+
+	data = kmalloc(len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
 	delay = 1000000 / sensor->odr;
 	usleep_range(delay, 2 * delay);
 
-	err = st_lsm6dsx_shub_read(sensor, ch->address, (u8 *)&data, len);
-	if (err < 0)
-		return err;
+	ret = st_lsm6dsx_shub_read(sensor, ch->address, data, len);
+	if (ret < 0)
+		goto out;
 
 	st_lsm6dsx_shub_set_enable(sensor, false);
 
 	switch (len) {
 	case 2:
-		*val = (s16)le16_to_cpu(data);
+		*val = (s16)le16_to_cpu(*((__le16 *)data));
+		ret = IIO_VAL_INT;
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
+		break;
 	}
 
-	return IIO_VAL_INT;
+out:
+	kfree(data);
+	return ret;
 }
 
 static int
Signature · 1 line
2.19.1
maintainer
#2 lore raw
Quoted 30 lines · On Sun, 11 Nov 2018 23:28:28 +0100 Lorenzo Bianconi…

Generalize st_lsm6dsx_shub_read_oneshot in order to not use a fixed read length and take into account iio channel realbits for single read operations

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c | 31 +++++++++++++-------
 1 file changed, 20 insertions(+), 11 deletions(-)
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +3 -2
index ee59b0cac84f..b908df0380b3 100644
@@ -27,6 +27,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/bitfield.h>
+#include <linux/slab.h>
 
 #include "st_lsm6dsx.h"
 

@@ -432,31 +433,39 @@ st_lsm6dsx_shub_read_oneshot(struct st_lsm6dsx_sensor *sensor,
 			     struct iio_chan_spec const *ch,
 			     int *val)
 {
-	int err, delay, len = ch->scan_type.realbits >> 3;
-	__le16 data;
+	int ret, delay, len = ch->scan_type.realbits >> 3;
+	u8 *data;

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c st_lsm6dsx_shub_read_oneshot(struct st_lsm6dsx_sensor *sensor, · line 445

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +7 -3
-	err = st_lsm6dsx_shub_set_enable(sensor, true);
-	if (err < 0)
-		return err;
+	ret = st_lsm6dsx_shub_set_enable(sensor, true);
+	if (ret < 0)
+		return ret;
+
+	data = kmalloc(len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;

Do we not want to disable should this fail?
Easier might be to just do the allocation before the set_enable call.

I'd also be tempted to just put a big enough buffer on the stack, or into the sensor structure.

Also, I haven't checked but is there potential for DMA into this buffer?

Quoted 32 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +10 -6
 	delay = 1000000 / sensor->odr;
 	usleep_range(delay, 2 * delay);
 
-	err = st_lsm6dsx_shub_read(sensor, ch->address, (u8 *)&data, len);
-	if (err < 0)
-		return err;
+	ret = st_lsm6dsx_shub_read(sensor, ch->address, data, len);
+	if (ret < 0)
+		goto out;
 
 	st_lsm6dsx_shub_set_enable(sensor, false);
 
 	switch (len) {
 	case 2:
-		*val = (s16)le16_to_cpu(data);
+		*val = (s16)le16_to_cpu(*((__le16 *)data));
+		ret = IIO_VAL_INT;
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
+		break;
 	}
 
-	return IIO_VAL_INT;
+out:
+	kfree(data);
+	return ret;
 }
 
 static int
author
#3 lore raw
Quoted 3 lines

On Sun, 11 Nov 2018 23:28:28 +0100
Lorenzo Bianconi <[email protected]> wrote:

[...]

Quoted 15 lines
Quoted 4 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +2 -1
-     __le16 data;
+     int ret, delay, len = ch->scan_type.realbits >> 3;
+     u8 *data;

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c st_lsm6dsx_shub_read_oneshot(struct st_lsm6dsx_sensor *sensor, · line 445

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +7 -3
-     err = st_lsm6dsx_shub_set_enable(sensor, true);
-     if (err < 0)
-             return err;
+     ret = st_lsm6dsx_shub_set_enable(sensor, true);
+     if (ret < 0)
+             return ret;
+
+     data = kmalloc(len, GFP_KERNEL);
+     if (!data)
+             return -ENOMEM;
Quoted 7 lines

Do we not want to disable should this fail?
Easier might be to just do the allocation before the set_enable call.

I'd also be tempted to just put a big enough buffer on the stack, or into the sensor structure.

ack, right. I will fix it in v2 using a u8 data[4] on the stack. I guess it is reasonable, agree?

Regards,
Lorenzo

Quoted 37 lines

Also, I haven't checked but is there potential for DMA into this buffer?

Quoted 32 lines
      delay = 1000000 / sensor->odr;
      usleep_range(delay, 2 * delay);
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +10 -6
-     err = st_lsm6dsx_shub_read(sensor, ch->address, (u8 *)&data, len);
-     if (err < 0)
-             return err;
+     ret = st_lsm6dsx_shub_read(sensor, ch->address, data, len);
+     if (ret < 0)
+             goto out;
 
      st_lsm6dsx_shub_set_enable(sensor, false);
 
      switch (len) {
      case 2:
-             *val = (s16)le16_to_cpu(data);
+             *val = (s16)le16_to_cpu(*((__le16 *)data));
+             ret = IIO_VAL_INT;
              break;
      default:
-             return -EINVAL;
+             ret = -EINVAL;
+             break;
      }
 
-     return IIO_VAL_INT;
+out:
+     kfree(data);
+     return ret;
 }
 
 static int
maintainer
#4 lore raw
Quoted 25 lines · On Fri, 16 Nov 2018 19:31:47 +0100 Lorenzo Bianconi…
Quoted 3 lines

On Sun, 11 Nov 2018 23:28:28 +0100
Lorenzo Bianconi <[email protected]> wrote:

[...]

Quoted 19 lines
Quoted 4 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +2 -1
-     __le16 data;
+     int ret, delay, len = ch->scan_type.realbits >> 3;
+     u8 *data;

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c st_lsm6dsx_shub_read_oneshot(struct st_lsm6dsx_sensor *sensor, · line 445

Quoted 10 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +7 -3
-     err = st_lsm6dsx_shub_set_enable(sensor, true);
-     if (err < 0)
-             return err;
+     ret = st_lsm6dsx_shub_set_enable(sensor, true);
+     if (ret < 0)
+             return ret;
+
+     data = kmalloc(len, GFP_KERNEL);
+     if (!data)
+             return -ENOMEM;

Do we not want to disable should this fail?
Easier might be to just do the allocation before the set_enable call.

Quoted 6 lines
Quoted 3 lines

I'd also be tempted to just put a big enough buffer on the stack, or into the sensor structure.

ack, right. I will fix it in v2 using a u8 data[4] on the stack. I guess it is reasonable, agree?

Can always be expanded later if we need it!

Thanks,

Jonathan

Quoted 41 lines

Regards,
Lorenzo

Quoted 37 lines

Also, I haven't checked but is there potential for DMA into this buffer?

Quoted 32 lines
      delay = 1000000 / sensor->odr;
      usleep_range(delay, 2 * delay);
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +10 -6
-     err = st_lsm6dsx_shub_read(sensor, ch->address, (u8 *)&data, len);
-     if (err < 0)
-             return err;
+     ret = st_lsm6dsx_shub_read(sensor, ch->address, data, len);
+     if (ret < 0)
+             goto out;
 
      st_lsm6dsx_shub_set_enable(sensor, false);
 
      switch (len) {
      case 2:
-             *val = (s16)le16_to_cpu(data);
+             *val = (s16)le16_to_cpu(*((__le16 *)data));
+             ret = IIO_VAL_INT;
              break;
      default:
-             return -EINVAL;
+             ret = -EINVAL;
+             break;
      }
 
-     return IIO_VAL_INT;
+out:
+     kfree(data);
+     return ret;
 }
 
 static int