Deal with iio trigger names
Subsystems (2)
cover Deal with iio trigger names
IIO documents that trigger names are unique but does not actually guarantee this. You can easily create a software trigger with a duplicate name if you enable CONFIG_IIO_HRTIMER_TRIGGER:
mkdir /sys/kernel/config/iio/triggers/hrtimer/\ `cat /sys/bus/iio/devices/trigger0/name`
You can also get in this situation if you connect two identical st_sensors.
My attempt to fix this is by adding a new 'current_trigger_id' ABI which is a numeric ID (X from triggerX) rather than a non-unique string. I tested that this works as expected when connecting two LIS3DH to the same system. This is in patches 4,5.
In theory the current_trigger ABI could be overloaded to looks for something matching "trigger[0-9]+" but this would be nastier. There would be a need to prevent registering triggers that match that expression in order to prevent stuff like:
mkdir /sys/kernel/config/iio/triggers/hrtimer/trigger1
Such an overload wouldn't work on read and make it impossible to unambigiously determine the currently selected trigger.
Patch 6 disallows registering duplicate trigger names. This makes drivers which currently do that break at probe time.
Patch 7 attempts to ensure that all drivers include indio_dev->id when calling iio_trigger_alloc. This obviously changes trigger names.
Either 4,5 or 6,7 fix this issue, but 4,5 causes fewer compat issues.
Patches 1,2,3 are just minor features in tools/generic_buffer. They supersede the v2 I posted earlier:
https://www.spinics.net/lists/linux-iio/msg24867.html
Crestez Dan Leonard (7):
iio: generic_buffer: Cleanup when receiving signals
iio: generic_buffer: Add --device-num option
iio: generic_buffer: Add --trigger-num option
iio: Add current_trigger_id alternative
iio: generic_buffer: Use current_trigger_id
iio: Refuse to register triggers with duplicate names
iio: Make trigger names unique
Documentation/ABI/testing/sysfs-bus-iio | 9 + Documentation/DocBook/iio.tmpl | 4 +- drivers/iio/adc/max1027.c | 4 +- drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +- drivers/iio/industrialio-trigger.c | 136 ++++++++--- drivers/iio/light/gp2ap020a00f.c | 4 +- tools/iio/generic_buffer.c | 269 ++++++++++++++------- 7 files changed, 309 insertions(+), 119 deletions(-)
Signature · 1 line
2.5.5
PATCH v3 1/7 iio: generic_buffer: Cleanup when receiving signals
This will clean (disable buffer/trigger/channels) when doing something like a CTRL-C. Otherwise restarting generic_buffer requires a manual echo 0 > buffer/enable
This also cleanup up all the code freeing string buffers at the end of main. We initialize all pointers to NULL so that cleanup can all be done under a single error label.
---
Changes since v2: Initialize data to NULL to prevent crash on free.
tools/iio/generic_buffer.c | 172 ++++++++++++++++++++++++++++----------------- 1 file changed, 108 insertions(+), 64 deletions(-)
tools/iio/generic_buffer.c +108 -64
@@ -32,6 +32,8 @@#include <endian.h> #include <getopt.h> #include <inttypes.h> +#include <stdbool.h> +#include <signal.h> #include "iio_utils.h" /**
@@ -254,6 +256,65 @@ void print_usage(void) " -w <n> Set delay between reads in us (event-less mode)\n"); } +enum autochan autochannels =AUTOCHANNELS_DISABLED ; +char *dev_dir_name =NULL ; +char *buf_dir_name =NULL ; +bool current_trigger_set = false; + +void cleanup (void ) +{ +int ret; + +/* Disable trigger */ +if (dev_dir_name && current_trigger_set) { +/* Disconnect the trigger - just write a dummy name. */ + ret =write_sysfs_string ("trigger/current_trigger" , + dev_dir_name,"NULL" ); +if (ret <0 ) +fprintf (stderr,"Failed to disable trigger: %s\n" , +strerror (-ret)); + current_trigger_set = false; + } + +/* Disable buffer */ +if (buf_dir_name) { + ret =write_sysfs_int ("enable" , buf_dir_name,0 ); +if (ret <0 ) +fprintf (stderr,"Failed to disable buffer: %s\n" , +strerror (-ret)); + } + +/* Disable channels if auto-enabled */ +if (dev_dir_name && autochannels ==AUTOCHANNELS_ACTIVE ) { + ret =enable_disable_all_channels (dev_dir_name,0 ); +if (ret) +fprintf (stderr,"Failed to disable all channels\n" ); + autochannels =AUTOCHANNELS_DISABLED ; + } +} + +void sig_handler (int signum) +{ +fprintf (stderr,"Caught signal %d\n" , signum); +cleanup (); +exit (-signum); +} + +void register_cleanup (void ) +{ +struct sigaction sa = { .sa_handler = sig_handler }; +const int signums[] = {SIGINT ,SIGTERM ,SIGABRT }; +int ret, i; + +for (i =0 ; i <ARRAY_SIZE (signums); ++i) { + ret =sigaction (signums[i], &sa,NULL ); +if (ret) { +perror ("Failed to register signal handler" ); +exit (-1 ); + } + } +} +int main (int argc,char **argv) {unsigned long num_loops =2 ;
@@ -261,25 +322,24 @@ int main(int argc, char **argv)unsigned long buf_len =128 ;int ret, c, i, j, toread; -int fp; +int fp =-1 ; -int num_channels; +int num_channels =0 ;char *trigger_name =NULL , *device_name =NULL ; -char *dev_dir_name, *buf_dir_name; -int datardytrigger =1 ; -char *data; +char *data =NULL ;ssize_t read_size;int dev_num, trig_num; -char *buffer_access; +char *buffer_access =NULL ;int scan_size;int noevents =0 ;int notrigger =0 ; -enum autochan autochannels =AUTOCHANNELS_DISABLED ;char *dummy;struct iio_channel_info *channels; +register_cleanup (); +while ((c =getopt (argc, argv,"ac:egl:n:t:w:" )) !=-1 ) {switch (c) {case 'a' :
@@ -288,8 +348,10 @@ int main(int argc, char **argv)case 'c' : errno =0 ; num_loops =strtoul (optarg, &dummy,10 ); -if (errno) -return -errno; +if (errno) { + ret = -errno; + goto error; + }break ;case 'e' :
@@ -301,26 +363,30 @@ int main(int argc, char **argv)case 'l' : errno =0 ; buf_len =strtoul (optarg, &dummy,10 ); -if (errno) -return -errno; +if (errno) { + ret = -errno; + goto error; + }break ;case 'n' : device_name = optarg;break ;case 't' : - trigger_name = optarg; - datardytrigger =0 ; + trigger_name =strdup (optarg);break ;case 'w' : errno =0 ; timedelay =strtoul (optarg, &dummy,10 ); -if (errno) -return -errno; +if (errno) { + ret = -errno; + goto error; + }break ;case '?' :print_usage (); -return -1 ; + ret =-1 ; + goto error; } }
@@ -334,14 +400,17 @@ int main(int argc, char **argv) dev_num =find_type_by_name (device_name,"iio:device" );if (dev_num <0 ) {fprintf (stderr,"Failed to find the %s\n" , device_name); -return dev_num; + ret = dev_num; + goto error; }printf ("iio device number being used is %d\n" , dev_num); ret =asprintf (&dev_dir_name,"%siio:device%d" , iio_dir, dev_num); -if (ret <0 ) -return -ENOMEM ; +if (ret <0 ) { + ret = -ENOMEM ; + goto error; + }if (!notrigger) {if (!trigger_name) {
@@ -354,7 +423,7 @@ int main(int argc, char **argv) "%s-dev%d", device_name, dev_num);if (ret <0 ) { ret = -ENOMEM ; - goto error_free_dev_dir_name; + goto error; } }
@@ -367,7 +436,7 @@ int main(int argc, char **argv) "%s-trigger", device_name);if (ret <0 ) { ret = -ENOMEM ; - goto error_free_dev_dir_name; + goto error; } }
@@ -376,7 +445,7 @@ int main(int argc, char **argv)fprintf (stderr,"Failed to find the trigger %s\n" , trigger_name); ret = trig_num; - goto error_free_triggername; + goto error; }printf ("iio trigger number being used is %d\n" , trig_num);
@@ -392,7 +461,7 @@ int main(int argc, char **argv)if (ret) {fprintf (stderr,"Problem reading scan element information\n" "diag %s\n" , dev_dir_name); - goto error_free_triggername; + goto error; }if (num_channels && autochannels ==AUTOCHANNELS_ENABLED ) { fprintf(stderr, "Auto-channels selected but some channels "
@@ -407,7 +476,7 @@ int main(int argc, char **argv) ret =enable_disable_all_channels (dev_dir_name,1 );if (ret) {fprintf (stderr,"Failed to enable all channels\n" ); - goto error_free_triggername; + goto error; }/* This flags that we need to disable the channels again */
@@ -419,12 +488,12 @@ int main(int argc, char **argv)fprintf (stderr,"Problem reading scan element " "information\n" "diag %s\n" , dev_dir_name); - goto error_disable_channels; + goto error; }if (!num_channels) {fprintf (stderr,"Still no channels after " "auto-enabling, giving up\n" ); - goto error_disable_channels; + goto error; } }
@@ -436,7 +505,7 @@ int main(int argc, char **argv)"/*_en or pass -a to autoenable channels and " "try again.\n", dev_dir_name); ret = -ENOENT ; - goto error_free_triggername; + goto error; }/*
@@ -448,7 +517,7 @@ int main(int argc, char **argv) "%siio:device%d/buffer", iio_dir, dev_num);if (ret <0 ) { ret = -ENOMEM ; - goto error_free_channels; + goto error; }if (!notrigger) {
@@ -463,34 +532,34 @@ int main(int argc, char **argv)if (ret <0 ) {fprintf (stderr,"Failed to write current_trigger file\n" ); - goto error_free_buf_dir_name; + goto error; } }/* Setup ring buffer parameters */ ret =write_sysfs_int ("length" , buf_dir_name, buf_len);if (ret <0 ) - goto error_free_buf_dir_name; + goto error;/* Enable the buffer */ ret =write_sysfs_int ("enable" , buf_dir_name,1 );if (ret <0 ) {fprintf (stderr,"Failed to enable buffer: %s\n" ,strerror (-ret)); - goto error_free_buf_dir_name; + goto error; } scan_size =size_from_channelarray (channels, num_channels); data =malloc (scan_size * buf_len);if (!data) { ret = -ENOMEM ; - goto error_free_buf_dir_name; + goto error; } ret =asprintf (&buffer_access,"/dev/iio:device%d" , dev_num);if (ret <0 ) { ret = -ENOMEM ; - goto error_free_data; + goto error; }/* Attempt to open non blocking the access dev */
@@ -498,7 +567,7 @@ int main(int argc, char **argv)if (fp ==-1 ) {/* TODO: If it isn't there make the node */ ret = -errno;fprintf (stderr,"Failed to open %s\n" , buffer_access); - goto error_free_buffer_access; + goto error; }for (j =0 ; j < num_loops; j++) {
@@ -511,7 +580,7 @@ int main(int argc, char **argv) ret =poll (&pfd,1 ,-1 );if (ret <0 ) { ret = -errno; - goto error_close_buffer_access; + goto error; }else if (ret ==0 ) {continue ; }
@@ -536,45 +605,20 @@ int main(int argc, char **argv) num_channels); } -/* Stop the buffer */ - ret =write_sysfs_int ("enable" , buf_dir_name,0 ); -if (ret <0 ) - goto error_close_buffer_access; - -if (!notrigger) -/* Disconnect the trigger - just write a dummy name. */ - ret =write_sysfs_string ("trigger/current_trigger" , - dev_dir_name,"NULL" ); -if (ret <0 ) -fprintf (stderr,"Failed to write to %s\n" , - dev_dir_name); +error: +cleanup (); -error_close_buffer_access: -if (close (fp) ==-1 ) +if (fp >=0 &&close (fp) ==-1 )perror ("Failed to close buffer" ); - -error_free_buffer_access:free (buffer_access); -error_free_data:free (data); -error_free_buf_dir_name:free (buf_dir_name); -error_free_channels:for (i = num_channels -1 ; i >=0 ; i--) {free (channels[i].name);free (channels[i].generic_name); }free (channels); -error_free_triggername: -if (datardytrigger) -free (trigger_name); -error_disable_channels: -if (autochannels ==AUTOCHANNELS_ACTIVE ) { - ret =enable_disable_all_channels (dev_dir_name,0 ); -if (ret) -fprintf (stderr,"Failed to disable all channels\n" ); - } -error_free_dev_dir_name: +free (trigger_name);free (dev_dir_name);return ret;
Signature · 1 line
2.5.5
Jonathan Cameron
maintainerRe: [PATCHv3 1/7] iio: generic_buffer: Cleanup when receiving signals
Quoted 9 lines · Crestez Dan Leonard
This will clean (disable buffer/trigger/channels) when doing something like a CTRL-C. Otherwise restarting
generic_bufferrequires a manual echo 0 > buffer/enableThis also cleanup up all the code freeing string buffers at the end of main. We initialize all pointers to NULL so that cleanup can all be done under a single error label.
Signed-off-by Crestez Dan Leonard <[email protected]>
Dropped v2 and applied this one with the same name mangling to match new naming as iio_generic_buffer
Thanks,
Jonathan
Quoted 376 lines
---
Changes since v2: Initialize data to NULL to prevent crash on free.tools/iio/generic_buffer.c | 172 ++++++++++++++++++++++++++++----------------- 1 file changed, 108 insertions(+), 64 deletions(-)tools/iio/generic_buffer.c +108
-64@@ -32,6 +32,8 @@#include <endian.h> #include <getopt.h> #include <inttypes.h> +#include <stdbool.h> +#include <signal.h> #include "iio_utils.h" /** @@ -254,6 +256,65 @@ void print_usage(void) " -w <n> Set delay between reads in us (event-less mode)\n"); } +enum autochan autochannels =AUTOCHANNELS_DISABLED ; +char *dev_dir_name =NULL ; +char *buf_dir_name =NULL ; +bool current_trigger_set = false; + +void cleanup (void ) +{ +int ret; + +/* Disable trigger */ +if (dev_dir_name && current_trigger_set) { +/* Disconnect the trigger - just write a dummy name. */ + ret =write_sysfs_string ("trigger/current_trigger" , + dev_dir_name,"NULL" ); +if (ret <0 ) +fprintf (stderr,"Failed to disable trigger: %s\n" , +strerror (-ret)); + current_trigger_set = false; + } + +/* Disable buffer */ +if (buf_dir_name) { + ret =write_sysfs_int ("enable" , buf_dir_name,0 ); +if (ret <0 ) +fprintf (stderr,"Failed to disable buffer: %s\n" , +strerror (-ret)); + } + +/* Disable channels if auto-enabled */ +if (dev_dir_name && autochannels ==AUTOCHANNELS_ACTIVE ) { + ret =enable_disable_all_channels (dev_dir_name,0 ); +if (ret) +fprintf (stderr,"Failed to disable all channels\n" ); + autochannels =AUTOCHANNELS_DISABLED ; + } +} + +void sig_handler (int signum) +{ +fprintf (stderr,"Caught signal %d\n" , signum); +cleanup (); +exit (-signum); +} + +void register_cleanup (void ) +{ +struct sigaction sa = { .sa_handler = sig_handler }; +const int signums[] = {SIGINT ,SIGTERM ,SIGABRT }; +int ret, i; + +for (i =0 ; i <ARRAY_SIZE (signums); ++i) { + ret =sigaction (signums[i], &sa,NULL ); +if (ret) { +perror ("Failed to register signal handler" ); +exit (-1 ); + } + } +} +int main (int argc,char **argv) {unsigned long num_loops =2 ;@@ -261,25 +322,24 @@ int main(int argc, char **argv)unsigned long buf_len =128 ;int ret, c, i, j, toread; -int fp; +int fp =-1 ; -int num_channels; +int num_channels =0 ;char *trigger_name =NULL , *device_name =NULL ; -char *dev_dir_name, *buf_dir_name; -int datardytrigger =1 ; -char *data; +char *data =NULL ;ssize_t read_size;int dev_num, trig_num; -char *buffer_access; +char *buffer_access =NULL ;int scan_size;int noevents =0 ;int notrigger =0 ; -enum autochan autochannels =AUTOCHANNELS_DISABLED ;char *dummy;struct iio_channel_info *channels; +register_cleanup (); +while ((c =getopt (argc, argv,"ac:egl:n:t:w:" )) !=-1 ) {switch (c) {case 'a' :@@ -288,8 +348,10 @@ int main(int argc, char **argv)case 'c' : errno =0 ; num_loops =strtoul (optarg, &dummy,10 ); -if (errno) -return -errno; +if (errno) { + ret = -errno; + goto error; + }break ;case 'e' :@@ -301,26 +363,30 @@ int main(int argc, char **argv)case 'l' : errno =0 ; buf_len =strtoul (optarg, &dummy,10 ); -if (errno) -return -errno; +if (errno) { + ret = -errno; + goto error; + }break ;case 'n' : device_name = optarg;break ;case 't' : - trigger_name = optarg; - datardytrigger =0 ; + trigger_name =strdup (optarg);break ;case 'w' : errno =0 ; timedelay =strtoul (optarg, &dummy,10 ); -if (errno) -return -errno; +if (errno) { + ret = -errno; + goto error; + }break ;case '?' :print_usage (); -return -1 ; + ret =-1 ; + goto error; } }@@ -334,14 +400,17 @@ int main(int argc, char **argv) dev_num =find_type_by_name (device_name,"iio:device" );if (dev_num <0 ) {fprintf (stderr,"Failed to find the %s\n" , device_name); -return dev_num; + ret = dev_num; + goto error; }printf ("iio device number being used is %d\n" , dev_num); ret =asprintf (&dev_dir_name,"%siio:device%d" , iio_dir, dev_num); -if (ret <0 ) -return -ENOMEM ; +if (ret <0 ) { + ret = -ENOMEM ; + goto error; + }if (!notrigger) {if (!trigger_name) {@@ -354,7 +423,7 @@ int main(int argc, char **argv) "%s-dev%d", device_name, dev_num);if (ret <0 ) { ret = -ENOMEM ; - goto error_free_dev_dir_name; + goto error; } }@@ -367,7 +436,7 @@ int main(int argc, char **argv) "%s-trigger", device_name);if (ret <0 ) { ret = -ENOMEM ; - goto error_free_dev_dir_name; + goto error; } }@@ -376,7 +445,7 @@ int main(int argc, char **argv)fprintf (stderr,"Failed to find the trigger %s\n" , trigger_name); ret = trig_num; - goto error_free_triggername; + goto error; }printf ("iio trigger number being used is %d\n" , trig_num);@@ -392,7 +461,7 @@ int main(int argc, char **argv)if (ret) {fprintf (stderr,"Problem reading scan element information\n" "diag %s\n" , dev_dir_name); - goto error_free_triggername; + goto error; }if (num_channels && autochannels ==AUTOCHANNELS_ENABLED ) { fprintf(stderr, "Auto-channels selected but some channels "@@ -407,7 +476,7 @@ int main(int argc, char **argv) ret =enable_disable_all_channels (dev_dir_name,1 );if (ret) {fprintf (stderr,"Failed to enable all channels\n" ); - goto error_free_triggername; + goto error; }/* This flags that we need to disable the channels again */ @@ -419,12 +488,12 @@ int main(int argc, char **argv)fprintf (stderr,"Problem reading scan element " "information\n" "diag %s\n" , dev_dir_name); - goto error_disable_channels; + goto error; }if (!num_channels) {fprintf (stderr,"Still no channels after " "auto-enabling, giving up\n" ); - goto error_disable_channels; + goto error; } }@@ -436,7 +505,7 @@ int main(int argc, char **argv)"/*_en or pass -a to autoenable channels and " "try again.\n", dev_dir_name); ret = -ENOENT ; - goto error_free_triggername; + goto error; }/* @@ -448,7 +517,7 @@ int main(int argc, char **argv) "%siio:device%d/buffer", iio_dir, dev_num);if (ret <0 ) { ret = -ENOMEM ; - goto error_free_channels; + goto error; }if (!notrigger) {@@ -463,34 +532,34 @@ int main(int argc, char **argv)if (ret <0 ) {fprintf (stderr,"Failed to write current_trigger file\n" ); - goto error_free_buf_dir_name; + goto error; } }/* Setup ring buffer parameters */ ret =write_sysfs_int ("length" , buf_dir_name, buf_len);if (ret <0 ) - goto error_free_buf_dir_name; + goto error;/* Enable the buffer */ ret =write_sysfs_int ("enable" , buf_dir_name,1 );if (ret <0 ) {fprintf (stderr,"Failed to enable buffer: %s\n" ,strerror (-ret)); - goto error_free_buf_dir_name; + goto error; } scan_size =size_from_channelarray (channels, num_channels); data =malloc (scan_size * buf_len);if (!data) { ret = -ENOMEM ; - goto error_free_buf_dir_name; + goto error; } ret =asprintf (&buffer_access,"/dev/iio:device%d" , dev_num);if (ret <0 ) { ret = -ENOMEM ; - goto error_free_data; + goto error; }/* Attempt to open non blocking the access dev */ @@ -498,7 +567,7 @@ int main(int argc, char **argv)if (fp ==-1 ) {/* TODO: If it isn't there make the node */ ret = -errno;fprintf (stderr,"Failed to open %s\n" , buffer_access); - goto error_free_buffer_access; + goto error; }for (j =0 ; j < num_loops; j++) {@@ -511,7 +580,7 @@ int main(int argc, char **argv) ret =poll (&pfd,1 ,-1 );if (ret <0 ) { ret = -errno; - goto error_close_buffer_access; + goto error; }else if (ret ==0 ) {continue ; }@@ -536,45 +605,20 @@ int main(int argc, char **argv) num_channels); } -/* Stop the buffer */ - ret =write_sysfs_int ("enable" , buf_dir_name,0 ); -if (ret <0 ) - goto error_close_buffer_access; - -if (!notrigger) -/* Disconnect the trigger - just write a dummy name. */ - ret =write_sysfs_string ("trigger/current_trigger" , - dev_dir_name,"NULL" ); -if (ret <0 ) -fprintf (stderr,"Failed to write to %s\n" , - dev_dir_name); +error: +cleanup (); -error_close_buffer_access: -if (close (fp) ==-1 ) +if (fp >=0 &&close (fp) ==-1 )perror ("Failed to close buffer" ); - -error_free_buffer_access:free (buffer_access); -error_free_data:free (data); -error_free_buf_dir_name:free (buf_dir_name); -error_free_channels:for (i = num_channels -1 ; i >=0 ; i--) {free (channels[i].name);free (channels[i].generic_name); }free (channels); -error_free_triggername: -if (datardytrigger) -free (trigger_name); -error_disable_channels: -if (autochannels ==AUTOCHANNELS_ACTIVE ) { - ret =enable_disable_all_channels (dev_dir_name,0 ); -if (ret) -fprintf (stderr,"Failed to disable all channels\n" ); - } -error_free_dev_dir_name: +free (trigger_name);free (dev_dir_name);return ret;
PATCH v3 2/7 iio: generic_buffer: Add --device-num option
In reply to Crestez Dan LeonardThis makes it possible to distinguish between iio devices with the same name.
--- tools/iio/generic_buffer.c | 69 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 18 deletions(-)
tools/iio/generic_buffer.c +51 -18
@@ -251,7 +251,9 @@ void print_usage(void)" -e Disable wait for event (new data)\n" " -g Use trigger-less mode\n" " -l <n> Set buffer length to n samples\n" -" -n <name> Set device name (mandatory)\n" +" --device-name -n <name>\n" +" --device-num -N <num>\n" +" Set device by name or number (mandatory)\n" " -t <name> Set trigger name\n" " -w <n> Set delay between reads in us (event-less mode)\n"); }
@@ -315,6 +317,12 @@ void register_cleanup(void) } } +static const struct option longopts[] = { + {"device-name" ,1 ,0 ,'n' }, + {"device-num" ,1 ,0 ,'N' }, + { }, +}; +int main (int argc,char **argv) {unsigned long num_loops =2 ;
@@ -329,7 +337,7 @@ int main(int argc, char **argv)char *data =NULL ;ssize_t read_size; -int dev_num, trig_num; +int dev_num =-1 , trig_num;char *buffer_access =NULL ;int scan_size;int noevents =0 ;
@@ -340,7 +348,7 @@ int main(int argc, char **argv)register_cleanup (); -while ((c =getopt (argc, argv,"ac:egl:n:t:w:" )) !=-1 ) { +while ((c =getopt_long (argc, argv,"ac:egl:n:N:t:w:" , longopts,NULL )) !=-1 ) {switch (c) {case 'a' : autochannels =AUTOCHANNELS_ENABLED ;
@@ -370,7 +378,15 @@ int main(int argc, char **argv)break ;case 'n' : - device_name = optarg; + device_name =strdup (optarg); +break ; +case 'N' : + errno =0 ; + dev_num =strtoul (optarg, &dummy,10 ); +if (errno) { + ret = -errno; + goto error; + }break ;case 't' : trigger_name =strdup (optarg);
@@ -390,26 +406,42 @@ int main(int argc, char **argv) } } -if (!device_name) { -fprintf (stderr,"Device name not set\n" ); -print_usage (); -return -1 ; - } -/* Find the device requested */ - dev_num =find_type_by_name (device_name,"iio:device" ); -if (dev_num <0 ) { -fprintf (stderr,"Failed to find the %s\n" , device_name); - ret = dev_num; +if (dev_num <0 && !device_name) { +fprintf (stderr,"Device not set\n" ); +print_usage (); + ret =-1 ; + goto error; + }else if (dev_num >=0 && device_name) { +fprintf (stderr,"Only one of --device-num or --device-name needs to be set\n" ); +print_usage (); + ret =-1 ; goto error; + }else if (dev_num <0 ) { + dev_num =find_type_by_name (device_name,"iio:device" ); +if (dev_num <0 ) { +fprintf (stderr,"Failed to find the %s\n" , device_name); + ret = dev_num; + goto error; + } } -printf ("iio device number being used is %d\n" , dev_num); ret =asprintf (&dev_dir_name,"%siio:device%d" , iio_dir, dev_num); -if (ret <0 ) { - ret = -ENOMEM ; - goto error; +if (ret <0 ) +return -ENOMEM ; +/* Fetch device_name if specified by number */ +if (!device_name) { + device_name =malloc (IIO_MAX_NAME_LENGTH ); +if (!device_name) { + ret = -ENOMEM ; + goto error; + } + ret =read_sysfs_string ("name" , dev_dir_name, device_name); +if (ret <0 ) { +fprintf (stderr,"Failed to read name of device %d\n" , dev_num); + goto error; + } }if (!notrigger) {
@@ -619,6 +651,7 @@ error: }free (channels);free (trigger_name); +free (device_name);free (dev_dir_name);return ret;
Signature · 1 line
2.5.5
Re: [PATCHv3 2/7] iio: generic_buffer: Add --device-num option
Quoted 4 lines · Crestez Dan Leonard
This makes it possible to distinguish between iio devices with the same name.
Signed-off-by Crestez Dan Leonard <[email protected]>
Applied.
Quoted 133 lines
--- tools/iio/generic_buffer.c | 69 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 18 deletions(-)tools/iio/generic_buffer.c +51
-18@@ -251,7 +251,9 @@ void print_usage(void)" -e Disable wait for event (new data)\n" " -g Use trigger-less mode\n" " -l <n> Set buffer length to n samples\n" -" -n <name> Set device name (mandatory)\n" +" --device-name -n <name>\n" +" --device-num -N <num>\n" +" Set device by name or number (mandatory)\n" " -t <name> Set trigger name\n" " -w <n> Set delay between reads in us (event-less mode)\n"); }@@ -315,6 +317,12 @@ void register_cleanup(void) } } +static const struct option longopts[] = { + {"device-name" ,1 ,0 ,'n' }, + {"device-num" ,1 ,0 ,'N' }, + { }, +}; +int main (int argc,char **argv) {unsigned long num_loops =2 ;@@ -329,7 +337,7 @@ int main(int argc, char **argv)char *data =NULL ;ssize_t read_size; -int dev_num, trig_num; +int dev_num =-1 , trig_num;char *buffer_access =NULL ;int scan_size;int noevents =0 ;@@ -340,7 +348,7 @@ int main(int argc, char **argv)register_cleanup (); -while ((c =getopt (argc, argv,"ac:egl:n:t:w:" )) !=-1 ) { +while ((c =getopt_long (argc, argv,"ac:egl:n:N:t:w:" , longopts,NULL )) !=-1 ) {switch (c) {case 'a' : autochannels =AUTOCHANNELS_ENABLED ;@@ -370,7 +378,15 @@ int main(int argc, char **argv)break ;case 'n' : - device_name = optarg; + device_name =strdup (optarg); +break ; +case 'N' : + errno =0 ; + dev_num =strtoul (optarg, &dummy,10 ); +if (errno) { + ret = -errno; + goto error; + }break ;case 't' : trigger_name =strdup (optarg);@@ -390,26 +406,42 @@ int main(int argc, char **argv) } } -if (!device_name) { -fprintf (stderr,"Device name not set\n" ); -print_usage (); -return -1 ; - } -/* Find the device requested */ - dev_num =find_type_by_name (device_name,"iio:device" ); -if (dev_num <0 ) { -fprintf (stderr,"Failed to find the %s\n" , device_name); - ret = dev_num; +if (dev_num <0 && !device_name) { +fprintf (stderr,"Device not set\n" ); +print_usage (); + ret =-1 ; + goto error; + }else if (dev_num >=0 && device_name) { +fprintf (stderr,"Only one of --device-num or --device-name needs to be set\n" ); +print_usage (); + ret =-1 ; goto error; + }else if (dev_num <0 ) { + dev_num =find_type_by_name (device_name,"iio:device" ); +if (dev_num <0 ) { +fprintf (stderr,"Failed to find the %s\n" , device_name); + ret = dev_num; + goto error; + } } -printf ("iio device number being used is %d\n" , dev_num); ret =asprintf (&dev_dir_name,"%siio:device%d" , iio_dir, dev_num); -if (ret <0 ) { - ret = -ENOMEM ; - goto error; +if (ret <0 ) +return -ENOMEM ; +/* Fetch device_name if specified by number */ +if (!device_name) { + device_name =malloc (IIO_MAX_NAME_LENGTH ); +if (!device_name) { + ret = -ENOMEM ; + goto error; + } + ret =read_sysfs_string ("name" , dev_dir_name, device_name); +if (ret <0 ) { +fprintf (stderr,"Failed to read name of device %d\n" , dev_num); + goto error; + } }if (!notrigger) {@@ -619,6 +651,7 @@ error: }free (channels);free (trigger_name); +free (device_name);free (dev_dir_name);return ret;
PATCH v3 3/7 iio: generic_buffer: Add --trigger-num option
In reply to Crestez Dan Leonard--- tools/iio/generic_buffer.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-)
tools/iio/generic_buffer.c +29 -5
@@ -254,7 +254,9 @@ void print_usage(void)" --device-name -n <name>\n" " --device-num -N <num>\n" " Set device by name or number (mandatory)\n" -" -t <name> Set trigger name\n" +" --trigger-name -t <name>\n" +" --trigger-num -T <num>\n" +" Set trigger by name or number\n" " -w <n> Set delay between reads in us (event-less mode)\n"); }
@@ -320,6 +322,8 @@ void register_cleanup(void)static const struct option longopts[] = { {"device-name" ,1 ,0 ,'n' }, {"device-num" ,1 ,0 ,'N' }, + {"trigger-name" ,1 ,0 ,'t' }, + {"trigger-num" ,1 ,0 ,'T' }, { }, };
@@ -348,7 +352,7 @@ int main(int argc, char **argv)register_cleanup (); -while ((c =getopt_long (argc, argv,"ac:egl:n:N:t:w:" , longopts,NULL )) !=-1 ) { +while ((c =getopt_long (argc, argv,"ac:egl:n:N:t:T:w:" , longopts,NULL )) !=-1 ) {switch (c) {case 'a' : autochannels =AUTOCHANNELS_ENABLED ;
@@ -391,6 +395,12 @@ int main(int argc, char **argv)case 't' : trigger_name =strdup (optarg);break ; +case 'T' : + errno =0 ; + trig_num =strtoul (optarg, &dummy,10 ); +if (errno) +return -errno; +break ;case 'w' : errno =0 ; timedelay =strtoul (optarg, &dummy,10 );
@@ -444,7 +454,23 @@ int main(int argc, char **argv) } } -if (!notrigger) { +if (notrigger) { +printf ("trigger-less mode selected\n" ); + }if (trig_num >0 ) { +char *trig_dev_name; + ret =asprintf (&trig_dev_name,"%strigger%d" , iio_dir, trig_num); +if (ret <0 ) { +return -ENOMEM ; + } + trigger_name =malloc (IIO_MAX_NAME_LENGTH ); + ret =read_sysfs_string ("name" , trig_dev_name, trigger_name); +free (trig_dev_name); +if (ret <0 ) { +fprintf (stderr,"Failed to read trigger%d name from\n" , trig_num); +return ret; + } +printf ("iio trigger number being used is %d\n" , trig_num); + }else {if (!trigger_name) {/* * Build the trigger name. If it is device associated
@@ -481,8 +507,6 @@ int main(int argc, char **argv) }printf ("iio trigger number being used is %d\n" , trig_num); - }else { -printf ("trigger-less mode selected\n" ); }/*
Signature · 1 line
2.5.5
Jonathan Cameron
maintainerRe: [PATCHv3 3/7] iio: generic_buffer: Add --trigger-num option
Quoted 1 line · Crestez Dan Leonard
Signed-off-by Crestez Dan Leonard <[email protected]>
Again, sensible and straight forward.
Thanks,
Jonathan
Quoted 85 lines
--- tools/iio/generic_buffer.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-)tools/iio/generic_buffer.c +29
-5@@ -254,7 +254,9 @@ void print_usage(void)" --device-name -n <name>\n" " --device-num -N <num>\n" " Set device by name or number (mandatory)\n" -" -t <name> Set trigger name\n" +" --trigger-name -t <name>\n" +" --trigger-num -T <num>\n" +" Set trigger by name or number\n" " -w <n> Set delay between reads in us (event-less mode)\n"); }@@ -320,6 +322,8 @@ void register_cleanup(void)static const struct option longopts[] = { {"device-name" ,1 ,0 ,'n' }, {"device-num" ,1 ,0 ,'N' }, + {"trigger-name" ,1 ,0 ,'t' }, + {"trigger-num" ,1 ,0 ,'T' }, { }, };@@ -348,7 +352,7 @@ int main(int argc, char **argv)register_cleanup (); -while ((c =getopt_long (argc, argv,"ac:egl:n:N:t:w:" , longopts,NULL )) !=-1 ) { +while ((c =getopt_long (argc, argv,"ac:egl:n:N:t:T:w:" , longopts,NULL )) !=-1 ) {switch (c) {case 'a' : autochannels =AUTOCHANNELS_ENABLED ;@@ -391,6 +395,12 @@ int main(int argc, char **argv)case 't' : trigger_name =strdup (optarg);break ; +case 'T' : + errno =0 ; + trig_num =strtoul (optarg, &dummy,10 ); +if (errno) +return -errno; +break ;case 'w' : errno =0 ; timedelay =strtoul (optarg, &dummy,10 );@@ -444,7 +454,23 @@ int main(int argc, char **argv) } } -if (!notrigger) { +if (notrigger) { +printf ("trigger-less mode selected\n" ); + }if (trig_num >0 ) { +char *trig_dev_name; + ret =asprintf (&trig_dev_name,"%strigger%d" , iio_dir, trig_num); +if (ret <0 ) { +return -ENOMEM ; + } + trigger_name =malloc (IIO_MAX_NAME_LENGTH ); + ret =read_sysfs_string ("name" , trig_dev_name, trigger_name); +free (trig_dev_name); +if (ret <0 ) { +fprintf (stderr,"Failed to read trigger%d name from\n" , trig_num); +return ret; + } +printf ("iio trigger number being used is %d\n" , trig_num); + }else {if (!trigger_name) {/* * Build the trigger name. If it is device associated @@ -481,8 +507,6 @@ int main(int argc, char **argv) }printf ("iio trigger number being used is %d\n" , trig_num); - }else { -printf ("trigger-less mode selected\n" ); }/*
RFC 4/7 iio: Add current_trigger_id alternative
In reply to Crestez Dan LeonardThis allows controlling the current trigger by numeric ID rather than name.
--- Documentation/ABI/testing/sysfs-bus-iio | 9 +++ Documentation/DocBook/iio.tmpl | 4 +- drivers/iio/industrialio-trigger.c | 115 ++++++++++++++++++++++++-------- 3 files changed, 98 insertions(+), 30 deletions(-)
Documentation/ABI/testing/sysfs-bus-iio +9 -0
@@ -1038,6 +1038,15 @@ Description: The name of the trigger source being used, as per string given in /sys/class/iio/triggerY/name. +What: /sys/bus/iio/devices/iio:deviceX/trigger/current_trigger_id +KernelVersion: 4.7 +Contact: [email protected] +Description: + The id of the trigger source being used. This is the Y from + /sys/class/iio/triggerY. This is an alternative to + /sys/bus/iio/devices/iio:deviceX/trigger/current_trigger. + Write a negative number to clear. + What: /sys/bus/iio/devices/iio:deviceX/buffer/length KernelVersion: 2.6.35 Contact: [email protected]
Documentation/DocBook/iio.tmpl +3 -1
@@ -523,7 +523,9 @@ <filename>/sys/bus/iio/devices/iio:deviceX/trigger/</filename>, this directory is created once the device supports a triggered buffer. We can associate a trigger with our device by writing - the trigger's name in the <filename>current_trigger</filename> file. + the trigger's name in the <filename>current_trigger</filename> + file. Alternatively we can write the numeric id Y from + triggerY into <filename>current_trigger_id</filename> </listitem> </itemizedlist> </sect2>
drivers/iio/industrialio-trigger.c +86 -29
169 lines not on this page · raw message
Signature · 1 line
2.5.5
Re: [RFC 4/7] iio: Add current_trigger_id alternative
Quoted 2 lines · Crestez Dan Leonard
This allows controlling the current trigger by numeric ID rather than name.
This is most certainly the far better interface, but so far we've tried to have only one way to do a certain task with the IIO userspace ABI and try to live with our mistakes of the past.
And while this is the better interface I don't think it is that much better that it justifies the ambiguity that is introduced by it.
Adding a new interface makes only sense in my opinion if there is a significant improvement in either usability or performance.
RFC 5/7 iio: generic_buffer: Use current_trigger_id
In reply to Crestez Dan LeonardThis is a preferred alternative to 'current_trigger'.
--- tools/iio/generic_buffer.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
tools/iio/generic_buffer.c +4 -4
@@ -582,12 +582,12 @@ int main(int argc, char **argv)* Set the device trigger to be the data ready trigger found * above */ - ret =write_sysfs_string_and_verify ("trigger/current_trigger" , - dev_dir_name, - trigger_name); + ret =write_sysfs_int_and_verify ("trigger/current_trigger_id" , + dev_dir_name, + trig_num);if (ret <0 ) {fprintf (stderr, -"Failed to write current_trigger file\n" ); +"Failed to write current_trigger_id file\n" ); goto error; } }
Signature · 1 line
2.5.5
RFC 6/7 iio: Refuse to register triggers with duplicate names
In reply to Crestez Dan LeonardThe trigger name is documented as unique but drivers are currently allowed to register triggers with duplicate names. This should be considered a bug since it makes the 'current_trigger' interface unusable.
--- drivers/iio/industrialio-trigger.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
drivers/iio/industrialio-trigger.c +21 -0
@@ -64,6 +64,8 @@ static struct attribute *iio_trig_dev_attrs[] = { };ATTRIBUTE_GROUPS (iio_trig_dev); +static struct iio_trigger *__iio_trigger_find_by_name (const char *name); +int iio_trigger_register (struct iio_trigger *trig_info) {int ret;
@@ -82,11 +84,18 @@ int iio_trigger_register(struct iio_trigger *trig_info)/* Add to list of available triggers held by the IIO core */ mutex_lock (&iio_trigger_list_lock); +if (__iio_trigger_find_by_name (trig_info->name)) { +pr_err ("Duplicate trigger name '%s'\n" , trig_info->name); + ret = -EEXIST ; + goto error_device_del; + }list_add_tail (&trig_info->list, &iio_trigger_list);mutex_unlock (&iio_trigger_list_lock);return 0 ; +error_device_del: +device_del (&trig_info->dev); error_unregister_id:ida_simple_remove (&iio_trigger_ida, trig_info->id);return ret;
@@ -105,6 +114,18 @@ void iio_trigger_unregister(struct iio_trigger *trig_info) }EXPORT_SYMBOL (iio_trigger_unregister); +/* Search for trigger by name, assuming iio_trigger_list_lock held */ +static struct iio_trigger *__iio_trigger_find_by_name (const char *name) +{ +struct iio_trigger *iter; + +list_for_each_entry (iter, &iio_trigger_list, list) +if (!strcmp (iter->name, name)) +return iter; + +return NULL ; +} +static struct iio_trigger *iio_trigger_find_by_name (const char *name,size_t len) {
Signature · 1 line
2.5.5
Jonathan Cameron
maintainerRe: [RFC 6/7] iio: Refuse to register triggers with duplicate names
Quoted 6 lines · Crestez Dan Leonard
The trigger name is documented as unique but drivers are currently allowed to register triggers with duplicate names. This should be considered a bug since it makes the '
current_trigger' interface unusable.Signed-off-by Crestez Dan Leonard <[email protected]>
This feels like the right approach to my mind (and should have been there all along - oops).
However, we do need to avoid breaking userspace. It's ugly but for those 3 drivers can we assume that using more than one on a board was impossible before this series and as such play a slight game in which we don't change the trigger name they are exporting, unless that name is already in use?
It's ugly but it gets us the nicest solution for all drivers for a bit of ugly in 3 of them...
Jonathan
Quoted 56 lines
--- drivers/iio/industrialio-trigger.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)drivers/iio/industrialio-trigger.c +21
-0@@ -64,6 +64,8 @@ static struct attribute *iio_trig_dev_attrs[] = { };ATTRIBUTE_GROUPS (iio_trig_dev); +static struct iio_trigger *__iio_trigger_find_by_name (const char *name); +int iio_trigger_register (struct iio_trigger *trig_info) {int ret;@@ -82,11 +84,18 @@ int iio_trigger_register(struct iio_trigger *trig_info)/* Add to list of available triggers held by the IIO core */ mutex_lock (&iio_trigger_list_lock); +if (__iio_trigger_find_by_name (trig_info->name)) { +pr_err ("Duplicate trigger name '%s'\n" , trig_info->name); + ret = -EEXIST ; + goto error_device_del; + }list_add_tail (&trig_info->list, &iio_trigger_list);mutex_unlock (&iio_trigger_list_lock);return 0 ; +error_device_del: +device_del (&trig_info->dev); error_unregister_id:ida_simple_remove (&iio_trigger_ida, trig_info->id);return ret;@@ -105,6 +114,18 @@ void iio_trigger_unregister(struct iio_trigger *trig_info) }EXPORT_SYMBOL (iio_trigger_unregister); +/* Search for trigger by name, assuming iio_trigger_list_lock held */ +static struct iio_trigger *__iio_trigger_find_by_name (const char *name) +{ +struct iio_trigger *iter; + +list_for_each_entry (iter, &iio_trigger_list, list) +if (!strcmp (iter->name, name)) +return iter; + +return NULL ; +} +static struct iio_trigger *iio_trigger_find_by_name (const char *name,size_t len) {
Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names
Quoted 10 lines · Jonathan Cameron
Quoted 6 lines · Crestez Dan Leonard
The trigger name is documented as unique but drivers are currently allowed to register triggers with duplicate names. This should be considered a bug since it makes the '
current_trigger' interface unusable.Signed-off-by Crestez Dan Leonard <[email protected]>This feels like the right approach to my mind (and should have been there all along - oops).
Quoted 7 lines
However, we do need to avoid breaking userspace. It's ugly but for those 3 drivers can we assume that using more than one on a board was impossible before this series and as such play a slight game in which we don't change the trigger name they are exporting, unless that name is already in use?
It's ugly but it gets us the nicest solution for all drivers for a bit of ugly in 3 of them...
How would that look like? I guess I could handle -EEXIST from iio_trigger_register and try again with another name? Unfortunately the name is initialized at alloc time while uniqueness can only be checked at register time. This would require some refactoring in drivers for devices I don't have.
An alternative would be to just submit patches 4/5 and only give a warning when non-unique trigger names are used. After all, iio device names are not unique, the easy way would be to give up on this guarantee for trigger names as well.
Signature · 2 lines
Regards, Leonard
Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names
Quoted 25 lines · Crestez Dan Leonard
Quoted 10 lines · Jonathan Cameron
Quoted 6 lines · Crestez Dan Leonard
The trigger name is documented as unique but drivers are currently allowed to register triggers with duplicate names. This should be considered a bug since it makes the '
current_trigger' interface unusable.Signed-off-by Crestez Dan Leonard <[email protected]>This feels like the right approach to my mind (and should have been there all along - oops).
Quoted 7 lines
However, we do need to avoid breaking userspace. It's ugly but for those 3 drivers can we assume that using more than one on a board was impossible before this series and as such play a slight game in which we don't change the trigger name they are exporting, unless that name is already in use?
It's ugly but it gets us the nicest solution for all drivers for a bit of ugly in 3 of them...
How would that look like? I guess I could handle
-EEXISTfromiio_trigger_registerand try again with another name? Unfortunately the name is initialized at alloc time while uniqueness can only be checked at register time. This would require some refactoring in drivers for devices I don't have.
Quoted 5 lines
An alternative would be to just submit patches 4/5 and only give a warning when non-unique trigger names are used. After all, iio device names are not unique, the easy way would be to give up on this guarantee for trigger names as well.
I'd say apply this patch keep things as they are in the drivers and if somebody creates a board with more than one of those devices let them come up with a fix.
Jonathan Cameron
maintainerRe: [RFC 6/7] iio: Refuse to register triggers with duplicate names
Quoted 32 lines · Lars-Peter Clausen
Quoted 25 lines · Crestez Dan Leonard
Quoted 10 lines · Jonathan Cameron
Quoted 6 lines · Crestez Dan Leonard
The trigger name is documented as unique but drivers are currently allowed to register triggers with duplicate names. This should be considered a bug since it makes the '
current_trigger' interface unusable.Signed-off-by Crestez Dan Leonard <[email protected]>This feels like the right approach to my mind (and should have been there all along - oops).
Quoted 7 lines
However, we do need to avoid breaking userspace. It's ugly but for those 3 drivers can we assume that using more than one on a board was impossible before this series and as such play a slight game in which we don't change the trigger name they are exporting, unless that name is already in use?
It's ugly but it gets us the nicest solution for all drivers for a bit of ugly in 3 of them...
How would that look like? I guess I could handle
-EEXISTfromiio_trigger_registerand try again with another name? Unfortunately the name is initialized at alloc time while uniqueness can only be checked at register time. This would require some refactoring in drivers for devices I don't have.Quoted 5 lines
An alternative would be to just submit patches 4/5 and only give a warning when non-unique trigger names are used. After all, iio device names are not unique, the easy way would be to give up on this guarantee for trigger names as well.
Quoted 3 lines
I'd say apply this patch keep things as they are in the drivers and if somebody creates a board with more than one of those devices let them come up with a fix.
Fair call. Lets do that.
Applied this patch to the togreg branch of iio.git (usual testing blah blah) Let's see who (hopefully no one) screams.
Jonathan
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC 7/7 iio: Make trigger names unique
In reply to Crestez Dan LeonardThis changes the format of trigger names for some drivers so that the result always includes indio_dev->id and is thus unique.
--- drivers/iio/adc/max1027.c | 4 ++-- drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +- drivers/iio/light/gp2ap020a00f.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)
drivers/iio/adc/max1027.c +2 -2
@@ -447,8 +447,8 @@ static int max1027_probe(struct spi_device *spi)return ret; } - st->trig =devm_iio_trigger_alloc (&spi->dev,"%s-trigger" , - indio_dev->name); + st->trig =devm_iio_trigger_alloc (&spi->dev,"%s-dev%d" , + indio_dev->name, indio_dev->iod);if (st->trig ==NULL ) { ret = -ENOMEM ;dev_err (&indio_dev->dev,"Failed to allocate iio trigger\n" );
drivers/iio/common/st_sensors/st_sensors_trigger.c +1 -1
@@ -24,7 +24,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,struct st_sensor_data *sdata =iio_priv (indio_dev);unsigned long irq_trig; - sdata->trig =iio_trigger_alloc ("%s-trigger" , indio_dev->name); + sdata->trig =iio_trigger_alloc ("%s-dev%d" , indio_dev->name, indio_dev->id);if (sdata->trig ==NULL ) {dev_err (&indio_dev->dev,"failed to allocate iio trigger.\n" );return -ENOMEM ;
drivers/iio/light/gp2ap020a00f.c +2 -2
@@ -1553,8 +1553,8 @@ static int gp2ap020a00f_probe(struct i2c_client *client, goto error_regulator_disable;/* Allocate trigger */ - data->trig =devm_iio_trigger_alloc (&client->dev,"%s-trigger" , - indio_dev->name); + data->trig =devm_iio_trigger_alloc (&client->dev,"%s-dev%d" , + indio_dev->name, indio_dev->id);if (data->trig ==NULL ) { err = -ENOMEM ;dev_err (&indio_dev->dev,"Failed to allocate iio trigger.\n" );
Signature · 1 line
2.5.5
Jonathan Cameron
maintainerQuoted 32 lines · Crestez Dan Leonard
IIO documents that trigger names are unique but does not actually guarantee this. You can easily create a software trigger with a duplicate name if you enable
CONFIG_IIO_HRTIMER_TRIGGER:mkdir /sys/kernel/config/iio/triggers/hrtimer/\ `cat /sys/bus/iio/devices/trigger0/name`You can also get in this situation if you connect two identical
st_sensors.My attempt to fix this is by adding a new '
current_trigger_id' ABI which is a numeric ID (X from triggerX) rather than a non-unique string. I tested that this works as expected when connecting two LIS3DH to the same system. This is in patches 4,5.In theory the
current_triggerABI could be overloaded to looks for something matching "trigger[0-9]+" but this would be nastier. There would be a need to prevent registering triggers that match that expression in order to prevent stuff like:mkdir
/sys/kernel/config/iio/triggers/hrtimer/trigger1Such an overload wouldn't work on read and make it impossible to unambigiously determine the currently selected trigger.
Patch 6 disallows registering duplicate trigger names. This makes drivers which currently do that break at probe time.
Patch 7 attempts to ensure that all drivers include
indio_dev->idwhen callingiio_trigger_alloc. This obviously changes trigger names.
Quoted 5 lines
Either 4,5 or 6,7 fix this issue, but 4,5 causes fewer compat issues.
Patches 1,2,3 are just minor features in
tools/generic_buffer. They supersede the v2 I posted earlier:
Please don't do this in future. When reviewing lots of series, one tends to relying on sorting by name to make sure you find the latest version of a series. Do this and people don't know there is a new version embedded in a different series (in this case I picked up V2 when you had found an issue in it).
Jonathan
Quoted 21 lines
https://www.spinics.net/lists/linux-iio/msg24867.htmlCrestez Dan Leonard (7):
iio:generic_buffer: Cleanup when receiving signals
iio:generic_buffer: Add --device-num option
iio:generic_buffer: Add --trigger-num option
iio: Addcurrent_trigger_idalternative
iio:generic_buffer: Usecurrent_trigger_id
iio: Refuse to register triggers with duplicate names
iio: Make trigger names uniqueDocumentation/ABI/testing/sysfs-bus-iio | 9 + Documentation/DocBook/iio.tmpl | 4 +- drivers/iio/adc/max1027.c | 4 +- drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +- drivers/iio/industrialio-trigger.c | 136 ++++++++--- drivers/iio/light/gp2ap020a00f.c | 4 +- tools/iio/generic_buffer.c | 269 ++++++++++++++------- 7 files changed, 309 insertions(+), 119 deletions(-)