[RFC 0/7] Deal with iio trigger names

[RFC 0/7] Deal with iio trigger names

[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_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(-)

-- 
2.5.5

[PATCHv3 1/7] iio: generic_buffer: Cleanup when receiving signals

[Crestez Dan Leonard]

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.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
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(-)

diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
index 2429c78..972f400 100644
--- a/tools/iio/generic_buffer.c
+++ b/tools/iio/generic_buffer.c
@@ -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;
-- 
2.5.5

[PATCHv3 2/7] iio: generic_buffer: Add --device-num option

[Crestez Dan Leonard]

This makes it possible to distinguish between iio devices with the same
name.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 tools/iio/generic_buffer.c | 69 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 18 deletions(-)

diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
index 972f400..3f16e9f 100644
--- a/tools/iio/generic_buffer.c
+++ b/tools/iio/generic_buffer.c
@@ -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;
-- 
2.5.5

[PATCHv3 3/7] iio: generic_buffer: Add --trigger-num option

[Crestez Dan Leonard]

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 tools/iio/generic_buffer.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
index 3f16e9f..e8c3052 100644
--- a/tools/iio/generic_buffer.c
+++ b/tools/iio/generic_buffer.c
@@ -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");
 	}
 
 	/*
-- 
2.5.5

[RFC 4/7] iio: Add current_trigger_id alternative

[Crestez Dan Leonard]

This allows controlling the current trigger by numeric ID rather than
name.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 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(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index df44998..e276032 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -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:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
diff --git a/Documentation/DocBook/iio.tmpl b/Documentation/DocBook/iio.tmpl
index f525bf5..599eff0 100644
--- a/Documentation/DocBook/iio.tmpl
+++ b/Documentation/DocBook/iio.tmpl
@@ -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>
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index ae2806a..e79c64c 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -121,6 +121,21 @@ static struct iio_trigger *iio_trigger_find_by_name(const char *name,
 	return trig;
 }
 
+static struct iio_trigger *iio_trigger_find_by_id(int id)
+{
+	struct iio_trigger *trig = NULL, *iter;
+
+	mutex_lock(&iio_trigger_list_lock);
+	list_for_each_entry(iter, &iio_trigger_list, list)
+		if (iter->id == id) {
+			trig = iter;
+			break;
+		}
+	mutex_unlock(&iio_trigger_list_lock);
+
+	return trig;
+}
+
 void iio_trigger_poll(struct iio_trigger *trig)
 {
 	int i;
@@ -294,7 +309,7 @@ void iio_dealloc_pollfunc(struct iio_poll_func *pf)
 EXPORT_SYMBOL_GPL(iio_dealloc_pollfunc);
 
 /**
- * iio_trigger_read_current() - trigger consumer sysfs query current trigger
+ * iio_trigger_read_current_name() - trigger consumer sysfs query current trigger
  * @dev:	device associated with an industrial I/O device
  * @attr:	pointer to the device_attribute structure that
  *		is being processed
@@ -306,9 +321,9 @@ EXPORT_SYMBOL_GPL(iio_dealloc_pollfunc);
  * Return: a negative number on failure, the number of characters written
  *	   on success or 0 if no trigger is available
  */
-static ssize_t iio_trigger_read_current(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
+static ssize_t iio_trigger_read_current_name(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 
@@ -317,28 +332,22 @@ static ssize_t iio_trigger_read_current(struct device *dev,
 	return 0;
 }
 
-/**
- * iio_trigger_write_current() - trigger consumer sysfs set current trigger
- * @dev:	device associated with an industrial I/O device
- * @attr:	device attribute that is being processed
- * @buf:	string buffer that holds the name of the trigger
- * @len:	length of the trigger name held by buf
- *
- * For trigger consumers the current_trigger interface allows the trigger
- * used for this device to be specified at run time based on the trigger's
- * name.
- *
- * Return: negative error code on failure or length of the buffer
- *	   on success
- */
-static ssize_t iio_trigger_write_current(struct device *dev,
-					 struct device_attribute *attr,
-					 const char *buf,
-					 size_t len)
+static ssize_t iio_trigger_read_current_id(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+
+	if (indio_dev->trig)
+		return sprintf(buf, "%d\n", indio_dev->trig->id);
+	return 0;
+}
+
+/* Set current trigger. Returns -errno or 0 on success */
+static int iio_trigger_set_current(struct iio_dev *indio_dev,
+				   struct iio_trigger *trig)
+{
 	struct iio_trigger *oldtrig = indio_dev->trig;
-	struct iio_trigger *trig;
 	int ret;
 
 	mutex_lock(&indio_dev->mlock);
@@ -348,18 +357,24 @@ static ssize_t iio_trigger_write_current(struct device *dev,
 	}
 	mutex_unlock(&indio_dev->mlock);
 
-	trig = iio_trigger_find_by_name(buf, len);
-	if (oldtrig == trig)
-		return len;
-
 	if (trig && indio_dev->info->validate_trigger) {
 		ret = indio_dev->info->validate_trigger(indio_dev, trig);
+		if (ret > 0) {
+			dev_err(&indio_dev->dev, "validate_trigger %ps incorrectly returned positive %d\n",
+				indio_dev->info->validate_trigger, ret);
+			return -EINVAL;
+		}
 		if (ret)
 			return ret;
 	}
 
 	if (trig && trig->ops && trig->ops->validate_device) {
 		ret = trig->ops->validate_device(trig, indio_dev);
+		if (ret > 0) {
+			dev_err(&indio_dev->dev, "validate_device %ps incorrectly returned positive %d\n",
+				trig->ops->validate_device, ret);
+			return -EINVAL;
+		}
 		if (ret)
 			return ret;
 	}
@@ -379,15 +394,57 @@ static ssize_t iio_trigger_write_current(struct device *dev,
 						     indio_dev->pollfunc_event);
 	}
 
+	return 0;
+}
+
+static ssize_t iio_trigger_write_current_name(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf,
+					      size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = iio_trigger_find_by_name(buf, len);
+	ret = iio_trigger_set_current(indio_dev, trig);
+
+	return ret ? ret : len;
+}
+
+static ssize_t iio_trigger_write_current_id(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf,
+					    size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_trigger *trig;
+	int ret, id;
+
+	ret = kstrtoint(buf, 10, &id);
+	if (ret)
+		return ret;
+	if (id >= 0) {
+		trig = iio_trigger_find_by_id(id);
+		if (!trig)
+			return -ENOENT;
+	} else
+		trig = NULL;
+	ret = iio_trigger_set_current(indio_dev, trig);
+
 	return len;
 }
 
 static DEVICE_ATTR(current_trigger, S_IRUGO | S_IWUSR,
-		   iio_trigger_read_current,
-		   iio_trigger_write_current);
+		   iio_trigger_read_current_name,
+		   iio_trigger_write_current_name);
+static DEVICE_ATTR(current_trigger_id, S_IRUGO | S_IWUSR,
+		   iio_trigger_read_current_id,
+		   iio_trigger_write_current_id);
 
 static struct attribute *iio_trigger_consumer_attrs[] = {
 	&dev_attr_current_trigger.attr,
+	&dev_attr_current_trigger_id.attr,
 	NULL,
 };
 
-- 
2.5.5

[RFC 5/7] iio: generic_buffer: Use current_trigger_id

[Crestez Dan Leonard]

This is a preferred alternative to 'current_trigger'.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 tools/iio/generic_buffer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
index e8c3052..b23371a 100644
--- a/tools/iio/generic_buffer.c
+++ b/tools/iio/generic_buffer.c
@@ -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;
 		}
 	}
-- 
2.5.5

[RFC 6/7] iio: Refuse to register triggers with duplicate names

[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 <leonard.crestez@intel.com>
---
 drivers/iio/industrialio-trigger.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index e79c64c..e77503c 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -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)
 {
-- 
2.5.5

[RFC 7/7] iio: Make trigger names unique

[Crestez Dan Leonard]

This changes the format of trigger names for some drivers so that the
result always includes indio_dev->id and is thus unique.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 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(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 41d495c..4020257 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -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");
diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index da72279..9d3e9ab 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -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;
diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index 6d41086..e2efaeb 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -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");
-- 
2.5.5

Re: [PATCHv3 1/7] iio: generic_buffer: Cleanup when receiving signals

[Jonathan Cameron]

On 23/05/16 19:39, Crestez Dan Leonard wrote:
> 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.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Dropped v2 and applied this one with the same name mangling to match new
naming as iio_generic_buffer

Thanks,

Jonathan
> ---
> 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(-)
> 
> diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
> index 2429c78..972f400 100644
> --- a/tools/iio/generic_buffer.c
> +++ b/tools/iio/generic_buffer.c
> @@ -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;
> 

Re: [PATCHv3 2/7] iio: generic_buffer: Add --device-num option

[Jonathan Cameron]

On 23/05/16 19:39, Crestez Dan Leonard wrote:
> This makes it possible to distinguish between iio devices with the same
> name.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Applied.
> ---
>  tools/iio/generic_buffer.c | 69 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
> index 972f400..3f16e9f 100644
> --- a/tools/iio/generic_buffer.c
> +++ b/tools/iio/generic_buffer.c
> @@ -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;
> 

Re: [PATCHv3 3/7] iio: generic_buffer: Add --trigger-num option

[Jonathan Cameron]

On 23/05/16 19:39, Crestez Dan Leonard wrote:
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Again, sensible and straight forward.

Thanks,

Jonathan
> ---
>  tools/iio/generic_buffer.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c
> index 3f16e9f..e8c3052 100644
> --- a/tools/iio/generic_buffer.c
> +++ b/tools/iio/generic_buffer.c
> @@ -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");
>  	}
>  
>  	/*
> 

Re: [RFC 0/7] Deal with iio trigger names

[Jonathan Cameron]

On 23/05/16 19:39, Crestez Dan Leonard wrote:
> 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:
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
> 
> 	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(-)
> 

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Jonathan Cameron]

On 23/05/16 19:40, Crestez Dan Leonard wrote:
> 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 <leonard.crestez@intel.com>
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
> ---
>  drivers/iio/industrialio-trigger.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index e79c64c..e77503c 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -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

[Crestez Dan Leonard]

On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
> On 23/05/16 19:40, Crestez Dan Leonard wrote:
>> 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 <leonard.crestez@intel.com>
> 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...

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.

-- 
Regards,
Leonard

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Lars-Peter Clausen]

On 05/30/2016 02:49 PM, Crestez Dan Leonard wrote:
> On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
>> On 23/05/16 19:40, Crestez Dan Leonard wrote:
>>> 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 <leonard.crestez@intel.com>
>> 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...
> 
> 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.
> 

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.

Re: [RFC 4/7] iio: Add current_trigger_id alternative

[Lars-Peter Clausen]

On 05/23/2016 08:39 PM, Crestez Dan Leonard wrote:
> 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.

Re: [RFC 6/7] iio: Refuse to register triggers with duplicate names

[Jonathan Cameron]

On 31/05/16 15:03, Lars-Peter Clausen wrote:
> On 05/30/2016 02:49 PM, Crestez Dan Leonard wrote:
>> On 05/29/2016 10:48 PM, Jonathan Cameron wrote:
>>> On 23/05/16 19:40, Crestez Dan Leonard wrote:
>>>> 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 <leonard.crestez@intel.com>
>>> 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...
>>
>> 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.
>>
> 
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>