diff mbox series

iio: event_monitor: Enable events before monitoring

Message ID 20210304154205.1918124-1-linus.walleij@linaro.org (mailing list archive)
State New
Headers show
Series iio: event_monitor: Enable events before monitoring | expand

Commit Message

Linus Walleij March 4, 2021, 3:42 p.m. UTC
After some painful sessions with a driver that register an
enable/disable sysfs knob (gp2ap002) and manually going
in and enabling the event before monitoring it:

  cd /sys/bus/iio/devices/iio\:device2/events
  # ls
  in_proximity_thresh_either_en
  # echo 1 > in_proximity_thresh_either_en

I realized that it's better if the iio_event_monitor is
smart enough to enable all events by itself and disable them
after use.

I didn't add a command line option for this, to me it
seems pretty intuitive and mostly what you want the tool
to do for you.

Cc: Bastien Nocera <hadess@hadess.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Bastien: added you on CC FYI because I suppose maybe
iio-sensor-proxy isn't yet doing this and one day people
will wonder why their events aren't arriving.
---
 tools/iio/iio_event_monitor.c | 51 +++++++++++++++++++++++++++++++++++
 tools/iio/iio_utils.h         |  1 +
 2 files changed, 52 insertions(+)

Comments

Bastien Nocera March 4, 2021, 3:59 p.m. UTC | #1
On Thu, 2021-03-04 at 16:42 +0100, Linus Walleij wrote:
> After some painful sessions with a driver that register an
> enable/disable sysfs knob (gp2ap002) and manually going
> in and enabling the event before monitoring it:
> 
>   cd /sys/bus/iio/devices/iio\:device2/events
>   # ls
>   in_proximity_thresh_either_en
>   # echo 1 > in_proximity_thresh_either_en
> 
> I realized that it's better if the iio_event_monitor is
> smart enough to enable all events by itself and disable them
> after use.
> 
> I didn't add a command line option for this, to me it
> seems pretty intuitive and mostly what you want the tool
> to do for you.
> 
> Cc: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

If I'm not mistaken, "-a" does that for the iio_generic_buffer tool.

Maybe moving enable_disable_all_channels() to a common location and
using that would cut down on the duplicated code?

> ---
> Bastien: added you on CC FYI because I suppose maybe
> iio-sensor-proxy isn't yet doing this and one day people
> will wonder why their events aren't arriving.
> ---
>  tools/iio/iio_event_monitor.c | 51
> +++++++++++++++++++++++++++++++++++
>  tools/iio/iio_utils.h         |  1 +
>  2 files changed, 52 insertions(+)
> 
> diff --git a/tools/iio/iio_event_monitor.c
> b/tools/iio/iio_event_monitor.c
> index bb03859db89d..27778fe28f82 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -14,6 +14,7 @@
>  
>  #include <unistd.h>
>  #include <stdlib.h>
> +#include <dirent.h>
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <errno.h>
> @@ -280,10 +281,49 @@ static void print_event(struct iio_event_data
> *event)
>         printf("\n");
>  }
>  
> +/* Enable or disable events in sysfs if the knob is available */
> +static void enable_events(char *dev_dir, int enable)
> +{
> +       const struct dirent *ent;
> +       char evdir[256];
> +       int ret;
> +       DIR *dp;
> +
> +       snprintf(evdir, sizeof(evdir), FORMAT_EVENTS_DIR, dev_dir);
> +       evdir[sizeof(evdir)-1] = '\0';
> +
> +       dp = opendir(evdir);
> +       if (!dp) {
> +               fprintf(stderr, "Enabling/disabling events: can't
> open %s\n",
> +                       evdir);
> +               return;
> +       }
> +
> +       while (ent = readdir(dp), ent) {
> +               if (iioutils_check_suffix(ent->d_name, "_en")) {
> +                       printf("%sabling: %s\n",
> +                              enable ? "En" : "Dis",
> +                              ent->d_name);
> +                       ret = write_sysfs_int(ent->d_name, evdir,
> +                                             enable);
> +                       if (ret < 0)
> +                               fprintf(stderr, "Failed to
> enable/disable %s\n",
> +                                       ent->d_name);
> +               }
> +       }
> +
> +       if (closedir(dp) == -1) {
> +               perror("Enabling/disabling channels: "
> +                      "Failed to close directory");
> +               return;
> +       }
> +}
> +
>  int main(int argc, char **argv)
>  {
>         struct iio_event_data event;
>         const char *device_name;
> +       char *dev_dir_name = NULL;
>         char *chrdev_name;
>         int ret;
>         int dev_num;
> @@ -303,6 +343,10 @@ int main(int argc, char **argv)
>                 ret = asprintf(&chrdev_name, "/dev/iio:device%d",
> dev_num);
>                 if (ret < 0)
>                         return -ENOMEM;
> +               /* Look up sysfs dir as well if we can */
> +               ret = asprintf(&dev_dir_name, "%siio:device%d",
> iio_dir, dev_num);
> +               if (ret < 0)
> +                       return -ENOMEM;
>         } else {
>                 /*
>                  * If we can't find an IIO device by name assume
> device_name is
> @@ -313,6 +357,9 @@ int main(int argc, char **argv)
>                         return -ENOMEM;
>         }
>  
> +       if (dev_dir_name)
> +               enable_events(dev_dir_name, 1);
> +
>         fd = open(chrdev_name, 0);
>         if (fd == -1) {
>                 ret = -errno;
> @@ -365,6 +412,10 @@ int main(int argc, char **argv)
>                 perror("Failed to close event file");
>  
>  error_free_chrdev_name:
> +       /* Disable events after use */
> +       if (dev_dir_name)
> +               enable_events(dev_dir_name, 0);
> +
>         free(chrdev_name);
>  
>         return ret;
> diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
> index 74bde4fde2c8..c01695049739 100644
> --- a/tools/iio/iio_utils.h
> +++ b/tools/iio/iio_utils.h
> @@ -13,6 +13,7 @@
>  #define IIO_MAX_NAME_LENGTH 64
>  
>  #define FORMAT_SCAN_ELEMENTS_DIR "%s/scan_elements"
> +#define FORMAT_EVENTS_DIR "%s/events"
>  #define FORMAT_TYPE_FILE "%s_type"
>  
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
Linus Walleij March 4, 2021, 8:48 p.m. UTC | #2
On Thu, Mar 4, 2021 at 5:00 PM Bastien Nocera <hadess@hadess.net> wrote:

> If I'm not mistaken, "-a" does that for the iio_generic_buffer tool.

Yeah I implemented that, and I thought about doing the same here
but ... the name of the tool sort of announce that one want to
listen to all events so I thought it should just default-enable
all of them in this case.

> Maybe moving enable_disable_all_channels() to a common location and
> using that would cut down on the duplicated code?

The event enablement is slightly different, the generic buffer
turns on various channels, which is conceptually different
from various events, but let's see what Jonathan says.

We are sharing most of the code already in the iio-utils.c
but I can try to break out more if it doesn't get to abstract.

Thanks Bastien,
Linus Walleij
Jonathan Cameron March 6, 2021, 3:39 p.m. UTC | #3
On Thu, 4 Mar 2021 21:48:25 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Mar 4, 2021 at 5:00 PM Bastien Nocera <hadess@hadess.net> wrote:
> 
> > If I'm not mistaken, "-a" does that for the iio_generic_buffer tool.  
> 
> Yeah I implemented that, and I thought about doing the same here
> but ... the name of the tool sort of announce that one want to
> listen to all events so I thought it should just default-enable
> all of them in this case.
> 
> > Maybe moving enable_disable_all_channels() to a common location and
> > using that would cut down on the duplicated code?  
> 
> The event enablement is slightly different, the generic buffer
> turns on various channels, which is conceptually different
> from various events, but let's see what Jonathan says.
> 
> We are sharing most of the code already in the iio-utils.c
> but I can try to break out more if it doesn't get to abstract.

Sadly this doesn't work for many devices.
It is a common thing for hardware to only support a much smaller
set of event monitoring registers / threshold detectors than the
number of channels.  In many cases we handle that by working on
a fifo basis.  So what this will do is enable a bunch of events
which will then be replaced by later events - end result some
random event will be enabled (or maybe 2 of them across N channels)

Not intuitive at all :(

I'm fine with it being controlled by a parameter though if
that works for you.  Docs should explain it doesn't always
result in all events being enabled however if the hardware
is not capable of doing that.

Jonathan


> 
> Thanks Bastien,
> Linus Walleij
Linus Walleij March 6, 2021, 9:29 p.m. UTC | #4
On Sat, Mar 6, 2021 at 4:39 PM Jonathan Cameron <jic23@kernel.org> wrote:

> Sadly this doesn't work for many devices.
> It is a common thing for hardware to only support a much smaller
> set of event monitoring registers / threshold detectors than the
> number of channels.  In many cases we handle that by working on
> a fifo basis.  So what this will do is enable a bunch of events
> which will then be replaced by later events - end result some
> random event will be enabled (or maybe 2 of them across N channels)

I understand.

What about augmenting the heuristics like this:

1. Count the available events.
2. If they are just one, then enable that event and disable after use.

This will make all proximity sensors and other things that just
provide a single event work out of the box.

Yours,
Linus Walleij
Jonathan Cameron March 7, 2021, 11:54 a.m. UTC | #5
On Sat, 6 Mar 2021 22:29:41 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sat, Mar 6, 2021 at 4:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > Sadly this doesn't work for many devices.
> > It is a common thing for hardware to only support a much smaller
> > set of event monitoring registers / threshold detectors than the
> > number of channels.  In many cases we handle that by working on
> > a fifo basis.  So what this will do is enable a bunch of events
> > which will then be replaced by later events - end result some
> > random event will be enabled (or maybe 2 of them across N channels)  
> 
> I understand.
> 
> What about augmenting the heuristics like this:
> 
> 1. Count the available events.
> 2. If they are just one, then enable that event and disable after use.
> 
> This will make all proximity sensors and other things that just
> provide a single event work out of the box.

Rather unintuitive interface.  I think we are better off just
adding a -a parameter like we have for the buffer example.

If people get used to it enabling sensible events for a simple device
then move on to a more complex one where the heuristic breaks down
then they will be very confused.

J

> 
> Yours,
> Linus Walleij
diff mbox series

Patch

diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index bb03859db89d..27778fe28f82 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -14,6 +14,7 @@ 
 
 #include <unistd.h>
 #include <stdlib.h>
+#include <dirent.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <errno.h>
@@ -280,10 +281,49 @@  static void print_event(struct iio_event_data *event)
 	printf("\n");
 }
 
+/* Enable or disable events in sysfs if the knob is available */
+static void enable_events(char *dev_dir, int enable)
+{
+	const struct dirent *ent;
+	char evdir[256];
+	int ret;
+	DIR *dp;
+
+	snprintf(evdir, sizeof(evdir), FORMAT_EVENTS_DIR, dev_dir);
+	evdir[sizeof(evdir)-1] = '\0';
+
+	dp = opendir(evdir);
+	if (!dp) {
+		fprintf(stderr, "Enabling/disabling events: can't open %s\n",
+			evdir);
+		return;
+	}
+
+	while (ent = readdir(dp), ent) {
+		if (iioutils_check_suffix(ent->d_name, "_en")) {
+			printf("%sabling: %s\n",
+			       enable ? "En" : "Dis",
+			       ent->d_name);
+			ret = write_sysfs_int(ent->d_name, evdir,
+					      enable);
+			if (ret < 0)
+				fprintf(stderr, "Failed to enable/disable %s\n",
+					ent->d_name);
+		}
+	}
+
+	if (closedir(dp) == -1) {
+		perror("Enabling/disabling channels: "
+		       "Failed to close directory");
+		return;
+	}
+}
+
 int main(int argc, char **argv)
 {
 	struct iio_event_data event;
 	const char *device_name;
+	char *dev_dir_name = NULL;
 	char *chrdev_name;
 	int ret;
 	int dev_num;
@@ -303,6 +343,10 @@  int main(int argc, char **argv)
 		ret = asprintf(&chrdev_name, "/dev/iio:device%d", dev_num);
 		if (ret < 0)
 			return -ENOMEM;
+		/* Look up sysfs dir as well if we can */
+		ret = asprintf(&dev_dir_name, "%siio:device%d", iio_dir, dev_num);
+		if (ret < 0)
+			return -ENOMEM;
 	} else {
 		/*
 		 * If we can't find an IIO device by name assume device_name is
@@ -313,6 +357,9 @@  int main(int argc, char **argv)
 			return -ENOMEM;
 	}
 
+	if (dev_dir_name)
+		enable_events(dev_dir_name, 1);
+
 	fd = open(chrdev_name, 0);
 	if (fd == -1) {
 		ret = -errno;
@@ -365,6 +412,10 @@  int main(int argc, char **argv)
 		perror("Failed to close event file");
 
 error_free_chrdev_name:
+	/* Disable events after use */
+	if (dev_dir_name)
+		enable_events(dev_dir_name, 0);
+
 	free(chrdev_name);
 
 	return ret;
diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
index 74bde4fde2c8..c01695049739 100644
--- a/tools/iio/iio_utils.h
+++ b/tools/iio/iio_utils.h
@@ -13,6 +13,7 @@ 
 #define IIO_MAX_NAME_LENGTH 64
 
 #define FORMAT_SCAN_ELEMENTS_DIR "%s/scan_elements"
+#define FORMAT_EVENTS_DIR "%s/events"
 #define FORMAT_TYPE_FILE "%s_type"
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))