Re: [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 11/05/2019 14.30, Lorenzo Bianconi wrote:
On Tue,  7 May 2019 10:02:25 +0200
Sean Nyekjaer <sean@xxxxxxxxxx> wrote:

This adds support for using iio triggers, this is needed because
our hardware guys forgot to connect the irq pins from imu device
to the SoC.

Signed-off-by: Sean Nyekjaer <sean@xxxxxxxxxx>
Hi Sean

A small question on the size of the buffer needed inline.  Otherwise looks
good to me.

Hi Sean,

this patch does not make sense to me since running st_lsm6dsx_read_oneshot
you need to wait to power up the device (and you will power it down at the
end). I guess you will not be able to read at a given ODR (e.g. 416Hz).
Moreover you can't read from the hw fifo without the irq line since
you need to read a full pattern from it in order to maintain the alignment.
We are not using the hw fifo if the hw irq isn't present...
So if I understand correctly we could speed things up a bit, with leaving the sensor powered by implementing a new reading function and
calling st_lsm6dsx_sensor_set_enable when we enable the trigger?

 From my point of view you have 2 possibility:
- poll the output registers from userspace (this is what you are actually
   doing from inside the kernel, what is the advantage of doing so?)
Yes this is exactly what I'm trying to accomplish here.
It would be nice for us to have the same hrtimer/trigger to sample this and our adc.

- fix the hw bug
Not possible on +20K devices. ;-)


Moreover if I read the patch correctly it has a NULL pointer dereference bug
since hw->iio_devs[i] can be NULL (e.g. if sensor hub is disabled)
Right above, there is:
if (!hw->iio_devs[i])
	return -ENOMEM;

Should be enough...

Regards,
Lorenzo


BR
/Sean



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux