* [PATCH] iio: imu: st_lsm6dsx: do not use a fixed read len in read_oneshot
[not found] <cover.1541975146.git.lorenzo.bianconi@redhat.com>
@ 2018-11-11 22:28 ` Lorenzo Bianconi
2018-11-16 18:21 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Bianconi @ 2018-11-11 22:28 UTC (permalink / raw)
To: jic23; +Cc: linux-iio
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 <lorenzo.bianconi@redhat.com>
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c | 31 +++++++++++++-------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
index ee59b0cac84f..b908df0380b3 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
@@ -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
--
2.19.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: do not use a fixed read len in read_oneshot
2018-11-11 22:28 ` [PATCH] iio: imu: st_lsm6dsx: do not use a fixed read len in read_oneshot Lorenzo Bianconi
@ 2018-11-16 18:21 ` Jonathan Cameron
2018-11-16 18:31 ` Lorenzo Bianconi
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2018-11-16 18:21 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio
On Sun, 11 Nov 2018 23:28:28 +0100
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> 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 <lorenzo.bianconi@redhat.com>
> ---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c | 31 +++++++++++++-------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> index ee59b0cac84f..b908df0380b3 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> @@ -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;
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?
>
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: do not use a fixed read len in read_oneshot
2018-11-16 18:21 ` Jonathan Cameron
@ 2018-11-16 18:31 ` Lorenzo Bianconi
2018-11-17 16:27 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Bianconi @ 2018-11-16 18:31 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
> On Sun, 11 Nov 2018 23:28:28 +0100
> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
>
[...]
> > - __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;
>
> 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
> Also, I haven't checked but is there potential for DMA into this
> buffer?
>
>
> >
> > 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: do not use a fixed read len in read_oneshot
2018-11-16 18:31 ` Lorenzo Bianconi
@ 2018-11-17 16:27 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2018-11-17 16:27 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio
On Fri, 16 Nov 2018 19:31:47 +0100
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> > On Sun, 11 Nov 2018 23:28:28 +0100
> > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> >
>
> [...]
>
> > > - __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;
> >
> > 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?
Can always be expanded later if we need it!
Thanks,
Jonathan
>
> Regards,
> Lorenzo
>
> > Also, I haven't checked but is there potential for DMA into this
> > buffer?
> >
> >
> > >
> > > 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
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-11-18 2:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1541975146.git.lorenzo.bianconi@redhat.com>
2018-11-11 22:28 ` [PATCH] iio: imu: st_lsm6dsx: do not use a fixed read len in read_oneshot Lorenzo Bianconi
2018-11-16 18:21 ` Jonathan Cameron
2018-11-16 18:31 ` Lorenzo Bianconi
2018-11-17 16:27 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.