mbox series

[v5,00/10] iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events

Message ID 20241205171343.308963-1-l.rubusch@gmail.com (mailing list archive)
Headers show
Series iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events | expand

Message

Lothar Rubusch Dec. 5, 2024, 5:13 p.m. UTC
The adxl345 sensor offers several features. Most of them are based on
using the hardware FIFO and reacting on events coming in on an interrupt
line. Add access to configure and read out the FIFO, handling of interrupts
and configuration and application of the watermark feature on that FIFO.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
Although I tried to implement most of the requested changes, now the code
is simplified and clearer, I still encounter some issues.

1) Unsure if my way reading out the FIFO elements with a regmap_noinc_read()
   is supposed to be like that. TBH, isn't there a better way, say, to
   configure the channel correctly and this is handled by the iio API
   internally? As I understood from v2 there might be a way using
   available data, also in iio_info I see now as buffer attributes an
   available data field. Where can I find information how to use it or
   did I get this wrong?

2) Overrun handling: I'm trying to reset the FIFO and registers. Unsure,
   if this is the correct dealing here.

3) I can see the IRQs coming in, and with a `watch -n 0.1 iio_info` I can
   see the correct fields changing. I tried the follwoing down below,
   but the iio_readdev shows me the following result. I don't quite
   understand if I still have an issue here, or if this is a calibration
   thing?

# iio_info 
Library version: 0.23 (git tag: v0.23)
Compiled with backends: local xml ip usb
IIO context created with local backend.
Backend version: 0.23 (git tag: v0.23)
Backend description string: Linux dut1138 6.6.21-lothar02 #3 SMP PREEMPT Wed Nov  6 21:21:14 UTC 2024 aarch64
IIO context has 2 attributes:
	local,kernel: 6.6.21-lothar02
	uri: local:
IIO context has 1 devices:
	iio:device0: adxl345 (buffer capable)
		3 channels found:
			accel_x:  (input, index: 0, format: le:s13/16>>0)
			4 channel-specific attributes found:
				attr  0: calibbias value: 0
				attr  1: raw value: -14                          <--- CHANGES
				attr  2: sampling_frequency value: 100.000000000
				attr  3: scale value: 0.038300
			accel_y:  (input, index: 1, format: le:s13/16>>0)
			4 channel-specific attributes found:
				attr  0: calibbias value: 0
				attr  1: raw value: 6                            <--- CHANGES
				attr  2: sampling_frequency value: 100.000000000
				attr  3: scale value: 0.038300
			accel_z:  (input, index: 2, format: le:s13/16>>0)
			4 channel-specific attributes found:
				attr  0: calibbias value: 0
				attr  1: raw value: 247                          <--- CHANGES
				attr  2: sampling_frequency value: 100.000000000
				attr  3: scale value: 0.038300
		2 device-specific attributes found:
				attr  0: sampling_frequency_available value: 0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200
				attr  1: waiting_for_supplier value: 0
		3 buffer-specific attributes found:
				attr  0: data_available value: 13
				attr  1: direction value: in
				attr  2: watermark value: 15
		No trigger on this device

  Above I marked what keeps changing with "CHANGES", that's what I expect. Then with readdev
  I obtain the following result.

# iio_attr -c adxl345
dev 'adxl345', channel 'accel_x' (input, index: 0, format: le:s13/16>>0), found 4 channel-specific attributes
dev 'adxl345', channel 'accel_y' (input, index: 1, format: le:s13/16>>0), found 4 channel-specific attributes
dev 'adxl345', channel 'accel_z' (input, index: 2, format: le:s13/16>>0), found 4 channel-specific attributes
# echo 1 > ./scan_elements/in_accel_x_en
# echo 1 > ./scan_elements/in_accel_y_en
# echo 1 > ./scan_elements/in_accel_z_en
# echo 32 > ./buffer0/length
# echo 15 > ./buffer0/watermark
# echo 1 > ./buffer0/enable
# iio_readdev -b 16 -s 21 adxl345 > samples.dat
# hexdump -d ./samples.dat 
0000000   65523   00006   00248   65523   00005   00235   65522   00006
0000010   00248   65522   00006   00248   65521   00005   00247   65522
0000020   00007   00249   65523   00005   00249   65521   00006   00248
0000030   65522   00006   00248   65522   00006   00250   65522   00006
0000040   00249   65522   00005   00248   65523   00005   00248   65521
0000050   00007   00248   65521   00006   00250   65522   00005   00248
0000060   65521   00006   00248   65522   00007   00247   65522   00006
0000070   00248   65522   00006   00248   65521   00004   00250        
000007e

  Am I doing this actually correctly?

---
v4 -> v5:
- fix dt-binding for enum array of INT1 and INT2
v3 -> v4:
- fix dt-binding indention 
v2 -> v3:
- reorganize commits, merge the watermark handling
- INT lines are defined by binding
- kfifo is prepared by devm_iio_kfifo_buffer_setup()
- event handler is registered w/ devm_request_threaded_irq()
v1 -> v2:
Fix comments according to Documentation/doc-guide/kernel-doc.rst
and missing static declaration of function.
---
Lothar Rubusch (10):
  iio: accel: adxl345: refrase comment on probe
  iio: accel: adxl345: rename variable data to st
  iio: accel: adxl345: measure right-justified
  iio: accel: adxl345: add function to switch measuring mode
  iio: accel: adxl345: complete list of defines
  dt-bindings: iio: accel: add interrupt-names
  iio: accel: adxl345: introduce interrupt handling
  iio: accel: adxl345: initialize FIFO delay value for SPI
  iio: accel: adxl345: prepare channel for scan_index
  iio: accel: adxl345: add FIFO with watermark events

 .../bindings/iio/accel/adi,adxl345.yaml       |   7 +
 drivers/iio/accel/adxl345.h                   |  90 +++-
 drivers/iio/accel/adxl345_core.c              | 427 ++++++++++++++++--
 drivers/iio/accel/adxl345_i2c.c               |   2 +-
 drivers/iio/accel/adxl345_spi.c               |   7 +-
 5 files changed, 478 insertions(+), 55 deletions(-)

Comments

Jonathan Cameron Dec. 11, 2024, 6:53 p.m. UTC | #1
On Thu,  5 Dec 2024 17:13:33 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> The adxl345 sensor offers several features. Most of them are based on
> using the hardware FIFO and reacting on events coming in on an interrupt
> line. Add access to configure and read out the FIFO, handling of interrupts
> and configuration and application of the watermark feature on that FIFO.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
> Although I tried to implement most of the requested changes, now the code
> is simplified and clearer, I still encounter some issues.
> 
> 1) Unsure if my way reading out the FIFO elements with a regmap_noinc_read()
>    is supposed to be like that. 

That is pretty standard looking.

>    TBH, isn't there a better way, say, to
>    configure the channel correctly and this is handled by the iio API
>    internally? As I understood from v2 there might be a way using
>    available data, also in iio_info I see now as buffer attributes an
>    available data field. Where can I find information how to use it or
>    did I get this wrong?

Could you give a reference for that. I'm not sure what you are referring to.
if you mean available_scan_masks then that is used to allow the core
to demux data if you have to read all channels out (or some subset) rather
than any combination.

We don't have a bulk put to the buffers. Have thought about it in the past
but it reality it's not that much efficient in the core because in many cases
we have to reorganize the data.




> 
> 2) Overrun handling: I'm trying to reset the FIFO and registers. Unsure,
>    if this is the correct dealing here.

It's a tricky corner. Sometimes people just drain the buffer on basis device
recovers, some other devices need a reset.  
There isn't a good way to communication it to userspace though.

> 
> 3) I can see the IRQs coming in, and with a `watch -n 0.1 iio_info` I can
>    see the correct fields changing. I tried the follwoing down below,
>    but the iio_readdev shows me the following result. I don't quite
>    understand if I still have an issue here, or if this is a calibration
>    thing?
> 
> # iio_info 
> Library version: 0.23 (git tag: v0.23)
> Compiled with backends: local xml ip usb
> IIO context created with local backend.
> Backend version: 0.23 (git tag: v0.23)
> Backend description string: Linux dut1138 6.6.21-lothar02 #3 SMP PREEMPT Wed Nov  6 21:21:14 UTC 2024 aarch64
> IIO context has 2 attributes:
> 	local,kernel: 6.6.21-lothar02
> 	uri: local:
> IIO context has 1 devices:
> 	iio:device0: adxl345 (buffer capable)
> 		3 channels found:
> 			accel_x:  (input, index: 0, format: le:s13/16>>0)
> 			4 channel-specific attributes found:
> 				attr  0: calibbias value: 0
> 				attr  1: raw value: -14                          <--- CHANGES
> 				attr  2: sampling_frequency value: 100.000000000
> 				attr  3: scale value: 0.038300
> 			accel_y:  (input, index: 1, format: le:s13/16>>0)
> 			4 channel-specific attributes found:
> 				attr  0: calibbias value: 0
> 				attr  1: raw value: 6                            <--- CHANGES
> 				attr  2: sampling_frequency value: 100.000000000
> 				attr  3: scale value: 0.038300
> 			accel_z:  (input, index: 2, format: le:s13/16>>0)
> 			4 channel-specific attributes found:
> 				attr  0: calibbias value: 0
> 				attr  1: raw value: 247                          <--- CHANGES
> 				attr  2: sampling_frequency value: 100.000000000
> 				attr  3: scale value: 0.038300
> 		2 device-specific attributes found:
> 				attr  0: sampling_frequency_available value: 0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200
> 				attr  1: waiting_for_supplier value: 0
> 		3 buffer-specific attributes found:
> 				attr  0: data_available value: 13
> 				attr  1: direction value: in
> 				attr  2: watermark value: 15
> 		No trigger on this device
> 
>   Above I marked what keeps changing with "CHANGES", that's what I expect. Then with readdev
>   I obtain the following result.
> 
> # iio_attr -c adxl345
> dev 'adxl345', channel 'accel_x' (input, index: 0, format: le:s13/16>>0), found 4 channel-specific attributes
> dev 'adxl345', channel 'accel_y' (input, index: 1, format: le:s13/16>>0), found 4 channel-specific attributes
> dev 'adxl345', channel 'accel_z' (input, index: 2, format: le:s13/16>>0), found 4 channel-specific attributes
> # echo 1 > ./scan_elements/in_accel_x_en
> # echo 1 > ./scan_elements/in_accel_y_en
> # echo 1 > ./scan_elements/in_accel_z_en
> # echo 32 > ./buffer0/length
> # echo 15 > ./buffer0/watermark
> # echo 1 > ./buffer0/enable
> # iio_readdev -b 16 -s 21 adxl345 > samples.dat
> # hexdump -d ./samples.dat 
> 0000000   65523   00006   00248   65523   00005   00235   65522   00006
> 0000010   00248   65522   00006   00248   65521   00005   00247   65522
> 0000020   00007   00249   65523   00005   00249   65521   00006   00248
> 0000030   65522   00006   00248   65522   00006   00250   65522   00006
> 0000040   00249   65522   00005   00248   65523   00005   00248   65521
> 0000050   00007   00248   65521   00006   00250   65522   00005   00248
> 0000060   65521   00006   00248   65522   00007   00247   65522   00006
> 0000070   00248   65522   00006   00248   65521   00004   00250        
> 000007e
> 
>   Am I doing this actually correctly?

I'm sorry to say I'm guessing here don't use this tool.  I'm still stuck in using my own tooling
(mostly the stuff in the kernel tree in tools/iio) that predate anyone writing nice
userspace code :)

What do you think looks wrong?  You should have 3 16 bit values.
You haven't turned on the timestamp so they will be tightly packed
That hex dump looks plausible if it's storing the actual raw buffer data.

To convert that to real data you need to apply masking and scale factors.

Jonathan


> 
> ---
> v4 -> v5:
> - fix dt-binding for enum array of INT1 and INT2
> v3 -> v4:
> - fix dt-binding indention 
> v2 -> v3:
> - reorganize commits, merge the watermark handling
> - INT lines are defined by binding
> - kfifo is prepared by devm_iio_kfifo_buffer_setup()
> - event handler is registered w/ devm_request_threaded_irq()
> v1 -> v2:
> Fix comments according to Documentation/doc-guide/kernel-doc.rst
> and missing static declaration of function.
> ---
> Lothar Rubusch (10):
>   iio: accel: adxl345: refrase comment on probe
>   iio: accel: adxl345: rename variable data to st
>   iio: accel: adxl345: measure right-justified
>   iio: accel: adxl345: add function to switch measuring mode
>   iio: accel: adxl345: complete list of defines
>   dt-bindings: iio: accel: add interrupt-names
>   iio: accel: adxl345: introduce interrupt handling
>   iio: accel: adxl345: initialize FIFO delay value for SPI
>   iio: accel: adxl345: prepare channel for scan_index
>   iio: accel: adxl345: add FIFO with watermark events
> 
>  .../bindings/iio/accel/adi,adxl345.yaml       |   7 +
>  drivers/iio/accel/adxl345.h                   |  90 +++-
>  drivers/iio/accel/adxl345_core.c              | 427 ++++++++++++++++--
>  drivers/iio/accel/adxl345_i2c.c               |   2 +-
>  drivers/iio/accel/adxl345_spi.c               |   7 +-
>  5 files changed, 478 insertions(+), 55 deletions(-)
>