* [PATCH v2] Add continuous sampling with IIO buffers for Vybrid
@ 2015-08-05 12:10 ` Sanchayan Maity
0 siblings, 0 replies; 12+ messages in thread
From: Sanchayan Maity @ 2015-08-05 12:10 UTC (permalink / raw)
To: jic23, linux-iio
Cc: stefan, B38611, hofrat, sanjeev_sharma, fabio.estevam, knaack.h,
lars, pmeerw, antoine.tenart, linux-kernel, linux-arm-kernel,
Sanchayan Maity
Hello,
This patch adds support for continuous sampling provided by the
ADC block on Vybrid by leveraging the IIO triggered buffer and
IIO sysfs trigger infrastructure.
The patch has been tested on Colibri VF50 and VF61 on shawn's
tree for-next branch with the patches [1] and [2] applied.
The below script was used for testing.
#!/bin/sh
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage8_en
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage9_en
echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_temp_en
echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_timestamp_en
echo 0 > /sys/bus/iio/devices/iio_sysfs_trigger/add_trigger
/home/root/generic_buffer -n 4003b000.adc -t sysfstrig0 -l 512 -c 10
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_timestamp_en
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_temp_en
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage8_en
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage9_en
Feedback and comments are most welcome.
Changes since v2:
1. Use a fixed size buffer instead of kmalloc allocated during update
scan mode
2. Remove a write to read only register ADC_HS (COCO bit)
Version 1 patch can be found here
http://www.spinics.net/lists/linux-iio/msg20053.html
[1]. https://lkml.org/lkml/2015/5/27/350
[2]. https://lkml.org/lkml/2015/7/14/395
Had some additional queries.
On shawn's tree, using imx_v6_v7_defconfig and running the above
script on VF50 gives me the following trace:
[ 199.976662] INFO: rcu_sched self-detected stall on CPU
[ 199.982015] 0: (2600 ticks this GP) idle=181/140000000000002/0 softirq=14591/14591 fqs=0
[ 199.990406] (t=2600 jiffies g=11024 c=11023 q=20)
[ 199.995430] rcu_sched kthread starved for 2600 jiffies! g11024 c11023 f0x0
[ 200.002523] Task dump for CPU 0:
[ 200.005830] generic_buffer R running 0 414 413 0x00000002
[ 200.012370] Backtrace:
[ 200.014974] [<80013708>] (dump_backtrace) from [<800138fc>] (show_stack+0x18/0x1c)
[ 200.022665] r7:80ae0f40 r6:80b374e8 r5:0000019d r4:85fdd080
[ 200.028578] [<800138e4>] (show_stack) from [<800557bc>] (sched_show_task+0x11c/0x22c)
[ 200.036555] [<800556a0>] (sched_show_task) from [<80057efc>] (dump_cpu_task+0x34/0x44)
[ 200.044596] r6:80000193 r5:80ae0f40 r4:00000000
[ 200.049432] [<80057ec8>] (dump_cpu_task) from [<800820f8>] (rcu_dump_cpu_stacks+0x8c/0xd0)
[ 200.057821] r5:80ae0f40 r4:00000000
[ 200.061563] [<8008206c>] (rcu_dump_cpu_stacks) from [<80085b88>] (rcu_check_callbacks+0x4dc/0x854)
[ 200.070655] r9:80ae0f40 r8:0720b000 r7:80ad09cc r6:80ae0f40 r5:80acd600 r4:87cd8600
[ 200.078701] [<800856ac>] (rcu_check_callbacks) from [<80088820>] (update_process_times+0x40/0x6c)
[ 200.087699] r10:87cd5fc0 r9:8009a180 r8:80aedb80 r7:0000002e r6:8764c12d r5:00000000
[ 200.095800] r4:85fdd080
[ 200.098462] [<800887e0>] (update_process_times) from [<8009a17c>] (tick_sched_handle.isra.19+0x4c/0x50)
[ 200.108049] r5:85e85d40 r4:87cd5fc0
[ 200.111822] [<8009a130>] (tick_sched_handle.isra.19) from [<8009a1e4>] (tick_sched_timer+0x64/0xb0)
[ 200.121098] [<8009a180>] (tick_sched_timer) from [<80089220>] (__hrtimer_run_queues+0xc4/0x1cc)
[ 200.129989] r7:0000002e r6:87649b3a r5:87cd5d00 r4:87cd5c80
[ 200.135915] [<8008915c>] (__hrtimer_run_queues) from [<80089950>] (hrtimer_interrupt+0xc8/0x21c)
[ 200.144898] r10:87cd5d38 r9:87cd5d58 r8:87cd5c80 r7:87cd5cc0 r6:ffffffff r5:7fffffff
[ 200.153061] r4:00000003
[ 200.155752] [<80089888>] (hrtimer_interrupt) from [<8054dcd0>] (gt_clockevent_interrupt+0x48/0x74)
[ 200.164905] r10:85e85f80 r9:87806000 r8:00000010 r7:87817b40 r6:87808100 r5:87cdab40
[ 200.173082] r4:00000001
[ 200.175758] [<8054dc88>] (gt_clockevent_interrupt) from [<8007cab0>] (handle_percpu_devid_irq+0x8c/0xac)
[ 200.185442] r5:80aedd20 r4:87cdab40
[ 200.189215] [<8007ca24>] (handle_percpu_devid_irq) from [<800786e4>] (generic_handle_irq+0x30/0x44)
[ 200.198460] r9:87806000 r8:00000001 r7:85e85e30 r6:80ad0b30 r5:00000010 r4:00000010
[ 200.206567] [<800786b4>] (generic_handle_irq) from [<80078a08>] (__handle_domain_irq+0x6c/0xe8)
[ 200.215455] r5:00000010 r4:80acb5fc
[ 200.219219] [<8007899c>] (__handle_domain_irq) from [<80009560>] (gic_handle_irq+0x28/0x68)
[ 200.227759] r9:87806000 r8:00000001 r7:88002100 r6:80ad0ca8 r5:8800210c r4:85e85d40
[ 200.235861] [<80009538>] (gic_handle_irq) from [<80014564>] (__irq_svc+0x44/0x5c)
[ 200.243526] Exception stack(0x85e85d40 to 0x85e85d88)
[ 200.248720] 5d40: 00000001 85fdd548 00000000 85fdd080 00000202 00000000 80ad0b30 80ad0080
[ 200.257104] 5d60: 00000001 87806000 85e85f80 85e85dcc 80c9f5d0 85e85d88 8006b5a0 8002ff80
[ 200.265468] 5d80: 60000113 ffffffff
[ 200.269056] r7:85e85d74 r6:ffffffff r5:60000113 r4:8002ff80
[ 200.274995] [<8002fec8>] (__do_softirq) from [<800304c8>] (irq_exit+0xc4/0x138)
[ 200.282486] r10:85e85f80 r9:87806000 r8:00000001 r7:00000000 r6:80ad0b30 r5:00000000
[ 200.290660] r4:80acb5fc
[ 200.293345] [<80030404>] (irq_exit) from [<80078a10>] (__handle_domain_irq+0x74/0xe8)
[ 200.301367] r5:00000000 r4:80acb5fc
[ 200.305135] [<8007899c>] (__handle_domain_irq) from [<80009560>] (gic_handle_irq+0x28/0x68)
[ 200.313680] r9:85efc60c r8:85e4bc00 r7:88002100 r6:80ad0ca8 r5:8800210c r4:85e85e30
[ 200.321777] [<80009538>] (gic_handle_irq) from [<80014564>] (__irq_svc+0x44/0x5c)
[ 200.329442] Exception stack(0x85e85e30 to 0x85e85e78)
[ 200.334628] 5e20: 00000001 85fdd548 00000000 85fdd080
[ 200.343010] 5e40: 00000001 803ce7c8 85e4bc00 00000000 85e4bc00 85efc60c 85e85f80 85e85e84
[ 200.351384] 5e60: 80c9f5d0 85e85e78 8006b5a0 803ce7e8 60000013 ffffffff
[ 200.358131] r7:85e85e64 r6:ffffffff r5:60000013 r4:803ce7e8
[ 200.364087] [<803ce7c8>] (dev_attr_store) from [<801739a0>] (sysfs_kf_write+0x54/0x58)
[ 200.372217] [<8017394c>] (sysfs_kf_write) from [<80172b60>] (kernfs_fop_write+0xc4/0x1a8)
[ 200.380579] r7:00000000 r6:00000000 r5:00000001 r4:85efc600
[ 200.386518] [<80172a9c>] (kernfs_fop_write) from [<80104f10>] (__vfs_write+0x2c/0xe0)
[ 200.394535] r10:00000000 r9:85e84000 r8:8000fc84 r7:85e85f80 r6:76fa3000 r5:85e85f80
[ 200.402696] r4:85fb2500
[ 200.405376] [<80104ee4>] (__vfs_write) from [<801057b4>] (vfs_write+0x98/0x16c)
[ 200.412865] r8:8000fc84 r7:85e85f80 r6:76fa3000 r5:00000001 r4:85fb2500
[ 200.419878] [<8010571c>] (vfs_write) from [<80106028>] (SyS_write+0x4c/0xa8)
[ 200.427072] r8:8000fc84 r7:00000001 r6:76fa3000 r5:85fb2500 r4:85fb2500
[ 200.434099] [<80105fdc>] (SyS_write) from [<8000faa0>] (ret_fast_syscall+0x0/0x54)
[ 200.441848] r7:00000004 r6:76fa3000 r5:00000001 r4:002be960
On shawn's tree, using our own defconfig it all works fine.
The RCU related configs in imx_v6_v7_defconfig are:
# RCU Subsystem
CONFIG_TREE_RCU=y
# CONFIG_RCU_EXPERT is not set
CONFIG_SRCU=y
# CONFIG_TASKS_RCU is not set
CONFIG_RCU_STALL_COMMON=y
# CONFIG_TREE_RCU_TRACE is not set
# CONFIG_RCU_NOCB_CPU is not set
# CONFIG_RCU_EXPEDITE_BOOT is not set
# RCU Debugging
CONFIG_PROVE_RCU=y
# CONFIG_PROVE_RCU_REPEATEDLY is not set
# CONFIG_SPARSE_RCU_POINTER is not set
# CONFIG_RCU_TORTURE_TEST is not set
CONFIG_RCU_CPU_STALL_TIMEOUT=21
CONFIG_RCU_CPU_STALL_INFO=y
# CONFIG_RCU_TRACE is not set
# CONFIG_RCU_EQS_DEBUG is not set
The RCU related configs in our own defconfig are:
# RCU Subsystem
CONFIG_TINY_RCU=y
CONFIG_SRCU=y
# CONFIG_TASKS_RCU is not set
# CONFIG_RCU_STALL_COMMON is not set
# CONFIG_TREE_RCU_TRACE is not set
CONFIG_RCU_KTHREAD_PRIO=0
# RCU Debugging
# CONFIG_SPARSE_RCU_POINTER is not set
# CONFIG_RCU_TORTURE_TEST is not set
# CONFIG_RCU_TRACE is not set
With the above second set of configs I do not encounter any problems.
I do not have any knowledge on the RCU front, can someone also comment
with regards to that? Am I doing something wrong in the driver?
My guess would be those RCU STALL configs?
Thanks & Regards,
Sanchayan Maity.
Sanchayan Maity (1):
iio: adc: vf610: Add IIO buffer support for Vybrid ADC
drivers/iio/adc/Kconfig | 4 ++
drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
2 files changed, 105 insertions(+), 6 deletions(-)
--
2.5.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] Add continuous sampling with IIO buffers for Vybrid
@ 2015-08-05 12:10 ` Sanchayan Maity
0 siblings, 0 replies; 12+ messages in thread
From: Sanchayan Maity @ 2015-08-05 12:10 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
This patch adds support for continuous sampling provided by the
ADC block on Vybrid by leveraging the IIO triggered buffer and
IIO sysfs trigger infrastructure.
The patch has been tested on Colibri VF50 and VF61 on shawn's
tree for-next branch with the patches [1] and [2] applied.
The below script was used for testing.
#!/bin/sh
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage8_en
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage9_en
echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_temp_en
echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_timestamp_en
echo 0 > /sys/bus/iio/devices/iio_sysfs_trigger/add_trigger
/home/root/generic_buffer -n 4003b000.adc -t sysfstrig0 -l 512 -c 10
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_timestamp_en
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_temp_en
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage8_en
echo 0 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltage9_en
Feedback and comments are most welcome.
Changes since v2:
1. Use a fixed size buffer instead of kmalloc allocated during update
scan mode
2. Remove a write to read only register ADC_HS (COCO bit)
Version 1 patch can be found here
http://www.spinics.net/lists/linux-iio/msg20053.html
[1]. https://lkml.org/lkml/2015/5/27/350
[2]. https://lkml.org/lkml/2015/7/14/395
Had some additional queries.
On shawn's tree, using imx_v6_v7_defconfig and running the above
script on VF50 gives me the following trace:
[ 199.976662] INFO: rcu_sched self-detected stall on CPU
[ 199.982015] 0: (2600 ticks this GP) idle=181/140000000000002/0 softirq=14591/14591 fqs=0
[ 199.990406] (t=2600 jiffies g=11024 c=11023 q=20)
[ 199.995430] rcu_sched kthread starved for 2600 jiffies! g11024 c11023 f0x0
[ 200.002523] Task dump for CPU 0:
[ 200.005830] generic_buffer R running 0 414 413 0x00000002
[ 200.012370] Backtrace:
[ 200.014974] [<80013708>] (dump_backtrace) from [<800138fc>] (show_stack+0x18/0x1c)
[ 200.022665] r7:80ae0f40 r6:80b374e8 r5:0000019d r4:85fdd080
[ 200.028578] [<800138e4>] (show_stack) from [<800557bc>] (sched_show_task+0x11c/0x22c)
[ 200.036555] [<800556a0>] (sched_show_task) from [<80057efc>] (dump_cpu_task+0x34/0x44)
[ 200.044596] r6:80000193 r5:80ae0f40 r4:00000000
[ 200.049432] [<80057ec8>] (dump_cpu_task) from [<800820f8>] (rcu_dump_cpu_stacks+0x8c/0xd0)
[ 200.057821] r5:80ae0f40 r4:00000000
[ 200.061563] [<8008206c>] (rcu_dump_cpu_stacks) from [<80085b88>] (rcu_check_callbacks+0x4dc/0x854)
[ 200.070655] r9:80ae0f40 r8:0720b000 r7:80ad09cc r6:80ae0f40 r5:80acd600 r4:87cd8600
[ 200.078701] [<800856ac>] (rcu_check_callbacks) from [<80088820>] (update_process_times+0x40/0x6c)
[ 200.087699] r10:87cd5fc0 r9:8009a180 r8:80aedb80 r7:0000002e r6:8764c12d r5:00000000
[ 200.095800] r4:85fdd080
[ 200.098462] [<800887e0>] (update_process_times) from [<8009a17c>] (tick_sched_handle.isra.19+0x4c/0x50)
[ 200.108049] r5:85e85d40 r4:87cd5fc0
[ 200.111822] [<8009a130>] (tick_sched_handle.isra.19) from [<8009a1e4>] (tick_sched_timer+0x64/0xb0)
[ 200.121098] [<8009a180>] (tick_sched_timer) from [<80089220>] (__hrtimer_run_queues+0xc4/0x1cc)
[ 200.129989] r7:0000002e r6:87649b3a r5:87cd5d00 r4:87cd5c80
[ 200.135915] [<8008915c>] (__hrtimer_run_queues) from [<80089950>] (hrtimer_interrupt+0xc8/0x21c)
[ 200.144898] r10:87cd5d38 r9:87cd5d58 r8:87cd5c80 r7:87cd5cc0 r6:ffffffff r5:7fffffff
[ 200.153061] r4:00000003
[ 200.155752] [<80089888>] (hrtimer_interrupt) from [<8054dcd0>] (gt_clockevent_interrupt+0x48/0x74)
[ 200.164905] r10:85e85f80 r9:87806000 r8:00000010 r7:87817b40 r6:87808100 r5:87cdab40
[ 200.173082] r4:00000001
[ 200.175758] [<8054dc88>] (gt_clockevent_interrupt) from [<8007cab0>] (handle_percpu_devid_irq+0x8c/0xac)
[ 200.185442] r5:80aedd20 r4:87cdab40
[ 200.189215] [<8007ca24>] (handle_percpu_devid_irq) from [<800786e4>] (generic_handle_irq+0x30/0x44)
[ 200.198460] r9:87806000 r8:00000001 r7:85e85e30 r6:80ad0b30 r5:00000010 r4:00000010
[ 200.206567] [<800786b4>] (generic_handle_irq) from [<80078a08>] (__handle_domain_irq+0x6c/0xe8)
[ 200.215455] r5:00000010 r4:80acb5fc
[ 200.219219] [<8007899c>] (__handle_domain_irq) from [<80009560>] (gic_handle_irq+0x28/0x68)
[ 200.227759] r9:87806000 r8:00000001 r7:88002100 r6:80ad0ca8 r5:8800210c r4:85e85d40
[ 200.235861] [<80009538>] (gic_handle_irq) from [<80014564>] (__irq_svc+0x44/0x5c)
[ 200.243526] Exception stack(0x85e85d40 to 0x85e85d88)
[ 200.248720] 5d40: 00000001 85fdd548 00000000 85fdd080 00000202 00000000 80ad0b30 80ad0080
[ 200.257104] 5d60: 00000001 87806000 85e85f80 85e85dcc 80c9f5d0 85e85d88 8006b5a0 8002ff80
[ 200.265468] 5d80: 60000113 ffffffff
[ 200.269056] r7:85e85d74 r6:ffffffff r5:60000113 r4:8002ff80
[ 200.274995] [<8002fec8>] (__do_softirq) from [<800304c8>] (irq_exit+0xc4/0x138)
[ 200.282486] r10:85e85f80 r9:87806000 r8:00000001 r7:00000000 r6:80ad0b30 r5:00000000
[ 200.290660] r4:80acb5fc
[ 200.293345] [<80030404>] (irq_exit) from [<80078a10>] (__handle_domain_irq+0x74/0xe8)
[ 200.301367] r5:00000000 r4:80acb5fc
[ 200.305135] [<8007899c>] (__handle_domain_irq) from [<80009560>] (gic_handle_irq+0x28/0x68)
[ 200.313680] r9:85efc60c r8:85e4bc00 r7:88002100 r6:80ad0ca8 r5:8800210c r4:85e85e30
[ 200.321777] [<80009538>] (gic_handle_irq) from [<80014564>] (__irq_svc+0x44/0x5c)
[ 200.329442] Exception stack(0x85e85e30 to 0x85e85e78)
[ 200.334628] 5e20: 00000001 85fdd548 00000000 85fdd080
[ 200.343010] 5e40: 00000001 803ce7c8 85e4bc00 00000000 85e4bc00 85efc60c 85e85f80 85e85e84
[ 200.351384] 5e60: 80c9f5d0 85e85e78 8006b5a0 803ce7e8 60000013 ffffffff
[ 200.358131] r7:85e85e64 r6:ffffffff r5:60000013 r4:803ce7e8
[ 200.364087] [<803ce7c8>] (dev_attr_store) from [<801739a0>] (sysfs_kf_write+0x54/0x58)
[ 200.372217] [<8017394c>] (sysfs_kf_write) from [<80172b60>] (kernfs_fop_write+0xc4/0x1a8)
[ 200.380579] r7:00000000 r6:00000000 r5:00000001 r4:85efc600
[ 200.386518] [<80172a9c>] (kernfs_fop_write) from [<80104f10>] (__vfs_write+0x2c/0xe0)
[ 200.394535] r10:00000000 r9:85e84000 r8:8000fc84 r7:85e85f80 r6:76fa3000 r5:85e85f80
[ 200.402696] r4:85fb2500
[ 200.405376] [<80104ee4>] (__vfs_write) from [<801057b4>] (vfs_write+0x98/0x16c)
[ 200.412865] r8:8000fc84 r7:85e85f80 r6:76fa3000 r5:00000001 r4:85fb2500
[ 200.419878] [<8010571c>] (vfs_write) from [<80106028>] (SyS_write+0x4c/0xa8)
[ 200.427072] r8:8000fc84 r7:00000001 r6:76fa3000 r5:85fb2500 r4:85fb2500
[ 200.434099] [<80105fdc>] (SyS_write) from [<8000faa0>] (ret_fast_syscall+0x0/0x54)
[ 200.441848] r7:00000004 r6:76fa3000 r5:00000001 r4:002be960
On shawn's tree, using our own defconfig it all works fine.
The RCU related configs in imx_v6_v7_defconfig are:
# RCU Subsystem
CONFIG_TREE_RCU=y
# CONFIG_RCU_EXPERT is not set
CONFIG_SRCU=y
# CONFIG_TASKS_RCU is not set
CONFIG_RCU_STALL_COMMON=y
# CONFIG_TREE_RCU_TRACE is not set
# CONFIG_RCU_NOCB_CPU is not set
# CONFIG_RCU_EXPEDITE_BOOT is not set
# RCU Debugging
CONFIG_PROVE_RCU=y
# CONFIG_PROVE_RCU_REPEATEDLY is not set
# CONFIG_SPARSE_RCU_POINTER is not set
# CONFIG_RCU_TORTURE_TEST is not set
CONFIG_RCU_CPU_STALL_TIMEOUT=21
CONFIG_RCU_CPU_STALL_INFO=y
# CONFIG_RCU_TRACE is not set
# CONFIG_RCU_EQS_DEBUG is not set
The RCU related configs in our own defconfig are:
# RCU Subsystem
CONFIG_TINY_RCU=y
CONFIG_SRCU=y
# CONFIG_TASKS_RCU is not set
# CONFIG_RCU_STALL_COMMON is not set
# CONFIG_TREE_RCU_TRACE is not set
CONFIG_RCU_KTHREAD_PRIO=0
# RCU Debugging
# CONFIG_SPARSE_RCU_POINTER is not set
# CONFIG_RCU_TORTURE_TEST is not set
# CONFIG_RCU_TRACE is not set
With the above second set of configs I do not encounter any problems.
I do not have any knowledge on the RCU front, can someone also comment
with regards to that? Am I doing something wrong in the driver?
My guess would be those RCU STALL configs?
Thanks & Regards,
Sanchayan Maity.
Sanchayan Maity (1):
iio: adc: vf610: Add IIO buffer support for Vybrid ADC
drivers/iio/adc/Kconfig | 4 ++
drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
2 files changed, 105 insertions(+), 6 deletions(-)
--
2.5.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
2015-08-05 12:10 ` Sanchayan Maity
@ 2015-08-05 12:10 ` Sanchayan Maity
-1 siblings, 0 replies; 12+ messages in thread
From: Sanchayan Maity @ 2015-08-05 12:10 UTC (permalink / raw)
To: jic23, linux-iio
Cc: stefan, B38611, hofrat, sanjeev_sharma, fabio.estevam, knaack.h,
lars, pmeerw, antoine.tenart, linux-kernel, linux-arm-kernel,
Sanchayan Maity
This patch adds support for IIO buffer to the Vybrid ADC driver.
IIO triggered buffer infrastructure along with iio sysfs trigger
is used to leverage continuous sampling support provided by the
ADC block.
Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
drivers/iio/adc/Kconfig | 4 ++
drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
2 files changed, 105 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 7c55658..4661241 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -337,6 +337,10 @@ config TWL6030_GPADC
config VF610_ADC
tristate "Freescale vf610 ADC driver"
depends on OF
+ select IIO_BUFFER
+ select IIO_TRIGGER
+ select IIO_SYSFS_TRIGGER
+ select IIO_TRIGGERED_BUFFER
help
Say yes here to support for Vybrid board analog-to-digital converter.
Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index 23b8fb9..97cb2ed 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -34,8 +34,11 @@
#include <linux/err.h>
#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
#include <linux/iio/sysfs.h>
-#include <linux/iio/driver.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
/* This will be the driver name the kernel reports */
#define DRIVER_NAME "vf610-adc"
@@ -170,6 +173,7 @@ struct vf610_adc {
u32 sample_freq_avail[5];
struct completion completion;
+ u16 buffer[2];
};
static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
@@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_SAMP_FREQ), \
.ext_info = vf610_ext_info, \
+ .address = (_idx), \
+ .scan_index = (_idx), \
+ .scan_type.sign = 'u', \
+ .scan_type.realbits = 12, \
+ .scan_type.storagebits = 16, \
}
#define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
.type = (_chan_type), \
.channel = (_idx), \
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
+ .address = (_idx), \
+ .scan_index = (_idx), \
+ .scan_type.sign = 'u', \
+ .scan_type.realbits = 12, \
+ .scan_type.storagebits = 16, \
}
static const struct iio_chan_spec vf610_adc_iio_channels[] = {
@@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
VF610_ADC_CHAN(14, IIO_VOLTAGE),
VF610_ADC_CHAN(15, IIO_VOLTAGE),
VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
+ IIO_CHAN_SOFT_TIMESTAMP(32),
/* sentinel */
};
@@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
{
- struct vf610_adc *info = (struct vf610_adc *)dev_id;
+ struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
+ struct vf610_adc *info = iio_priv(indio_dev);
int coco;
coco = readl(info->regs + VF610_REG_ADC_HS);
if (coco & VF610_ADC_HS_COCO0) {
info->value = vf610_adc_read_data(info);
- complete(&info->completion);
+ if (iio_buffer_enabled(indio_dev)) {
+ info->buffer[0] = info->value;
+ iio_push_to_buffers_with_timestamp(indio_dev,
+ info->buffer, iio_get_time_ns());
+ iio_trigger_notify_done(indio_dev->trig);
+ } else
+ complete(&info->completion);
}
return IRQ_HANDLED;
@@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
case IIO_CHAN_INFO_PROCESSED:
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
+
mutex_lock(&indio_dev->mlock);
reinit_completion(&info->completion);
@@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+ struct vf610_adc *info = iio_priv(indio_dev);
+ unsigned int channel;
+ int ret;
+ int val;
+
+ ret = iio_triggered_buffer_postenable(indio_dev);
+ if (ret)
+ return ret;
+
+ val = readl(info->regs + VF610_REG_ADC_GC);
+ val |= VF610_ADC_ADCON;
+ writel(val, info->regs + VF610_REG_ADC_GC);
+
+ channel = find_first_bit(indio_dev->active_scan_mask,
+ indio_dev->masklength);
+
+ val = VF610_ADC_ADCHC(channel);
+ val |= VF610_ADC_AIEN;
+
+ writel(val, info->regs + VF610_REG_ADC_HC0);
+
+ return 0;
+}
+
+static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct vf610_adc *info = iio_priv(indio_dev);
+ unsigned int hc_cfg = 0;
+ int val;
+
+ val = readl(info->regs + VF610_REG_ADC_GC);
+ val &= ~VF610_ADC_ADCON;
+ writel(val, info->regs + VF610_REG_ADC_GC);
+
+ hc_cfg |= VF610_ADC_CONV_DISABLE;
+ hc_cfg &= ~VF610_ADC_AIEN;
+
+ writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
+
+ return 0;
+}
+
+static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
+ .postenable = &vf610_adc_buffer_postenable,
+ .predisable = &iio_triggered_buffer_predisable,
+ .postdisable = &vf610_adc_buffer_postdisable,
+ .validate_scan_mask = &iio_validate_scan_mask_onehot,
+};
+
+static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
+{
+ return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
+ NULL, &iio_triggered_buffer_setup_ops);
+}
+
+static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
+{
+ iio_triggered_buffer_cleanup(indio_dev);
+}
+
static int vf610_adc_reg_access(struct iio_dev *indio_dev,
unsigned reg, unsigned writeval,
unsigned *readval)
@@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
ret = devm_request_irq(info->dev, irq,
vf610_adc_isr, 0,
- dev_name(&pdev->dev), info);
+ dev_name(&pdev->dev), indio_dev);
if (ret < 0) {
dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
return ret;
@@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
vf610_adc_cfg_init(info);
vf610_adc_hw_init(info);
+ ret = vf610_adc_buffer_init(indio_dev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
+ goto error_iio_device_register;
+ }
+
ret = iio_device_register(indio_dev);
if (ret) {
dev_err(&pdev->dev, "Couldn't register the device.\n");
- goto error_iio_device_register;
+ goto error_adc_buffer_init;
}
return 0;
-
+error_adc_buffer_init:
+ vf610_adc_buffer_remove(indio_dev);
error_iio_device_register:
clk_disable_unprepare(info->clk);
error_adc_clk_enable:
@@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
struct vf610_adc *info = iio_priv(indio_dev);
iio_device_unregister(indio_dev);
+ vf610_adc_buffer_remove(indio_dev);
regulator_disable(info->vref);
clk_disable_unprepare(info->clk);
--
2.5.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
@ 2015-08-05 12:10 ` Sanchayan Maity
0 siblings, 0 replies; 12+ messages in thread
From: Sanchayan Maity @ 2015-08-05 12:10 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds support for IIO buffer to the Vybrid ADC driver.
IIO triggered buffer infrastructure along with iio sysfs trigger
is used to leverage continuous sampling support provided by the
ADC block.
Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
drivers/iio/adc/Kconfig | 4 ++
drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
2 files changed, 105 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 7c55658..4661241 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -337,6 +337,10 @@ config TWL6030_GPADC
config VF610_ADC
tristate "Freescale vf610 ADC driver"
depends on OF
+ select IIO_BUFFER
+ select IIO_TRIGGER
+ select IIO_SYSFS_TRIGGER
+ select IIO_TRIGGERED_BUFFER
help
Say yes here to support for Vybrid board analog-to-digital converter.
Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index 23b8fb9..97cb2ed 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -34,8 +34,11 @@
#include <linux/err.h>
#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
#include <linux/iio/sysfs.h>
-#include <linux/iio/driver.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
/* This will be the driver name the kernel reports */
#define DRIVER_NAME "vf610-adc"
@@ -170,6 +173,7 @@ struct vf610_adc {
u32 sample_freq_avail[5];
struct completion completion;
+ u16 buffer[2];
};
static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
@@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_SAMP_FREQ), \
.ext_info = vf610_ext_info, \
+ .address = (_idx), \
+ .scan_index = (_idx), \
+ .scan_type.sign = 'u', \
+ .scan_type.realbits = 12, \
+ .scan_type.storagebits = 16, \
}
#define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
.type = (_chan_type), \
.channel = (_idx), \
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
+ .address = (_idx), \
+ .scan_index = (_idx), \
+ .scan_type.sign = 'u', \
+ .scan_type.realbits = 12, \
+ .scan_type.storagebits = 16, \
}
static const struct iio_chan_spec vf610_adc_iio_channels[] = {
@@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
VF610_ADC_CHAN(14, IIO_VOLTAGE),
VF610_ADC_CHAN(15, IIO_VOLTAGE),
VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
+ IIO_CHAN_SOFT_TIMESTAMP(32),
/* sentinel */
};
@@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
{
- struct vf610_adc *info = (struct vf610_adc *)dev_id;
+ struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
+ struct vf610_adc *info = iio_priv(indio_dev);
int coco;
coco = readl(info->regs + VF610_REG_ADC_HS);
if (coco & VF610_ADC_HS_COCO0) {
info->value = vf610_adc_read_data(info);
- complete(&info->completion);
+ if (iio_buffer_enabled(indio_dev)) {
+ info->buffer[0] = info->value;
+ iio_push_to_buffers_with_timestamp(indio_dev,
+ info->buffer, iio_get_time_ns());
+ iio_trigger_notify_done(indio_dev->trig);
+ } else
+ complete(&info->completion);
}
return IRQ_HANDLED;
@@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
case IIO_CHAN_INFO_PROCESSED:
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
+
mutex_lock(&indio_dev->mlock);
reinit_completion(&info->completion);
@@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+ struct vf610_adc *info = iio_priv(indio_dev);
+ unsigned int channel;
+ int ret;
+ int val;
+
+ ret = iio_triggered_buffer_postenable(indio_dev);
+ if (ret)
+ return ret;
+
+ val = readl(info->regs + VF610_REG_ADC_GC);
+ val |= VF610_ADC_ADCON;
+ writel(val, info->regs + VF610_REG_ADC_GC);
+
+ channel = find_first_bit(indio_dev->active_scan_mask,
+ indio_dev->masklength);
+
+ val = VF610_ADC_ADCHC(channel);
+ val |= VF610_ADC_AIEN;
+
+ writel(val, info->regs + VF610_REG_ADC_HC0);
+
+ return 0;
+}
+
+static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct vf610_adc *info = iio_priv(indio_dev);
+ unsigned int hc_cfg = 0;
+ int val;
+
+ val = readl(info->regs + VF610_REG_ADC_GC);
+ val &= ~VF610_ADC_ADCON;
+ writel(val, info->regs + VF610_REG_ADC_GC);
+
+ hc_cfg |= VF610_ADC_CONV_DISABLE;
+ hc_cfg &= ~VF610_ADC_AIEN;
+
+ writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
+
+ return 0;
+}
+
+static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
+ .postenable = &vf610_adc_buffer_postenable,
+ .predisable = &iio_triggered_buffer_predisable,
+ .postdisable = &vf610_adc_buffer_postdisable,
+ .validate_scan_mask = &iio_validate_scan_mask_onehot,
+};
+
+static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
+{
+ return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
+ NULL, &iio_triggered_buffer_setup_ops);
+}
+
+static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
+{
+ iio_triggered_buffer_cleanup(indio_dev);
+}
+
static int vf610_adc_reg_access(struct iio_dev *indio_dev,
unsigned reg, unsigned writeval,
unsigned *readval)
@@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
ret = devm_request_irq(info->dev, irq,
vf610_adc_isr, 0,
- dev_name(&pdev->dev), info);
+ dev_name(&pdev->dev), indio_dev);
if (ret < 0) {
dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
return ret;
@@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
vf610_adc_cfg_init(info);
vf610_adc_hw_init(info);
+ ret = vf610_adc_buffer_init(indio_dev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
+ goto error_iio_device_register;
+ }
+
ret = iio_device_register(indio_dev);
if (ret) {
dev_err(&pdev->dev, "Couldn't register the device.\n");
- goto error_iio_device_register;
+ goto error_adc_buffer_init;
}
return 0;
-
+error_adc_buffer_init:
+ vf610_adc_buffer_remove(indio_dev);
error_iio_device_register:
clk_disable_unprepare(info->clk);
error_adc_clk_enable:
@@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
struct vf610_adc *info = iio_priv(indio_dev);
iio_device_unregister(indio_dev);
+ vf610_adc_buffer_remove(indio_dev);
regulator_disable(info->vref);
clk_disable_unprepare(info->clk);
--
2.5.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
2015-08-05 12:10 ` Sanchayan Maity
@ 2015-08-08 14:29 ` Jonathan Cameron
-1 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2015-08-08 14:29 UTC (permalink / raw)
To: Sanchayan Maity, linux-iio
Cc: stefan, B38611, hofrat, sanjeev_sharma, fabio.estevam, knaack.h,
lars, pmeerw, antoine.tenart, linux-kernel, linux-arm-kernel
On 05/08/15 13:10, Sanchayan Maity wrote:
> This patch adds support for IIO buffer to the Vybrid ADC driver.
> IIO triggered buffer infrastructure along with iio sysfs trigger
> is used to leverage continuous sampling support provided by the
> ADC block.
Looking good. Just a couple more bits and pieces inline from me.
One or two of which aren't corrected from Peter's review of v1.
I will want Fugang Dong's ack / review tag on the final version
before applying it however. This driver is some distance form my
area of expertise!
Jonathan
>
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
> drivers/iio/adc/Kconfig | 4 ++
> drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 105 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7c55658..4661241 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -337,6 +337,10 @@ config TWL6030_GPADC
> config VF610_ADC
> tristate "Freescale vf610 ADC driver"
> depends on OF
> + select IIO_BUFFER
> + select IIO_TRIGGER
> + select IIO_SYSFS_TRIGGER
Unless I missed something there is no dependency on this particular
trigger (it just happens to be the one you've been testing with I guess).
Could be driven from a hardware trigger belonging to another device for
example.
> + select IIO_TRIGGERED_BUFFER
> help
> Say yes here to support for Vybrid board analog-to-digital converter.
> Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index 23b8fb9..97cb2ed 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -34,8 +34,11 @@
> #include <linux/err.h>
>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/sysfs.h>
> -#include <linux/iio/driver.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> /* This will be the driver name the kernel reports */
> #define DRIVER_NAME "vf610-adc"
> @@ -170,6 +173,7 @@ struct vf610_adc {
> u32 sample_freq_avail[5];
>
> struct completion completion;
> + u16 buffer[2];
Note the requirements on the buffer provided to
iio_push_to_buffers_with_timestamp
Needs to be u16 buffer[8] to allow for the aligned 64bit (4 word) timestamp.
Peter pointed this out in his follow up email and you said you'd implement
it.. Guessing this got lost somewhere.
> };
>
> static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> @@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> .ext_info = vf610_ext_info, \
> + .address = (_idx), \
> + .scan_index = (_idx), \
> + .scan_type.sign = 'u', \
> + .scan_type.realbits = 12, \
> + .scan_type.storagebits = 16, \
> }
>
> #define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
> .type = (_chan_type), \
> .channel = (_idx), \
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .address = (_idx), \
> + .scan_index = (_idx), \
.scan_type = {
.sign = 'u',
etc.
Peter picked up on this..
> + .scan_type.sign = 'u', \
> + .scan_type.realbits = 12, \
> + .scan_type.storagebits = 16, \
> }
>
> static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> @@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> VF610_ADC_CHAN(14, IIO_VOLTAGE),
> VF610_ADC_CHAN(15, IIO_VOLTAGE),
> VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
> + IIO_CHAN_SOFT_TIMESTAMP(32),
It's bit extreme throwing it out at scan_index 32. Is there a reason
to think that migh be neccesary? Mind you, why is the temperature
channel down at 26? Are we dealing with a set of reserved real channels
inbetween?
> /* sentinel */
> };
>
> @@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
>
> static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
> {
> - struct vf610_adc *info = (struct vf610_adc *)dev_id;
> + struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
> + struct vf610_adc *info = iio_priv(indio_dev);
> int coco;
>
> coco = readl(info->regs + VF610_REG_ADC_HS);
> if (coco & VF610_ADC_HS_COCO0) {
> info->value = vf610_adc_read_data(info);
I'd be tempted to make the non buffered path also use
info->bufffer[0] and drop info->value entirely.
A more invasive patch, but a cleaner resulting code (slightly!)
> - complete(&info->completion);
> + if (iio_buffer_enabled(indio_dev)) {
> + info->buffer[0] = info->value;
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + info->buffer, iio_get_time_ns());
> + iio_trigger_notify_done(indio_dev->trig);
> + } else
> + complete(&info->completion);
> }
>
> return IRQ_HANDLED;
> @@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> case IIO_CHAN_INFO_PROCESSED:
To avoid possible races this check should be done under the mlock.
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
> +
> mutex_lock(&indio_dev->mlock);
> reinit_completion(&info->completion);
>
> @@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
> return -EINVAL;
> }
>
> +static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct vf610_adc *info = iio_priv(indio_dev);
> + unsigned int channel;
> + int ret;
> + int val;
> +
> + ret = iio_triggered_buffer_postenable(indio_dev);
> + if (ret)
> + return ret;
> +
> + val = readl(info->regs + VF610_REG_ADC_GC);
> + val |= VF610_ADC_ADCON;
> + writel(val, info->regs + VF610_REG_ADC_GC);
> +
> + channel = find_first_bit(indio_dev->active_scan_mask,
> + indio_dev->masklength);
> +
> + val = VF610_ADC_ADCHC(channel);
> + val |= VF610_ADC_AIEN;
> +
> + writel(val, info->regs + VF610_REG_ADC_HC0);
> +
> + return 0;
> +}
> +
> +static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct vf610_adc *info = iio_priv(indio_dev);
> + unsigned int hc_cfg = 0;
> + int val;
> +
> + val = readl(info->regs + VF610_REG_ADC_GC);
> + val &= ~VF610_ADC_ADCON;
> + writel(val, info->regs + VF610_REG_ADC_GC);
> +
> + hc_cfg |= VF610_ADC_CONV_DISABLE;
> + hc_cfg &= ~VF610_ADC_AIEN;
> +
> + writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> +
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
> + .postenable = &vf610_adc_buffer_postenable,
> + .predisable = &iio_triggered_buffer_predisable,
> + .postdisable = &vf610_adc_buffer_postdisable,
> + .validate_scan_mask = &iio_validate_scan_mask_onehot,
> +};
> +
> +static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
> +{
> + return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
> + NULL, &iio_triggered_buffer_setup_ops);
> +}
> +
> +static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
> +{
> + iio_triggered_buffer_cleanup(indio_dev);
> +}
These to wrappers seem a little superflous. I'd have just put the code
inline, but it's obviously a matter of personal taste and I don't care
that much!
> +
> static int vf610_adc_reg_access(struct iio_dev *indio_dev,
> unsigned reg, unsigned writeval,
> unsigned *readval)
> @@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
>
> ret = devm_request_irq(info->dev, irq,
> vf610_adc_isr, 0,
> - dev_name(&pdev->dev), info);
> + dev_name(&pdev->dev), indio_dev);
> if (ret < 0) {
> dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> return ret;
> @@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
> vf610_adc_cfg_init(info);
> vf610_adc_hw_init(info);
>
> + ret = vf610_adc_buffer_init(indio_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
> + goto error_iio_device_register;
> + }
> +
> ret = iio_device_register(indio_dev);
> if (ret) {
> dev_err(&pdev->dev, "Couldn't register the device.\n");
> - goto error_iio_device_register;
> + goto error_adc_buffer_init;
> }
>
> return 0;
>
> -
> +error_adc_buffer_init:
> + vf610_adc_buffer_remove(indio_dev);
> error_iio_device_register:
> clk_disable_unprepare(info->clk);
> error_adc_clk_enable:
> @@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
> struct vf610_adc *info = iio_priv(indio_dev);
>
> iio_device_unregister(indio_dev);
> + vf610_adc_buffer_remove(indio_dev);
> regulator_disable(info->vref);
> clk_disable_unprepare(info->clk);
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
@ 2015-08-08 14:29 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2015-08-08 14:29 UTC (permalink / raw)
To: linux-arm-kernel
On 05/08/15 13:10, Sanchayan Maity wrote:
> This patch adds support for IIO buffer to the Vybrid ADC driver.
> IIO triggered buffer infrastructure along with iio sysfs trigger
> is used to leverage continuous sampling support provided by the
> ADC block.
Looking good. Just a couple more bits and pieces inline from me.
One or two of which aren't corrected from Peter's review of v1.
I will want Fugang Dong's ack / review tag on the final version
before applying it however. This driver is some distance form my
area of expertise!
Jonathan
>
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
> drivers/iio/adc/Kconfig | 4 ++
> drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 105 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7c55658..4661241 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -337,6 +337,10 @@ config TWL6030_GPADC
> config VF610_ADC
> tristate "Freescale vf610 ADC driver"
> depends on OF
> + select IIO_BUFFER
> + select IIO_TRIGGER
> + select IIO_SYSFS_TRIGGER
Unless I missed something there is no dependency on this particular
trigger (it just happens to be the one you've been testing with I guess).
Could be driven from a hardware trigger belonging to another device for
example.
> + select IIO_TRIGGERED_BUFFER
> help
> Say yes here to support for Vybrid board analog-to-digital converter.
> Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index 23b8fb9..97cb2ed 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -34,8 +34,11 @@
> #include <linux/err.h>
>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/sysfs.h>
> -#include <linux/iio/driver.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> /* This will be the driver name the kernel reports */
> #define DRIVER_NAME "vf610-adc"
> @@ -170,6 +173,7 @@ struct vf610_adc {
> u32 sample_freq_avail[5];
>
> struct completion completion;
> + u16 buffer[2];
Note the requirements on the buffer provided to
iio_push_to_buffers_with_timestamp
Needs to be u16 buffer[8] to allow for the aligned 64bit (4 word) timestamp.
Peter pointed this out in his follow up email and you said you'd implement
it.. Guessing this got lost somewhere.
> };
>
> static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> @@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> .ext_info = vf610_ext_info, \
> + .address = (_idx), \
> + .scan_index = (_idx), \
> + .scan_type.sign = 'u', \
> + .scan_type.realbits = 12, \
> + .scan_type.storagebits = 16, \
> }
>
> #define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
> .type = (_chan_type), \
> .channel = (_idx), \
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .address = (_idx), \
> + .scan_index = (_idx), \
.scan_type = {
.sign = 'u',
etc.
Peter picked up on this..
> + .scan_type.sign = 'u', \
> + .scan_type.realbits = 12, \
> + .scan_type.storagebits = 16, \
> }
>
> static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> @@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> VF610_ADC_CHAN(14, IIO_VOLTAGE),
> VF610_ADC_CHAN(15, IIO_VOLTAGE),
> VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
> + IIO_CHAN_SOFT_TIMESTAMP(32),
It's bit extreme throwing it out at scan_index 32. Is there a reason
to think that migh be neccesary? Mind you, why is the temperature
channel down at 26? Are we dealing with a set of reserved real channels
inbetween?
> /* sentinel */
> };
>
> @@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
>
> static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
> {
> - struct vf610_adc *info = (struct vf610_adc *)dev_id;
> + struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
> + struct vf610_adc *info = iio_priv(indio_dev);
> int coco;
>
> coco = readl(info->regs + VF610_REG_ADC_HS);
> if (coco & VF610_ADC_HS_COCO0) {
> info->value = vf610_adc_read_data(info);
I'd be tempted to make the non buffered path also use
info->bufffer[0] and drop info->value entirely.
A more invasive patch, but a cleaner resulting code (slightly!)
> - complete(&info->completion);
> + if (iio_buffer_enabled(indio_dev)) {
> + info->buffer[0] = info->value;
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + info->buffer, iio_get_time_ns());
> + iio_trigger_notify_done(indio_dev->trig);
> + } else
> + complete(&info->completion);
> }
>
> return IRQ_HANDLED;
> @@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> case IIO_CHAN_INFO_PROCESSED:
To avoid possible races this check should be done under the mlock.
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
> +
> mutex_lock(&indio_dev->mlock);
> reinit_completion(&info->completion);
>
> @@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
> return -EINVAL;
> }
>
> +static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct vf610_adc *info = iio_priv(indio_dev);
> + unsigned int channel;
> + int ret;
> + int val;
> +
> + ret = iio_triggered_buffer_postenable(indio_dev);
> + if (ret)
> + return ret;
> +
> + val = readl(info->regs + VF610_REG_ADC_GC);
> + val |= VF610_ADC_ADCON;
> + writel(val, info->regs + VF610_REG_ADC_GC);
> +
> + channel = find_first_bit(indio_dev->active_scan_mask,
> + indio_dev->masklength);
> +
> + val = VF610_ADC_ADCHC(channel);
> + val |= VF610_ADC_AIEN;
> +
> + writel(val, info->regs + VF610_REG_ADC_HC0);
> +
> + return 0;
> +}
> +
> +static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct vf610_adc *info = iio_priv(indio_dev);
> + unsigned int hc_cfg = 0;
> + int val;
> +
> + val = readl(info->regs + VF610_REG_ADC_GC);
> + val &= ~VF610_ADC_ADCON;
> + writel(val, info->regs + VF610_REG_ADC_GC);
> +
> + hc_cfg |= VF610_ADC_CONV_DISABLE;
> + hc_cfg &= ~VF610_ADC_AIEN;
> +
> + writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> +
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
> + .postenable = &vf610_adc_buffer_postenable,
> + .predisable = &iio_triggered_buffer_predisable,
> + .postdisable = &vf610_adc_buffer_postdisable,
> + .validate_scan_mask = &iio_validate_scan_mask_onehot,
> +};
> +
> +static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
> +{
> + return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
> + NULL, &iio_triggered_buffer_setup_ops);
> +}
> +
> +static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
> +{
> + iio_triggered_buffer_cleanup(indio_dev);
> +}
These to wrappers seem a little superflous. I'd have just put the code
inline, but it's obviously a matter of personal taste and I don't care
that much!
> +
> static int vf610_adc_reg_access(struct iio_dev *indio_dev,
> unsigned reg, unsigned writeval,
> unsigned *readval)
> @@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
>
> ret = devm_request_irq(info->dev, irq,
> vf610_adc_isr, 0,
> - dev_name(&pdev->dev), info);
> + dev_name(&pdev->dev), indio_dev);
> if (ret < 0) {
> dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> return ret;
> @@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
> vf610_adc_cfg_init(info);
> vf610_adc_hw_init(info);
>
> + ret = vf610_adc_buffer_init(indio_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
> + goto error_iio_device_register;
> + }
> +
> ret = iio_device_register(indio_dev);
> if (ret) {
> dev_err(&pdev->dev, "Couldn't register the device.\n");
> - goto error_iio_device_register;
> + goto error_adc_buffer_init;
> }
>
> return 0;
>
> -
> +error_adc_buffer_init:
> + vf610_adc_buffer_remove(indio_dev);
> error_iio_device_register:
> clk_disable_unprepare(info->clk);
> error_adc_clk_enable:
> @@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
> struct vf610_adc *info = iio_priv(indio_dev);
>
> iio_device_unregister(indio_dev);
> + vf610_adc_buffer_remove(indio_dev);
> regulator_disable(info->vref);
> clk_disable_unprepare(info->clk);
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
2015-08-08 14:29 ` Jonathan Cameron
@ 2015-08-08 17:22 ` maitysanchayan at gmail.com
-1 siblings, 0 replies; 12+ messages in thread
From: maitysanchayan @ 2015-08-08 17:22 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, stefan, B38611, hofrat, sanjeev_sharma, fabio.estevam,
knaack.h, lars, pmeerw, antoine.tenart, linux-kernel,
linux-arm-kernel
Hello Jonathan,
On 15-08-08 15:29:47, Jonathan Cameron wrote:
> On 05/08/15 13:10, Sanchayan Maity wrote:
> > This patch adds support for IIO buffer to the Vybrid ADC driver.
> > IIO triggered buffer infrastructure along with iio sysfs trigger
> > is used to leverage continuous sampling support provided by the
> > ADC block.
> Looking good. Just a couple more bits and pieces inline from me.
> One or two of which aren't corrected from Peter's review of v1.
>
> I will want Fugang Dong's ack / review tag on the final version
> before applying it however.
Sure.
> This driver is some distance form my area of expertise!
I doubt :).
>
> Jonathan
> >
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> > drivers/iio/adc/Kconfig | 4 ++
> > drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
> > 2 files changed, 105 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 7c55658..4661241 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -337,6 +337,10 @@ config TWL6030_GPADC
> > config VF610_ADC
> > tristate "Freescale vf610 ADC driver"
> > depends on OF
> > + select IIO_BUFFER
> > + select IIO_TRIGGER
> > + select IIO_SYSFS_TRIGGER
> Unless I missed something there is no dependency on this particular
> trigger (it just happens to be the one you've been testing with I guess).
> Could be driven from a hardware trigger belonging to another device for
> example.
Yes right in a way. Right now we do not provide or there is no provision
for hardware triggers. On the Vybrid, the Peripheral Delay Block (PDB)
does the job of providing support for software and hardware triggers. PDB
support is not yet there in Linux however.
There is also the question of where the Vybrid PDB driver would belong
to? In the TRM it is put in the timers but the kernel has no generic timer
framework that I am aware of. After some internal reviews we decided to
do a platform driver which provided functions ADC driver could called into.
I have a patchset ready which provides trigger support using PDB however
configuring the PDB properly has proven to be tricky. While it works but
not reliably with multiple channels and it would be a while before I get
that working and post that patchset. So kind of stalled there and just
because of two registers which need to be written with the correct value
:). For what it's worth if someone comes across this, some discussion
here [1] along with patches. (Note however those are a bit old patches
not exactly my new work).
Sorry for digressing from the topic. Anyways so the idea was to provide
sysfs triggers as default for using this continuous sampling. Later the
driver may provide additional triggers. So for now I added the sysfs
trigger as a select option so that a user won't have to recompile the
kernel for using the buffers with continuous sampling.
>
> > + select IIO_TRIGGERED_BUFFER
> > help
> > Say yes here to support for Vybrid board analog-to-digital converter.
> > Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
> > diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> > index 23b8fb9..97cb2ed 100644
> > --- a/drivers/iio/adc/vf610_adc.c
> > +++ b/drivers/iio/adc/vf610_adc.c
> > @@ -34,8 +34,11 @@
> > #include <linux/err.h>
> >
> > #include <linux/iio/iio.h>
> > +#include <linux/iio/buffer.h>
> > #include <linux/iio/sysfs.h>
> > -#include <linux/iio/driver.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> >
> > /* This will be the driver name the kernel reports */
> > #define DRIVER_NAME "vf610-adc"
> > @@ -170,6 +173,7 @@ struct vf610_adc {
> > u32 sample_freq_avail[5];
> >
> > struct completion completion;
> > + u16 buffer[2];
> Note the requirements on the buffer provided to
> iio_push_to_buffers_with_timestamp
> Needs to be u16 buffer[8] to allow for the aligned 64bit (4 word) timestamp.
>
> Peter pointed this out in his follow up email and you said you'd implement
> it.. Guessing this got lost somewhere.
No, I meant to implement what Peter recommended but I guess I did not completely
grasp what he intended. Sorry about that. Will fix this and ask further if in
more doubts.
>
>
> > };
> >
> > static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> > @@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
> > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> > BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> > .ext_info = vf610_ext_info, \
> > + .address = (_idx), \
> > + .scan_index = (_idx), \
> > + .scan_type.sign = 'u', \
> > + .scan_type.realbits = 12, \
> > + .scan_type.storagebits = 16, \
> > }
> >
> > #define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
> > .type = (_chan_type), \
> > .channel = (_idx), \
> > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .address = (_idx), \
> > + .scan_index = (_idx), \
> .scan_type = {
> .sign = 'u',
> etc.
>
> Peter picked up on this..
Sorry yes missed that. Will definitely fix.
>
> > + .scan_type.sign = 'u', \
> > + .scan_type.realbits = 12, \
> > + .scan_type.storagebits = 16, \
> > }
> >
> > static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> > @@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> > VF610_ADC_CHAN(14, IIO_VOLTAGE),
> > VF610_ADC_CHAN(15, IIO_VOLTAGE),
> > VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
> > + IIO_CHAN_SOFT_TIMESTAMP(32),
> It's bit extreme throwing it out at scan_index 32. Is there a reason
> to think that migh be neccesary? Mind you, why is the temperature
> channel down at 26? Are we dealing with a set of reserved real channels
> inbetween?
The temperature channel is the 26th channel and there are some reserved
channels in between. 31st is the channel which acts as a conversion
disabled setting. So I put the timestamp at index 32.
> > /* sentinel */
> > };
> >
> > @@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
> >
> > static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
> > {
> > - struct vf610_adc *info = (struct vf610_adc *)dev_id;
> > + struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
> > + struct vf610_adc *info = iio_priv(indio_dev);
> > int coco;
> >
> > coco = readl(info->regs + VF610_REG_ADC_HS);
> > if (coco & VF610_ADC_HS_COCO0) {
> > info->value = vf610_adc_read_data(info);
>
> I'd be tempted to make the non buffered path also use
> info->bufffer[0] and drop info->value entirely.
> A more invasive patch, but a cleaner resulting code (slightly!)
I did think of that but decided against it as I wanted the changes
to as less invasive as possible making only the necessary changes
and keeping the old code as is.
> > - complete(&info->completion);
> > + if (iio_buffer_enabled(indio_dev)) {
> > + info->buffer[0] = info->value;
> > + iio_push_to_buffers_with_timestamp(indio_dev,
> > + info->buffer, iio_get_time_ns());
> > + iio_trigger_notify_done(indio_dev->trig);
> > + } else
> > + complete(&info->completion);
> > }
> >
> > return IRQ_HANDLED;
> > @@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > case IIO_CHAN_INFO_PROCESSED:
>
> To avoid possible races this check should be done under the mlock
Thanks for picking this. Somehow I remember seeing it outside of the
mlock. However grepping again shows otherwise. Will fix.
.
> > + if (iio_buffer_enabled(indio_dev))
> > + return -EBUSY;
> > +
> > mutex_lock(&indio_dev->mlock);
> > reinit_completion(&info->completion);
> >
> > @@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
> > return -EINVAL;
> > }
> >
> > +static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > + struct vf610_adc *info = iio_priv(indio_dev);
> > + unsigned int channel;
> > + int ret;
> > + int val;
> > +
> > + ret = iio_triggered_buffer_postenable(indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + val = readl(info->regs + VF610_REG_ADC_GC);
> > + val |= VF610_ADC_ADCON;
> > + writel(val, info->regs + VF610_REG_ADC_GC);
> > +
> > + channel = find_first_bit(indio_dev->active_scan_mask,
> > + indio_dev->masklength);
> > +
> > + val = VF610_ADC_ADCHC(channel);
> > + val |= VF610_ADC_AIEN;
> > +
> > + writel(val, info->regs + VF610_REG_ADC_HC0);
> > +
> > + return 0;
> > +}
> > +
> > +static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > + struct vf610_adc *info = iio_priv(indio_dev);
> > + unsigned int hc_cfg = 0;
> > + int val;
> > +
> > + val = readl(info->regs + VF610_REG_ADC_GC);
> > + val &= ~VF610_ADC_ADCON;
> > + writel(val, info->regs + VF610_REG_ADC_GC);
> > +
> > + hc_cfg |= VF610_ADC_CONV_DISABLE;
> > + hc_cfg &= ~VF610_ADC_AIEN;
> > +
> > + writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
> > + .postenable = &vf610_adc_buffer_postenable,
> > + .predisable = &iio_triggered_buffer_predisable,
> > + .postdisable = &vf610_adc_buffer_postdisable,
> > + .validate_scan_mask = &iio_validate_scan_mask_onehot,
> > +};
> > +
> > +static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
> > +{
> > + return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
> > + NULL, &iio_triggered_buffer_setup_ops);
> > +}
> > +
> > +static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
> > +{
> > + iio_triggered_buffer_cleanup(indio_dev);
> > +}
> These to wrappers seem a little superflous. I'd have just put the code
> inline, but it's obviously a matter of personal taste and I don't care
> that much!
Ok. I do not have strong opinions on this. I just tried to follow how the
at91 adc code did it considering it as a good example. Will drop it.
>
> > +
> > static int vf610_adc_reg_access(struct iio_dev *indio_dev,
> > unsigned reg, unsigned writeval,
> > unsigned *readval)
> > @@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
> >
> > ret = devm_request_irq(info->dev, irq,
> > vf610_adc_isr, 0,
> > - dev_name(&pdev->dev), info);
> > + dev_name(&pdev->dev), indio_dev);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> > return ret;
> > @@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
> > vf610_adc_cfg_init(info);
> > vf610_adc_hw_init(info);
> >
> > + ret = vf610_adc_buffer_init(indio_dev);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
> > + goto error_iio_device_register;
> > + }
> > +
> > ret = iio_device_register(indio_dev);
> > if (ret) {
> > dev_err(&pdev->dev, "Couldn't register the device.\n");
> > - goto error_iio_device_register;
> > + goto error_adc_buffer_init;
> > }
> >
> > return 0;
> >
> > -
> > +error_adc_buffer_init:
> > + vf610_adc_buffer_remove(indio_dev);
> > error_iio_device_register:
> > clk_disable_unprepare(info->clk);
> > error_adc_clk_enable:
> > @@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
> > struct vf610_adc *info = iio_priv(indio_dev);
> >
> > iio_device_unregister(indio_dev);
> > + vf610_adc_buffer_remove(indio_dev);
> > regulator_disable(info->vref);
> > clk_disable_unprepare(info->clk);
> >
> >
>
Thanks for the review.
- Sanchayan.
[1]. https://community.freescale.com/thread/357619
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
@ 2015-08-08 17:22 ` maitysanchayan at gmail.com
0 siblings, 0 replies; 12+ messages in thread
From: maitysanchayan at gmail.com @ 2015-08-08 17:22 UTC (permalink / raw)
To: linux-arm-kernel
Hello Jonathan,
On 15-08-08 15:29:47, Jonathan Cameron wrote:
> On 05/08/15 13:10, Sanchayan Maity wrote:
> > This patch adds support for IIO buffer to the Vybrid ADC driver.
> > IIO triggered buffer infrastructure along with iio sysfs trigger
> > is used to leverage continuous sampling support provided by the
> > ADC block.
> Looking good. Just a couple more bits and pieces inline from me.
> One or two of which aren't corrected from Peter's review of v1.
>
> I will want Fugang Dong's ack / review tag on the final version
> before applying it however.
Sure.
> This driver is some distance form my area of expertise!
I doubt :).
>
> Jonathan
> >
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> > drivers/iio/adc/Kconfig | 4 ++
> > drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
> > 2 files changed, 105 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 7c55658..4661241 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -337,6 +337,10 @@ config TWL6030_GPADC
> > config VF610_ADC
> > tristate "Freescale vf610 ADC driver"
> > depends on OF
> > + select IIO_BUFFER
> > + select IIO_TRIGGER
> > + select IIO_SYSFS_TRIGGER
> Unless I missed something there is no dependency on this particular
> trigger (it just happens to be the one you've been testing with I guess).
> Could be driven from a hardware trigger belonging to another device for
> example.
Yes right in a way. Right now we do not provide or there is no provision
for hardware triggers. On the Vybrid, the Peripheral Delay Block (PDB)
does the job of providing support for software and hardware triggers. PDB
support is not yet there in Linux however.
There is also the question of where the Vybrid PDB driver would belong
to? In the TRM it is put in the timers but the kernel has no generic timer
framework that I am aware of. After some internal reviews we decided to
do a platform driver which provided functions ADC driver could called into.
I have a patchset ready which provides trigger support using PDB however
configuring the PDB properly has proven to be tricky. While it works but
not reliably with multiple channels and it would be a while before I get
that working and post that patchset. So kind of stalled there and just
because of two registers which need to be written with the correct value
:). For what it's worth if someone comes across this, some discussion
here [1] along with patches. (Note however those are a bit old patches
not exactly my new work).
Sorry for digressing from the topic. Anyways so the idea was to provide
sysfs triggers as default for using this continuous sampling. Later the
driver may provide additional triggers. So for now I added the sysfs
trigger as a select option so that a user won't have to recompile the
kernel for using the buffers with continuous sampling.
>
> > + select IIO_TRIGGERED_BUFFER
> > help
> > Say yes here to support for Vybrid board analog-to-digital converter.
> > Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
> > diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> > index 23b8fb9..97cb2ed 100644
> > --- a/drivers/iio/adc/vf610_adc.c
> > +++ b/drivers/iio/adc/vf610_adc.c
> > @@ -34,8 +34,11 @@
> > #include <linux/err.h>
> >
> > #include <linux/iio/iio.h>
> > +#include <linux/iio/buffer.h>
> > #include <linux/iio/sysfs.h>
> > -#include <linux/iio/driver.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> >
> > /* This will be the driver name the kernel reports */
> > #define DRIVER_NAME "vf610-adc"
> > @@ -170,6 +173,7 @@ struct vf610_adc {
> > u32 sample_freq_avail[5];
> >
> > struct completion completion;
> > + u16 buffer[2];
> Note the requirements on the buffer provided to
> iio_push_to_buffers_with_timestamp
> Needs to be u16 buffer[8] to allow for the aligned 64bit (4 word) timestamp.
>
> Peter pointed this out in his follow up email and you said you'd implement
> it.. Guessing this got lost somewhere.
No, I meant to implement what Peter recommended but I guess I did not completely
grasp what he intended. Sorry about that. Will fix this and ask further if in
more doubts.
>
>
> > };
> >
> > static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> > @@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
> > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> > BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> > .ext_info = vf610_ext_info, \
> > + .address = (_idx), \
> > + .scan_index = (_idx), \
> > + .scan_type.sign = 'u', \
> > + .scan_type.realbits = 12, \
> > + .scan_type.storagebits = 16, \
> > }
> >
> > #define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
> > .type = (_chan_type), \
> > .channel = (_idx), \
> > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .address = (_idx), \
> > + .scan_index = (_idx), \
> .scan_type = {
> .sign = 'u',
> etc.
>
> Peter picked up on this..
Sorry yes missed that. Will definitely fix.
>
> > + .scan_type.sign = 'u', \
> > + .scan_type.realbits = 12, \
> > + .scan_type.storagebits = 16, \
> > }
> >
> > static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> > @@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> > VF610_ADC_CHAN(14, IIO_VOLTAGE),
> > VF610_ADC_CHAN(15, IIO_VOLTAGE),
> > VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
> > + IIO_CHAN_SOFT_TIMESTAMP(32),
> It's bit extreme throwing it out at scan_index 32. Is there a reason
> to think that migh be neccesary? Mind you, why is the temperature
> channel down at 26? Are we dealing with a set of reserved real channels
> inbetween?
The temperature channel is the 26th channel and there are some reserved
channels in between. 31st is the channel which acts as a conversion
disabled setting. So I put the timestamp at index 32.
> > /* sentinel */
> > };
> >
> > @@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
> >
> > static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
> > {
> > - struct vf610_adc *info = (struct vf610_adc *)dev_id;
> > + struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
> > + struct vf610_adc *info = iio_priv(indio_dev);
> > int coco;
> >
> > coco = readl(info->regs + VF610_REG_ADC_HS);
> > if (coco & VF610_ADC_HS_COCO0) {
> > info->value = vf610_adc_read_data(info);
>
> I'd be tempted to make the non buffered path also use
> info->bufffer[0] and drop info->value entirely.
> A more invasive patch, but a cleaner resulting code (slightly!)
I did think of that but decided against it as I wanted the changes
to as less invasive as possible making only the necessary changes
and keeping the old code as is.
> > - complete(&info->completion);
> > + if (iio_buffer_enabled(indio_dev)) {
> > + info->buffer[0] = info->value;
> > + iio_push_to_buffers_with_timestamp(indio_dev,
> > + info->buffer, iio_get_time_ns());
> > + iio_trigger_notify_done(indio_dev->trig);
> > + } else
> > + complete(&info->completion);
> > }
> >
> > return IRQ_HANDLED;
> > @@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > case IIO_CHAN_INFO_PROCESSED:
>
> To avoid possible races this check should be done under the mlock
Thanks for picking this. Somehow I remember seeing it outside of the
mlock. However grepping again shows otherwise. Will fix.
.
> > + if (iio_buffer_enabled(indio_dev))
> > + return -EBUSY;
> > +
> > mutex_lock(&indio_dev->mlock);
> > reinit_completion(&info->completion);
> >
> > @@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
> > return -EINVAL;
> > }
> >
> > +static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > + struct vf610_adc *info = iio_priv(indio_dev);
> > + unsigned int channel;
> > + int ret;
> > + int val;
> > +
> > + ret = iio_triggered_buffer_postenable(indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + val = readl(info->regs + VF610_REG_ADC_GC);
> > + val |= VF610_ADC_ADCON;
> > + writel(val, info->regs + VF610_REG_ADC_GC);
> > +
> > + channel = find_first_bit(indio_dev->active_scan_mask,
> > + indio_dev->masklength);
> > +
> > + val = VF610_ADC_ADCHC(channel);
> > + val |= VF610_ADC_AIEN;
> > +
> > + writel(val, info->regs + VF610_REG_ADC_HC0);
> > +
> > + return 0;
> > +}
> > +
> > +static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > + struct vf610_adc *info = iio_priv(indio_dev);
> > + unsigned int hc_cfg = 0;
> > + int val;
> > +
> > + val = readl(info->regs + VF610_REG_ADC_GC);
> > + val &= ~VF610_ADC_ADCON;
> > + writel(val, info->regs + VF610_REG_ADC_GC);
> > +
> > + hc_cfg |= VF610_ADC_CONV_DISABLE;
> > + hc_cfg &= ~VF610_ADC_AIEN;
> > +
> > + writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
> > + .postenable = &vf610_adc_buffer_postenable,
> > + .predisable = &iio_triggered_buffer_predisable,
> > + .postdisable = &vf610_adc_buffer_postdisable,
> > + .validate_scan_mask = &iio_validate_scan_mask_onehot,
> > +};
> > +
> > +static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
> > +{
> > + return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
> > + NULL, &iio_triggered_buffer_setup_ops);
> > +}
> > +
> > +static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
> > +{
> > + iio_triggered_buffer_cleanup(indio_dev);
> > +}
> These to wrappers seem a little superflous. I'd have just put the code
> inline, but it's obviously a matter of personal taste and I don't care
> that much!
Ok. I do not have strong opinions on this. I just tried to follow how the
at91 adc code did it considering it as a good example. Will drop it.
>
> > +
> > static int vf610_adc_reg_access(struct iio_dev *indio_dev,
> > unsigned reg, unsigned writeval,
> > unsigned *readval)
> > @@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
> >
> > ret = devm_request_irq(info->dev, irq,
> > vf610_adc_isr, 0,
> > - dev_name(&pdev->dev), info);
> > + dev_name(&pdev->dev), indio_dev);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> > return ret;
> > @@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
> > vf610_adc_cfg_init(info);
> > vf610_adc_hw_init(info);
> >
> > + ret = vf610_adc_buffer_init(indio_dev);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
> > + goto error_iio_device_register;
> > + }
> > +
> > ret = iio_device_register(indio_dev);
> > if (ret) {
> > dev_err(&pdev->dev, "Couldn't register the device.\n");
> > - goto error_iio_device_register;
> > + goto error_adc_buffer_init;
> > }
> >
> > return 0;
> >
> > -
> > +error_adc_buffer_init:
> > + vf610_adc_buffer_remove(indio_dev);
> > error_iio_device_register:
> > clk_disable_unprepare(info->clk);
> > error_adc_clk_enable:
> > @@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
> > struct vf610_adc *info = iio_priv(indio_dev);
> >
> > iio_device_unregister(indio_dev);
> > + vf610_adc_buffer_remove(indio_dev);
> > regulator_disable(info->vref);
> > clk_disable_unprepare(info->clk);
> >
> >
>
Thanks for the review.
- Sanchayan.
[1]. https://community.freescale.com/thread/357619
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
2015-08-08 17:22 ` maitysanchayan at gmail.com
@ 2015-08-08 18:46 ` Jonathan Cameron
-1 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2015-08-08 18:46 UTC (permalink / raw)
To: maitysanchayan
Cc: linux-iio, stefan, B38611, hofrat, sanjeev_sharma, fabio.estevam,
knaack.h, lars, pmeerw, antoine.tenart, linux-kernel,
linux-arm-kernel
On 08/08/15 18:22, maitysanchayan@gmail.com wrote:
> Hello Jonathan,
>
> On 15-08-08 15:29:47, Jonathan Cameron wrote:
>> On 05/08/15 13:10, Sanchayan Maity wrote:
>>> This patch adds support for IIO buffer to the Vybrid ADC driver.
>>> IIO triggered buffer infrastructure along with iio sysfs trigger
>>> is used to leverage continuous sampling support provided by the
>>> ADC block.
>> Looking good. Just a couple more bits and pieces inline from me.
>> One or two of which aren't corrected from Peter's review of v1.
>>
>> I will want Fugang Dong's ack / review tag on the final version
>> before applying it however.
>
> Sure.
>
>> This driver is some distance form my area of expertise!
>
> I doubt :).
>
>>
>> Jonathan
>>>
>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>> ---
>>> drivers/iio/adc/Kconfig | 4 ++
>>> drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
>>> 2 files changed, 105 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 7c55658..4661241 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -337,6 +337,10 @@ config TWL6030_GPADC
>>> config VF610_ADC
>>> tristate "Freescale vf610 ADC driver"
>>> depends on OF
>>> + select IIO_BUFFER
>>> + select IIO_TRIGGER
>>> + select IIO_SYSFS_TRIGGER
>> Unless I missed something there is no dependency on this particular
>> trigger (it just happens to be the one you've been testing with I guess).
>> Could be driven from a hardware trigger belonging to another device for
>> example.
>
> Yes right in a way. Right now we do not provide or there is no provision
> for hardware triggers. On the Vybrid, the Peripheral Delay Block (PDB)
> does the job of providing support for software and hardware triggers. PDB
> support is not yet there in Linux however.
Not supplying a trigger is fine, but any trigger (that allows other devices
to bind to it) will be fine.
>
> There is also the question of where the Vybrid PDB driver would belong
> to? In the TRM it is put in the timers but the kernel has no generic timer
> framework that I am aware of.
It's been debated a number of times, but no one has ever written one.
Not seen anything recently on this topic.. Lars, you seen anything?
(the blackfin-timer trigger is similar to what you are describing I think).
> After some internal reviews we decided to
> do a platform driver which provided functions ADC driver could called into.
Does rather feel like there ought to be at least a standard home for these.
Might be worth asking the arm-soc guys...
>
> I have a patchset ready which provides trigger support using PDB however
> configuring the PDB properly has proven to be tricky. While it works but
> not reliably with multiple channels and it would be a while before I get
> that working and post that patchset. So kind of stalled there and just
> because of two registers which need to be written with the correct value
> :). For what it's worth if someone comes across this, some discussion
> here [1] along with patches. (Note however those are a bit old patches
> not exactly my new work).
>
> Sorry for digressing from the topic. Anyways so the idea was to provide
> sysfs triggers as default for using this continuous sampling. Later the
> driver may provide additional triggers. So for now I added the sysfs
> trigger as a select option so that a user won't have to recompile the
> kernel for using the buffers with continuous sampling.
It's a policy decision so should really be left to the distro builders.
There are lots of possible triggers out there (though sysfs might be
the most likely one!)
Hence don't put the select there in Kconfig. We should shortly have
Daniel's update to the patches for the high resolution timer
which would give another obvious choice for starters.
I doubt many distros build without the sysfs triggers but with other IIO
stuff.
>
>>
>>> + select IIO_TRIGGERED_BUFFER
>>> help
>>> Say yes here to support for Vybrid board analog-to-digital converter.
>>> Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
>>> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
>>> index 23b8fb9..97cb2ed 100644
>>> --- a/drivers/iio/adc/vf610_adc.c
>>> +++ b/drivers/iio/adc/vf610_adc.c
>>> @@ -34,8 +34,11 @@
>>> #include <linux/err.h>
>>>
>>> #include <linux/iio/iio.h>
>>> +#include <linux/iio/buffer.h>
>>> #include <linux/iio/sysfs.h>
>>> -#include <linux/iio/driver.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>>
>>> /* This will be the driver name the kernel reports */
>>> #define DRIVER_NAME "vf610-adc"
>>> @@ -170,6 +173,7 @@ struct vf610_adc {
>>> u32 sample_freq_avail[5];
>>>
>>> struct completion completion;
>>> + u16 buffer[2];
>> Note the requirements on the buffer provided to
>> iio_push_to_buffers_with_timestamp
>> Needs to be u16 buffer[8] to allow for the aligned 64bit (4 word) timestamp.
>>
>> Peter pointed this out in his follow up email and you said you'd implement
>> it.. Guessing this got lost somewhere.
>
> No, I meant to implement what Peter recommended but I guess I did not completely
> grasp what he intended. Sorry about that. Will fix this and ask further if in
> more doubts.
>
>>
>>
>>> };
>>>
>>> static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
>>> @@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
>>> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>> .ext_info = vf610_ext_info, \
>>> + .address = (_idx), \
>>> + .scan_index = (_idx), \
>>> + .scan_type.sign = 'u', \
>>> + .scan_type.realbits = 12, \
>>> + .scan_type.storagebits = 16, \
>>> }
>>>
>>> #define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
>>> .type = (_chan_type), \
>>> .channel = (_idx), \
>>> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
>>> + .address = (_idx), \
>>> + .scan_index = (_idx), \
>> .scan_type = {
>> .sign = 'u',
>> etc.
>>
>> Peter picked up on this..
>
> Sorry yes missed that. Will definitely fix.
>
>>
>>> + .scan_type.sign = 'u', \
>>> + .scan_type.realbits = 12, \
>>> + .scan_type.storagebits = 16, \
>>> }
>>>
>>> static const struct iio_chan_spec vf610_adc_iio_channels[] = {
>>> @@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
>>> VF610_ADC_CHAN(14, IIO_VOLTAGE),
>>> VF610_ADC_CHAN(15, IIO_VOLTAGE),
>>> VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
>>> + IIO_CHAN_SOFT_TIMESTAMP(32),
>> It's bit extreme throwing it out at scan_index 32. Is there a reason
>> to think that migh be neccesary? Mind you, why is the temperature
>> channel down at 26? Are we dealing with a set of reserved real channels
>> inbetween?
>
>
> The temperature channel is the 26th channel and there are some reserved
> channels in between. 31st is the channel which acts as a conversion
> disabled setting. So I put the timestamp at index 32.
Fair enough if weird ;)
>
>
>>> /* sentinel */
>>> };
>>>
>>> @@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
>>>
>>> static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
>>> {
>>> - struct vf610_adc *info = (struct vf610_adc *)dev_id;
>>> + struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
>>> + struct vf610_adc *info = iio_priv(indio_dev);
>>> int coco;
>>>
>>> coco = readl(info->regs + VF610_REG_ADC_HS);
>>> if (coco & VF610_ADC_HS_COCO0) {
>>> info->value = vf610_adc_read_data(info);
>>
>> I'd be tempted to make the non buffered path also use
>> info->bufffer[0] and drop info->value entirely.
>> A more invasive patch, but a cleaner resulting code (slightly!)
>
> I did think of that but decided against it as I wanted the changes
> to as less invasive as possible making only the necessary changes
> and keeping the old code as is.
You could do this then clean up in a follow up patch which would be
really easy to review.
>
>>> - complete(&info->completion);
>>> + if (iio_buffer_enabled(indio_dev)) {
>>> + info->buffer[0] = info->value;
>>> + iio_push_to_buffers_with_timestamp(indio_dev,
>>> + info->buffer, iio_get_time_ns());
>>> + iio_trigger_notify_done(indio_dev->trig);
>>> + } else
>>> + complete(&info->completion);
>>> }
>>>
>>> return IRQ_HANDLED;
>>> @@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
>>> switch (mask) {
>>> case IIO_CHAN_INFO_RAW:
>>> case IIO_CHAN_INFO_PROCESSED:
>>
>> To avoid possible races this check should be done under the mlock
>
> Thanks for picking this. Somehow I remember seeing it outside of the
> mlock. However grepping again shows otherwise. Will fix.
>
> .
>>> + if (iio_buffer_enabled(indio_dev))
>>> + return -EBUSY;
>>> +
>>> mutex_lock(&indio_dev->mlock);
>>> reinit_completion(&info->completion);
>>>
>>> @@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
>>> return -EINVAL;
>>> }
>>>
>>> +static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
>>> +{
>>> + struct vf610_adc *info = iio_priv(indio_dev);
>>> + unsigned int channel;
>>> + int ret;
>>> + int val;
>>> +
>>> + ret = iio_triggered_buffer_postenable(indio_dev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + val = readl(info->regs + VF610_REG_ADC_GC);
>>> + val |= VF610_ADC_ADCON;
>>> + writel(val, info->regs + VF610_REG_ADC_GC);
>>> +
>>> + channel = find_first_bit(indio_dev->active_scan_mask,
>>> + indio_dev->masklength);
>>> +
>>> + val = VF610_ADC_ADCHC(channel);
>>> + val |= VF610_ADC_AIEN;
>>> +
>>> + writel(val, info->regs + VF610_REG_ADC_HC0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>> +{
>>> + struct vf610_adc *info = iio_priv(indio_dev);
>>> + unsigned int hc_cfg = 0;
>>> + int val;
>>> +
>>> + val = readl(info->regs + VF610_REG_ADC_GC);
>>> + val &= ~VF610_ADC_ADCON;
>>> + writel(val, info->regs + VF610_REG_ADC_GC);
>>> +
>>> + hc_cfg |= VF610_ADC_CONV_DISABLE;
>>> + hc_cfg &= ~VF610_ADC_AIEN;
>>> +
>>> + writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
>>> + .postenable = &vf610_adc_buffer_postenable,
>>> + .predisable = &iio_triggered_buffer_predisable,
>>> + .postdisable = &vf610_adc_buffer_postdisable,
>>> + .validate_scan_mask = &iio_validate_scan_mask_onehot,
>>> +};
>>> +
>>> +static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
>>> +{
>>> + return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
>>> + NULL, &iio_triggered_buffer_setup_ops);
>>> +}
>>> +
>>> +static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
>>> +{
>>> + iio_triggered_buffer_cleanup(indio_dev);
>>> +}
>> These to wrappers seem a little superflous. I'd have just put the code
>> inline, but it's obviously a matter of personal taste and I don't care
>> that much!
>
> Ok. I do not have strong opinions on this. I just tried to follow how the
> at91 adc code did it considering it as a good example. Will drop it.
>
>>
>>> +
>>> static int vf610_adc_reg_access(struct iio_dev *indio_dev,
>>> unsigned reg, unsigned writeval,
>>> unsigned *readval)
>>> @@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
>>>
>>> ret = devm_request_irq(info->dev, irq,
>>> vf610_adc_isr, 0,
>>> - dev_name(&pdev->dev), info);
>>> + dev_name(&pdev->dev), indio_dev);
>>> if (ret < 0) {
>>> dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
>>> return ret;
>>> @@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
>>> vf610_adc_cfg_init(info);
>>> vf610_adc_hw_init(info);
>>>
>>> + ret = vf610_adc_buffer_init(indio_dev);
>>> + if (ret < 0) {
>>> + dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
>>> + goto error_iio_device_register;
>>> + }
>>> +
>>> ret = iio_device_register(indio_dev);
>>> if (ret) {
>>> dev_err(&pdev->dev, "Couldn't register the device.\n");
>>> - goto error_iio_device_register;
>>> + goto error_adc_buffer_init;
>>> }
>>>
>>> return 0;
>>>
>>> -
>>> +error_adc_buffer_init:
>>> + vf610_adc_buffer_remove(indio_dev);
>>> error_iio_device_register:
>>> clk_disable_unprepare(info->clk);
>>> error_adc_clk_enable:
>>> @@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
>>> struct vf610_adc *info = iio_priv(indio_dev);
>>>
>>> iio_device_unregister(indio_dev);
>>> + vf610_adc_buffer_remove(indio_dev);
>>> regulator_disable(info->vref);
>>> clk_disable_unprepare(info->clk);
>>>
>>>
>>
>
> Thanks for the review.
>
> - Sanchayan.
>
> [1]. https://community.freescale.com/thread/357619
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
@ 2015-08-08 18:46 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2015-08-08 18:46 UTC (permalink / raw)
To: linux-arm-kernel
On 08/08/15 18:22, maitysanchayan at gmail.com wrote:
> Hello Jonathan,
>
> On 15-08-08 15:29:47, Jonathan Cameron wrote:
>> On 05/08/15 13:10, Sanchayan Maity wrote:
>>> This patch adds support for IIO buffer to the Vybrid ADC driver.
>>> IIO triggered buffer infrastructure along with iio sysfs trigger
>>> is used to leverage continuous sampling support provided by the
>>> ADC block.
>> Looking good. Just a couple more bits and pieces inline from me.
>> One or two of which aren't corrected from Peter's review of v1.
>>
>> I will want Fugang Dong's ack / review tag on the final version
>> before applying it however.
>
> Sure.
>
>> This driver is some distance form my area of expertise!
>
> I doubt :).
>
>>
>> Jonathan
>>>
>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>> ---
>>> drivers/iio/adc/Kconfig | 4 ++
>>> drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
>>> 2 files changed, 105 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 7c55658..4661241 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -337,6 +337,10 @@ config TWL6030_GPADC
>>> config VF610_ADC
>>> tristate "Freescale vf610 ADC driver"
>>> depends on OF
>>> + select IIO_BUFFER
>>> + select IIO_TRIGGER
>>> + select IIO_SYSFS_TRIGGER
>> Unless I missed something there is no dependency on this particular
>> trigger (it just happens to be the one you've been testing with I guess).
>> Could be driven from a hardware trigger belonging to another device for
>> example.
>
> Yes right in a way. Right now we do not provide or there is no provision
> for hardware triggers. On the Vybrid, the Peripheral Delay Block (PDB)
> does the job of providing support for software and hardware triggers. PDB
> support is not yet there in Linux however.
Not supplying a trigger is fine, but any trigger (that allows other devices
to bind to it) will be fine.
>
> There is also the question of where the Vybrid PDB driver would belong
> to? In the TRM it is put in the timers but the kernel has no generic timer
> framework that I am aware of.
It's been debated a number of times, but no one has ever written one.
Not seen anything recently on this topic.. Lars, you seen anything?
(the blackfin-timer trigger is similar to what you are describing I think).
> After some internal reviews we decided to
> do a platform driver which provided functions ADC driver could called into.
Does rather feel like there ought to be at least a standard home for these.
Might be worth asking the arm-soc guys...
>
> I have a patchset ready which provides trigger support using PDB however
> configuring the PDB properly has proven to be tricky. While it works but
> not reliably with multiple channels and it would be a while before I get
> that working and post that patchset. So kind of stalled there and just
> because of two registers which need to be written with the correct value
> :). For what it's worth if someone comes across this, some discussion
> here [1] along with patches. (Note however those are a bit old patches
> not exactly my new work).
>
> Sorry for digressing from the topic. Anyways so the idea was to provide
> sysfs triggers as default for using this continuous sampling. Later the
> driver may provide additional triggers. So for now I added the sysfs
> trigger as a select option so that a user won't have to recompile the
> kernel for using the buffers with continuous sampling.
It's a policy decision so should really be left to the distro builders.
There are lots of possible triggers out there (though sysfs might be
the most likely one!)
Hence don't put the select there in Kconfig. We should shortly have
Daniel's update to the patches for the high resolution timer
which would give another obvious choice for starters.
I doubt many distros build without the sysfs triggers but with other IIO
stuff.
>
>>
>>> + select IIO_TRIGGERED_BUFFER
>>> help
>>> Say yes here to support for Vybrid board analog-to-digital converter.
>>> Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
>>> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
>>> index 23b8fb9..97cb2ed 100644
>>> --- a/drivers/iio/adc/vf610_adc.c
>>> +++ b/drivers/iio/adc/vf610_adc.c
>>> @@ -34,8 +34,11 @@
>>> #include <linux/err.h>
>>>
>>> #include <linux/iio/iio.h>
>>> +#include <linux/iio/buffer.h>
>>> #include <linux/iio/sysfs.h>
>>> -#include <linux/iio/driver.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>>
>>> /* This will be the driver name the kernel reports */
>>> #define DRIVER_NAME "vf610-adc"
>>> @@ -170,6 +173,7 @@ struct vf610_adc {
>>> u32 sample_freq_avail[5];
>>>
>>> struct completion completion;
>>> + u16 buffer[2];
>> Note the requirements on the buffer provided to
>> iio_push_to_buffers_with_timestamp
>> Needs to be u16 buffer[8] to allow for the aligned 64bit (4 word) timestamp.
>>
>> Peter pointed this out in his follow up email and you said you'd implement
>> it.. Guessing this got lost somewhere.
>
> No, I meant to implement what Peter recommended but I guess I did not completely
> grasp what he intended. Sorry about that. Will fix this and ask further if in
> more doubts.
>
>>
>>
>>> };
>>>
>>> static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
>>> @@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
>>> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>> .ext_info = vf610_ext_info, \
>>> + .address = (_idx), \
>>> + .scan_index = (_idx), \
>>> + .scan_type.sign = 'u', \
>>> + .scan_type.realbits = 12, \
>>> + .scan_type.storagebits = 16, \
>>> }
>>>
>>> #define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
>>> .type = (_chan_type), \
>>> .channel = (_idx), \
>>> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
>>> + .address = (_idx), \
>>> + .scan_index = (_idx), \
>> .scan_type = {
>> .sign = 'u',
>> etc.
>>
>> Peter picked up on this..
>
> Sorry yes missed that. Will definitely fix.
>
>>
>>> + .scan_type.sign = 'u', \
>>> + .scan_type.realbits = 12, \
>>> + .scan_type.storagebits = 16, \
>>> }
>>>
>>> static const struct iio_chan_spec vf610_adc_iio_channels[] = {
>>> @@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
>>> VF610_ADC_CHAN(14, IIO_VOLTAGE),
>>> VF610_ADC_CHAN(15, IIO_VOLTAGE),
>>> VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
>>> + IIO_CHAN_SOFT_TIMESTAMP(32),
>> It's bit extreme throwing it out at scan_index 32. Is there a reason
>> to think that migh be neccesary? Mind you, why is the temperature
>> channel down at 26? Are we dealing with a set of reserved real channels
>> inbetween?
>
>
> The temperature channel is the 26th channel and there are some reserved
> channels in between. 31st is the channel which acts as a conversion
> disabled setting. So I put the timestamp at index 32.
Fair enough if weird ;)
>
>
>>> /* sentinel */
>>> };
>>>
>>> @@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
>>>
>>> static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
>>> {
>>> - struct vf610_adc *info = (struct vf610_adc *)dev_id;
>>> + struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
>>> + struct vf610_adc *info = iio_priv(indio_dev);
>>> int coco;
>>>
>>> coco = readl(info->regs + VF610_REG_ADC_HS);
>>> if (coco & VF610_ADC_HS_COCO0) {
>>> info->value = vf610_adc_read_data(info);
>>
>> I'd be tempted to make the non buffered path also use
>> info->bufffer[0] and drop info->value entirely.
>> A more invasive patch, but a cleaner resulting code (slightly!)
>
> I did think of that but decided against it as I wanted the changes
> to as less invasive as possible making only the necessary changes
> and keeping the old code as is.
You could do this then clean up in a follow up patch which would be
really easy to review.
>
>>> - complete(&info->completion);
>>> + if (iio_buffer_enabled(indio_dev)) {
>>> + info->buffer[0] = info->value;
>>> + iio_push_to_buffers_with_timestamp(indio_dev,
>>> + info->buffer, iio_get_time_ns());
>>> + iio_trigger_notify_done(indio_dev->trig);
>>> + } else
>>> + complete(&info->completion);
>>> }
>>>
>>> return IRQ_HANDLED;
>>> @@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
>>> switch (mask) {
>>> case IIO_CHAN_INFO_RAW:
>>> case IIO_CHAN_INFO_PROCESSED:
>>
>> To avoid possible races this check should be done under the mlock
>
> Thanks for picking this. Somehow I remember seeing it outside of the
> mlock. However grepping again shows otherwise. Will fix.
>
> .
>>> + if (iio_buffer_enabled(indio_dev))
>>> + return -EBUSY;
>>> +
>>> mutex_lock(&indio_dev->mlock);
>>> reinit_completion(&info->completion);
>>>
>>> @@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
>>> return -EINVAL;
>>> }
>>>
>>> +static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
>>> +{
>>> + struct vf610_adc *info = iio_priv(indio_dev);
>>> + unsigned int channel;
>>> + int ret;
>>> + int val;
>>> +
>>> + ret = iio_triggered_buffer_postenable(indio_dev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + val = readl(info->regs + VF610_REG_ADC_GC);
>>> + val |= VF610_ADC_ADCON;
>>> + writel(val, info->regs + VF610_REG_ADC_GC);
>>> +
>>> + channel = find_first_bit(indio_dev->active_scan_mask,
>>> + indio_dev->masklength);
>>> +
>>> + val = VF610_ADC_ADCHC(channel);
>>> + val |= VF610_ADC_AIEN;
>>> +
>>> + writel(val, info->regs + VF610_REG_ADC_HC0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
>>> +{
>>> + struct vf610_adc *info = iio_priv(indio_dev);
>>> + unsigned int hc_cfg = 0;
>>> + int val;
>>> +
>>> + val = readl(info->regs + VF610_REG_ADC_GC);
>>> + val &= ~VF610_ADC_ADCON;
>>> + writel(val, info->regs + VF610_REG_ADC_GC);
>>> +
>>> + hc_cfg |= VF610_ADC_CONV_DISABLE;
>>> + hc_cfg &= ~VF610_ADC_AIEN;
>>> +
>>> + writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
>>> + .postenable = &vf610_adc_buffer_postenable,
>>> + .predisable = &iio_triggered_buffer_predisable,
>>> + .postdisable = &vf610_adc_buffer_postdisable,
>>> + .validate_scan_mask = &iio_validate_scan_mask_onehot,
>>> +};
>>> +
>>> +static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
>>> +{
>>> + return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
>>> + NULL, &iio_triggered_buffer_setup_ops);
>>> +}
>>> +
>>> +static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
>>> +{
>>> + iio_triggered_buffer_cleanup(indio_dev);
>>> +}
>> These to wrappers seem a little superflous. I'd have just put the code
>> inline, but it's obviously a matter of personal taste and I don't care
>> that much!
>
> Ok. I do not have strong opinions on this. I just tried to follow how the
> at91 adc code did it considering it as a good example. Will drop it.
>
>>
>>> +
>>> static int vf610_adc_reg_access(struct iio_dev *indio_dev,
>>> unsigned reg, unsigned writeval,
>>> unsigned *readval)
>>> @@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
>>>
>>> ret = devm_request_irq(info->dev, irq,
>>> vf610_adc_isr, 0,
>>> - dev_name(&pdev->dev), info);
>>> + dev_name(&pdev->dev), indio_dev);
>>> if (ret < 0) {
>>> dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
>>> return ret;
>>> @@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
>>> vf610_adc_cfg_init(info);
>>> vf610_adc_hw_init(info);
>>>
>>> + ret = vf610_adc_buffer_init(indio_dev);
>>> + if (ret < 0) {
>>> + dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
>>> + goto error_iio_device_register;
>>> + }
>>> +
>>> ret = iio_device_register(indio_dev);
>>> if (ret) {
>>> dev_err(&pdev->dev, "Couldn't register the device.\n");
>>> - goto error_iio_device_register;
>>> + goto error_adc_buffer_init;
>>> }
>>>
>>> return 0;
>>>
>>> -
>>> +error_adc_buffer_init:
>>> + vf610_adc_buffer_remove(indio_dev);
>>> error_iio_device_register:
>>> clk_disable_unprepare(info->clk);
>>> error_adc_clk_enable:
>>> @@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
>>> struct vf610_adc *info = iio_priv(indio_dev);
>>>
>>> iio_device_unregister(indio_dev);
>>> + vf610_adc_buffer_remove(indio_dev);
>>> regulator_disable(info->vref);
>>> clk_disable_unprepare(info->clk);
>>>
>>>
>>
>
> Thanks for the review.
>
> - Sanchayan.
>
> [1]. https://community.freescale.com/thread/357619
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
2015-08-08 18:46 ` Jonathan Cameron
@ 2015-08-10 6:35 ` maitysanchayan at gmail.com
-1 siblings, 0 replies; 12+ messages in thread
From: maitysanchayan @ 2015-08-10 6:35 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, stefan, B38611, hofrat, sanjeev_sharma, fabio.estevam,
knaack.h, lars, pmeerw, antoine.tenart, linux-kernel,
linux-arm-kernel
Hello,
On 15-08-08 19:46:00, Jonathan Cameron wrote:
> On 08/08/15 18:22, maitysanchayan@gmail.com wrote:
> > Hello Jonathan,
> >
> > On 15-08-08 15:29:47, Jonathan Cameron wrote:
> >> On 05/08/15 13:10, Sanchayan Maity wrote:
> >>> This patch adds support for IIO buffer to the Vybrid ADC driver.
> >>> IIO triggered buffer infrastructure along with iio sysfs trigger
> >>> is used to leverage continuous sampling support provided by the
> >>> ADC block.
> >> Looking good. Just a couple more bits and pieces inline from me.
> >> One or two of which aren't corrected from Peter's review of v1.
> >>
> >> I will want Fugang Dong's ack / review tag on the final version
> >> before applying it however.
> >
> > Sure.
> >
> >> This driver is some distance form my area of expertise!
> >
> > I doubt :).
> >
> >>
> >> Jonathan
> >>>
> >>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >>> ---
> >>> drivers/iio/adc/Kconfig | 4 ++
> >>> drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
> >>> 2 files changed, 105 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >>> index 7c55658..4661241 100644
> >>> --- a/drivers/iio/adc/Kconfig
> >>> +++ b/drivers/iio/adc/Kconfig
> >>> @@ -337,6 +337,10 @@ config TWL6030_GPADC
> >>> config VF610_ADC
> >>> tristate "Freescale vf610 ADC driver"
> >>> depends on OF
> >>> + select IIO_BUFFER
> >>> + select IIO_TRIGGER
> >>> + select IIO_SYSFS_TRIGGER
> >> Unless I missed something there is no dependency on this particular
> >> trigger (it just happens to be the one you've been testing with I guess).
> >> Could be driven from a hardware trigger belonging to another device for
> >> example.
> >
> > Yes right in a way. Right now we do not provide or there is no provision
> > for hardware triggers. On the Vybrid, the Peripheral Delay Block (PDB)
> > does the job of providing support for software and hardware triggers. PDB
> > support is not yet there in Linux however.
> Not supplying a trigger is fine, but any trigger (that allows other devices
> to bind to it) will be fine.
Ok.
> >
> > There is also the question of where the Vybrid PDB driver would belong
> > to? In the TRM it is put in the timers but the kernel has no generic timer
> > framework that I am aware of.
> It's been debated a number of times, but no one has ever written one.
> Not seen anything recently on this topic.. Lars, you seen anything?
> (the blackfin-timer trigger is similar to what you are describing I think).
>
> > After some internal reviews we decided to
> > do a platform driver which provided functions ADC driver could called into.
> Does rather feel like there ought to be at least a standard home for these.
> Might be worth asking the arm-soc guys...
> >
> > I have a patchset ready which provides trigger support using PDB however
> > configuring the PDB properly has proven to be tricky. While it works but
> > not reliably with multiple channels and it would be a while before I get
> > that working and post that patchset. So kind of stalled there and just
> > because of two registers which need to be written with the correct value
> > :). For what it's worth if someone comes across this, some discussion
> > here [1] along with patches. (Note however those are a bit old patches
> > not exactly my new work).
> >
> > Sorry for digressing from the topic. Anyways so the idea was to provide
> > sysfs triggers as default for using this continuous sampling. Later the
> > driver may provide additional triggers. So for now I added the sysfs
> > trigger as a select option so that a user won't have to recompile the
> > kernel for using the buffers with continuous sampling.
> It's a policy decision so should really be left to the distro builders.
> There are lots of possible triggers out there (though sysfs might be
> the most likely one!)
>
> Hence don't put the select there in Kconfig. We should shortly have
> Daniel's update to the patches for the high resolution timer
> which would give another obvious choice for starters.
Alright. Will drop that iio sysfs tigger select from the Kconfig.
>
> I doubt many distros build without the sysfs triggers but with other IIO
> stuff.
>
> >
> >>
> >>> + select IIO_TRIGGERED_BUFFER
> >>> help
> >>> Say yes here to support for Vybrid board analog-to-digital converter.
> >>> Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
> >>> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> >>> index 23b8fb9..97cb2ed 100644
> >>> --- a/drivers/iio/adc/vf610_adc.c
> >>> +++ b/drivers/iio/adc/vf610_adc.c
> >>> @@ -34,8 +34,11 @@
> >>> #include <linux/err.h>
> >>>
> >>> #include <linux/iio/iio.h>
> >>> +#include <linux/iio/buffer.h>
> >>> #include <linux/iio/sysfs.h>
> >>> -#include <linux/iio/driver.h>
> >>> +#include <linux/iio/trigger.h>
> >>> +#include <linux/iio/trigger_consumer.h>
> >>> +#include <linux/iio/triggered_buffer.h>
> >>>
> >>> /* This will be the driver name the kernel reports */
> >>> #define DRIVER_NAME "vf610-adc"
> >>> @@ -170,6 +173,7 @@ struct vf610_adc {
> >>> u32 sample_freq_avail[5];
> >>>
> >>> struct completion completion;
> >>> + u16 buffer[2];
> >> Note the requirements on the buffer provided to
> >> iio_push_to_buffers_with_timestamp
> >> Needs to be u16 buffer[8] to allow for the aligned 64bit (4 word) timestamp.
> >>
> >> Peter pointed this out in his follow up email and you said you'd implement
> >> it.. Guessing this got lost somewhere.
> >
> > No, I meant to implement what Peter recommended but I guess I did not completely
> > grasp what he intended. Sorry about that. Will fix this and ask further if in
> > more doubts.
> >
> >>
> >>
> >>> };
> >>>
> >>> static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> >>> @@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
> >>> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> >>> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> >>> .ext_info = vf610_ext_info, \
> >>> + .address = (_idx), \
> >>> + .scan_index = (_idx), \
> >>> + .scan_type.sign = 'u', \
> >>> + .scan_type.realbits = 12, \
> >>> + .scan_type.storagebits = 16, \
> >>> }
> >>>
> >>> #define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
> >>> .type = (_chan_type), \
> >>> .channel = (_idx), \
> >>> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> >>> + .address = (_idx), \
> >>> + .scan_index = (_idx), \
> >> .scan_type = {
> >> .sign = 'u',
> >> etc.
> >>
> >> Peter picked up on this..
> >
> > Sorry yes missed that. Will definitely fix.
> >
> >>
> >>> + .scan_type.sign = 'u', \
> >>> + .scan_type.realbits = 12, \
> >>> + .scan_type.storagebits = 16, \
> >>> }
> >>>
> >>> static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> >>> @@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> >>> VF610_ADC_CHAN(14, IIO_VOLTAGE),
> >>> VF610_ADC_CHAN(15, IIO_VOLTAGE),
> >>> VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
> >>> + IIO_CHAN_SOFT_TIMESTAMP(32),
> >> It's bit extreme throwing it out at scan_index 32. Is there a reason
> >> to think that migh be neccesary? Mind you, why is the temperature
> >> channel down at 26? Are we dealing with a set of reserved real channels
> >> inbetween?
> >
> >
> > The temperature channel is the 26th channel and there are some reserved
> > channels in between. 31st is the channel which acts as a conversion
> > disabled setting. So I put the timestamp at index 32.
> Fair enough if weird ;)
> >
> >
> >>> /* sentinel */
> >>> };
> >>>
> >>> @@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
> >>>
> >>> static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
> >>> {
> >>> - struct vf610_adc *info = (struct vf610_adc *)dev_id;
> >>> + struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
> >>> + struct vf610_adc *info = iio_priv(indio_dev);
> >>> int coco;
> >>>
> >>> coco = readl(info->regs + VF610_REG_ADC_HS);
> >>> if (coco & VF610_ADC_HS_COCO0) {
> >>> info->value = vf610_adc_read_data(info);
> >>
> >> I'd be tempted to make the non buffered path also use
> >> info->bufffer[0] and drop info->value entirely.
> >> A more invasive patch, but a cleaner resulting code (slightly!)
> >
> > I did think of that but decided against it as I wanted the changes
> > to as less invasive as possible making only the necessary changes
> > and keeping the old code as is.
> You could do this then clean up in a follow up patch which would be
> really easy to review.
Ok. Will implement the same with a follow up second patch in the third
revision.
- Sanchayan.
> >
> >>> - complete(&info->completion);
> >>> + if (iio_buffer_enabled(indio_dev)) {
> >>> + info->buffer[0] = info->value;
> >>> + iio_push_to_buffers_with_timestamp(indio_dev,
> >>> + info->buffer, iio_get_time_ns());
> >>> + iio_trigger_notify_done(indio_dev->trig);
> >>> + } else
> >>> + complete(&info->completion);
> >>> }
> >>>
> >>> return IRQ_HANDLED;
> >>> @@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> >>> switch (mask) {
> >>> case IIO_CHAN_INFO_RAW:
> >>> case IIO_CHAN_INFO_PROCESSED:
> >>
> >> To avoid possible races this check should be done under the mlock
> >
> > Thanks for picking this. Somehow I remember seeing it outside of the
> > mlock. However grepping again shows otherwise. Will fix.
> >
> > .
> >>> + if (iio_buffer_enabled(indio_dev))
> >>> + return -EBUSY;
> >>> +
> >>> mutex_lock(&indio_dev->mlock);
> >>> reinit_completion(&info->completion);
> >>>
> >>> @@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
> >>> return -EINVAL;
> >>> }
> >>>
> >>> +static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
> >>> +{
> >>> + struct vf610_adc *info = iio_priv(indio_dev);
> >>> + unsigned int channel;
> >>> + int ret;
> >>> + int val;
> >>> +
> >>> + ret = iio_triggered_buffer_postenable(indio_dev);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + val = readl(info->regs + VF610_REG_ADC_GC);
> >>> + val |= VF610_ADC_ADCON;
> >>> + writel(val, info->regs + VF610_REG_ADC_GC);
> >>> +
> >>> + channel = find_first_bit(indio_dev->active_scan_mask,
> >>> + indio_dev->masklength);
> >>> +
> >>> + val = VF610_ADC_ADCHC(channel);
> >>> + val |= VF610_ADC_AIEN;
> >>> +
> >>> + writel(val, info->regs + VF610_REG_ADC_HC0);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
> >>> +{
> >>> + struct vf610_adc *info = iio_priv(indio_dev);
> >>> + unsigned int hc_cfg = 0;
> >>> + int val;
> >>> +
> >>> + val = readl(info->regs + VF610_REG_ADC_GC);
> >>> + val &= ~VF610_ADC_ADCON;
> >>> + writel(val, info->regs + VF610_REG_ADC_GC);
> >>> +
> >>> + hc_cfg |= VF610_ADC_CONV_DISABLE;
> >>> + hc_cfg &= ~VF610_ADC_AIEN;
> >>> +
> >>> + writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
> >>> + .postenable = &vf610_adc_buffer_postenable,
> >>> + .predisable = &iio_triggered_buffer_predisable,
> >>> + .postdisable = &vf610_adc_buffer_postdisable,
> >>> + .validate_scan_mask = &iio_validate_scan_mask_onehot,
> >>> +};
> >>> +
> >>> +static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
> >>> +{
> >>> + return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
> >>> + NULL, &iio_triggered_buffer_setup_ops);
> >>> +}
> >>> +
> >>> +static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
> >>> +{
> >>> + iio_triggered_buffer_cleanup(indio_dev);
> >>> +}
> >> These to wrappers seem a little superflous. I'd have just put the code
> >> inline, but it's obviously a matter of personal taste and I don't care
> >> that much!
> >
> > Ok. I do not have strong opinions on this. I just tried to follow how the
> > at91 adc code did it considering it as a good example. Will drop it.
> >
> >>
> >>> +
> >>> static int vf610_adc_reg_access(struct iio_dev *indio_dev,
> >>> unsigned reg, unsigned writeval,
> >>> unsigned *readval)
> >>> @@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
> >>>
> >>> ret = devm_request_irq(info->dev, irq,
> >>> vf610_adc_isr, 0,
> >>> - dev_name(&pdev->dev), info);
> >>> + dev_name(&pdev->dev), indio_dev);
> >>> if (ret < 0) {
> >>> dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> >>> return ret;
> >>> @@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
> >>> vf610_adc_cfg_init(info);
> >>> vf610_adc_hw_init(info);
> >>>
> >>> + ret = vf610_adc_buffer_init(indio_dev);
> >>> + if (ret < 0) {
> >>> + dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
> >>> + goto error_iio_device_register;
> >>> + }
> >>> +
> >>> ret = iio_device_register(indio_dev);
> >>> if (ret) {
> >>> dev_err(&pdev->dev, "Couldn't register the device.\n");
> >>> - goto error_iio_device_register;
> >>> + goto error_adc_buffer_init;
> >>> }
> >>>
> >>> return 0;
> >>>
> >>> -
> >>> +error_adc_buffer_init:
> >>> + vf610_adc_buffer_remove(indio_dev);
> >>> error_iio_device_register:
> >>> clk_disable_unprepare(info->clk);
> >>> error_adc_clk_enable:
> >>> @@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
> >>> struct vf610_adc *info = iio_priv(indio_dev);
> >>>
> >>> iio_device_unregister(indio_dev);
> >>> + vf610_adc_buffer_remove(indio_dev);
> >>> regulator_disable(info->vref);
> >>> clk_disable_unprepare(info->clk);
> >>>
> >>>
> >>
> >
> > Thanks for the review.
> >
> > - Sanchayan.
> >
> > [1]. https://community.freescale.com/thread/357619
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC
@ 2015-08-10 6:35 ` maitysanchayan at gmail.com
0 siblings, 0 replies; 12+ messages in thread
From: maitysanchayan at gmail.com @ 2015-08-10 6:35 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On 15-08-08 19:46:00, Jonathan Cameron wrote:
> On 08/08/15 18:22, maitysanchayan at gmail.com wrote:
> > Hello Jonathan,
> >
> > On 15-08-08 15:29:47, Jonathan Cameron wrote:
> >> On 05/08/15 13:10, Sanchayan Maity wrote:
> >>> This patch adds support for IIO buffer to the Vybrid ADC driver.
> >>> IIO triggered buffer infrastructure along with iio sysfs trigger
> >>> is used to leverage continuous sampling support provided by the
> >>> ADC block.
> >> Looking good. Just a couple more bits and pieces inline from me.
> >> One or two of which aren't corrected from Peter's review of v1.
> >>
> >> I will want Fugang Dong's ack / review tag on the final version
> >> before applying it however.
> >
> > Sure.
> >
> >> This driver is some distance form my area of expertise!
> >
> > I doubt :).
> >
> >>
> >> Jonathan
> >>>
> >>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >>> ---
> >>> drivers/iio/adc/Kconfig | 4 ++
> >>> drivers/iio/adc/vf610_adc.c | 107 +++++++++++++++++++++++++++++++++++++++++---
> >>> 2 files changed, 105 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >>> index 7c55658..4661241 100644
> >>> --- a/drivers/iio/adc/Kconfig
> >>> +++ b/drivers/iio/adc/Kconfig
> >>> @@ -337,6 +337,10 @@ config TWL6030_GPADC
> >>> config VF610_ADC
> >>> tristate "Freescale vf610 ADC driver"
> >>> depends on OF
> >>> + select IIO_BUFFER
> >>> + select IIO_TRIGGER
> >>> + select IIO_SYSFS_TRIGGER
> >> Unless I missed something there is no dependency on this particular
> >> trigger (it just happens to be the one you've been testing with I guess).
> >> Could be driven from a hardware trigger belonging to another device for
> >> example.
> >
> > Yes right in a way. Right now we do not provide or there is no provision
> > for hardware triggers. On the Vybrid, the Peripheral Delay Block (PDB)
> > does the job of providing support for software and hardware triggers. PDB
> > support is not yet there in Linux however.
> Not supplying a trigger is fine, but any trigger (that allows other devices
> to bind to it) will be fine.
Ok.
> >
> > There is also the question of where the Vybrid PDB driver would belong
> > to? In the TRM it is put in the timers but the kernel has no generic timer
> > framework that I am aware of.
> It's been debated a number of times, but no one has ever written one.
> Not seen anything recently on this topic.. Lars, you seen anything?
> (the blackfin-timer trigger is similar to what you are describing I think).
>
> > After some internal reviews we decided to
> > do a platform driver which provided functions ADC driver could called into.
> Does rather feel like there ought to be at least a standard home for these.
> Might be worth asking the arm-soc guys...
> >
> > I have a patchset ready which provides trigger support using PDB however
> > configuring the PDB properly has proven to be tricky. While it works but
> > not reliably with multiple channels and it would be a while before I get
> > that working and post that patchset. So kind of stalled there and just
> > because of two registers which need to be written with the correct value
> > :). For what it's worth if someone comes across this, some discussion
> > here [1] along with patches. (Note however those are a bit old patches
> > not exactly my new work).
> >
> > Sorry for digressing from the topic. Anyways so the idea was to provide
> > sysfs triggers as default for using this continuous sampling. Later the
> > driver may provide additional triggers. So for now I added the sysfs
> > trigger as a select option so that a user won't have to recompile the
> > kernel for using the buffers with continuous sampling.
> It's a policy decision so should really be left to the distro builders.
> There are lots of possible triggers out there (though sysfs might be
> the most likely one!)
>
> Hence don't put the select there in Kconfig. We should shortly have
> Daniel's update to the patches for the high resolution timer
> which would give another obvious choice for starters.
Alright. Will drop that iio sysfs tigger select from the Kconfig.
>
> I doubt many distros build without the sysfs triggers but with other IIO
> stuff.
>
> >
> >>
> >>> + select IIO_TRIGGERED_BUFFER
> >>> help
> >>> Say yes here to support for Vybrid board analog-to-digital converter.
> >>> Since the IP is used for i.MX6SLX, the driver also support i.MX6SLX.
> >>> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> >>> index 23b8fb9..97cb2ed 100644
> >>> --- a/drivers/iio/adc/vf610_adc.c
> >>> +++ b/drivers/iio/adc/vf610_adc.c
> >>> @@ -34,8 +34,11 @@
> >>> #include <linux/err.h>
> >>>
> >>> #include <linux/iio/iio.h>
> >>> +#include <linux/iio/buffer.h>
> >>> #include <linux/iio/sysfs.h>
> >>> -#include <linux/iio/driver.h>
> >>> +#include <linux/iio/trigger.h>
> >>> +#include <linux/iio/trigger_consumer.h>
> >>> +#include <linux/iio/triggered_buffer.h>
> >>>
> >>> /* This will be the driver name the kernel reports */
> >>> #define DRIVER_NAME "vf610-adc"
> >>> @@ -170,6 +173,7 @@ struct vf610_adc {
> >>> u32 sample_freq_avail[5];
> >>>
> >>> struct completion completion;
> >>> + u16 buffer[2];
> >> Note the requirements on the buffer provided to
> >> iio_push_to_buffers_with_timestamp
> >> Needs to be u16 buffer[8] to allow for the aligned 64bit (4 word) timestamp.
> >>
> >> Peter pointed this out in his follow up email and you said you'd implement
> >> it.. Guessing this got lost somewhere.
> >
> > No, I meant to implement what Peter recommended but I guess I did not completely
> > grasp what he intended. Sorry about that. Will fix this and ask further if in
> > more doubts.
> >
> >>
> >>
> >>> };
> >>>
> >>> static const u32 vf610_hw_avgs[] = { 1, 4, 8, 16, 32 };
> >>> @@ -505,12 +509,22 @@ static const struct iio_chan_spec_ext_info vf610_ext_info[] = {
> >>> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> >>> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> >>> .ext_info = vf610_ext_info, \
> >>> + .address = (_idx), \
> >>> + .scan_index = (_idx), \
> >>> + .scan_type.sign = 'u', \
> >>> + .scan_type.realbits = 12, \
> >>> + .scan_type.storagebits = 16, \
> >>> }
> >>>
> >>> #define VF610_ADC_TEMPERATURE_CHAN(_idx, _chan_type) { \
> >>> .type = (_chan_type), \
> >>> .channel = (_idx), \
> >>> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> >>> + .address = (_idx), \
> >>> + .scan_index = (_idx), \
> >> .scan_type = {
> >> .sign = 'u',
> >> etc.
> >>
> >> Peter picked up on this..
> >
> > Sorry yes missed that. Will definitely fix.
> >
> >>
> >>> + .scan_type.sign = 'u', \
> >>> + .scan_type.realbits = 12, \
> >>> + .scan_type.storagebits = 16, \
> >>> }
> >>>
> >>> static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> >>> @@ -531,6 +545,7 @@ static const struct iio_chan_spec vf610_adc_iio_channels[] = {
> >>> VF610_ADC_CHAN(14, IIO_VOLTAGE),
> >>> VF610_ADC_CHAN(15, IIO_VOLTAGE),
> >>> VF610_ADC_TEMPERATURE_CHAN(26, IIO_TEMP),
> >>> + IIO_CHAN_SOFT_TIMESTAMP(32),
> >> It's bit extreme throwing it out at scan_index 32. Is there a reason
> >> to think that migh be neccesary? Mind you, why is the temperature
> >> channel down at 26? Are we dealing with a set of reserved real channels
> >> inbetween?
> >
> >
> > The temperature channel is the 26th channel and there are some reserved
> > channels in between. 31st is the channel which acts as a conversion
> > disabled setting. So I put the timestamp at index 32.
> Fair enough if weird ;)
> >
> >
> >>> /* sentinel */
> >>> };
> >>>
> >>> @@ -559,13 +574,20 @@ static int vf610_adc_read_data(struct vf610_adc *info)
> >>>
> >>> static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
> >>> {
> >>> - struct vf610_adc *info = (struct vf610_adc *)dev_id;
> >>> + struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
> >>> + struct vf610_adc *info = iio_priv(indio_dev);
> >>> int coco;
> >>>
> >>> coco = readl(info->regs + VF610_REG_ADC_HS);
> >>> if (coco & VF610_ADC_HS_COCO0) {
> >>> info->value = vf610_adc_read_data(info);
> >>
> >> I'd be tempted to make the non buffered path also use
> >> info->bufffer[0] and drop info->value entirely.
> >> A more invasive patch, but a cleaner resulting code (slightly!)
> >
> > I did think of that but decided against it as I wanted the changes
> > to as less invasive as possible making only the necessary changes
> > and keeping the old code as is.
> You could do this then clean up in a follow up patch which would be
> really easy to review.
Ok. Will implement the same with a follow up second patch in the third
revision.
- Sanchayan.
> >
> >>> - complete(&info->completion);
> >>> + if (iio_buffer_enabled(indio_dev)) {
> >>> + info->buffer[0] = info->value;
> >>> + iio_push_to_buffers_with_timestamp(indio_dev,
> >>> + info->buffer, iio_get_time_ns());
> >>> + iio_trigger_notify_done(indio_dev->trig);
> >>> + } else
> >>> + complete(&info->completion);
> >>> }
> >>>
> >>> return IRQ_HANDLED;
> >>> @@ -612,6 +634,9 @@ static int vf610_read_raw(struct iio_dev *indio_dev,
> >>> switch (mask) {
> >>> case IIO_CHAN_INFO_RAW:
> >>> case IIO_CHAN_INFO_PROCESSED:
> >>
> >> To avoid possible races this check should be done under the mlock
> >
> > Thanks for picking this. Somehow I remember seeing it outside of the
> > mlock. However grepping again shows otherwise. Will fix.
> >
> > .
> >>> + if (iio_buffer_enabled(indio_dev))
> >>> + return -EBUSY;
> >>> +
> >>> mutex_lock(&indio_dev->mlock);
> >>> reinit_completion(&info->completion);
> >>>
> >>> @@ -694,6 +719,68 @@ static int vf610_write_raw(struct iio_dev *indio_dev,
> >>> return -EINVAL;
> >>> }
> >>>
> >>> +static int vf610_adc_buffer_postenable(struct iio_dev *indio_dev)
> >>> +{
> >>> + struct vf610_adc *info = iio_priv(indio_dev);
> >>> + unsigned int channel;
> >>> + int ret;
> >>> + int val;
> >>> +
> >>> + ret = iio_triggered_buffer_postenable(indio_dev);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + val = readl(info->regs + VF610_REG_ADC_GC);
> >>> + val |= VF610_ADC_ADCON;
> >>> + writel(val, info->regs + VF610_REG_ADC_GC);
> >>> +
> >>> + channel = find_first_bit(indio_dev->active_scan_mask,
> >>> + indio_dev->masklength);
> >>> +
> >>> + val = VF610_ADC_ADCHC(channel);
> >>> + val |= VF610_ADC_AIEN;
> >>> +
> >>> + writel(val, info->regs + VF610_REG_ADC_HC0);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int vf610_adc_buffer_postdisable(struct iio_dev *indio_dev)
> >>> +{
> >>> + struct vf610_adc *info = iio_priv(indio_dev);
> >>> + unsigned int hc_cfg = 0;
> >>> + int val;
> >>> +
> >>> + val = readl(info->regs + VF610_REG_ADC_GC);
> >>> + val &= ~VF610_ADC_ADCON;
> >>> + writel(val, info->regs + VF610_REG_ADC_GC);
> >>> +
> >>> + hc_cfg |= VF610_ADC_CONV_DISABLE;
> >>> + hc_cfg &= ~VF610_ADC_AIEN;
> >>> +
> >>> + writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
> >>> + .postenable = &vf610_adc_buffer_postenable,
> >>> + .predisable = &iio_triggered_buffer_predisable,
> >>> + .postdisable = &vf610_adc_buffer_postdisable,
> >>> + .validate_scan_mask = &iio_validate_scan_mask_onehot,
> >>> +};
> >>> +
> >>> +static int vf610_adc_buffer_init(struct iio_dev *indio_dev)
> >>> +{
> >>> + return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
> >>> + NULL, &iio_triggered_buffer_setup_ops);
> >>> +}
> >>> +
> >>> +static void vf610_adc_buffer_remove(struct iio_dev *indio_dev)
> >>> +{
> >>> + iio_triggered_buffer_cleanup(indio_dev);
> >>> +}
> >> These to wrappers seem a little superflous. I'd have just put the code
> >> inline, but it's obviously a matter of personal taste and I don't care
> >> that much!
> >
> > Ok. I do not have strong opinions on this. I just tried to follow how the
> > at91 adc code did it considering it as a good example. Will drop it.
> >
> >>
> >>> +
> >>> static int vf610_adc_reg_access(struct iio_dev *indio_dev,
> >>> unsigned reg, unsigned writeval,
> >>> unsigned *readval)
> >>> @@ -753,7 +840,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
> >>>
> >>> ret = devm_request_irq(info->dev, irq,
> >>> vf610_adc_isr, 0,
> >>> - dev_name(&pdev->dev), info);
> >>> + dev_name(&pdev->dev), indio_dev);
> >>> if (ret < 0) {
> >>> dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> >>> return ret;
> >>> @@ -806,15 +893,22 @@ static int vf610_adc_probe(struct platform_device *pdev)
> >>> vf610_adc_cfg_init(info);
> >>> vf610_adc_hw_init(info);
> >>>
> >>> + ret = vf610_adc_buffer_init(indio_dev);
> >>> + if (ret < 0) {
> >>> + dev_err(&pdev->dev, "Couldn't initialise the buffer\n");
> >>> + goto error_iio_device_register;
> >>> + }
> >>> +
> >>> ret = iio_device_register(indio_dev);
> >>> if (ret) {
> >>> dev_err(&pdev->dev, "Couldn't register the device.\n");
> >>> - goto error_iio_device_register;
> >>> + goto error_adc_buffer_init;
> >>> }
> >>>
> >>> return 0;
> >>>
> >>> -
> >>> +error_adc_buffer_init:
> >>> + vf610_adc_buffer_remove(indio_dev);
> >>> error_iio_device_register:
> >>> clk_disable_unprepare(info->clk);
> >>> error_adc_clk_enable:
> >>> @@ -829,6 +923,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
> >>> struct vf610_adc *info = iio_priv(indio_dev);
> >>>
> >>> iio_device_unregister(indio_dev);
> >>> + vf610_adc_buffer_remove(indio_dev);
> >>> regulator_disable(info->vref);
> >>> clk_disable_unprepare(info->clk);
> >>>
> >>>
> >>
> >
> > Thanks for the review.
> >
> > - Sanchayan.
> >
> > [1]. https://community.freescale.com/thread/357619
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-08-10 6:38 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-05 12:10 [PATCH v2] Add continuous sampling with IIO buffers for Vybrid Sanchayan Maity
2015-08-05 12:10 ` Sanchayan Maity
2015-08-05 12:10 ` [PATCH v2] iio: adc: vf610: Add IIO buffer support for Vybrid ADC Sanchayan Maity
2015-08-05 12:10 ` Sanchayan Maity
2015-08-08 14:29 ` Jonathan Cameron
2015-08-08 14:29 ` Jonathan Cameron
2015-08-08 17:22 ` maitysanchayan
2015-08-08 17:22 ` maitysanchayan at gmail.com
2015-08-08 18:46 ` Jonathan Cameron
2015-08-08 18:46 ` Jonathan Cameron
2015-08-10 6:35 ` maitysanchayan
2015-08-10 6:35 ` maitysanchayan at gmail.com
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.